[PATCH v2 0/6] devfreq: Add refcounts for governor modules

[PATCH v2 0/6] 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-6 add the reference counting mechanism for devfreq governor modules.

Changelog:
v2:
- 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:
https://lore.kernel.org/all/20260326123428.800407-1-zhanjie9@hisilicon.com/

Jie Zhan (6):
  devfreq: Use mutex guard in governor_store()
  devfreq: Use mutex guard in devfreq_add/remove_governor()
  devfreq: Factor out devfreq_set_governor()
  devfreq: Add module owner to devfreq governor
  devfreq: Get and put module refcount when switching governor
  devfreq: Get module refcount in try_then_request_governor()

 drivers/devfreq/devfreq.c        | 210 +++++++++++++++----------------
 include/linux/devfreq-governor.h |  26 +++-
 2 files changed, 127 insertions(+), 109 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] 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.

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

Re: [PATCH v2 1/6] devfreq: Use mutex guard in governor_store()

[Yaxiong Tian]

在 2026/5/13 17:38, Jie Zhan 写道:
> Use mutex guard in governor_store() so as to simplify the locking logic.
>
> No functional impact intended.
>
> 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);
>   
Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>

[PATCH v2 2/6] 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.

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

Re: [PATCH v2 2/6] devfreq: Use mutex guard in devfreq_add/remove_governor()

[Yaxiong Tian]

在 2026/5/13 17:38, Jie Zhan 写道:
> Use mutex guard in devfreq_add/remove_governor() so as to simplify the
> locking logic.
>
> No functional impact intended.
>
> 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);
>   
Reviewed-by: Yaxiong Tian <tianyaxiong@kylinos.cn>

[PATCH v2 3/6] devfreq: Factor out devfreq_set_governor()

[Jie Zhan]

Factor out common governor setting logic into devfreq_set_governor() so as
to consolidate the code in governor_store() and devfreq_add_device().

The caller is expected to hold 'devfreq_list_lock', enforced via
lockdep_assert_held().  This is required because devfreq_add_device() must
hold the lock from setting governor until the device is added to
'devfreq_list', so that a concurrent devfreq_remove_governor() cannot free
the governor in between.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 53c40d795a13..9e3e6a7348f8 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -318,6 +318,59 @@ 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;
+		}
+	}
+
+	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 +995,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 +1003,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 +1427,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 +1438,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

Re: [PATCH v2 3/6] devfreq: Factor out devfreq_set_governor()

[Yaxiong Tian]

在 2026/5/13 17:38, Jie Zhan 写道:
> Factor out common governor setting logic into devfreq_set_governor() so as
> to consolidate the code in governor_store() and devfreq_add_device().
>
> The caller is expected to hold 'devfreq_list_lock', enforced via
> lockdep_assert_held().  This is required because devfreq_add_device() must
> hold the lock from setting governor until the device is added to
> 'devfreq_list', so that a concurrent devfreq_remove_governor() cannot free
> the governor in between.
>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>

I'm a bit unclear about the purpose of this commit, or the problem it 
solves. Can you explain it more clearly?


> ---
>   drivers/devfreq/devfreq.c | 117 ++++++++++++++++++--------------------
>   1 file changed, 56 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 53c40d795a13..9e3e6a7348f8 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -318,6 +318,59 @@ 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;
> +		}
> +	}
> +
> +	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 +995,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 +1003,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 +1427,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 +1438,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;
>   

[PATCH v2 4/6] devfreq: Add module owner to devfreq governor

[Jie Zhan]

Add an 'owner' member to struct devfreq_governor, such that we can find
the module that holds the governor code when it's compiled as a kernel
module.  This allows the devfreq core to properly manage the lifecycle
of governors.

Update devfreq_add_governor() and devm_devfreq_add_governor() to
automatically set 'owner' to THIS_MODULE via helper macros.

This is a prerequisite for implementing governor reference counting to
prevent a governor module from being unloaded (except for force unload)
while a governor is in use.

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c        | 26 +++++++++-----------------
 include/linux/devfreq-governor.h | 26 +++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 9e3e6a7348f8..e1363ab69173 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1301,11 +1301,8 @@ void devfreq_resume(void)
 	mutex_unlock(&devfreq_list_lock);
 }
 
