| Message ID | 20250123075321.4442-1-zuoqian113@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: scpi: compare against frequency instead of rate | expand |
On Thu, Jan 23, 2025 at 07:53:20AM +0000, zuoqian wrote: > The CPU rate from clk_get_rate() may not be divisible by 1000 > (e.g., 133333333). But the rate calculated from frequency is always > divisible by 1000 (e.g., 133333000). > Comparing the rate causes a warning during CPU scaling: > "cpufreq: __target_index: Failed to change cpu frequency: -5". > When we choose to compare frequency here, the issue does not occur. > > Signed-off-by: zuoqian <zuoqian113@gmail.com> > --- > drivers/cpufreq/scpi-cpufreq.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > index cd89c1b9832c..3bff4bb5ab4a 100644 > --- a/drivers/cpufreq/scpi-cpufreq.c > +++ b/drivers/cpufreq/scpi-cpufreq.c > @@ -39,8 +39,9 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) > static int > scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > { > - u64 rate = policy->freq_table[index].frequency * 1000; policy->freq_table[index].frequency is a u32 so in this original calculation, even though "rate" is declared as a u64, it can't actually be more than UINT_MAX. > + unsigned long freq = policy->freq_table[index].frequency; > struct scpi_data *priv = policy->driver_data; > + u64 rate = freq * 1000; So you've fixed this by casting policy->freq_table[index].frequency to unsigned long, which fixes the problem on 64bit systems but it still remains on 32bit systems. It would be better to declare freq as a u64. We keep fixing and then breaking this as undocumented parts of larger patches. :P It should really be done by itself and the Fixes tag would point to: Fixes: 1a0419b0db46 ("cpufreq: move invariance setter calls in cpufreq core") > int ret; > > ret = clk_set_rate(priv->clk, rate); > @@ -48,7 +49,7 @@ scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > if (ret) > return ret; > > - if (clk_get_rate(priv->clk) != rate) > + if (clk_get_rate(priv->clk) / 1000 != freq) Sure, I don't know this code well but your commit message seems reasonable. Add a Fixes tag for this line. Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency") regards, dan carpenter
(for some reason I don't have the original email) On Thu, Jan 23, 2025 at 02:12:14PM +0300, Dan Carpenter wrote: > On Thu, Jan 23, 2025 at 07:53:20AM +0000, zuoqian wrote: > > The CPU rate from clk_get_rate() may not be divisible by 1000 > > (e.g., 133333333). But the rate calculated from frequency is always > > divisible by 1000 (e.g., 133333000). > > Comparing the rate causes a warning during CPU scaling: > > "cpufreq: __target_index: Failed to change cpu frequency: -5". > > When we choose to compare frequency here, the issue does not occur. > > > > Signed-off-by: zuoqian <zuoqian113@gmail.com> > > --- > > drivers/cpufreq/scpi-cpufreq.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > > index cd89c1b9832c..3bff4bb5ab4a 100644 > > --- a/drivers/cpufreq/scpi-cpufreq.c > > +++ b/drivers/cpufreq/scpi-cpufreq.c > > @@ -39,8 +39,9 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) > > static int > > scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > > { > > - u64 rate = policy->freq_table[index].frequency * 1000; > > policy->freq_table[index].frequency is a u32 so in this original > calculation, even though "rate" is declared as a u64, it can't actually > be more than UINT_MAX. > Agreed and understood. > > + unsigned long freq = policy->freq_table[index].frequency; > > struct scpi_data *priv = policy->driver_data; > > + u64 rate = freq * 1000; > > So you've fixed this by casting policy->freq_table[index].frequency > to unsigned long, which fixes the problem on 64bit systems but it still > remains on 32bit systems. It would be better to declare freq as a u64. > Just trying to understand if that matters. freq is in kHz as copied from policy->freq_table[index].frequency and we compare it with kHZ below as the obtained clock rate is divided by 1000. What am I missing ? If it helps, it can be renamed as freq_in_khz and even keep it as "unsigned int" as in struct cpufreq_frequency_table.
On Thu, Jan 23, 2025 at 12:16:50PM +0000, Sudeep Holla wrote: > (for some reason I don't have the original email) > > On Thu, Jan 23, 2025 at 02:12:14PM +0300, Dan Carpenter wrote: > > On Thu, Jan 23, 2025 at 07:53:20AM +0000, zuoqian wrote: > > > The CPU rate from clk_get_rate() may not be divisible by 1000 > > > (e.g., 133333333). But the rate calculated from frequency is always > > > divisible by 1000 (e.g., 133333000). > > > Comparing the rate causes a warning during CPU scaling: > > > "cpufreq: __target_index: Failed to change cpu frequency: -5". > > > When we choose to compare frequency here, the issue does not occur. > > > > > > Signed-off-by: zuoqian <zuoqian113@gmail.com> > > > --- > > > drivers/cpufreq/scpi-cpufreq.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c > > > index cd89c1b9832c..3bff4bb5ab4a 100644 > > > --- a/drivers/cpufreq/scpi-cpufreq.c > > > +++ b/drivers/cpufreq/scpi-cpufreq.c > > > @@ -39,8 +39,9 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) > > > static int > > > scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > > > { > > > - u64 rate = policy->freq_table[index].frequency * 1000; > > > > policy->freq_table[index].frequency is a u32 so in this original > > calculation, even though "rate" is declared as a u64, it can't actually > > be more than UINT_MAX. > > > > Agreed and understood. > > > > + unsigned long freq = policy->freq_table[index].frequency; > > > struct scpi_data *priv = policy->driver_data; > > > + u64 rate = freq * 1000; > > > > So you've fixed this by casting policy->freq_table[index].frequency > > to unsigned long, which fixes the problem on 64bit systems but it still > > remains on 32bit systems. It would be better to declare freq as a u64. > > > > Just trying to understand if that matters. freq is in kHz as copied > from policy->freq_table[index].frequency and we compare it with > kHZ below as the obtained clock rate is divided by 1000. What am I > missing ? If it helps, it can be renamed as freq_in_khz and even keep > it as "unsigned int" as in struct cpufreq_frequency_table. > I misunderstood the integer overflow bug because I read too much into the fact that "rate" was declared as a u64. It would have been fine to declare it as a unsigned long. The cpufreq internals don't support anything more than ULONG_MAX. I have heard someone say that new systems are bumping up against the 4GHz limit but presumably that would only be high end 64bit systems, not old 32bit system. The ->freq_table[] frequency is in kHz so a u32 is fine. I guess if we get frequencies of a THz then we'll have to update that. But when we convert to Hz then we need a cast to avoid an integer overflow for systems which are over the 4GHz boundary. unsigned long rate = (unsigned long)khz * 1000; The second bug is that we need to compare kHz instead of Hz and that's straight forward. regards, dan carpenter
diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c index cd89c1b9832c..3bff4bb5ab4a 100644 --- a/drivers/cpufreq/scpi-cpufreq.c +++ b/drivers/cpufreq/scpi-cpufreq.c @@ -39,8 +39,9 @@ static unsigned int scpi_cpufreq_get_rate(unsigned int cpu) static int scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) { - u64 rate = policy->freq_table[index].frequency * 1000; + unsigned long freq = policy->freq_table[index].frequency; struct scpi_data *priv = policy->driver_data; + u64 rate = freq * 1000; int ret; ret = clk_set_rate(priv->clk, rate); @@ -48,7 +49,7 @@ scpi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) if (ret) return ret; - if (clk_get_rate(priv->clk) != rate) + if (clk_get_rate(priv->clk) / 1000 != freq) return -EIO; return 0;
The CPU rate from clk_get_rate() may not be divisible by 1000 (e.g., 133333333). But the rate calculated from frequency is always divisible by 1000 (e.g., 133333000). Comparing the rate causes a warning during CPU scaling: "cpufreq: __target_index: Failed to change cpu frequency: -5". When we choose to compare frequency here, the issue does not occur. Signed-off-by: zuoqian <zuoqian113@gmail.com> --- drivers/cpufreq/scpi-cpufreq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)