| Message ID | 20250123075321.4442-1-zuoqian113@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| 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
On Thu, Jan 23, 2025 at 04:04:13PM +0300, Dan Carpenter wrote: > 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 > Thank you for your valuable feedback.I will make the changes to the patch and resubmit it, including renaming freq and keeping it as an "unsigned int". Regards, Zuoqian
On Fri, Jan 24, 2025 at 09:42:01AM +0000, zuoqian wrote: > On Thu, Jan 23, 2025 at 04:04:13PM +0300, Dan Carpenter wrote: > > 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 > > > > Thank you for your valuable feedback.I will make the changes to the patch and > resubmit it, including renaming freq and keeping it as an "unsigned int". If you keep it as unsigned int then you will need to add a cast when you do the "* 1000" multiplication. Please make freq and rate both unsigned longs. 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(-)