[PATCH v4 0/4] devfreq: Add refcounts for governor modules

[PATCH v4 0/4] devfreq: Add refcounts for governor modules

[Jie Zhan]

A governor module can be dynamically inserted or removed if compiled as a
kernel module.  'devfreq->governor' would become NULL if the governor
module is removed when it's in use.

For user-friendliness, get and put the module refcount of a governor module
when it's in use.  As a result, unloading a governor module in use returns
an error, e.g.:

$ cat governor
performance
$ rmmod governor_performance
rmmod: ERROR: Module governor_performance is in use

Note that this can't stop force unload, so it's more of a
user-friendliness improvement rather than strict protection.  The
existing code that keeps devfreq working when 'devfreq->governor' is
NULL should still be there.

Patch 1-3 clean up mutex with guards and factor out a common governor
setting function, so as to prepare for implementing governor reference
counting.  They can be applied separately.

Patch 4 adds the reference counting mechanism for devfreq governor
modules.

This set is based on devfreq-next of 7.1-rc1.

Changelog
---------
v4:
- Patch 3: Return an error if starting new governor fails and restoring
  old governor succeeds, such that the caller won't get misled.

v3:
- Link: https://lore.kernel.org/all/20260519113251.3745140-1-zhanjie9@hisilicon.com/
- Rework patch 4 to make the 'owner' field assignment optional in the
  governor code.  This prevents device-driver-bundled governors (such as
  those in hisi_uncore_freq and tegra30-devfreq) from causing
  self-reference issues that block module removal. Additionally, merge
  patches 4 and 5 to clearly present the new 'owner' member alongside
  its usage.
- Drop patch 6 because it's no longer needed since v2.
- Update commit logs of patch 3 and 4 to clarify background and
  motivation.
- Pick up 2 tags from Yaxiong (thanks!).
- Trivial cleanups.

v2:
- Link: https://lore.kernel.org/all/20260513093832.1645890-1-zhanjie9@hisilicon.com/
- Rebase on devfreq-next of 7.1-rc1.
- Drop the patches related to the NULL pointer deference issue of
  'devfreq->governor', which has been solved and merged recently.
- Remove the dedicated mutex for 'devfreq_governor_list' because the
  refcount changes don't depend on that.
- Some minor cleanups and fixes.

v1:
- Link: https://lore.kernel.org/all/20260326123428.800407-1-zhanjie9@hisilicon.com/

Jie Zhan (4):
  devfreq: Use mutex guard in governor_store()
  devfreq: Use mutex guard in devfreq_add/remove_governor()
  devfreq: Factor out devfreq_set_governor()
  devfreq: Refcount governor modules while in use

 drivers/devfreq/devfreq.c                 | 185 +++++++++++-----------
 drivers/devfreq/governor_passive.c        |   1 +
 drivers/devfreq/governor_performance.c    |   1 +
 drivers/devfreq/governor_powersave.c      |   1 +
 drivers/devfreq/governor_simpleondemand.c |   1 +
 drivers/devfreq/governor_userspace.c      |   1 +
 include/linux/devfreq-governor.h          |  11 ++
 7 files changed, 112 insertions(+), 89 deletions(-)

-- 
2.43.0

[PATCH v4 1/4] devfreq: Use mutex guard in governor_store()

[Jie Zhan]

Use mutex guard in governor_store() so as to simplify the locking logic.

No functional impact intended.

Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index f08fc6966eae..7a70dd051644 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1394,23 +1394,20 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (ret != 1)
 		return -EINVAL;
 
-	mutex_lock(&devfreq_list_lock);
+	guard(mutex)(&devfreq_list_lock);
 	governor = try_then_request_governor(str_governor);
-	if (IS_ERR(governor)) {
-		ret = PTR_ERR(governor);
-		goto out;
-	}
+	if (IS_ERR(governor))
+		return PTR_ERR(governor);
+
 	if (!df->governor)
 		goto start_new_governor;
 
-	if (df->governor == governor) {
-		ret = 0;
-		goto out;
-	} else if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)
-		|| IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (df->governor == governor)
+		return count;
+
+	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE) ||
+	    IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE))
+		return -EINVAL;
 
 	/*
 	 * Stop the current governor and remove the specific sysfs files
@@ -1420,7 +1417,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (ret) {
 		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
 			 __func__, df->governor->name, ret);
-		goto out;
+		return ret;
 	}
 
 start_new_governor:
@@ -1438,7 +1435,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 		/* Restore previous governor */
 		df->governor = prev_governor;
 		if (!df->governor)