-/**
- * devfreq_add_governor() - Add devfreq governor
- * @governor:	the devfreq governor to be added
- */
-int devfreq_add_governor(struct devfreq_governor *governor)
+int __devfreq_add_governor(struct devfreq_governor *governor,
+			   struct module *mod)
 {
 	struct devfreq_governor *g;
 
@@ -1322,37 +1319,32 @@ int devfreq_add_governor(struct devfreq_governor *governor)
 		return -EINVAL;
 	}
 
+	governor->owner = mod;
 	list_add(&governor->node, &devfreq_governor_list);
 
 	return 0;
 }
-EXPORT_SYMBOL(devfreq_add_governor);
+EXPORT_SYMBOL(__devfreq_add_governor);
 
 static void devm_devfreq_remove_governor(void *governor)
 {
 	WARN_ON(devfreq_remove_governor(governor));
 }
 
-/**
- * devm_devfreq_add_governor() - Add devfreq governor
- * @dev:	device which adds devfreq governor
- * @governor:	the devfreq governor to be added
- *
- * This is a resource-managed variant of devfreq_add_governor().
- */
-int devm_devfreq_add_governor(struct device *dev,
-			      struct devfreq_governor *governor)
+int __devm_devfreq_add_governor(struct device *dev,
+				struct devfreq_governor *governor,
+				struct module *mod)
 {
 	int err;
 
-	err = devfreq_add_governor(governor);
+	err = __devfreq_add_governor(governor, mod);
 	if (err)
 		return err;
 
 	return devm_add_action_or_reset(dev, devm_devfreq_remove_governor,
 					governor);
 }
-EXPORT_SYMBOL(devm_devfreq_add_governor);
+EXPORT_SYMBOL(__devm_devfreq_add_governor);
 
 /**
  * devfreq_remove_governor() - Remove devfreq feature from a device.
diff --git a/include/linux/devfreq-governor.h b/include/linux/devfreq-governor.h
index dfdd0160a29f..1c4ff57e24de 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
 
@@ -50,6 +51,7 @@
 /**
  * struct devfreq_governor - Devfreq policy governor
  * @node:		list node - contains registered devfreq governors
+ * @owner:		Module that this governor belongs to
  * @name:		Governor's name
  * @attrs:		Governor's sysfs attribute flags
  * @flags:		Governor's feature flags
@@ -67,6 +69,7 @@
 struct devfreq_governor {
 	struct list_head node;
 
+	struct module *owner;
 	const char name[DEVFREQ_NAME_LEN];
 	const u64 attrs;
 	const u64 flags;
@@ -81,11 +84,28 @@ void devfreq_monitor_suspend(struct devfreq *devfreq);
 void devfreq_monitor_resume(struct devfreq *devfreq);
 void devfreq_update_interval(struct devfreq *devfreq, unsigned int *delay);
 
-int devfreq_add_governor(struct devfreq_governor *governor);
+/**
+ * devfreq_add_governor() - Add devfreq governor
+ * @governor:	The devfreq governor to be added
+ */
+#define devfreq_add_governor(governor) \
+	__devfreq_add_governor((governor), THIS_MODULE)
+int __devfreq_add_governor(struct devfreq_governor *governor,
+			   struct module *mod);
 int devfreq_remove_governor(struct devfreq_governor *governor);
 
-int devm_devfreq_add_governor(struct device *dev,
-			      struct devfreq_governor *governor);
+/**
+ * devm_devfreq_add_governor() - Add devfreq governor
+ * @dev:	device which adds devfreq governor
+ * @governor:	the devfreq governor to be added
+ *
+ * This is a resource-managed variant of devfreq_add_governor().
+ */
+#define devm_devfreq_add_governor(dev, governor) \
+	__devm_devfreq_add_governor((dev), (governor), THIS_MODULE)
+int __devm_devfreq_add_governor(struct device *dev,
+				struct devfreq_governor *governor,
+				struct module *mod);
 
 int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
 int devfreq_update_target(struct devfreq *devfreq, unsigned long freq);
-- 
2.43.0

Re: [PATCH v2 4/6] devfreq: Add module owner to devfreq governor

[Yaxiong Tian]

