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!