-			goto out;
+			return ret;
 
 		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
 		if (ret) {
@@ -1446,7 +1443,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 				"%s: reverting to Governor %s failed (%d)\n",
 				__func__, prev_governor->name, ret);
 			df->governor = NULL;
-			goto out;
+			return ret;
 		}
 	}
 
@@ -1455,13 +1452,10 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	 * the new governor, restore the sysfs files of previous governor.
 	 */
 	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
+	if (ret)
+		return ret;
 
-out:
-	mutex_unlock(&devfreq_list_lock);
-
-	if (!ret)
-		ret = count;
-	return ret;
+	return count;
 }
 static DEVICE_ATTR_RW(governor);
 
-- 
2.43.0

[PATCH v4 2/4] devfreq: Use mutex guard in devfreq_add/remove_governor()

[Jie Zhan]

Use mutex guard in devfreq_add/remove_governor() so as to simplify the
locking logic.

No functional impact intended.

Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 7a70dd051644..53c40d795a13 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1261,28 +1261,23 @@ void devfreq_resume(void)
 int devfreq_add_governor(struct devfreq_governor *governor)
 {
 	struct devfreq_governor *g;
-	int err = 0;
 
 	if (!governor) {
 		pr_err("%s: Invalid parameters.\n", __func__);
 		return -EINVAL;
 	}
 
-	mutex_lock(&devfreq_list_lock);
+	guard(mutex)(&devfreq_list_lock);
 	g = find_devfreq_governor(governor->name);
 	if (!IS_ERR(g)) {
 		pr_err("%s: governor %s already registered\n", __func__,
 		       g->name);
-		err = -EINVAL;
-		goto err_out;
+		return -EINVAL;
 	}
 
 	list_add(&governor->node, &devfreq_governor_list);
 
-err_out:
-	mutex_unlock(&devfreq_list_lock);
-
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(devfreq_add_governor);
 
@@ -1320,21 +1315,20 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
 {
 	struct devfreq_governor *g;
 	struct devfreq *devfreq;
-	int err = 0;
 
 	if (!governor) {
 		pr_err("%s: Invalid parameters.\n", __func__);
 		return -EINVAL;
 	}
 
-	mutex_lock(&devfreq_list_lock);
+	guard(mutex)(&devfreq_list_lock);
 	g = find_devfreq_governor(governor->name);
 	if (IS_ERR(g)) {
 		pr_err("%s: governor %s not registered\n", __func__,
 		       governor->name);
-		err = PTR_ERR(g);
-		goto err_out;
+		return PTR_ERR(g);
 	}
+
 	list_for_each_entry(devfreq, &devfreq_list, node) {
 		int ret;
 		struct device *dev = devfreq->dev.parent;
@@ -1356,10 +1350,8 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
 	}
 
 	list_del(&governor->node);
-err_out:
-	mutex_unlock(&devfreq_list_lock);
 
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL(devfreq_remove_governor);
 
-- 
2.43.0

[PATCH v4 3/4] devfreq: Factor out devfreq_set_governor()

[Jie Zhan]

governor_store() and devfreq_add_device() contain similar logic for setting
a governor when devfreq->governor is NULL.  Merge this into a common
function, devfreq_set_governor(), to reduce code duplication and unify the
entry of setting governors.

This also prepares for further changes that get / put a module refcount of
the active governor and prevent the governor module from being unloaded
while it's in use.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c | 128 ++++++++++++++++++++------------------
 1 file changed, 67 insertions(+), 61 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 53c40d795a13..959f0b08ed36 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -318,6 +318,70 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
 	return governor;
 }
 
+static int devfreq_set_governor(struct devfreq *df,
+				const struct devfreq_governor *new_gov)
+{
+	const struct devfreq_governor *old_gov;
+	struct device *dev;
+	int ret;
+
+	lockdep_assert_held(&devfreq_list_lock);
+
+	old_gov = df->governor;
+	dev = &df->dev;
+
+	if (old_gov) {
+		if (old_gov == new_gov)
+			return 0;
+
+		if (IS_SUPPORTED_FLAG(old_gov->flags, IMMUTABLE) ||
+		    IS_SUPPORTED_FLAG(new_gov->flags, IMMUTABLE))
+			return -EINVAL;
+
+		/* Stop the current governor */
+		ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
+		if (ret) {
+			dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
+				 __func__, df->governor->name, ret);
+			return ret;
+		}
+	}
+
+	/* Start the new governor */
+	df->governor = new_gov;
+	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);
+
+		/* Restore previous governor */
+		df->governor = old_gov;
+		if (!df->governor)
+			return ret;
+
+		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
+		if (ret) {
+			dev_err(dev, "%s: restore Governor %s failed (%d)\n",
+				__func__, df->governor->name, ret);
+			df->governor = NULL;
+			return ret;
+		}
+
+		ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
+		if (ret)
+			dev_warn(dev, "sysfs update failed (%d)\n", ret);
+
+		/*
+		 * Old governor restored successfully, but the new governor
+		 * failed to start.  Return a distinct error so the caller is
+		 * not misled into thinking the switch succeeded.
+		 */
+		return -EAGAIN;
+	}
+
+	return sysfs_update_group(&df->dev.kobj, &gov_attr_group);
+}
+
 static int devfreq_notify_transition(struct devfreq *devfreq,
 		struct devfreq_freqs *freqs, unsigned int state)
 {
@@ -942,9 +1006,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_init;
 	}
 
