| Message ID | 20230519190522.194729-3-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Fix 64 bit issues in common clock framework | expand |
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 > >
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)
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) >
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;
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(-)