[PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Recent commit e61a41256edf ("cpufreq: dev_pm_qos_update_request() can
return 1 on success") fixed a problem when active policies traverse
was falsely stopped due to invalidly treating the non-zero return value
from freq_qos_update_request() method as an error. Yes, that function
can return positive values if the requested update actually took place.
The current problem is that the returned value is then passed to the
return cell of the cpufreq_boost_set_sw() (set_boost callback) method.
This value is then also analyzed for being non-zero, which is also
treated as having an error. As a result during the boost activation
we'll get an error returned while having the QOS frequency update
successfully performed. Fix this by returning a negative value from the
cpufreq_boost_set_sw() if actual error was encountered and zero
otherwise treating any positive values as the successful operations
completion.

Fixes: 18c49926c4bf ("cpufreq: Add QoS requests for userspace constraints")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-mips@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: stable@vger.kernel.org
---
 drivers/cpufreq/cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 045f9fe157ce..5870cdca88cf 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
 			break;
 	}
 
-	return ret;
+	return ret < 0 ? ret : 0;
 }
 
 int cpufreq_boost_trigger_state(int state)
-- 
2.25.1

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On 5/6/2020 7:42 PM, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>
> Recent commit e61a41256edf ("cpufreq: dev_pm_qos_update_request() can
> return 1 on success") fixed a problem when active policies traverse
> was falsely stopped due to invalidly treating the non-zero return value
> from freq_qos_update_request() method as an error. Yes, that function
> can return positive values if the requested update actually took place.
> The current problem is that the returned value is then passed to the
> return cell of the cpufreq_boost_set_sw() (set_boost callback) method.
> This value is then also analyzed for being non-zero, which is also
> treated as having an error. As a result during the boost activation
> we'll get an error returned while having the QOS frequency update
> successfully performed. Fix this by returning a negative value from the
> cpufreq_boost_set_sw() if actual error was encountered and zero
> otherwise treating any positive values as the successful operations
> completion.
>
> Fixes: 18c49926c4bf ("cpufreq: Add QoS requests for userspace constraints")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-mips@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: stable@vger.kernel.org
> ---
>   drivers/cpufreq/cpufreq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 045f9fe157ce..5870cdca88cf 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
>   			break;
>   	}
>   
> -	return ret;
> +	return ret < 0 ? ret : 0;
>   }
>   
>   int cpufreq_boost_trigger_state(int state)

IMO it is better to update the caller of this function to handle the 
positive value possibly returned by it correctly.

Thanks!

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Serge Semin]

Hello Rafael,

On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote:
> On 5/6/2020 7:42 PM, Sergey.Semin@baikalelectronics.ru wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > Recent commit e61a41256edf ("cpufreq: dev_pm_qos_update_request() can
> > return 1 on success") fixed a problem when active policies traverse
> > was falsely stopped due to invalidly treating the non-zero return value
> > from freq_qos_update_request() method as an error. Yes, that function
> > can return positive values if the requested update actually took place.
> > The current problem is that the returned value is then passed to the
> > return cell of the cpufreq_boost_set_sw() (set_boost callback) method.
> > This value is then also analyzed for being non-zero, which is also
> > treated as having an error. As a result during the boost activation
> > we'll get an error returned while having the QOS frequency update
> > successfully performed. Fix this by returning a negative value from the
> > cpufreq_boost_set_sw() if actual error was encountered and zero
> > otherwise treating any positive values as the successful operations
> > completion.
> > 
> > Fixes: 18c49926c4bf ("cpufreq: Add QoS requests for userspace constraints")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: linux-mips@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > ---
> >   drivers/cpufreq/cpufreq.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 045f9fe157ce..5870cdca88cf 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
> >   			break;
> >   	}
> > -	return ret;
> > +	return ret < 0 ? ret : 0;
> >   }
> >   int cpufreq_boost_trigger_state(int state)
> 
> IMO it is better to update the caller of this function to handle the
> positive value possibly returned by it correctly.

Could you elaborate why? Viresh seems to be ok with this solution.

As I see it the caller doesn't expect the positive value returned by the
original freq_qos_update_request(). It just doesn't need to know whether the
effective policy has been updated or not, it only needs to make sure the
operations has been successful. Moreover the positive value is related only
to the !last! active policy, which doesn't give the caller a full picture
of the policy change anyway. So taking all of these into account I'd leave the
fix as is.