-	devfreq->governor = governor;
-	err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
-						NULL);
+	err = devfreq_set_governor(devfreq, governor);
 	if (err) {
 		dev_err_probe(dev, err,
 			"%s: Unable to start governor for the device\n",
@@ -952,10 +1014,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_init;
 	}
 
-	err = sysfs_update_group(&devfreq->dev.kobj, &gov_attr_group);
-	if (err)
-		goto err_init;
-
 	list_add(&devfreq->node, &devfreq_list);
 
 	mutex_unlock(&devfreq_list_lock);
@@ -1380,7 +1438,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	struct devfreq *df = to_devfreq(dev);
 	int ret;
 	char str_governor[DEVFREQ_NAME_LEN + 1];
-	const struct devfreq_governor *governor, *prev_governor;
+	const struct devfreq_governor *governor;
 
 	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
 	if (ret != 1)
@@ -1391,59 +1449,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 	if (IS_ERR(governor))
 		return PTR_ERR(governor);
 
-	if (!df->governor)
-		goto start_new_governor;
-
-	if (df->governor == governor)
-		return count;
-
-	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE) ||
-	    IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE))
-		return -EINVAL;
-
-	/*
-	 * Stop the current governor and remove the specific sysfs files
-	 * which depend on current governor.
-	 */
-	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
-	if (ret) {
-		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
-			 __func__, df->governor->name, ret);
-		return ret;
-	}
-
-start_new_governor:
-	/*
-	 * Start the new governor and create the specific sysfs files
-	 * which depend on the new governor.
-	 */
-	prev_governor = 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);
-
-		/* Restore previous governor */
-		df->governor = prev_governor;
-		if (!df->governor)
-			return ret;
-
-		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
-		if (ret) {
-			dev_err(dev,
-				"%s: reverting to Governor %s failed (%d)\n",
-				__func__, prev_governor->name, ret);
-			df->governor = NULL;
-			return ret;
-		}
-	}
-
-	/*
-	 * Create the sysfs files for the new governor. But if failed to start
-	 * the new governor, restore the sysfs files of previous governor.
-	 */
-	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
+	ret = devfreq_set_governor(df, governor);
 	if (ret)
 		return ret;
 
-- 
2.43.0

[PATCH v4 4/4] devfreq: Refcount governor modules while in use

[Jie Zhan]

A governor module can be inserted or removed dynamically when built as a
kernel module.  'devfreq->governor' would become NULL if the governor
module is removed when it's in use.

Add a refcount mechanism for governor modules to prevent the governor
module from being removed (except for force unload):
1. Add an optional 'owner' member to struct devfreq_governor so the devfreq
   core can identify the module that holds the governor code.
2. Get and put a refcount of the governor module when starting and stopping
   the governor.

The new 'owner' field is optional:
- Common governor modules (performance, powersave, simple_ondemand,
  userspace, passive) set 'owner' to THIS_MODULE.
- Governors that are bundled into a device driver module must leave 'owner'
  NULL.  The device's lifetime already pins that module, and setting
  'owner' would create a self-reference that blocks the driver from being
  unloaded.

As a result, a non-forced rmmod of an in-use stand-alone governor now
fails with -EBUSY, e.g.:

  $ cat governor
  performance
  $ rmmod governor_performance
  rmmod: ERROR: Module governor_performance is in use

