[PATCH RESEND 0/4] Fix some errors in the devfreq core layer when governor is NULL

[PATCH RESEND 0/4] Fix some errors in the devfreq core layer when governor is NULL

[Yaxiong Tian]

While doing some development work with devfreq_add_governor()/
devfreq_remove_governor(), I discovered several bugs caused when
devfreq->governor is NULL. Specifically:

1) A possible null pointer issue in devfreq_add_governor(), caused
by devfreq_remove_governor() setting devfreq->governor to NULL in
certain situations, while devfreq_add_governor() lacks corresponding
checks for devfreq->governor.

2) When operating on governor and available_governors under /sys,
there are also some unexpected errors.

See the following patches for details.

Yaxiong Tian (4):
  PM / devfreq: Fix possible null pointer issue in
    devfreq_add_governor()
  PM / devfreq: Fix available_governors_show() when no governor is set
  PM / devfreq: Fix governor_store() failing when device has no current
    governor
  PM / devfreq: Optimize error return value of governor_show()

 drivers/devfreq/devfreq.c | 46 +++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

-- 
2.25.1

[PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Yaxiong Tian]

When a user removes a governor using devfreq_remove_governor(), if
the current device is using this governor, devfreq->governor will
be set to NULL. When the user registers any governor
using devfreq_add_governor(), since devfreq->governor is NULL, a
null pointer error occurs in strncmp().

For example: A user loads the userspace gov through a module, then
a device selects userspace. When unloading the userspace module and
then loading it again, the null pointer error occurs:

Unable to handle kernel NULL pointer dereference at virtual address
0000000000000010
Mem abort info:
ESR = 0x0000000096000004
EC = 0x25: DABT (current EL), IL = 32 bits
*******************skip *********************
Call trace:
__pi_strncmp+0x20/0x1b8
devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
do_one_initcall+0x4c/0x278
do_init_module+0x5c/0x218
load_module+0x1f1c/0x1fc8
init_module_from_file+0x8c/0xd0
__arm64_sys_finit_module+0x220/0x3d8
invoke_syscall+0x48/0x110
el0_svc_common.constprop.0+0xbc/0xe8
do_el0_svc+0x20/0x30
el0_svc+0x24/0xb8
el0t_64_sync_handler+0xb8/0xc0
el0t_64_sync+0x14c/0x150

To fix this issue, modify the relevant logic in devfreq_add_governor():
Only check whether the new governor matches the existing one when
devfreq->governor exists. When devfreq->governor is NULL, directly
select the new governor and perform the DEVFREQ_GOV_START operation.

Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
---
 drivers/devfreq/devfreq.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 54f0b18536db..63ce6e25abe2 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
 		int ret = 0;
 		struct device *dev = devfreq->dev.parent;
 