Regards,
-Sergey

> 
> Thanks!
> 
> 

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Viresh Kumar]

On 16-05-20, 15:52, Serge Semin wrote:
> On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote:
> > > @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
> > >   			break;
> > >   	}
> > > -	return ret;
> > > +	return ret < 0 ? ret : 0;
> > >   }
> > >   int cpufreq_boost_trigger_state(int state)
> > 
> > IMO it is better to update the caller of this function to handle the
> > positive value possibly returned by it correctly.
> 
> Could you elaborate why? Viresh seems to be ok with this solution.

And it is absolutely fine for Rafael to not agree with it :)

> As I see it the caller doesn't expect the positive value returned by the
> original freq_qos_update_request(). It just doesn't need to know whether the
> effective policy has been updated or not, it only needs to make sure the
> operations has been successful. Moreover the positive value is related only
> to the !last! active policy, which doesn't give the caller a full picture
> of the policy change anyway. So taking all of these into account I'd leave the
> fix as is.

Rafael: This function is called via a function pointer, which can call
this or a platform dependent routine (like in acpi-cpufreq.c), and it
would be reasonable IMO for the return of that callback to only look
for 0 or negative values, as is generally done in the kernel. And so
this solution looked okay to me as the positive return is coming from
the implementation detail here.

-- 
viresh

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On 5/18/2020 9:41 AM, Viresh Kumar wrote:
> On 16-05-20, 15:52, Serge Semin wrote:
>> On Fri, May 15, 2020 at 05:58:47PM +0200, Rafael J. Wysocki wrote:
>>>> @@ -2554,7 +2554,7 @@ static int cpufreq_boost_set_sw(int state)
>>>>    			break;
>>>>    	}
>>>> -	return ret;
>>>> +	return ret < 0 ? ret : 0;
>>>>    }
>>>>    int cpufreq_boost_trigger_state(int state)
>>> IMO it is better to update the caller of this function to handle the
>>> positive value possibly returned by it correctly.
>> Could you elaborate why? Viresh seems to be ok with this solution.
> And it is absolutely fine for Rafael to not agree with it :)
>
>> As I see it the caller doesn't expect the positive value returned by the
>> original freq_qos_update_request(). It just doesn't need to know whether the
>> effective policy has been updated or not, it only needs to make sure the
>> operations has been successful. Moreover the positive value is related only
>> to the !last! active policy, which doesn't give the caller a full picture
>> of the policy change anyway. So taking all of these into account I'd leave the
>> fix as is.
> Rafael: This function is called via a function pointer, which can call
> this or a platform dependent routine (like in acpi-cpufreq.c), and it
> would be reasonable IMO for the return of that callback to only look
> for 0 or negative values, as is generally done in the kernel.

But it only has one caller that can easily check ret < 0 instead of just 
ret, so the extra branch can be saved.

That said if you really only want it to return 0 on success, you may as 
well add a ret = 0; statement (with a comment explaining why it is 
needed) after the last break in the loop.

Cheers!

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Viresh Kumar]

On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> That said if you really only want it to return 0 on success, you may as well
> add a ret = 0; statement (with a comment explaining why it is needed) after
> the last break in the loop.

That can be done as well, but will be a bit less efficient as the loop
will execute once for each policy, and so the statement will run
multiple times. Though it isn't going to add any significant latency
in the code.

-- 
viresh

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > That said if you really only want it to return 0 on success, you may as well
> > add a ret = 0; statement (with a comment explaining why it is needed) after
> > the last break in the loop.
> 
> That can be done as well, but will be a bit less efficient as the loop
> will execute once for each policy, and so the statement will run
> multiple times. Though it isn't going to add any significant latency
> in the code.

Right.

However, the logic in this entire function looks somewhat less than
straightforward to me, because it looks like it should return an
error on the first policy without a frequency table (having a frequency
table depends on the driver and that is the same for all policies, so it
is pointless to iterate any further in that case).

Also, the error should not be -EINVAL, because that means "invalid
argument" which would be the state value.

So I would do something like this:

---
 drivers/cpufreq/cpufreq.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
 static int cpufreq_boost_set_sw(int state)
 {
 	struct cpufreq_policy *policy;
-	int ret = -EINVAL;
 
 	for_each_active_policy(policy) {
+		int ret;
+
 		if (!policy->freq_table)
-			continue;
+			return -ENXIO;
 
 		ret = cpufreq_frequency_table_cpuinfo(policy,
 						      policy->freq_table);
 		if (ret) {
 			pr_err("%s: Policy frequency update failed\n",
 			       __func__);
-			break;
+			return ret;
 		}
 
 		ret = freq_qos_update_request(policy->max_freq_req, policy->max);
 		if (ret < 0)
-			break;
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 int cpufreq_boost_trigger_state(int state)

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Viresh Kumar]

On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > That said if you really only want it to return 0 on success, you may as well
> > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > the last break in the loop.
> > 
> > That can be done as well, but will be a bit less efficient as the loop
> > will execute once for each policy, and so the statement will run
> > multiple times. Though it isn't going to add any significant latency
> > in the code.
> 
> Right.
> 
> However, the logic in this entire function looks somewhat less than
> straightforward to me, because it looks like it should return an
> error on the first policy without a frequency table (having a frequency
> table depends on the driver and that is the same for all policies, so it
> is pointless to iterate any further in that case).
> 
> Also, the error should not be -EINVAL, because that means "invalid
> argument" which would be the state value.
> 
> So I would do something like this:
> 
> ---
>  drivers/cpufreq/cpufreq.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
>  static int cpufreq_boost_set_sw(int state)
>  {
>  	struct cpufreq_policy *policy;
> -	int ret = -EINVAL;
>  
>  	for_each_active_policy(policy) {
> +		int ret;
> +
>  		if (!policy->freq_table)
> -			continue;
> +			return -ENXIO;
>  
>  		ret = cpufreq_frequency_table_cpuinfo(policy,
>  						      policy->freq_table);
>  		if (ret) {
>  			pr_err("%s: Policy frequency update failed\n",
>  			       __func__);
> -			break;
> +			return ret;
>  		}
>  
>  		ret = freq_qos_update_request(policy->max_freq_req, policy->max);
>  		if (ret < 0)
> -			break;
> +			return ret;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  int cpufreq_boost_trigger_state(int state)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Serge Semin]

On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > That said if you really only want it to return 0 on success, you may as well
> > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > the last break in the loop.
> > > 
> > > That can be done as well, but will be a bit less efficient as the loop
> > > will execute once for each policy, and so the statement will run
> > > multiple times. Though it isn't going to add any significant latency
> > > in the code.
> > 
> > Right.
> > 
> > However, the logic in this entire function looks somewhat less than
> > straightforward to me, because it looks like it should return an
> > error on the first policy without a frequency table (having a frequency
> > table depends on the driver and that is the same for all policies, so it
> > is pointless to iterate any further in that case).
> > 
> > Also, the error should not be -EINVAL, because that means "invalid
> > argument" which would be the state value.
> > 
> > So I would do something like this:
> > 
> > ---
> >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> >  static int cpufreq_boost_set_sw(int state)
> >  {
> >  	struct cpufreq_policy *policy;
> > -	int ret = -EINVAL;
> >  
> >  	for_each_active_policy(policy) {
> > +		int ret;
> > +
> >  		if (!policy->freq_table)
> > -			continue;
> > +			return -ENXIO;
> >  
> >  		ret = cpufreq_frequency_table_cpuinfo(policy,
> >  						      policy->freq_table);
> >  		if (ret) {
> >  			pr_err("%s: Policy frequency update failed\n",
> >  			       __func__);
> > -			break;
> > +			return ret;
> >  		}
> >  
> >  		ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> >  		if (ret < 0)
> > -			break;
> > +			return ret;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  int cpufreq_boost_trigger_state(int state)
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Ok. Thanks for the comments. Shall I resend the patch with update Rafael
suggests or you'll merge the Rafael's fix in yourself?

-Sergey
> 
> -- 
> viresh

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > the last break in the loop.
> > > > 
> > > > That can be done as well, but will be a bit less efficient as the loop
> > > > will execute once for each policy, and so the statement will run
> > > > multiple times. Though it isn't going to add any significant latency
> > > > in the code.
> > > 
> > > Right.
> > > 
> > > However, the logic in this entire function looks somewhat less than
> > > straightforward to me, because it looks like it should return an
> > > error on the first policy without a frequency table (having a frequency
> > > table depends on the driver and that is the same for all policies, so it
> > > is pointless to iterate any further in that case).
> > > 
> > > Also, the error should not be -EINVAL, because that means "invalid
> > > argument" which would be the state value.
> > > 
> > > So I would do something like this:
> > > 
> > > ---
> > >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > 
> > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > ===================================================================
> > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > >  static int cpufreq_boost_set_sw(int state)
> > >  {
> > >  	struct cpufreq_policy *policy;
> > > -	int ret = -EINVAL;
> > >  
> > >  	for_each_active_policy(policy) {
> > > +		int ret;
> > > +
> > >  		if (!policy->freq_table)
> > > -			continue;
> > > +			return -ENXIO;
> > >  
> > >  		ret = cpufreq_frequency_table_cpuinfo(policy,
> > >  						      policy->freq_table);
> > >  		if (ret) {
> > >  			pr_err("%s: Policy frequency update failed\n",
> > >  			       __func__);
> > > -			break;
> > > +			return ret;
> > >  		}
> > >  
> > >  		ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > >  		if (ret < 0)
> > > -			break;
> > > +			return ret;
> > >  	}
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  }
> > >  
> > >  int cpufreq_boost_trigger_state(int state)
> > 
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> suggests or you'll merge the Rafael's fix in yourself?

I'll apply the fix directly, thanks!

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Serge Semin]

On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > the last break in the loop.
> > > > > 
> > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > will execute once for each policy, and so the statement will run
> > > > > multiple times. Though it isn't going to add any significant latency
> > > > > in the code.
> > > > 
> > > > Right.
> > > > 
> > > > However, the logic in this entire function looks somewhat less than
> > > > straightforward to me, because it looks like it should return an
> > > > error on the first policy without a frequency table (having a frequency
> > > > table depends on the driver and that is the same for all policies, so it
> > > > is pointless to iterate any further in that case).
> > > > 
> > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > argument" which would be the state value.
> > > > 
> > > > So I would do something like this:
> > > > 
> > > > ---
> > > >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > 
> > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > ===================================================================
> > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > >  static int cpufreq_boost_set_sw(int state)
> > > >  {
> > > >  	struct cpufreq_policy *policy;
> > > > -	int ret = -EINVAL;
> > > >  
> > > >  	for_each_active_policy(policy) {
> > > > +		int ret;
> > > > +
> > > >  		if (!policy->freq_table)
> > > > -			continue;
> > > > +			return -ENXIO;
> > > >  
> > > >  		ret = cpufreq_frequency_table_cpuinfo(policy,
> > > >  						      policy->freq_table);
> > > >  		if (ret) {
> > > >  			pr_err("%s: Policy frequency update failed\n",
> > > >  			       __func__);
> > > > -			break;
> > > > +			return ret;
> > > >  		}
> > > >  
> > > >  		ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > >  		if (ret < 0)
> > > > -			break;
> > > > +			return ret;
> > > >  	}
> > > >  
> > > > -	return ret;
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  int cpufreq_boost_trigger_state(int state)
> > > 
> > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > 
> > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > suggests or you'll merge the Rafael's fix in yourself?
> 
> I'll apply the fix directly, thanks!

Great. Is it going to be available in the repo:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
?
I'll need it to back port into my local kernel tree. Thanks.

-Sergey

> 
> 
> 

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On Mon, May 18, 2020 at 12:46 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > > the last break in the loop.
> > > > > >
> > > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > > will execute once for each policy, and so the statement will run
> > > > > > multiple times. Though it isn't going to add any significant latency
> > > > > > in the code.
> > > > >
> > > > > Right.
> > > > >
> > > > > However, the logic in this entire function looks somewhat less than
> > > > > straightforward to me, because it looks like it should return an
> > > > > error on the first policy without a frequency table (having a frequency
> > > > > table depends on the driver and that is the same for all policies, so it
> > > > > is pointless to iterate any further in that case).
> > > > >
> > > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > > argument" which would be the state value.
> > > > >
> > > > > So I would do something like this:
> > > > >
> > > > > ---
> > > > >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> > > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > >
> > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > > ===================================================================
> > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > > >  static int cpufreq_boost_set_sw(int state)
> > > > >  {
> > > > >         struct cpufreq_policy *policy;
> > > > > -       int ret = -EINVAL;
> > > > >
> > > > >         for_each_active_policy(policy) {
> > > > > +               int ret;
> > > > > +
> > > > >                 if (!policy->freq_table)
> > > > > -                       continue;
> > > > > +                       return -ENXIO;
> > > > >
> > > > >                 ret = cpufreq_frequency_table_cpuinfo(policy,
> > > > >                                                       policy->freq_table);
> > > > >                 if (ret) {
> > > > >                         pr_err("%s: Policy frequency update failed\n",
> > > > >                                __func__);
> > > > > -                       break;
> > > > > +                       return ret;
> > > > >                 }
> > > > >
> > > > >                 ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > > >                 if (ret < 0)
> > > > > -                       break;
> > > > > +                       return ret;
> > > > >         }
> > > > >
> > > > > -       return ret;
> > > > > +       return 0;
> > > > >  }
> > > > >
> > > > >  int cpufreq_boost_trigger_state(int state)
> > > >
> > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > >
> > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > > suggests or you'll merge the Rafael's fix in yourself?
> >
> > I'll apply the fix directly, thanks!
>
> Great. Is it going to be available in the repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> ?

Yes, it is.  Please see the bleeding-edge branch in there, thanks!

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Serge Semin]

On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote:
> On Mon, May 18, 2020 at 12:46 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> >
> > On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> > > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > > > the last break in the loop.
> > > > > > >
> > > > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > > > will execute once for each policy, and so the statement will run
> > > > > > > multiple times. Though it isn't going to add any significant latency
> > > > > > > in the code.
> > > > > >
> > > > > > Right.
> > > > > >
> > > > > > However, the logic in this entire function looks somewhat less than
> > > > > > straightforward to me, because it looks like it should return an
> > > > > > error on the first policy without a frequency table (having a frequency
> > > > > > table depends on the driver and that is the same for all policies, so it
> > > > > > is pointless to iterate any further in that case).
> > > > > >
> > > > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > > > argument" which would be the state value.
> > > > > >
> > > > > > So I would do something like this:
> > > > > >
> > > > > > ---
> > > > > >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> > > > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > > > ===================================================================
> > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > > > >  static int cpufreq_boost_set_sw(int state)
> > > > > >  {
> > > > > >         struct cpufreq_policy *policy;
> > > > > > -       int ret = -EINVAL;
> > > > > >
> > > > > >         for_each_active_policy(policy) {
> > > > > > +               int ret;
> > > > > > +
> > > > > >                 if (!policy->freq_table)
> > > > > > -                       continue;
> > > > > > +                       return -ENXIO;
> > > > > >
> > > > > >                 ret = cpufreq_frequency_table_cpuinfo(policy,
> > > > > >                                                       policy->freq_table);
> > > > > >                 if (ret) {
> > > > > >                         pr_err("%s: Policy frequency update failed\n",
> > > > > >                                __func__);
> > > > > > -                       break;
> > > > > > +                       return ret;
> > > > > >                 }
> > > > > >
> > > > > >                 ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > > > >                 if (ret < 0)
> > > > > > -                       break;
> > > > > > +                       return ret;
> > > > > >         }
> > > > > >
> > > > > > -       return ret;
> > > > > > +       return 0;
> > > > > >  }
> > > > > >
> > > > > >  int cpufreq_boost_trigger_state(int state)
> > > > >
> > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > >
> > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > > > suggests or you'll merge the Rafael's fix in yourself?
> > >
> > > I'll apply the fix directly, thanks!
> >
> > Great. Is it going to be available in the repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> > ?
> 
> Yes, it is.  Please see the bleeding-edge branch in there, thanks!

No credits with at least Reported-by tag? That's sad.(

-Sergey

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Rafael J. Wysocki]

On Mon, May 18, 2020 at 12:56 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
>
> On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote:
> > On Mon, May 18, 2020 at 12:46 PM Serge Semin
> > <Sergey.Semin@baikalelectronics.ru> wrote:
> > >
> > > On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
> > > > On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
> > > > > On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
> > > > > > On 18-05-20, 12:22, Rafael J. Wysocki wrote:
> > > > > > > On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
> > > > > > > > On 18-05-20, 11:53, Rafael J. Wysocki wrote:
> > > > > > > > > That said if you really only want it to return 0 on success, you may as well
> > > > > > > > > add a ret = 0; statement (with a comment explaining why it is needed) after
> > > > > > > > > the last break in the loop.
> > > > > > > >
> > > > > > > > That can be done as well, but will be a bit less efficient as the loop
> > > > > > > > will execute once for each policy, and so the statement will run
> > > > > > > > multiple times. Though it isn't going to add any significant latency
> > > > > > > > in the code.
> > > > > > >
> > > > > > > Right.
> > > > > > >
> > > > > > > However, the logic in this entire function looks somewhat less than
> > > > > > > straightforward to me, because it looks like it should return an
> > > > > > > error on the first policy without a frequency table (having a frequency
> > > > > > > table depends on the driver and that is the same for all policies, so it
> > > > > > > is pointless to iterate any further in that case).
> > > > > > >
> > > > > > > Also, the error should not be -EINVAL, because that means "invalid
> > > > > > > argument" which would be the state value.
> > > > > > >
> > > > > > > So I would do something like this:
> > > > > > >
> > > > > > > ---
> > > > > > >  drivers/cpufreq/cpufreq.c |   11 ++++++-----
> > > > > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > > > >
> > > > > > > Index: linux-pm/drivers/cpufreq/cpufreq.c
> > > > > > > ===================================================================
> > > > > > > --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> > > > > > > +++ linux-pm/drivers/cpufreq/cpufreq.c
> > > > > > > @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
> > > > > > >  static int cpufreq_boost_set_sw(int state)
> > > > > > >  {
> > > > > > >         struct cpufreq_policy *policy;
> > > > > > > -       int ret = -EINVAL;
> > > > > > >
> > > > > > >         for_each_active_policy(policy) {
> > > > > > > +               int ret;
> > > > > > > +
> > > > > > >                 if (!policy->freq_table)
> > > > > > > -                       continue;
> > > > > > > +                       return -ENXIO;
> > > > > > >
> > > > > > >                 ret = cpufreq_frequency_table_cpuinfo(policy,
> > > > > > >                                                       policy->freq_table);
> > > > > > >                 if (ret) {
> > > > > > >                         pr_err("%s: Policy frequency update failed\n",
> > > > > > >                                __func__);
> > > > > > > -                       break;
> > > > > > > +                       return ret;
> > > > > > >                 }
> > > > > > >
> > > > > > >                 ret = freq_qos_update_request(policy->max_freq_req, policy->max);
> > > > > > >                 if (ret < 0)
> > > > > > > -                       break;
> > > > > > > +                       return ret;
> > > > > > >         }
> > > > > > >
> > > > > > > -       return ret;
> > > > > > > +       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > >  int cpufreq_boost_trigger_state(int state)
> > > > > >
> > > > > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > > >
> > > > > Ok. Thanks for the comments. Shall I resend the patch with update Rafael
> > > > > suggests or you'll merge the Rafael's fix in yourself?
> > > >
> > > > I'll apply the fix directly, thanks!
> > >
> > > Great. Is it going to be available in the repo:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
> > > ?
> >
> > Yes, it is.  Please see the bleeding-edge branch in there, thanks!
>
> No credits with at least Reported-by tag? That's sad.(

OK, done now, but you are not the only reported of it, so I've added
the other reporter too.

Thanks!

Re: [PATCH v2 20/20] cpufreq: Return zero on success in boost sw setting

[Xiongfeng Wang]

Hi Rafael,

On 2020/5/18 19:05, Rafael J. Wysocki wrote:
> On Mon, May 18, 2020 at 12:56 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
>>
>> On Mon, May 18, 2020 at 12:51:15PM +0200, Rafael J. Wysocki wrote:
>>> On Mon, May 18, 2020 at 12:46 PM Serge Semin
>>> <Sergey.Semin@baikalelectronics.ru> wrote:
>>>>
>>>> On Mon, May 18, 2020 at 12:41:19PM +0200, Rafael J. Wysocki wrote:
>>>>> On Monday, May 18, 2020 12:31:02 PM CEST Serge Semin wrote:
>>>>>> On Mon, May 18, 2020 at 03:54:15PM +0530, Viresh Kumar wrote:
>>>>>>> On 18-05-20, 12:22, Rafael J. Wysocki wrote:
>>>>>>>> On Monday, May 18, 2020 12:11:09 PM CEST Viresh Kumar wrote:
>>>>>>>>> On 18-05-20, 11:53, Rafael J. Wysocki wrote:
>>>>>>>>>> That said if you really only want it to return 0 on success, you may as well
>>>>>>>>>> add a ret = 0; statement (with a comment explaining why it is needed) after
>>>>>>>>>> the last break in the loop.
>>>>>>>>>
>>>>>>>>> That can be done as well, but will be a bit less efficient as the loop
>>>>>>>>> will execute once for each policy, and so the statement will run
>>>>>>>>> multiple times. Though it isn't going to add any significant latency
>>>>>>>>> in the code.
>>>>>>>>
>>>>>>>> Right.
>>>>>>>>
>>>>>>>> However, the logic in this entire function looks somewhat less than
>>>>>>>> straightforward to me, because it looks like it should return an
>>>>>>>> error on the first policy without a frequency table (having a frequency
>>>>>>>> table depends on the driver and that is the same for all policies, so it
>>>>>>>> is pointless to iterate any further in that case).
>>>>>>>>
>>>>>>>> Also, the error should not be -EINVAL, because that means "invalid
>>>>>>>> argument" which would be the state value.
>>>>>>>>
>>>>>>>> So I would do something like this:
>>>>>>>>
>>>>>>>> ---
>>>>>>>>  drivers/cpufreq/cpufreq.c |   11 ++++++-----
>>>>>>>>  1 file changed, 6 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> Index: linux-pm/drivers/cpufreq/cpufreq.c
>>>>>>>> ===================================================================
>>>>>>>> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
>>>>>>>> +++ linux-pm/drivers/cpufreq/cpufreq.c
>>>>>>>> @@ -2535,26 +2535,27 @@ EXPORT_SYMBOL_GPL(cpufreq_update_limits)
>>>>>>>>  static int cpufreq_boost_set_sw(int state)
>>>>>>>>  {
>>>>>>>>         struct cpufreq_policy *policy;
>>>>>>>> -       int ret = -EINVAL;
>>>>>>>>
>>>>>>>>         for_each_active_policy(policy) {
>>>>>>>> +               int ret;
>>>>>>>> +
>>>>>>>>                 if (!policy->freq_table)
>>>>>>>> -                       continue;
>>>>>>>> +                       return -ENXIO;
>>>>>>>>
>>>>>>>>                 ret = cpufreq_frequency_table_cpuinfo(policy,
>>>>>>>>                                                       policy->freq_table);
>>>>>>>>                 if (ret) {
>>>>>>>>                         pr_err("%s: Policy frequency update failed\n",
>>>>>>>>                                __func__);
>>>>>>>> -                       break;
>>>>>>>> +                       return ret;
>>>>>>>>                 }
>>>>>>>>
>>>>>>>>                 ret = freq_qos_update_request(policy->max_freq_req, policy->max);
>>>>>>>>                 if (ret < 0)
>>>>>>>> -                       break;
>>>>>>>> +                       return ret;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -       return ret;
>>>>>>>> +       return 0;
>>>>>>>>  }
>>>>>>>>
>>>>>>>>  int cpufreq_boost_trigger_state(int state)
>>>>>>>
>>>>>>> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>
>>>>>> Ok. Thanks for the comments. Shall I resend the patch with update Rafael
>>>>>> suggests or you'll merge the Rafael's fix in yourself?
>>>>>
>>>>> I'll apply the fix directly, thanks!
>>>>
>>>> Great. Is it going to be available in the repo:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/
>>>> ?
>>>
>>> Yes, it is.  Please see the bleeding-edge branch in there, thanks!

Thanks for CCing me. I will write my next version based on this branch.

Thanks,
Xiongfeng

>>
>> No credits with at least Reported-by tag? That's sad.(
> 
> OK, done now, but you are not the only reported of it, so I've added
> the other reporter too.
> 
> Thanks!
> 
> .
>