Force unloads (rmmod -f, if configured) can't be blocked.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c                 | 17 ++++++++++++++++-
 drivers/devfreq/governor_passive.c        |  1 +
 drivers/devfreq/governor_performance.c    |  1 +
 drivers/devfreq/governor_powersave.c      |  1 +
 drivers/devfreq/governor_simpleondemand.c |  1 +
 drivers/devfreq/governor_userspace.c      |  1 +
 include/linux/devfreq-governor.h          | 11 +++++++++++
 7 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 959f0b08ed36..4a01dad21c16 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -345,24 +345,37 @@ static int devfreq_set_governor(struct devfreq *df,
 				 __func__, df->governor->name, ret);
 			return ret;
 		}
+		module_put(old_gov->owner);
 	}
 
 	/* Start the new governor */
+	if (!try_module_get(new_gov->owner)) {
+		df->governor = NULL;
+		return -EINVAL;
+	}
+
 	df->governor = new_gov;
 	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);
+		module_put(new_gov->owner);
 
 		/* Restore previous governor */
 		df->governor = old_gov;
 		if (!df->governor)
 			return ret;
 
+		if (!try_module_get(old_gov->owner)) {
+			df->governor = NULL;
+			return -EINVAL;
+		}
+
 		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
 		if (ret) {
 			dev_err(dev, "%s: restore Governor %s failed (%d)\n",
 				__func__, df->governor->name, ret);
+			module_put(old_gov->owner);
 			df->governor = NULL;
 			return ret;
 		}
@@ -1051,9 +1064,11 @@ int devfreq_remove_device(struct devfreq *devfreq)
 
 	devfreq_cooling_unregister(devfreq->cdev);
 
-	if (devfreq->governor)
+	if (devfreq->governor) {
 		devfreq->governor->event_handler(devfreq,
 						 DEVFREQ_GOV_STOP, NULL);
+		module_put(devfreq->governor->owner);
+	}
 	device_unregister(&devfreq->dev);
 
 	return 0;
