[PATCH v1 0/2] Fix 64 bit issues in common clock framework

[PATCH v1 0/2] Fix 64 bit issues in common clock framework

[Sebastian Reichel]

Hi,

I found two independent issues related to handling of big rates
in the common clock framework.

Thanks,

-- Sebastian

Sebastian Reichel (2):
  clk: composite: Fix handling of high clock rates
  clk: divider: Properly handle rates exceeding UINT_MAX

 drivers/clk/clk-composite.c | 5 ++++-
 drivers/clk/clk-divider.c   | 6 +++---
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH v1 1/2] clk: composite: Fix handling of high clock rates

[Sebastian Reichel]

ULONG_MAX is used by a few drivers to figure out the highest available
clock rate via clk_round_rate(clk, ULONG_MAX). Since abs() takes a
signed value as input, the current logic effectively calculates with
ULONG_MAX = -1, which results in the worst parent clock being chosen
instead of the best one.

For example on Rockchip RK3588 the eMMC driver tries to figure out
the highest available clock rate. There are three parent clocks
available resulting in the following rate diffs with the existing
logic:

GPLL:   abs(18446744073709551615 - 1188000000) = 1188000001
CPLL:   abs(18446744073709551615 - 1500000000) = 1500000001
XIN24M: abs(18446744073709551615 -   24000000) =   24000001

As a result the clock framework will promote a maximum supported
clock rate of 24 MHz, even though 1.5GHz are possible. With the
updated logic any casting between signed and unsigned is avoided
and the numbers look like this instead:

GPLL:   18446744073709551615 - 1188000000 = 18446744072521551615
CPLL:   18446744073709551615 - 1500000000 = 18446744072209551615
XIN24M: 18446744073709551615 -   24000000 = 18446744073685551615

As a result the parent with the highest acceptable rate is chosen
instead of the parent clock with the lowest one.

Cc: stable@vger.kernel.org
Fixes: 49502408007b ("mmc: sdhci-of-dwcmshc: properly determine max clock on Rockchip")
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/clk/clk-composite.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
index edfa94641bbf..66759fe28fad 100644
--- a/drivers/clk/clk-composite.c
+++ b/drivers/clk/clk-composite.c
@@ -119,7 +119,10 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
 			if (ret)
 				continue;
 
-			rate_diff = abs(req->rate - tmp_req.rate);
+			if (req->rate >= tmp_req.rate)
+				rate_diff = req->rate - tmp_req.rate;
+			else
+				rate_diff = tmp_req.rate - req->rate;
 
 			if (!rate_diff || !req->best_parent_hw
 				       || best_rate_diff > rate_diff) {
-- 
2.39.2

Re: [PATCH v1 1/2] clk: composite: Fix handling of high clock rates

[Christopher Obbard]

Hi Sebastian,

On Fri, 2023-05-19 at 21:05 +0200, Sebastian Reichel wrote:
> ULONG_MAX is used by a few drivers to figure out the highest available
> clock rate via clk_round_rate(clk, ULONG_MAX). Since abs() takes a
> signed value as input, the current logic effectively calculates with
> ULONG_MAX = -1, which results in the worst parent clock being chosen
> instead of the best one.
> 
> For example on Rockchip RK3588 the eMMC driver tries to figure out
> the highest available clock rate. There are three parent clocks
> available resulting in the following rate diffs with the existing
> logic:
> 
> GPLL:   abs(18446744073709551615 - 1188000000) = 1188000001
> CPLL:   abs(18446744073709551615 - 1500000000) = 1500000001
> XIN24M: abs(18446744073709551615 -   24000000) =   24000001
> 
> As a result the clock framework will promote a maximum supported
> clock rate of 24 MHz, even though 1.5GHz are possible. With the
> updated logic any casting between signed and unsigned is avoided
> and the numbers look like this instead:
> 
> GPLL:   18446744073709551615 - 1188000000 = 18446744072521551615
> CPLL:   18446744073709551615 - 1500000000 = 18446744072209551615
> XIN24M: 18446744073709551615 -   24000000 = 18446744073685551615
> 
> As a result the parent with the highest acceptable rate is chosen
> instead of the parent clock with the lowest one.
> 
> Cc: stable@vger.kernel.org
> Fixes: 49502408007b ("mmc: sdhci-of-dwcmshc: properly determine max clock on Rockchip")
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>

This patch series fixes the error on Rockchip RK3588 on ROCK 5 Model B.

Tested-by: Christopher Obbard <chris.obbard@collabora.com>

> ---
>  drivers/clk/clk-composite.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index edfa94641bbf..66759fe28fad 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -119,7 +119,10 @@ static int clk_composite_determine_rate(struct clk_hw *hw,
>                         if (ret)
>                                 continue;
>  
> -                       rate_diff = abs(req->rate - tmp_req.rate);
> +                       if (req->rate >= tmp_req.rate)
> +                               rate_diff = req->rate - tmp_req.rate;
> +                       else
> +                               rate_diff = tmp_req.rate - req->rate;
>  
>                         if (!rate_diff || !req->best_parent_hw
>                                        || best_rate_diff > rate_diff) {
> -- 
> 2.39.2
> 
> 

Re: [PATCH v1 1/2] clk: composite: Fix handling of high clock rates

[Andy Shevchenko]

On Sat, May 20, 2023 at 08:57:16AM +0100, Christopher Obbard wrote:
> On Fri, 2023-05-19 at 21:05 +0200, Sebastian Reichel wrote:

...

> > -                       rate_diff = abs(req->rate - tmp_req.rate);
> > +                       if (req->rate >= tmp_req.rate)
> > +                               rate_diff = req->rate - tmp_req.rate;
> > +                       else
> > +                               rate_diff = tmp_req.rate - req->rate;

Btw, we have abs_diff() helper for this kind of cases.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/2] clk: divider: Properly handle rates exceeding UINT_MAX

[Sebastian Reichel]

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.

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

Re: [PATCH v1 2/2] clk: divider: Properly handle rates exceeding UINT_MAX

[Christopher Obbard]

Hi Sebastian,

On Fri, 2023-05-19 at 21:05 +0200, Sebastian Reichel wrote:
> 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.
> 
> 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>

This patch series fixes the error on Rockchip RK3588 on ROCK 5 Model B.

Tested-by: Christopher Obbard <chris.obbard@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
> 
> 

RE: [PATCH v1 2/2] clk: divider: Properly handle rates exceeding UINT_MAX

[David Laight]

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)

Re: [PATCH v1 2/2] clk: divider: Properly handle rates exceeding UINT_MAX

['Sebastian Reichel']

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

Hi,

On Mon, May 22, 2023 at 08:18:53AM +0000, David Laight wrote:
> 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.

The common clock framework uses 'unsigned long' for clock rates
everywhere, so it's limited to ~4.3 GHz on 32-bit. I guess it does
not really matter - at least I don't expect new 32-bit platforms
with such high clock rates. Considering Intel and AMD already reach
5 GHz boost rates nowadays this is definetly not true for 64-bit.

The current function does (u64 / u32), so it's wrong on 32-bit
(rate can never be bigger than u32, so the u64 is useless) and
also wrong on 64-bit architectures (second argument may be bigger
than u32).

I will prepare a v2 using DIV_ROUND_UP(), which should improve the
performance on 32-bit and fix the bug on 64-bit architectures.

-- Sebastian

> 	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)
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]