* [PATCH v4 1/4] devfreq: Use mutex guard in governor_store()
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
@ 2026-07-29 10:23 ` Jie Zhan
2026-11-15 22:27 ` Chanwoo Choi
2026-07-29 10:23 ` [PATCH v4 2/4] devfreq: Use mutex guard in devfreq_add/remove_governor() Jie Zhan
` (4 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Jie Zhan @ 2026-07-29 10:23 UTC (permalink / raw)
To: cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhanjie9, zhenglifeng1, zhangpengjie2,
lihuisong, prime.zeng
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 1/4] devfreq: Use mutex guard in governor_store()
2026-07-29 10:23 ` [PATCH v4 1/4] devfreq: Use mutex guard in governor_store() Jie Zhan
@ 2026-11-15 22:27 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2026-11-15 22:27 UTC (permalink / raw)
To: Jie Zhan, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhenglifeng1, zhangpengjie2, lihuisong,
prime.zeng
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/4] devfreq: Use mutex guard in devfreq_add/remove_governor()
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
2026-07-29 10:23 ` [PATCH v4 1/4] devfreq: Use mutex guard in governor_store() Jie Zhan
@ 2026-07-29 10:23 ` Jie Zhan
2026-07-29 10:23 ` [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor() Jie Zhan
` (3 subsequent siblings)
5 siblings, 0 replies; 10+ messages in thread
From: Jie Zhan @ 2026-07-29 10:23 UTC (permalink / raw)
To: cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhanjie9, zhenglifeng1, zhangpengjie2,
lihuisong, prime.zeng
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor()
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
2026-07-29 10:23 ` [PATCH v4 1/4] devfreq: Use mutex guard in governor_store() Jie Zhan
2026-07-29 10:23 ` [PATCH v4 2/4] devfreq: Use mutex guard in devfreq_add/remove_governor() Jie Zhan
@ 2026-07-29 10:23 ` Jie Zhan
2026-11-15 22:36 ` Chanwoo Choi
2026-07-29 10:23 ` [PATCH v4 4/4] devfreq: Refcount governor modules while in use Jie Zhan
` (2 subsequent siblings)
5 siblings, 1 reply; 10+ messages in thread
From: Jie Zhan @ 2026-07-29 10:23 UTC (permalink / raw)
To: cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhanjie9, zhenglifeng1, zhangpengjie2,
lihuisong, prime.zeng
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor()
2026-07-29 10:23 ` [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor() Jie Zhan
@ 2026-11-15 22:36 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2026-11-15 22:36 UTC (permalink / raw)
To: Jie Zhan, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhenglifeng1, zhangpengjie2, lihuisong,
prime.zeng
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 4/4] devfreq: Refcount governor modules while in use
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
` (2 preceding siblings ...)
2026-07-29 10:23 ` [PATCH v4 3/4] devfreq: Factor out devfreq_set_governor() Jie Zhan
@ 2026-07-29 10:23 ` Jie Zhan
2026-11-15 22:50 ` Chanwoo Choi
2026-08-04 9:38 ` [PATCH v4 0/4] devfreq: Add refcounts for governor modules zhenglifeng (A)
2026-10-13 14:21 ` Jie Zhan
5 siblings, 1 reply; 10+ messages in thread
From: Jie Zhan @ 2026-07-29 10:23 UTC (permalink / raw)
To: cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhanjie9, zhenglifeng1, zhangpengjie2,
lihuisong, prime.zeng
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
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v4 4/4] devfreq: Refcount governor modules while in use
2026-07-29 10:23 ` [PATCH v4 4/4] devfreq: Refcount governor modules while in use Jie Zhan
@ 2026-11-15 22:50 ` Chanwoo Choi
0 siblings, 0 replies; 10+ messages in thread
From: Chanwoo Choi @ 2026-11-15 22:50 UTC (permalink / raw)
To: Jie Zhan, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhenglifeng1, zhangpengjie2, lihuisong,
prime.zeng
On 7/29/2026 7:23 PM, Jie Zhan wrote:
> 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);
I agree this suggestion. But this patch is based on patch1/2/3.
After resolving the patch1,2,3, we better to review this patch.
--
Best Regards,
Chanwoo Choi
Principal Engineer, Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 0/4] devfreq: Add refcounts for governor modules
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
` (3 preceding siblings ...)
2026-07-29 10:23 ` [PATCH v4 4/4] devfreq: Refcount governor modules while in use Jie Zhan
@ 2026-08-04 9:38 ` zhenglifeng (A)
2026-10-13 14:21 ` Jie Zhan
5 siblings, 0 replies; 10+ messages in thread
From: zhenglifeng (A) @ 2026-08-04 9:38 UTC (permalink / raw)
To: Jie Zhan, cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park,
linux-pm, linux-arm-kernel
Cc: linuxarm, tianyaxiong, zhangpengjie2, lihuisong, prime.zeng
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>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v4 0/4] devfreq: Add refcounts for governor modules
2026-07-29 10:23 [PATCH v4 0/4] devfreq: Add refcounts for governor modules Jie Zhan
` (4 preceding siblings ...)
2026-08-04 9:38 ` [PATCH v4 0/4] devfreq: Add refcounts for governor modules zhenglifeng (A)
@ 2026-10-13 14:21 ` Jie Zhan
5 siblings, 0 replies; 10+ messages in thread
From: Jie Zhan @ 2026-10-13 14:21 UTC (permalink / raw)
To: cwchoi00, cw00.choi, myungjoo.ham, kyungmin.park, linux-pm,
linux-arm-kernel, Rafael J. Wysocki
Cc: linuxarm, tianyaxiong, zhenglifeng1, zhangpengjie2, lihuisong,
prime.zeng
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(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread