linux-kernel - Re: [PATCH 1/4] clk: Introduce 'clk_find_nearest

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 1 Jul 2014 10:23:02 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	Soren Brinkmann <soren.brinkmann@...inx.com>
Cc:	Mike Turquette <mturquette@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Russell King <linux@....linux.org.uk>,
	Michal Simek <michal.simek@...inx.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
	cpufreq@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/4] clk: Introduce 'clk_find_nearest_rate()'

On Mon, Jun 30, 2014 at 09:56:33AM -0700, Soren Brinkmann wrote:
> Introduce a new API function to find the rate a clock can provide which
> is closest to a given rate.
> 
> clk_round_rate() leaves it to the clock driver how rounding is done.
> Commonly implementations round down due to use-cases that have a certain
> frequency maximum that must not be exceeded.
> 
> The new API call enables use-cases where accuracy is preferred. E.g.
> Ethernet clocks.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@...inx.com>
> ---
> 
>  drivers/clk/clk.c   | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/clk.h |  9 +++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8b73edef151d..fce1165cd879 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1030,6 +1030,63 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  EXPORT_SYMBOL_GPL(clk_round_rate);
>  
>  /**
> + * clk_find_nearest_rate - round the given rate for a clk
> + * @clk: the clk for which we are rounding a rate
> + * @rate: the rate which is to be rounded
> + *
> + * Takes in a rate as input and finds the closest rate that the clk
> + * can actually use which is then returned.
> + * Note: This function relies on the clock's clk_round_rate() implementation.
> + * For cases clk_round_rate() rounds up, not the closest but the rounded up
> + * rate is found.
> + */
> +long clk_find_nearest_rate(struct clk *clk, unsigned long rate)
> +{
> +	long ret, lower, upper;
> +	unsigned long tmp;
> +
> +	clk_prepare_lock();
> +
> +	lower = __clk_round_rate(clk, rate);
> +	if (lower >= rate || lower < 0) {
> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	tmp = rate + (rate - lower) - 1;
> +	if (tmp > LONG_MAX)
> +		upper = LONG_MAX;
> +	else
> +		upper = tmp;
Consider rate = 0xf0000000, lower = 0x7fffffff (= LONG_MAX). Then tmp =
(unsigned long)0x160000000 = 0x60000000. In this case you pick upper =
0x60000000 while you should use upper = LONG_MAX.

I think you need

-	if (tmp > LONG_MAX)
+	if (tmp > LONG_MAX || tmp < rate)

(and a comment)

> +
> +	upper = __clk_round_rate(clk, upper);
> +	if (upper <= lower || upper < 0) {
Is it an idea to do something like:

		if (IS_ENABLED(CONFIG_CLK_SANITY_CHECKS))
			WARN_ON(upper < lower && upper >= 0);

here?

> +		ret = lower;
> +		goto unlock;
> +	}
> +
> +	lower = rate + 1;
> +	while (lower < upper) {
> +		long rounded, mid;
> +
> +		mid = lower + ((upper - lower) >> 1);
> +		rounded = __clk_round_rate(clk, mid);
> +		if (rounded < lower)
> +			lower = mid + 1;
> +		else
> +			upper = rounded;
> +	}
This is broken if you don't assume that __clk_round_rate rounds down.
Consider an implementation that already does round_nearest and clk can
assume the values 0x60000 and 0x85000 (and nothing in between), and rate
= 0x70000. This results in

	lower = 0x60000;
	tmp = 0x7ffff;
	upper = __clk_round_rate(clk, 0x7ffff) = 0x85000

before the loop and the loop then doesn't even terminate.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/