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@kylinos.cn>
---
 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;
+	}
+
 	if (df->governor == governor) {
 		ret = 0;
 		goto out;
-- 
2.25.1

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@kylinos.cn>
> ---
>  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?
>  	if (df->governor == governor) {
>  		ret = 0;
>  		goto out;

在 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@kylinos.cn>
>> ---
>>   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.

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

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@kylinos.cn>
>>> ---
>>>   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().
>>>       if (df->governor == governor) {
>>>           ret = 0;
>>>           goto out;

在 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@kylinos.cn>
>>>> ---
>>>>    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?
>>>>        if (df->governor == governor) {
>>>>            ret = 0;
>>>>            goto out;

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@kylinos.cn>
>>>>> ---
>>>>>    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;

在 2026/3/31 17:37, Jie Zhan 写道:
---skip------
> 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.

Yes, I missed that. However, we still need to check df->governor later 
and perform corresponding operations.
How about adding ret = sysfs_update_group(&df->dev.kobj, 
&gov_attr_group) based on the original code (against the latest linux-next)?

在 2026/3/31 17:54, Yaxiong Tian 写道:
>
> 在 2026/3/31 17:37, Jie Zhan 写道:
> ---skip------
>> 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.
>
> Yes, I missed that. However, we still need to check df->governor later 
> and perform corresponding operations.
> How about adding ret = sysfs_update_group(&df->dev.kobj, 
> &gov_attr_group) based on the original code (against the latest 
> linux-next)?
>
Sorry, please disregard the earlier part for the time being.
The line sysfs_update_group(&df->dev.kobj, &gov_attr_group); may not be 
needed, and I need to review it further.