Re: [PATCH] cpufreq: Preserve global boost state on failed updates

[Christian Loehle <christian.loehle@arm.com>]

On 11/5/26 08:41, Zhongqiu Han wrote:
> On 11/5/2026 12:16 AM, Christian Loehle wrote:
>> cpufreq_boost_trigger_state() restores the opposite of the requested
>> state when updating policy boost state fails. For example, a failed
>> write of 0 when global boost is already disabled enables the global
>> flag without enabling any policies.
>>
>> Save the original global boost state using a mutex and restore it on
>> failure.
>>
>> Fixes: 121baab7b88e ("cpufreq: Force sync policy boost with global boost on sysfs update")
>> Fixes: 6f19efc0a1ca ("cpufreq: Add boost frequency support in core")
> 
> It seems the only appropriate Fixes tag here is 121baab7b88e ("cpufreq:
> Force sync policy boost with global boost on sysfs update"). Commit
> 6f19efc0a1ca ("cpufreq: Add boost frequency support in core") did not
> have this issue, since it returned early when the state was unchanged:
> 
>     if (cpufreq_driver->boost_enabled == state)
>         return 0;

Yes, that wouldn't flip it, although still race between sysfs writers.

> 
> so restoring "!state" on failure was equivalent to restoring the old
> value. 121baab7b88e removed that check, which is what introduced the
> bug.
> 
> Also, is the new mutex meant to address a separate race between
> concurrent sysfs writers? If so, it would be good to mention that in the
> commit message or using separate patch?

Yes indeed, otherwise we could fold it into cpufreq_driver_lock.
I can split the two including the Fixes tag

> 
>> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
>> ---
>> On a platform without boost_supported the unfixed behaviour is:
>> # cat /sys/devices/system/cpu/cpufreq/boost
>> 0
>> # echo 1 > /sys/devices/system/cpu/cpufreq/boost
>> echo: write error: invalid argument
>> # cat /sys/devices/system/cpu/cpufreq/boost
>> 0
>> # echo 0 > /sys/devices/system/cpu/cpufreq/boost
>> echo: write error: invalid argument
>> # cat /sys/devices/system/cpu/cpufreq/boost
>> 1
>>
>>   drivers/cpufreq/cpufreq.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 54dde8419bdc..d95c849f79c5 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -2810,18 +2810,24 @@ int cpufreq_boost_set_sw(struct cpufreq_policy *policy, int state)
>>   }
>>   EXPORT_SYMBOL_GPL(cpufreq_boost_set_sw);
>>   +static DEFINE_MUTEX(cpufreq_boost_mutex);
>> +
>>   static int cpufreq_boost_trigger_state(int state)
>>   {
>>       struct cpufreq_policy *policy;
>>       unsigned long flags;
>> +    bool boost_enabled;
>>       int ret = -EOPNOTSUPP;
>>   +    guard(mutex)(&cpufreq_boost_mutex);
>> +
>>       /*
>>        * Don't compare 'cpufreq_driver->boost_enabled' with 'state' here to
>>        * make sure all policies are in sync with global boost flag.
>>        */
>>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>> +    boost_enabled = cpufreq_driver->boost_enabled;
>>       cpufreq_driver->boost_enabled = state;
>>       write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>   @@ -2841,7 +2847,7 @@ static int cpufreq_boost_trigger_state(int state)
>>           return 0;
>>         write_lock_irqsave(&cpufreq_driver_lock, flags);
>> -    cpufreq_driver->boost_enabled = !state;
>> +    cpufreq_driver->boost_enabled = boost_enabled;
>>       write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>>         pr_err("%s: Cannot %s BOOST\n",
> 
>