在 2026/5/13 17:38, Jie Zhan 写道:
> Add an 'owner' member to struct devfreq_governor, such that we can find
> the module that holds the governor code when it's compiled as a kernel
> module.  This allows the devfreq core to properly manage the lifecycle
> of governors.
>
> Update devfreq_add_governor() and devm_devfreq_add_governor() to
> automatically set 'owner' to THIS_MODULE via helper macros.
>
> This is a prerequisite for implementing governor reference counting to
> prevent a governor module from being unloaded (except for force unload)
> while a governor is in use.
>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>   drivers/devfreq/devfreq.c        | 26 +++++++++-----------------
>   include/linux/devfreq-governor.h | 26 +++++++++++++++++++++++---
>   2 files changed, 32 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 9e3e6a7348f8..e1363ab69173 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1301,11 +1301,8 @@ void devfreq_resume(void)
>   	mutex_unlock(&devfreq_list_lock);
>   }
>   
> -/**
> - * devfreq_add_governor() - Add devfreq governor
> - * @governor:	the devfreq governor to be added
> - */
> -int devfreq_add_governor(struct devfreq_governor *governor)
> +int __devfreq_add_governor(struct devfreq_governor *governor,
> +			   struct module *mod)
>   {
>   	struct devfreq_governor *g;
>   
> @@ -1322,37 +1319,32 @@ int devfreq_add_governor(struct devfreq_governor *governor)
>   		return -EINVAL;
>   	}
>   
> +	governor->owner = mod;
>   	list_add(&governor->node, &devfreq_governor_list);
>   
>   	return 0;
>   }
> -EXPORT_SYMBOL(devfreq_add_governor);
> +EXPORT_SYMBOL(__devfreq_add_governor);

It's a bit strange to use the __ symbol here. Generally speaking, 
functions exported with EXPORT_SYMBOL are public, while __ indicates 
internal.

The same applies below.

>   
>   static void devm_devfreq_remove_governor(void *governor)
>   {
>   	WARN_ON(devfreq_remove_governor(governor));
>   }
>   
> -/**
> - * devm_devfreq_add_governor() - Add devfreq governor
> - * @dev:	device which adds devfreq governor
> - * @governor:	the devfreq governor to be added
> - *
> - * This is a resource-managed variant of devfreq_add_governor().
> - */
> -int devm_devfreq_add_governor(struct device *dev,
> -			      struct devfreq_governor *governor)
> +int __devm_devfreq_add_governor(struct device *dev,
> +				struct devfreq_governor *governor,
> +				struct module *mod)
>   {
>   	int err;
>   
> -	err = devfreq_add_governor(governor);
> +	err = __devfreq_add_governor(governor, mod);
>   	if (err)
>   		return err;
>   
>   	return devm_add_action_or_reset(dev, devm_devfreq_remove_governor,
>   					governor);
>   }
> -EXPORT_SYMBOL(devm_devfreq_add_governor);
> +EXPORT_SYMBOL(__devm_devfreq_add_governor);
>   
>   /**
>    * devfreq_remove_governor() - Remove devfreq feature from a device.
> diff --git a/include/linux/devfreq-governor.h b/include/linux/devfreq-governor.h
> index dfdd0160a29f..1c4ff57e24de 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
>   
> @@ -50,6 +51,7 @@
>   /**
>    * struct devfreq_governor - Devfreq policy governor
>    * @node:		list node - contains registered devfreq governors
> + * @owner:		Module that this governor belongs to
>    * @name:		Governor's name
>    * @attrs:		Governor's sysfs attribute flags
>    * @flags:		Governor's feature flags
> @@ -67,6 +69,7 @@
>   struct devfreq_governor {
>   	struct list_head node;
>   
> +	struct module *owner;
>   	const char name[DEVFREQ_NAME_LEN];
>   	const u64 attrs;
>   	const u64 flags;
> @@ -81,11 +84,28 @@ void devfreq_monitor_suspend(struct devfreq *devfreq);
>   void devfreq_monitor_resume(struct devfreq *devfreq);
>   void devfreq_update_interval(struct devfreq *devfreq, unsigned int *delay);
>   
> -int devfreq_add_governor(struct devfreq_governor *governor);
> +/**
> + * devfreq_add_governor() - Add devfreq governor
> + * @governor:	The devfreq governor to be added
> + */
> +#define devfreq_add_governor(governor) \
> +	__devfreq_add_governor((governor), THIS_MODULE)
> +int __devfreq_add_governor(struct devfreq_governor *governor,
> +			   struct module *mod);
>   int devfreq_remove_governor(struct devfreq_governor *governor);
>   
> -int devm_devfreq_add_governor(struct device *dev,
> -			      struct devfreq_governor *governor);
> +/**
> + * devm_devfreq_add_governor() - Add devfreq governor
> + * @dev:	device which adds devfreq governor
> + * @governor:	the devfreq governor to be added
> + *
> + * This is a resource-managed variant of devfreq_add_governor().
> + */
> +#define devm_devfreq_add_governor(dev, governor) \
> +	__devm_devfreq_add_governor((dev), (governor), THIS_MODULE)
> +int __devm_devfreq_add_governor(struct device *dev,
> +				struct devfreq_governor *governor,
> +				struct module *mod);
>   
>   int devfreq_update_status(struct devfreq *devfreq, unsigned long freq);
>   int devfreq_update_target(struct devfreq *devfreq, unsigned long freq);

