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@xxxxxxxxxxxxxxxxxxxx wrote:
> > From: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > Acked-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> > Cc: Alexey Malahov <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>
> > Cc: Thomas Bogendoerfer <tsbogend@xxxxxxxxxxxxxxxx>
> > Cc: Paul Burton <paulburton@xxxxxxxxxx>
> > Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Cc: linux-mips@xxxxxxxxxxxxxxx
> > Cc: devicetree@xxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >   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!
> 
>