Re: [PATCH RESEND 3/4] PM / devfreq: Fix governor_store() failing when device has no current governor

[Jie Zhan]

On 3/31/2026 5:30 PM, Yaxiong Tian wrote:
> 
> 在 2026/3/31 16:53, Jie Zhan 写道:
>>
>> On 3/31/2026 4:02 PM, Yaxiong Tian wrote:
>>> 在 2026/3/31 15:16, Jie Zhan 写道:
>>>> On 3/19/2026 5:17 PM, Yaxiong Tian wrote:
>>>>> Since devfreq_remove_governor() may clear the device's current governor
>>>>> in certain situations, while governors actually exist independently
>>>>> of the device, directly returning EINVAL in this case is inaccurate.
>>>>>
>>>>> To fix this issue, remove this check and add relevant logic for when
>>>>> df->governor is NULL.
>>>>>
>>>>> Fixes: 483d557ee9a3 ("PM / devfreq: Clean up the devfreq instance name in sysfs attr")
>>>>> Signed-off-by: Yaxiong Tian <tianyaxiong@xxxxxxxxxx>
>>>>> ---
>>>>>    drivers/devfreq/devfreq.c | 15 ++++++++++++---
>>>>>    1 file changed, 12 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index 0bf320123e3a..4a312f3c2421 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -1425,9 +1425,6 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>        char str_governor[DEVFREQ_NAME_LEN + 1];
>>>>>        const struct devfreq_governor *governor, *prev_governor;
>>>>>    -    if (!df->governor)
>>>>> -        return -EINVAL;
>>>>> -
>>>>>        ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>>>>>        if (ret != 1)
>>>>>            return -EINVAL;
>>>>> @@ -1438,6 +1435,18 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>>>>>            ret = PTR_ERR(governor);
>>>>>            goto out;
>>>>>        }
>>>>> +
>>>>> +    if (!df->governor) {
>>>>> +        df->governor = governor;
>>>>> +        ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>>>>> +        if (ret) {
>>>>> +            dev_warn(dev, "%s: Governor %s not started(%d)\n",
>>>>> +                __func__, df->governor->name, ret);
>>>>> +            df->governor = NULL;
>>>>> +        }
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>> The sequence that starts the governor, and stops, and then re-starts looks
>>>> quite weird.
>>>> Can you do a NULL pointer check before the IMMUTABLE flag check and
>>>> stopping governor, rather than this?
>>> This patch only addresses the issue raised in the commit message. As for the original
>>>
>>> start, stop, and then restart sequence, I haven't found any problems with it so far.
>>>
>> You just added the first 'start' here.
>> Please see the other process in governor_store().
> Could you explain what you mean?
> Wouldn't it be correct to directly operate on the devices without a governor first and then exit?


Ok, the code here starts the governor and directly goes to 'out' that
unlocks mutex, so create_sysfs_files() is skipped.
For example, setting 'userspace' as the governor won't create its belonging
files.

Therefore, I'd suggest that doing a NULL pointer check before the IMMUTABLE
flag check and stopping governor, where df->governor is used, so the
existing logic is not changed.

>>>>>        if (df->governor == governor) {
>>>>>            ret = 0;
>>>>>            goto out;