[PATCH v2 5/6] devfreq: Get and put module refcount when switching governor

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

To prevent the governor module from being removed (except for force
unload) when it's in use, get and put a refcount of the governor module
when starting and stopping the governor.

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

Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
---
 drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index e1363ab69173..2ea42325d030 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;
 		}
@@ -1040,9 +1053,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;
-- 
2.43.0

Re: [PATCH v2 5/6] devfreq: Get and put module refcount when switching governor

[Yaxiong Tian]

在 2026/5/13 17:38, 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.
>
> To prevent the governor module from being removed (except for force
> unload) when it's in use, get and put a refcount of the governor module
> when starting and stopping the governor.
>
> 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
>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>
> ---
>   drivers/devfreq/devfreq.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index e1363ab69173..2ea42325d030 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;
> +	}
> +

Here, new_gov has already been checked in try_then_request_governor() 
for whether the module can be obtained.

I think it might be more reasonable to merge try_then_request_governor() 
and devfreq_set_governor(), so that when new_gov cannot be obtained, the 
operation of the old governor is not affected.

>   	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;
>   		}
> @@ -1040,9 +1053,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;

[PATCH v2 6/6] devfreq: Get module refcount in try_then_request_governor()

[Jie Zhan]

Get a refcount of the governor module in try_then_request_governor() so
that the governor module cannot be unloaded between finding the governor
and the caller using it.

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

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2ea42325d030..fb1a7aa168aa 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -285,6 +285,9 @@ static struct devfreq_governor *find_devfreq_governor(const char *name)
  * and the driver that call devfreq_add_device) are built as modules.
  * devfreq_list_lock should be held by the caller. Returns the matched
  * governor's pointer or an error pointer.
+ * On success, this holds a refcount of the governor module to prevent the
+ * module from being unloaded during usage, so the caller should put a module
+ * refcount after using it.
  */
 static struct devfreq_governor *try_then_request_governor(const char *name)
 {
@@ -313,8 +316,13 @@ static struct devfreq_governor *try_then_request_governor(const char *name)
 			return (err < 0) ? ERR_PTR(err) : ERR_PTR(-EINVAL);
 
 		governor = find_devfreq_governor(name);
+		if (IS_ERR(governor))
+			return governor;
 	}
 
+	if (!try_module_get(governor->owner))
+		return ERR_PTR(-EBUSY);
+
 	return governor;
 }
 
@@ -1009,6 +1017,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	}
 
 	err = devfreq_set_governor(devfreq, governor);
+	module_put(governor->owner);
 	if (err) {
 		dev_err_probe(dev, err,
 			"%s: Unable to start governor for the device\n",
@@ -1446,6 +1455,7 @@ static ssize_t governor_store(struct device *dev, struct device_attribute *attr,
 		return PTR_ERR(governor);
 
 	ret = devfreq_set_governor(df, governor);
+	module_put(governor->owner);
 	if (ret)
 		return ret;
 
-- 
2.43.0