Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Sebastian Reichel' <sebastian.reichel@collabora.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "kernel@collabora.com" <kernel@collabora.com>
Subject: RE: [PATCH v1 2/2] clk: divider: Properly handle rates exceeding
 UINT_MAX
Thread-Topic: [PATCH v1 2/2] clk: divider: Properly handle rates exceeding
 UINT_MAX
Thread-Index: AQHZioTsgOA8rKF4Y0+0s2ErF0La269l9j4w
Date:   Mon, 22 May 2023 08:18:53 +0000
Message-ID: <1b74d2ea2c3a458694c4c74f2381fcab@AcuMS.aculab.com>
References: <20230519190522.194729-1-sebastian.reichel@collabora.com>
 <20230519190522.194729-3-sebastian.reichel@collabora.com>
In-Reply-To: <20230519190522.194729-3-sebastian.reichel@collabora.com>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

From: Sebastian Reichel
> Sent: 19 May 2023 20:05
> 
> Requesting rates exceeding UINT_MAX (so roughly 4.3 GHz) results
> in very small rate being chosen instead of very high ones, since
> DIV_ROUND_UP_ULL takes a 32 bit integer as second argument.
> 
> Correct this by using DIV64_U64_ROUND_UP instead, which takes proper
> 64 bit values for dividend and divisor.

This doesn't look right on 32-bit architectures.
While you really don't want to be doing full 64bit divides
there is also the problem that any input values over 4.3Ghz
have already been masked.

In the values can be over 4.3GHz then the function arguments
need to be 64bit - not long.

	David

> 
> Note, that this is usually not an issue. ULONG_MAX sets the lower
> 32 bits and thus effectively requests UINT_MAX. On most platforms
> that is good enough. To trigger a real bug one of the following
> conditions must be met:
> 
>  * A parent clock with more than 8.5 GHz is available
>  * Instead of ULONG_MAX a specific frequency like 4.3 GHz is
>    requested. That would end up becoming 5 MHz instead :)
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/clk/clk-divider.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
> index a2c2b5203b0a..dddaaf0f9d25 100644
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -220,7 +220,7 @@ static int _div_round_up(const struct clk_div_table *table,
>  			 unsigned long parent_rate, unsigned long rate,
>  			 unsigned long flags)
>  {
> -	int div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
> +	int div = DIV64_U64_ROUND_UP(parent_rate, rate);
> 
>  	if (flags & CLK_DIVIDER_POWER_OF_TWO)
>  		div = __roundup_pow_of_two(div);
> @@ -237,7 +237,7 @@ static int _div_round_closest(const struct clk_div_table *table,
>  	int up, down;
>  	unsigned long up_rate, down_rate;
> 
> -	up = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
> +	up = DIV64_U64_ROUND_UP(parent_rate, rate);
>  	down = parent_rate / rate;
> 
>  	if (flags & CLK_DIVIDER_POWER_OF_TWO) {
> @@ -473,7 +473,7 @@ int divider_get_val(unsigned long rate, unsigned long parent_rate,
>  {
>  	unsigned int div, value;
> 
> -	div = DIV_ROUND_UP_ULL((u64)parent_rate, rate);
> +	div = DIV64_U64_ROUND_UP(parent_rate, rate);
> 
>  	if (!_is_valid_div(table, div, flags))
>  		return -EINVAL;
> --
> 2.39.2

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)