diff --git a/drivers/devfreq/governor_passive.c b/drivers/devfreq/governor_passive.c
index d7feecd900f1..3e63dd200a04 100644
--- a/drivers/devfreq/governor_passive.c
+++ b/drivers/devfreq/governor_passive.c
@@ -449,6 +449,7 @@ static int devfreq_passive_event_handler(struct devfreq *devfreq,
 static struct devfreq_governor devfreq_passive = {
 	.name = DEVFREQ_GOV_PASSIVE,
 	.flags = DEVFREQ_GOV_FLAG_IMMUTABLE,
+	.owner = THIS_MODULE,
 	.get_target_freq = devfreq_passive_get_target_freq,
 	.event_handler = devfreq_passive_event_handler,
 };
diff --git a/drivers/devfreq/governor_performance.c b/drivers/devfreq/governor_performance.c
index fdb22bf512cf..0a08b067ea6b 100644
--- a/drivers/devfreq/governor_performance.c
+++ b/drivers/devfreq/governor_performance.c
@@ -37,6 +37,7 @@ static int devfreq_performance_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_performance = {
 	.name = DEVFREQ_GOV_PERFORMANCE,
+	.owner = THIS_MODULE,
 	.get_target_freq = devfreq_performance_func,
 	.event_handler = devfreq_performance_handler,
 };
diff --git a/drivers/devfreq/governor_powersave.c b/drivers/devfreq/governor_powersave.c
index ee2d6ec8a512..3ae296d55e3e 100644
--- a/drivers/devfreq/governor_powersave.c
+++ b/drivers/devfreq/governor_powersave.c
@@ -37,6 +37,7 @@ static int devfreq_powersave_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_powersave = {
 	.name = DEVFREQ_GOV_POWERSAVE,
+	.owner = THIS_MODULE,
 	.get_target_freq = devfreq_powersave_func,
 	.event_handler = devfreq_powersave_handler,
 };
diff --git a/drivers/devfreq/governor_simpleondemand.c b/drivers/devfreq/governor_simpleondemand.c
index ac9c5e9e51a4..46287b279ded 100644
--- a/drivers/devfreq/governor_simpleondemand.c
+++ b/drivers/devfreq/governor_simpleondemand.c
@@ -119,6 +119,7 @@ static struct devfreq_governor devfreq_simple_ondemand = {
 	.name = DEVFREQ_GOV_SIMPLE_ONDEMAND,
 	.attrs = DEVFREQ_GOV_ATTR_POLLING_INTERVAL
 		| DEVFREQ_GOV_ATTR_TIMER,
+	.owner = THIS_MODULE,
 	.get_target_freq = devfreq_simple_ondemand_func,
 	.event_handler = devfreq_simple_ondemand_handler,
 };
diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
index 3906ebedbae8..b1acccc79f7f 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -135,6 +135,7 @@ static int devfreq_userspace_handler(struct devfreq *devfreq,
 
 static struct devfreq_governor devfreq_userspace = {
 	.name = DEVFREQ_GOV_USERSPACE,
+	.owner = THIS_MODULE,
 	.get_target_freq = devfreq_userspace_func,
 	.event_handler = devfreq_userspace_handler,
 };
diff --git a/include/linux/devfreq-governor.h b/include/linux/devfreq-governor.h
index dfdd0160a29f..ae1721e58401 100644
--- a/include/linux/devfreq-governor.h
+++ b/include/linux/devfreq-governor.h
@@ -12,6 +12,7 @@
 #define __LINUX_DEVFREQ_DEVFREQ_H__
 
 #include <linux/devfreq.h>
+struct module;
 
 #define DEVFREQ_NAME_LEN			16
 
@@ -53,6 +54,15 @@
  * @name:		Governor's name
  * @attrs:		Governor's sysfs attribute flags
  * @flags:		Governor's feature flags
+ * @owner:		Optional, module that owns this governor.
+ *			When set (typically to THIS_MODULE), the devfreq core
+ *			takes a reference on @owner while this governor is in
+ *			use so that the governor module cannot be removed,
+ *			except by a forced unload. Governors that are bundled
+ *			into a device driver module must leave @owner NULL: the
+ *			device's lifetime already pins that module, and setting
+ *			@owner would create a self-reference that prevents the
+ *			driver from being unloaded.
  * @get_target_freq:	Returns desired operating frequency for the device.
  *			Basically, get_target_freq will run
  *			devfreq_dev_profile.get_dev_status() to get the
@@ -70,6 +80,7 @@ struct devfreq_governor {
 	const char name[DEVFREQ_NAME_LEN];
 	const u64 attrs;
 	const u64 flags;
+	struct module *owner;
 	int (*get_target_freq)(struct devfreq *this, unsigned long *freq);
 	int (*event_handler)(struct devfreq *devfreq,
 				unsigned int event, void *data);
-- 
2.43.0

Re: [PATCH v4 0/4] devfreq: Add refcounts for governor modules

[zhenglifeng (A)]

On 7/29/2026 6:23 PM, Jie Zhan wrote:
> A governor module can be dynamically inserted or removed if compiled as a
> kernel module.  'devfreq->governor' would become NULL if the governor
> module is removed when it's in use.
> 
> For user-friendliness, get and put the module refcount of a governor module
> when it's in use.  As a result, unloading a governor module in use returns
> an error, e.g.:
> 
> $ cat governor
> performance
> $ rmmod governor_performance
> rmmod: ERROR: Module governor_performance is in use
> 
> Note that this can't stop force unload, so it's more of a
> user-friendliness improvement rather than strict protection.  The
> existing code that keeps devfreq working when 'devfreq->governor' is
> NULL should still be there.
> 
> Patch 1-3 clean up mutex with guards and factor out a common governor
> setting function, so as to prepare for implementing governor reference
> counting.  They can be applied separately.
> 
> Patch 4 adds the reference counting mechanism for devfreq governor
> modules.
> 
> This set is based on devfreq-next of 7.1-rc1.
> 
> Changelog
> ---------
> v4:
> - Patch 3: Return an error if starting new governor fails and restoring
>   old governor succeeds, such that the caller won't get misled.
> 
> v3:
> - Link: https://lore.kernel.org/all/20260519113251.3745140-1-zhanjie9@hisilicon.com/
> - Rework patch 4 to make the 'owner' field assignment optional in the
>   governor code.  This prevents device-driver-bundled governors (such as
>   those in hisi_uncore_freq and tegra30-devfreq) from causing
>   self-reference issues that block module removal. Additionally, merge
>   patches 4 and 5 to clearly present the new 'owner' member alongside
>   its usage.
> - Drop patch 6 because it's no longer needed since v2.
> - Update commit logs of patch 3 and 4 to clarify background and
>   motivation.
> - Pick up 2 tags from Yaxiong (thanks!).
> - Trivial cleanups.
> 
> v2:
> - Link: https://lore.kernel.org/all/20260513093832.1645890-1-zhanjie9@hisilicon.com/
> - Rebase on devfreq-next of 7.1-rc1.
> - Drop the patches related to the NULL pointer deference issue of
>   'devfreq->governor', which has been solved and merged recently.
> - Remove the dedicated mutex for 'devfreq_governor_list' because the
>   refcount changes don't depend on that.
> - Some minor cleanups and fixes.
> 
> v1:
> - Link: https://lore.kernel.org/all/20260326123428.800407-1-zhanjie9@hisilicon.com/
> 
> Jie Zhan (4):
>   devfreq: Use mutex guard in governor_store()
>   devfreq: Use mutex guard in devfreq_add/remove_governor()
>   devfreq: Factor out devfreq_set_governor()
>   devfreq: Refcount governor modules while in use
> 
>  drivers/devfreq/devfreq.c                 | 185 +++++++++++-----------
>  drivers/devfreq/governor_passive.c        |   1 +
>  drivers/devfreq/governor_performance.c    |   1 +
>  drivers/devfreq/governor_powersave.c      |   1 +
>  drivers/devfreq/governor_simpleondemand.c |   1 +
>  drivers/devfreq/governor_userspace.c      |   1 +
>  include/linux/devfreq-governor.h          |  11 ++
>  7 files changed, 112 insertions(+), 89 deletions(-)
> 

LGTM.

Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>

Re: [PATCH v4 0/4] devfreq: Add refcounts for governor modules

[Jie Zhan]

Hi Chanwoo,

Can we get some feedback on this series to move things forward?

Thanks!
Jie

On 7/29/2026 6:23 PM, Jie Zhan wrote:
> A governor module can be dynamically inserted or removed if compiled as a
> kernel module.  'devfreq->governor' would become NULL if the governor
> module is removed when it's in use.
> 
> For user-friendliness, get and put the module refcount of a governor module
> when it's in use.  As a result, unloading a governor module in use returns
> an error, e.g.:
> 
> $ cat governor
> performance
> $ rmmod governor_performance
> rmmod: ERROR: Module governor_performance is in use
> 
> Note that this can't stop force unload, so it's more of a
> user-friendliness improvement rather than strict protection.  The
> existing code that keeps devfreq working when 'devfreq->governor' is
> NULL should still be there.
> 
> Patch 1-3 clean up mutex with guards and factor out a common governor
> setting function, so as to prepare for implementing governor reference
> counting.  They can be applied separately.
> 
> Patch 4 adds the reference counting mechanism for devfreq governor
> modules.
> 
> This set is based on devfreq-next of 7.1-rc1.
> 
> Changelog
> ---------
> v4:
> - Patch 3: Return an error if starting new governor fails and restoring
>   old governor succeeds, such that the caller won't get misled.
> 
> v3:
> - Link: https://lore.kernel.org/all/20260519113251.3745140-1-zhanjie9@hisilicon.com/
> - Rework patch 4 to make the 'owner' field assignment optional in the
>   governor code.  This prevents device-driver-bundled governors (such as
>   those in hisi_uncore_freq and tegra30-devfreq) from causing
>   self-reference issues that block module removal. Additionally, merge
>   patches 4 and 5 to clearly present the new 'owner' member alongside
>   its usage.
> - Drop patch 6 because it's no longer needed since v2.
> - Update commit logs of patch 3 and 4 to clarify background and
>   motivation.
> - Pick up 2 tags from Yaxiong (thanks!).
> - Trivial cleanups.
> 
> v2:
> - Link: https://lore.kernel.org/all/20260513093832.1645890-1-zhanjie9@hisilicon.com/
> - Rebase on devfreq-next of 7.1-rc1.
> - Drop the patches related to the NULL pointer deference issue of
>   'devfreq->governor', which has been solved and merged recently.
> - Remove the dedicated mutex for 'devfreq_governor_list' because the
>   refcount changes don't depend on that.
> - Some minor cleanups and fixes.
> 
> v1:
> - Link: https://lore.kernel.org/all/20260326123428.800407-1-zhanjie9@hisilicon.com/
> 
> Jie Zhan (4):
>   devfreq: Use mutex guard in governor_store()
>   devfreq: Use mutex guard in devfreq_add/remove_governor()
>   devfreq: Factor out devfreq_set_governor()
>   devfreq: Refcount governor modules while in use
> 
>  drivers/devfreq/devfreq.c                 | 185 +++++++++++-----------
>  drivers/devfreq/governor_passive.c        |   1 +
>  drivers/devfreq/governor_performance.c    |   1 +
>  drivers/devfreq/governor_powersave.c      |   1 +
>  drivers/devfreq/governor_simpleondemand.c |   1 +
>  drivers/devfreq/governor_userspace.c      |   1 +
>  include/linux/devfreq-governor.h          |  11 ++
>  7 files changed, 112 insertions(+), 89 deletions(-)
> 

Re: [PATCH v4 1/4] devfreq: Use mutex guard in governor_store()

[Chanwoo Choi]

On 7/29/2026 7:23 PM, Jie Zhan wrote:
> Use mutex guard in governor_store() so as to simplify the locking logic.
> 
> No functional impact intended.
> 
> Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>  drivers/devfreq/devfreq.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index f08fc6966eae..7a70dd051644 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1394,23 +1394,20 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	mutex_lock(&devfreq_list_lock);
> +	guard(mutex)(&devfreq_list_lock);
>  	governor = try_then_request_governor(str_governor);
> -	if (IS_ERR(governor)) {
> -		ret = PTR_ERR(governor);
> -		goto out;
> -	}
> +	if (IS_ERR(governor))
> +		return PTR_ERR(governor);
> +
>  	if (!df->governor)
>  		goto start_new_governor;
>  
> -	if (df->governor == governor) {
> -		ret = 0;
> -		goto out;
> -	} else if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)
> -		|| IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (df->governor == governor)
> +		return count;
> +
> +	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE) ||
> +	    IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE))
> +		return -EINVAL;
>  
>  	/*
>  	 * Stop the current governor and remove the specific sysfs files
> @@ -1420,7 +1417,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (ret) {
>  		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
>  			 __func__, df->governor->name, ret);
> -		goto out;
> +		return ret;
>  	}
>  
>  start_new_governor:
> @@ -1438,7 +1435,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  		/* Restore previous governor */
>  		df->governor = prev_governor;
>  		if (!df->governor)
> -			goto out;
> +			return ret;
>  
>  		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>  		if (ret) {
> @@ -1446,7 +1443,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  				"%s: reverting to Governor %s failed (%d)\n",
>  				__func__, prev_governor->name, ret);
>  			df->governor = NULL;
> -			goto out;
> +			return ret;
>  		}
>  	}
>  
> @@ -1455,13 +1452,10 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	 * the new governor, restore the sysfs files of previous governor.
>  	 */
>  	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> +	if (ret)
> +		return ret;
>  
> -out:
> -	mutex_unlock(&devfreq_list_lock);
> -
> -	if (!ret)
> -		ret = count;
> -	return ret;
> +	return count;
>  }
>  static DEVICE_ATTR_RW(governor);
>  

