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;
+	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;
-- 
2.43.0

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.

-- 
Regards,
Sudeep

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

The CPU rate from clk_get_rate() may not be divisible by 1000
(e.g., 133333333). But the rate calculated from frequency(kHz) 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 kHz here, the issue does not occur.

Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency")
Signed-off-by: zuoqian <zuoqian113@gmail.com>
---
V1 -> V2: rename freq to freq_khz, change rate to unsigned long, and
update patch summary.
---
 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..9e09565e41c0 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_khz = policy->freq_table[index].frequency;
 	struct scpi_data *priv = policy->driver_data;
+	unsigned long rate = freq_khz * 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_khz)
 		return -EIO;
 
 	return 0;
-- 
2.43.0

On Sat, Jan 25, 2025 at 08:49:49AM +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(kHz) 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 kHz here, the issue does not occur.
> 
> Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency")
> Signed-off-by: zuoqian <zuoqian113@gmail.com>
> ---
> V1 -> V2: rename freq to freq_khz, change rate to unsigned long, and
> update patch summary.

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

On Sat, Jan 25, 2025 at 08:49:49AM +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(kHz) 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 kHz here, the issue does not occur.
> 
> Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency")
> Signed-off-by: zuoqian <zuoqian113@gmail.com>
> ---
> V1 -> V2: rename freq to freq_khz, change rate to unsigned long, and
> update patch summary.

Thanks!

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter

On 25-01-25, 16:04, Dan Carpenter wrote:
> On Sat, Jan 25, 2025 at 08:49:49AM +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(kHz) 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 kHz here, the issue does not occur.
> > 
> > Fixes: 343a8d17fa8d ("cpufreq: scpi: remove arm_big_little dependency")
> > Signed-off-by: zuoqian <zuoqian113@gmail.com>
> > ---
> > V1 -> V2: rename freq to freq_khz, change rate to unsigned long, and
> > update patch summary.
> 
> Thanks!
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

Applied. Thanks.

-- 
viresh