-		if (!strncmp(devfreq->governor->name, governor->name,
+		if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
 			     DEVFREQ_NAME_LEN)) {
 			/* The following should never occur */
-			if (devfreq->governor) {
+			dev_warn(dev,
+				 "%s: Governor %s already present\n",
+				 __func__, devfreq->governor->name);
+			ret = devfreq->governor->event_handler(devfreq,
+						DEVFREQ_GOV_STOP, NULL);
+			if (ret) {
 				dev_warn(dev,
-					 "%s: Governor %s already present\n",
-					 __func__, devfreq->governor->name);
-				ret = devfreq->governor->event_handler(devfreq,
-							DEVFREQ_GOV_STOP, NULL);
-				if (ret) {
-					dev_warn(dev,
-						 "%s: Governor %s stop = %d\n",
-						 __func__,
-						 devfreq->governor->name, ret);
-				}
-				/* Fall through */
+					 "%s: Governor %s stop = %d\n",
+					 __func__,
+					 devfreq->governor->name, ret);
 			}
+		} else if (!devfreq->governor) {
 			devfreq->governor = governor;
 			ret = devfreq->governor->event_handler(devfreq,
 						DEVFREQ_GOV_START, NULL);
-- 
2.25.1

[PATCH RESEND 2/4] PM / devfreq: Fix available_governors_show() when no governor is set

[Yaxiong Tian]

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 use df->governor for validity
verification in the following code.

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 | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 63ce6e25abe2..0bf320123e3a 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1504,16 +1504,13 @@ static ssize_t available_governors_show(struct device *d,
 	struct devfreq *df = to_devfreq(d);
 	ssize_t count = 0;
 
-	if (!df->governor)
-		return -EINVAL;
-
 	mutex_lock(&devfreq_list_lock);
 
 	/*
 	 * The devfreq with immutable governor (e.g., passive) shows
 	 * only own governor.
 	 */
-	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
+	if (df->governor && IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
 		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
 				  "%s ", df->governor->name);
 	/*
-- 
2.25.1

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

[Yaxiong Tian]

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

[PATCH RESEND 4/4] PM / devfreq: Optimize error return value of governor_show()

[Yaxiong Tian]

When df->governor is NULL, governor_show() returns -EINVAL, which
confuses users.

To fix this issue, return -ENOENT to indicate that no governor is
currently set for the device.

Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 4a312f3c2421..7cc60711fafd 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1412,7 +1412,7 @@ static ssize_t governor_show(struct device *dev,
 	struct devfreq *df = to_devfreq(dev);
 
 	if (!df->governor)
-		return -EINVAL;
+		return -ENOENT;
 
 	return sprintf(buf, "%s\n", df->governor->name);
 }
-- 
2.25.1

Re: [PATCH RESEND 0/4] Fix some errors in the devfreq core layer when governor is NULL

[Jie Zhan]

On 3/19/2026 5:14 PM, Yaxiong Tian wrote:
> While doing some development work with devfreq_add_governor()/
> devfreq_remove_governor(), I discovered several bugs caused when
> devfreq->governor is NULL. Specifically:
> 
> 1) A possible null pointer issue in devfreq_add_governor(), caused
> by devfreq_remove_governor() setting devfreq->governor to NULL in
> certain situations, while devfreq_add_governor() lacks corresponding
> checks for devfreq->governor.
> 
> 2) When operating on governor and available_governors under /sys,
> there are also some unexpected errors.
> 
> See the following patches for details.
Hi Yaxiong,

Sorry for catching this late.

I happen to be working on the same issues recently on a local tree, though
delayed for quite a while due to other business.

It would be a totally different approach by reworking the locking logic,
which, I supposed, can make the devfreq framework cleaner to maintain.

I'll send it out shortly.  Hopefully it can be a reference for us to
discuss with.

Thanks!
Jie
> 
> Yaxiong Tian (4):
>   PM / devfreq: Fix possible null pointer issue in
>     devfreq_add_governor()
>   PM / devfreq: Fix available_governors_show() when no governor is set
>   PM / devfreq: Fix governor_store() failing when device has no current
>     governor
>   PM / devfreq: Optimize error return value of governor_show()
> 
>  drivers/devfreq/devfreq.c | 46 +++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
> 

Re: [PATCH RESEND 0/4] Fix some errors in the devfreq core layer when governor is NULL

[Yaxiong Tian]

在 2026/3/25 18:06, Jie Zhan 写道:
>
> On 3/19/2026 5:14 PM, Yaxiong Tian wrote:
>> While doing some development work with devfreq_add_governor()/
>> devfreq_remove_governor(), I discovered several bugs caused when
>> devfreq->governor is NULL. Specifically:
>>
>> 1) A possible null pointer issue in devfreq_add_governor(), caused
>> by devfreq_remove_governor() setting devfreq->governor to NULL in
>> certain situations, while devfreq_add_governor() lacks corresponding
>> checks for devfreq->governor.
>>
>> 2) When operating on governor and available_governors under /sys,
>> there are also some unexpected errors.
>>
>> See the following patches for details.
> Hi Yaxiong,
>
> Sorry for catching this late.
>
> I happen to be working on the same issues recently on a local tree, though
> delayed for quite a while due to other business.
>
> It would be a totally different approach by reworking the locking logic,
> which, I supposed, can make the devfreq framework cleaner to maintain.
>
> I'll send it out shortly.  Hopefully it can be a reference for us to
> discuss with.
>
> Thanks!
> Jie

Hi  Jie
	Thank you for your reply. I saw your PATCH in the public linked list, and I think your optimization work, such as the lock mechanism improvements, should be placed after my FIX. This will make it easier to backport the patches and fix lower-version kernels.
	I see the patch is already in the devfreq-testing branch. I hope this work can be pushed forward as soon as possible to resolve the null pointer and other bugs.
	Finally, if you have any further optimizations, remember to CC me on the patches. :)

>> Yaxiong Tian (4):
>>    PM / devfreq: Fix possible null pointer issue in
>>      devfreq_add_governor()
>>    PM / devfreq: Fix available_governors_show() when no governor is set
>>    PM / devfreq: Fix governor_store() failing when device has no current
>>      governor
>>    PM / devfreq: Optimize error return value of governor_show()
>>
>>   drivers/devfreq/devfreq.c | 46 +++++++++++++++++++++------------------
>>   1 file changed, 25 insertions(+), 21 deletions(-)
>>

Re: [PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Jie Zhan]

On 3/19/2026 5:16 PM, Yaxiong Tian wrote:
> When a user removes a governor using devfreq_remove_governor(), if
> the current device is using this governor, devfreq->governor will
> be set to NULL. When the user registers any governor
> using devfreq_add_governor(), since devfreq->governor is NULL, a
> null pointer error occurs in strncmp().
> 
> For example: A user loads the userspace gov through a module, then
> a device selects userspace. When unloading the userspace module and
> then loading it again, the null pointer error occurs:
> 
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000010
> Mem abort info:
> ESR = 0x0000000096000004
> EC = 0x25: DABT (current EL), IL = 32 bits
> *******************skip *********************
> Call trace:
> __pi_strncmp+0x20/0x1b8
> devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
> do_one_initcall+0x4c/0x278
> do_init_module+0x5c/0x218
> load_module+0x1f1c/0x1fc8
> init_module_from_file+0x8c/0xd0
> __arm64_sys_finit_module+0x220/0x3d8
> invoke_syscall+0x48/0x110
> el0_svc_common.constprop.0+0xbc/0xe8
> do_el0_svc+0x20/0x30
> el0_svc+0x24/0xb8
> el0t_64_sync_handler+0xb8/0xc0
> el0t_64_sync+0x14c/0x150
> 
> To fix this issue, modify the relevant logic in devfreq_add_governor():
> Only check whether the new governor matches the existing one when
> devfreq->governor exists. When devfreq->governor is NULL, directly
> select the new governor and perform the DEVFREQ_GOV_START operation.
> 
> Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
> ---
>  drivers/devfreq/devfreq.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 54f0b18536db..63ce6e25abe2 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>  		int ret = 0;
>  		struct device *dev = devfreq->dev.parent;
>  
> -		if (!strncmp(devfreq->governor->name, governor->name,
> +		if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
>  			     DEVFREQ_NAME_LEN)) {

Probaly do:
if (!devfreq->governor)
	continue;
so as to keep the line short and the "else if (!devfreq->governor) {" below
can be removed.

and also:
if (!strncmp(devfreq->governor->name, governor->name,
	DEVFREQ_NAME_LEN))
	continue;
so we don't have to indent that much.

>  			/* The following should never occur */
> -			if (devfreq->governor) {
> +			dev_warn(dev,
> +				 "%s: Governor %s already present\n",
> +				 __func__, devfreq->governor->name);
> +			ret = devfreq->governor->event_handler(devfreq,
> +						DEVFREQ_GOV_STOP, NULL);
> +			if (ret) {
>  				dev_warn(dev,
> -					 "%s: Governor %s already present\n",
> -					 __func__, devfreq->governor->name);
> -				ret = devfreq->governor->event_handler(devfreq,
> -							DEVFREQ_GOV_STOP, NULL);
> -				if (ret) {
> -					dev_warn(dev,
> -						 "%s: Governor %s stop = %d\n",
> -						 __func__,
> -						 devfreq->governor->name, ret);
> -				}
> -				/* Fall through */
> +					 "%s: Governor %s stop = %d\n",
> +					 __func__,
> +					 devfreq->governor->name, ret);
>  			}
> +		} else if (!devfreq->governor) {
>  			devfreq->governor = governor;
>  			ret = devfreq->governor->event_handler(devfreq,
>  						DEVFREQ_GOV_START, NULL);

Re: [PATCH RESEND 2/4] PM / devfreq: Fix available_governors_show() when no governor is set

[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 use df->governor for validity
> verification in the following code.
> 
> Fixes: 483d557ee9a3 ("PM / devfreq: Clean up the devfreq instance name in sysfs attr")
> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
LGTM

Reviewed-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>  drivers/devfreq/devfreq.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 63ce6e25abe2..0bf320123e3a 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1504,16 +1504,13 @@ static ssize_t available_governors_show(struct device *d,
>  	struct devfreq *df = to_devfreq(d);
>  	ssize_t count = 0;
>  
> -	if (!df->governor)
> -		return -EINVAL;
> -
>  	mutex_lock(&devfreq_list_lock);
>  
>  	/*
>  	 * The devfreq with immutable governor (e.g., passive) shows
>  	 * only own governor.
>  	 */
> -	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
> +	if (df->governor && IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)) {
>  		count = scnprintf(&buf[count], DEVFREQ_NAME_LEN,
>  				  "%s ", df->governor->name);
>  	/*

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

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

Re: [PATCH RESEND 4/4] PM / devfreq: Optimize error return value of governor_show()

[Jie Zhan]

On 3/19/2026 5:17 PM, Yaxiong Tian wrote:
> When df->governor is NULL, governor_show() returns -EINVAL, which
> confuses users.
> 
> To fix this issue, return -ENOENT to indicate that no governor is
> currently set for the device.
> 
> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
> ---
>  drivers/devfreq/devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 4a312f3c2421..7cc60711fafd 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1412,7 +1412,7 @@ static ssize_t governor_show(struct device *dev,
>  	struct devfreq *df = to_devfreq(dev);
>  
>  	if (!df->governor)
> -		return -EINVAL;
> +		return -ENOENT;
What about -ENODEV?
>  
>  	return sprintf(buf, "%s\n", df->governor->name);
>  }

Re: [PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Yaxiong Tian]

在 2026/3/31 15:04, Jie Zhan 写道:
>
> On 3/19/2026 5:16 PM, Yaxiong Tian wrote:
>> When a user removes a governor using devfreq_remove_governor(), if
>> the current device is using this governor, devfreq->governor will
>> be set to NULL. When the user registers any governor
>> using devfreq_add_governor(), since devfreq->governor is NULL, a
>> null pointer error occurs in strncmp().
>>
>> For example: A user loads the userspace gov through a module, then
>> a device selects userspace. When unloading the userspace module and
>> then loading it again, the null pointer error occurs:
>>
>> Unable to handle kernel NULL pointer dereference at virtual address
>> 0000000000000010
>> Mem abort info:
>> ESR = 0x0000000096000004
>> EC = 0x25: DABT (current EL), IL = 32 bits
>> *******************skip *********************
>> Call trace:
>> __pi_strncmp+0x20/0x1b8
>> devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
>> do_one_initcall+0x4c/0x278
>> do_init_module+0x5c/0x218
>> load_module+0x1f1c/0x1fc8
>> init_module_from_file+0x8c/0xd0
>> __arm64_sys_finit_module+0x220/0x3d8
>> invoke_syscall+0x48/0x110
>> el0_svc_common.constprop.0+0xbc/0xe8
>> do_el0_svc+0x20/0x30
>> el0_svc+0x24/0xb8
>> el0t_64_sync_handler+0xb8/0xc0
>> el0t_64_sync+0x14c/0x150
>>
>> To fix this issue, modify the relevant logic in devfreq_add_governor():
>> Only check whether the new governor matches the existing one when
>> devfreq->governor exists. When devfreq->governor is NULL, directly
>> select the new governor and perform the DEVFREQ_GOV_START operation.
>>
>> Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
>> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
>> ---
>>   drivers/devfreq/devfreq.c | 24 +++++++++++-------------
>>   1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 54f0b18536db..63ce6e25abe2 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>>   		int ret = 0;
>>   		struct device *dev = devfreq->dev.parent;
>>   
>> -		if (!strncmp(devfreq->governor->name, governor->name,
>> +		if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
>>   			     DEVFREQ_NAME_LEN)) {
> Probaly do:
> if (!devfreq->governor)
> 	continue;
> so as to keep the line short and the "else if (!devfreq->governor) {" below
> can be removed.
If that's the case, a dev without a governor would not be able to add a 
new governor.
>
> and also:
> if (!strncmp(devfreq->governor->name, governor->name,
> 	DEVFREQ_NAME_LEN))
> 	continue;
> so we don't have to indent that much.

Currently, governor->name can be set arbitrarily, so duplicate names are 
possible.

But they could still be two different governors. Leaving it unchanged 
seems fine to me.

>
>>   			/* The following should never occur */
>> -			if (devfreq->governor) {
>> +			dev_warn(dev,
>> +				 "%s: Governor %s already present\n",
>> +				 __func__, devfreq->governor->name);
>> +			ret = devfreq->governor->event_handler(devfreq,
>> +						DEVFREQ_GOV_STOP, NULL);
>> +			if (ret) {
>>   				dev_warn(dev,
>> -					 "%s: Governor %s already present\n",
>> -					 __func__, devfreq->governor->name);
>> -				ret = devfreq->governor->event_handler(devfreq,
>> -							DEVFREQ_GOV_STOP, NULL);
>> -				if (ret) {
>> -					dev_warn(dev,
>> -						 "%s: Governor %s stop = %d\n",
>> -						 __func__,
>> -						 devfreq->governor->name, ret);
>> -				}
>> -				/* Fall through */
>> +					 "%s: Governor %s stop = %d\n",
>> +					 __func__,
>> +					 devfreq->governor->name, ret);
>>   			}
>> +		} else if (!devfreq->governor) {
>>   			devfreq->governor = governor;
>>   			ret = devfreq->governor->event_handler(devfreq,
>>   						DEVFREQ_GOV_START, NULL);

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

[Yaxiong Tian]

在 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;

Re: [PATCH RESEND 4/4] PM / devfreq: Optimize error return value of governor_show()

[Yaxiong Tian]

在 2026/3/31 15:21, Jie Zhan 写道:
>
> On 3/19/2026 5:17 PM, Yaxiong Tian wrote:
>> When df->governor is NULL, governor_show() returns -EINVAL, which
>> confuses users.
>>
>> To fix this issue, return -ENOENT to indicate that no governor is
>> currently set for the device.
>>
>> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
>> ---
>>   drivers/devfreq/devfreq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 4a312f3c2421..7cc60711fafd 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -1412,7 +1412,7 @@ static ssize_t governor_show(struct device *dev,
>>   	struct devfreq *df = to_devfreq(dev);
>>   
>>   	if (!df->governor)
>> -		return -EINVAL;
>> +		return -ENOENT;
> What about -ENODEV?
I think a governor is not a device, so -ENOENT would be more appropriate.
>>   
>>   	return sprintf(buf, "%s\n", df->governor->name);
>>   }

Re: [PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Jie Zhan]

On 3/31/2026 3:49 PM, Yaxiong Tian wrote:
> 
> 在 2026/3/31 15:04, Jie Zhan 写道:
>>
>> On 3/19/2026 5:16 PM, Yaxiong Tian wrote:
>>> When a user removes a governor using devfreq_remove_governor(), if
>>> the current device is using this governor, devfreq->governor will
>>> be set to NULL. When the user registers any governor
>>> using devfreq_add_governor(), since devfreq->governor is NULL, a
>>> null pointer error occurs in strncmp().
>>>
>>> For example: A user loads the userspace gov through a module, then
>>> a device selects userspace. When unloading the userspace module and
>>> then loading it again, the null pointer error occurs:
>>>
>>> Unable to handle kernel NULL pointer dereference at virtual address
>>> 0000000000000010
>>> Mem abort info:
>>> ESR = 0x0000000096000004
>>> EC = 0x25: DABT (current EL), IL = 32 bits
>>> *******************skip *********************
>>> Call trace:
>>> __pi_strncmp+0x20/0x1b8
>>> devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
>>> do_one_initcall+0x4c/0x278
>>> do_init_module+0x5c/0x218
>>> load_module+0x1f1c/0x1fc8
>>> init_module_from_file+0x8c/0xd0
>>> __arm64_sys_finit_module+0x220/0x3d8
>>> invoke_syscall+0x48/0x110
>>> el0_svc_common.constprop.0+0xbc/0xe8
>>> do_el0_svc+0x20/0x30
>>> el0_svc+0x24/0xb8
>>> el0t_64_sync_handler+0xb8/0xc0
>>> el0t_64_sync+0x14c/0x150
>>>
>>> To fix this issue, modify the relevant logic in devfreq_add_governor():
>>> Only check whether the new governor matches the existing one when
>>> devfreq->governor exists. When devfreq->governor is NULL, directly
>>> select the new governor and perform the DEVFREQ_GOV_START operation.
>>>
>>> Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
>>> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
>>> ---
>>>   drivers/devfreq/devfreq.c | 24 +++++++++++-------------
>>>   1 file changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 54f0b18536db..63ce6e25abe2 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>>>           int ret = 0;
>>>           struct device *dev = devfreq->dev.parent;
>>>   -        if (!strncmp(devfreq->governor->name, governor->name,
>>> +        if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
>>>                    DEVFREQ_NAME_LEN)) {
>> Probaly do:
>> if (!devfreq->governor)
>>     continue;
>> so as to keep the line short and the "else if (!devfreq->governor) {" below
>> can be removed.
> If that's the case, a dev without a governor would not be able to add a new governor.
It's not about adding.

The whole point here (the block inside list_for_each_entry()) is a very
rare safe check that you're adding a governor of the same name of a
governor used by an existing devfreq device.  If so, stop the old one and
start the new one.

Therefore, it doesn't have to start/restart it if devfreq->governor is
NULL.
>>
>> and also:
>> if (!strncmp(devfreq->governor->name, governor->name,
>>     DEVFREQ_NAME_LEN))
>>     continue;
>> so we don't have to indent that much.
> 
> Currently, governor->name can be set arbitrarily, so duplicate names are possible.
> 
> But they could still be two different governors. Leaving it unchanged seems fine to me.
> 
Sorry, my mistake!

I mean:
if (strncmp(devfreq->governor->name, governor->name,
	DEVFREQ_NAME_LEN))
	continue;

>>
>>>               /* The following should never occur */
>>> -            if (devfreq->governor) {
>>> +            dev_warn(dev,
>>> +                 "%s: Governor %s already present\n",
>>> +                 __func__, devfreq->governor->name);
>>> +            ret = devfreq->governor->event_handler(devfreq,
>>> +                        DEVFREQ_GOV_STOP, NULL);
>>> +            if (ret) {
>>>                   dev_warn(dev,
>>> -                     "%s: Governor %s already present\n",
>>> -                     __func__, devfreq->governor->name);
>>> -                ret = devfreq->governor->event_handler(devfreq,
>>> -                            DEVFREQ_GOV_STOP, NULL);
>>> -                if (ret) {
>>> -                    dev_warn(dev,
>>> -                         "%s: Governor %s stop = %d\n",
>>> -                         __func__,
>>> -                         devfreq->governor->name, ret);
>>> -                }
>>> -                /* Fall through */
>>> +                     "%s: Governor %s stop = %d\n",
>>> +                     __func__,
>>> +                     devfreq->governor->name, ret);
>>>               }
>>> +        } else if (!devfreq->governor) {
>>>               devfreq->governor = governor;
>>>               ret = devfreq->governor->event_handler(devfreq,
>>>                           DEVFREQ_GOV_START, NULL);

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

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

Re: [PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Yaxiong Tian]

在 2026/3/31 16:49, Jie Zhan 写道:
>
> On 3/31/2026 3:49 PM, Yaxiong Tian wrote:
>> 在 2026/3/31 15:04, Jie Zhan 写道:
>>> On 3/19/2026 5:16 PM, Yaxiong Tian wrote:
>>>> When a user removes a governor using devfreq_remove_governor(), if
>>>> the current device is using this governor, devfreq->governor will
>>>> be set to NULL. When the user registers any governor
>>>> using devfreq_add_governor(), since devfreq->governor is NULL, a
>>>> null pointer error occurs in strncmp().
>>>>
>>>> For example: A user loads the userspace gov through a module, then
>>>> a device selects userspace. When unloading the userspace module and
>>>> then loading it again, the null pointer error occurs:
>>>>
>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>> 0000000000000010
>>>> Mem abort info:
>>>> ESR = 0x0000000096000004
>>>> EC = 0x25: DABT (current EL), IL = 32 bits
>>>> *******************skip *********************
>>>> Call trace:
>>>> __pi_strncmp+0x20/0x1b8
>>>> devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
>>>> do_one_initcall+0x4c/0x278
>>>> do_init_module+0x5c/0x218
>>>> load_module+0x1f1c/0x1fc8
>>>> init_module_from_file+0x8c/0xd0
>>>> __arm64_sys_finit_module+0x220/0x3d8
>>>> invoke_syscall+0x48/0x110
>>>> el0_svc_common.constprop.0+0xbc/0xe8
>>>> do_el0_svc+0x20/0x30
>>>> el0_svc+0x24/0xb8
>>>> el0t_64_sync_handler+0xb8/0xc0
>>>> el0t_64_sync+0x14c/0x150
>>>>
>>>> To fix this issue, modify the relevant logic in devfreq_add_governor():
>>>> Only check whether the new governor matches the existing one when
>>>> devfreq->governor exists. When devfreq->governor is NULL, directly
>>>> select the new governor and perform the DEVFREQ_GOV_START operation.
>>>>
>>>> Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
>>>> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
>>>> ---
>>>>    drivers/devfreq/devfreq.c | 24 +++++++++++-------------
>>>>    1 file changed, 11 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>> index 54f0b18536db..63ce6e25abe2 100644
>>>> --- a/drivers/devfreq/devfreq.c
>>>> +++ b/drivers/devfreq/devfreq.c
>>>> @@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>>>>            int ret = 0;
>>>>            struct device *dev = devfreq->dev.parent;
>>>>    -        if (!strncmp(devfreq->governor->name, governor->name,
>>>> +        if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
>>>>                     DEVFREQ_NAME_LEN)) {
>>> Probaly do:
>>> if (!devfreq->governor)
>>>      continue;
>>> so as to keep the line short and the "else if (!devfreq->governor) {" below
>>> can be removed.
>> If that's the case, a dev without a governor would not be able to add a new governor.
> It's not about adding.
>
> The whole point here (the block inside list_for_each_entry()) is a very
> rare safe check that you're adding a governor of the same name of a
> governor used by an existing devfreq device.  If so, stop the old one and
> start the new one.
>
> Therefore, it doesn't have to start/restart it if devfreq->governor is
> NULL.
In the current patch, it directly start when devfreq->governor is NULL, 
and only follows the same-name logic you mentioned when it is non-NULL.

There isn't the issue you mentioned.

>>> and also:
>>> if (!strncmp(devfreq->governor->name, governor->name,
>>>      DEVFREQ_NAME_LEN))
>>>      continue;
>>> so we don't have to indent that much.
>> Currently, governor->name can be set arbitrarily, so duplicate names are possible.
>>
>> But they could still be two different governors. Leaving it unchanged seems fine to me.
>>
> Sorry, my mistake!
>
> I mean:
> if (strncmp(devfreq->governor->name, governor->name,
> 	DEVFREQ_NAME_LEN))
> 	continue;
>
>>>>                /* The following should never occur */
>>>> -            if (devfreq->governor) {
>>>> +            dev_warn(dev,
>>>> +                 "%s: Governor %s already present\n",
>>>> +                 __func__, devfreq->governor->name);
>>>> +            ret = devfreq->governor->event_handler(devfreq,
>>>> +                        DEVFREQ_GOV_STOP, NULL);
>>>> +            if (ret) {
>>>>                    dev_warn(dev,
>>>> -                     "%s: Governor %s already present\n",
>>>> -                     __func__, devfreq->governor->name);
>>>> -                ret = devfreq->governor->event_handler(devfreq,
>>>> -                            DEVFREQ_GOV_STOP, NULL);
>>>> -                if (ret) {
>>>> -                    dev_warn(dev,
>>>> -                         "%s: Governor %s stop = %d\n",
>>>> -                         __func__,
>>>> -                         devfreq->governor->name, ret);
>>>> -                }
>>>> -                /* Fall through */
>>>> +                     "%s: Governor %s stop = %d\n",
>>>> +                     __func__,
>>>> +                     devfreq->governor->name, ret);
>>>>                }
>>>> +        } else if (!devfreq->governor) {
>>>>                devfreq->governor = governor;
>>>>                ret = devfreq->governor->event_handler(devfreq,
>>>>                            DEVFREQ_GOV_START, NULL);

Re: [PATCH RESEND 1/4] PM / devfreq: Fix possible null pointer issue in devfreq_add_governor()

[Jie Zhan]

On 3/31/2026 5:17 PM, Yaxiong Tian wrote:
> 
> 在 2026/3/31 16:49, Jie Zhan 写道:
>>
>> On 3/31/2026 3:49 PM, Yaxiong Tian wrote:
>>> 在 2026/3/31 15:04, Jie Zhan 写道:
>>>> On 3/19/2026 5:16 PM, Yaxiong Tian wrote:
>>>>> When a user removes a governor using devfreq_remove_governor(), if
>>>>> the current device is using this governor, devfreq->governor will
>>>>> be set to NULL. When the user registers any governor
>>>>> using devfreq_add_governor(), since devfreq->governor is NULL, a
>>>>> null pointer error occurs in strncmp().
>>>>>
>>>>> For example: A user loads the userspace gov through a module, then
>>>>> a device selects userspace. When unloading the userspace module and
>>>>> then loading it again, the null pointer error occurs:
>>>>>
>>>>> Unable to handle kernel NULL pointer dereference at virtual address
>>>>> 0000000000000010
>>>>> Mem abort info:
>>>>> ESR = 0x0000000096000004
>>>>> EC = 0x25: DABT (current EL), IL = 32 bits
>>>>> *******************skip *********************
>>>>> Call trace:
>>>>> __pi_strncmp+0x20/0x1b8
>>>>> devfreq_userspace_init+0x1c/0xff8 [governor_userspace]
>>>>> do_one_initcall+0x4c/0x278
>>>>> do_init_module+0x5c/0x218
>>>>> load_module+0x1f1c/0x1fc8
>>>>> init_module_from_file+0x8c/0xd0
>>>>> __arm64_sys_finit_module+0x220/0x3d8
>>>>> invoke_syscall+0x48/0x110
>>>>> el0_svc_common.constprop.0+0xbc/0xe8
>>>>> do_el0_svc+0x20/0x30
>>>>> el0_svc+0x24/0xb8
>>>>> el0t_64_sync_handler+0xb8/0xc0
>>>>> el0t_64_sync+0x14c/0x150
>>>>>
>>>>> To fix this issue, modify the relevant logic in devfreq_add_governor():
>>>>> Only check whether the new governor matches the existing one when
>>>>> devfreq->governor exists. When devfreq->governor is NULL, directly
>>>>> select the new governor and perform the DEVFREQ_GOV_START operation.
>>>>>
>>>>> Fixes: 1b5c1be2c88e ("PM / devfreq: map devfreq drivers to governor using name")
>>>>> Signed-off-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
>>>>> ---
>>>>>    drivers/devfreq/devfreq.c | 24 +++++++++++-------------
>>>>>    1 file changed, 11 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index 54f0b18536db..63ce6e25abe2 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -1288,23 +1288,21 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>>>>>            int ret = 0;
>>>>>            struct device *dev = devfreq->dev.parent;
>>>>>    -        if (!strncmp(devfreq->governor->name, governor->name,
>>>>> +        if (devfreq->governor && !strncmp(devfreq->governor->name, governor->name,
>>>>>                     DEVFREQ_NAME_LEN)) {
>>>> Probaly do:
>>>> if (!devfreq->governor)
>>>>      continue;
>>>> so as to keep the line short and the "else if (!devfreq->governor) {" below
>>>> can be removed.
>>> If that's the case, a dev without a governor would not be able to add a new governor.
>> It's not about adding.
>>
>> The whole point here (the block inside list_for_each_entry()) is a very
>> rare safe check that you're adding a governor of the same name of a
>> governor used by an existing devfreq device.  If so, stop the old one and
>> start the new one.
>>
>> Therefore, it doesn't have to start/restart it if devfreq->governor is
>> NULL.
> In the current patch, it directly start when devfreq->governor is NULL, and only follows the same-name logic you mentioned when it is non-NULL.
> 
> There isn't the issue you mentioned.
> 
Ok, so create_sysfs_files() is skipped...
For example, setting 'userspace' as the governor won't create its belonging
files.

I'd recommend sticking with the existing logic here and avoid adding a new
block."

>>>> and also:
>>>> if (!strncmp(devfreq->governor->name, governor->name,
>>>>      DEVFREQ_NAME_LEN))
>>>>      continue;
>>>> so we don't have to indent that much.
>>> Currently, governor->name can be set arbitrarily, so duplicate names are possible.
>>>
>>> But they could still be two different governors. Leaving it unchanged seems fine to me.
>>>
>> Sorry, my mistake!
>>
>> I mean:
>> if (strncmp(devfreq->governor->name, governor->name,
>>     DEVFREQ_NAME_LEN))
>>     continue;
>>
>>>>>                /* The following should never occur */
>>>>> -            if (devfreq->governor) {
>>>>> +            dev_warn(dev,
>>>>> +                 "%s: Governor %s already present\n",
>>>>> +                 __func__, devfreq->governor->name);
>>>>> +            ret = devfreq->governor->event_handler(devfreq,
>>>>> +                        DEVFREQ_GOV_STOP, NULL);
>>>>> +            if (ret) {
>>>>>                    dev_warn(dev,
>>>>> -                     "%s: Governor %s already present\n",
>>>>> -                     __func__, devfreq->governor->name);
>>>>> -                ret = devfreq->governor->event_handler(devfreq,
>>>>> -                            DEVFREQ_GOV_STOP, NULL);
>>>>> -                if (ret) {
>>>>> -                    dev_warn(dev,
>>>>> -                         "%s: Governor %s stop = %d\n",
>>>>> -                         __func__,
>>>>> -                         devfreq->governor->name, ret);
>>>>> -                }
>>>>> -                /* Fall through */
>>>>> +                     "%s: Governor %s stop = %d\n",
>>>>> +                     __func__,
>>>>> +                     devfreq->governor->name, ret);
>>>>>                }
>>>>> +        } else if (!devfreq->governor) {
>>>>>                devfreq->governor = governor;
>>>>>                ret = devfreq->governor->event_handler(devfreq,
>>>>>                            DEVFREQ_GOV_START, NULL);

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

[Yaxiong Tian]

在 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;

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@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;

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

[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)?

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

[Yaxiong Tian]

在 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.