Hi,

include/linux/cleanup.h includes the following comment
"148  * Lastly, given that the benefit of cleanup helpers is removal of
149  * "goto", and that the "goto" statement can jump between scopes, the
150  * expectation is that usage of "goto" and cleanup helpers is never
151  * mixed in the same function. I.e. for a given routine, convert all
152  * resources that need a "goto" cleanup to scope-based cleanup, or
153  * convert none of them.
"

A Patch should be atomic. This patch still uses both guard and goto.
even if you removed goto on patch3.

-- 
Best Regards,
Chanwoo Choi
Principal Engineer, Samsung Electronics

Re: [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor()

[Chanwoo Choi]

On 7/29/2026 7:23 PM, Jie Zhan wrote:
> governor_store() and devfreq_add_device() contain similar logic for setting
> a governor when devfreq->governor is NULL.  Merge this into a common
> function, devfreq_set_governor(), to reduce code duplication and unify the
> entry of setting governors.
> 
> This also prepares for further changes that get / put a module refcount of
> the active governor and prevent the governor module from being unloaded
> while it's in use.
> 
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>  drivers/devfreq/devfreq.c | 128 ++++++++++++++++++++------------------
>  1 file changed, 67 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 53c40d795a13..959f0b08ed36 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -318,6 +318,70 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
>  	return governor;
>  }
>  
> +static int devfreq_set_governor(struct devfreq *df,
> +				const struct devfreq_governor *new_gov)
> +{
> +	const struct devfreq_governor *old_gov;
> +	struct device *dev;
> +	int ret;
> +
> +	lockdep_assert_held(&devfreq_list_lock);
> +
> +	old_gov = df->governor;
> +	dev = &df->dev;
> +
> +	if (old_gov) {
> +		if (old_gov == new_gov)
> +			return 0;
> +
> +		if (IS_SUPPORTED_FLAG(old_gov->flags, IMMUTABLE) ||
> +		    IS_SUPPORTED_FLAG(new_gov->flags, IMMUTABLE))
> +			return -EINVAL;
> +
> +		/* Stop the current governor */
> +		ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
> +		if (ret) {
> +			dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> +				 __func__, df->governor->name, ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* Start the new governor */
> +	df->governor = new_gov;
> +	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);
> +
> +		/* Restore previous governor */
> +		df->governor = old_gov;
> +		if (!df->governor)
> +			return ret;
> +
> +		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> +		if (ret) {
> +			dev_err(dev, "%s: restore Governor %s failed (%d)\n",
> +				__func__, df->governor->name, ret);
> +			df->governor = NULL;
> +			return ret;
> +		}
> +
> +		ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> +		if (ret)
> +			dev_warn(dev, "sysfs update failed (%d)\n", ret);
> +
> +		/*
> +		 * Old governor restored successfully, but the new governor
> +		 * failed to start.  Return a distinct error so the caller is
> +		 * not misled into thinking the switch succeeded.
> +		 */
> +		return -EAGAIN;
> +	}
> +
> +	return sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> +}
> +
>  static int devfreq_notify_transition(struct devfreq *devfreq,
>  		struct devfreq_freqs *freqs, unsigned int state)
>  {
> @@ -942,9 +1006,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		goto err_init;
>  	}
>  
> -	devfreq->governor = governor;
> -	err = devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_START,
> -						NULL);
> +	err = devfreq_set_governor(devfreq, governor);
>  	if (err) {
>  		dev_err_probe(dev, err,
>  			"%s: Unable to start governor for the device\n",
> @@ -952,10 +1014,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  		goto err_init;
>  	}
>  
> -	err = sysfs_update_group(&devfreq->dev.kobj, &gov_attr_group);
> -	if (err)
> -		goto err_init;
> -
>  	list_add(&devfreq->node, &devfreq_list);
>  
>  	mutex_unlock(&devfreq_list_lock);
> @@ -1380,7 +1438,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	struct devfreq *df = to_devfreq(dev);
>  	int ret;
>  	char str_governor[DEVFREQ_NAME_LEN + 1];
> -	const struct devfreq_governor *governor, *prev_governor;
> +	const struct devfreq_governor *governor;
>  
>  	ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>  	if (ret != 1)
> @@ -1391,59 +1449,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
>  	if (IS_ERR(governor))
>  		return PTR_ERR(governor);
>  
> -	if (!df->governor)
> -		goto start_new_governor;
> -
> -	if (df->governor == governor)
> -		return count;
> -
> -	if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE) ||
> -	    IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE))
> -		return -EINVAL;
> -
> -	/*
> -	 * Stop the current governor and remove the specific sysfs files
> -	 * which depend on current governor.
> -	 */
> -	ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
> -	if (ret) {
> -		dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
> -			 __func__, df->governor->name, ret);
> -		return ret;
> -	}
> -
> -start_new_governor:
> -	/*
> -	 * Start the new governor and create the specific sysfs files
> -	 * which depend on the new governor.
> -	 */
> -	prev_governor = 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);
> -
> -		/* Restore previous governor */
> -		df->governor = prev_governor;
> -		if (!df->governor)
> -			return ret;
> -
> -		ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
> -		if (ret) {
> -			dev_err(dev,
> -				"%s: reverting to Governor %s failed (%d)\n",
> -				__func__, prev_governor->name, ret);
> -			df->governor = NULL;
> -			return ret;
> -		}
> -	}
> -
> -	/*
> -	 * Create the sysfs files for the new governor. But if failed to start
> -	 * the new governor, restore the sysfs files of previous governor.
> -	 */
> -	ret = sysfs_update_group(&df->dev.kobj, &gov_attr_group);
> +	ret = devfreq_set_governor(df, governor);
>  	if (ret)
>  		return ret;
>  

Actually, I didn't want to merge it becaus there are fully test for all governor
and various case to switch governor(stop/start/switch). It makes the problem.
Also, there is an AI report indicating that this patch has a problem related to passive govenor.

We must guarantee the safe operation without any side-effect.
Don't update devfreq_add_device and better to make the separate function 'start_new_governor'
and then do next update separately.

-- 
Best Regards,
Chanwoo Choi
Principal Engineer, Samsung Electronics