[PATCH 0/4] PM / devfreq: Some optimizations of devfreq

[PATCH 0/4] PM / devfreq: Some optimizations of devfreq

[Lifeng Zheng]

Lifeng Zheng (4):
  PM / devfreq: governor: Replace sscanf() with kstrtoul() in
    set_freq_store()
  PM / devfreq: Limit max_freq with scaling_min_freq
  PM / devfreq: Remove redundant devfreq_get_freq_range() calling in
    devfreq_add_device()
  PM / devfreq: Check governor before using governor->name

 drivers/devfreq/devfreq.c            | 20 +++++---------------
 drivers/devfreq/governor_userspace.c |  6 +++++-
 2 files changed, 10 insertions(+), 16 deletions(-)

-- 
2.33.0

[PATCH 1/4] PM / devfreq: governor: Replace sscanf() with kstrtoul() in set_freq_store()

[Lifeng Zheng]

Replace sscanf() with kstrtoul() in set_freq_store() and check the result
to avoid invalid input.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/devfreq/governor_userspace.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
index d1aa6806b683..175de0c0b50e 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -9,6 +9,7 @@
 #include <linux/slab.h>
 #include <linux/device.h>
 #include <linux/devfreq.h>
+#include <linux/kstrtox.h>
 #include <linux/pm.h>
 #include <linux/mutex.h>
 #include <linux/module.h>
@@ -39,10 +40,13 @@ static ssize_t set_freq_store(struct device *dev, struct device_attribute *attr,
 	unsigned long wanted;
 	int err = 0;
 
+	err = kstrtoul(buf, 0, &wanted);
+	if (err)
+		return err;
+
 	mutex_lock(&devfreq->lock);
 	data = devfreq->governor_data;
 
-	sscanf(buf, "%lu", &wanted);
 	data->user_frequency = wanted;
 	data->valid = true;
 	err = update_devfreq(devfreq);
-- 
2.33.0

[PATCH 2/4] PM / devfreq: Limit max_freq with scaling_min_freq

[Lifeng Zheng]

Limit max_freq in devfreq_get_freq_range() with scaling_min_freq to avoid
showing an unreachable freq when reading it.

Use macro clamp to simplify code.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/devfreq/devfreq.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 98657d3b9435..2810c84b9f8a 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -152,11 +152,8 @@ void devfreq_get_freq_range(struct devfreq *devfreq,
 				(unsigned long)HZ_PER_KHZ * qos_max_freq);
 
 	/* Apply constraints from OPP interface */
-	*min_freq = max(*min_freq, devfreq->scaling_min_freq);
-	*max_freq = min(*max_freq, devfreq->scaling_max_freq);
-
-	if (*min_freq > *max_freq)
-		*min_freq = *max_freq;
+	*max_freq = clamp(*max_freq, devfreq->scaling_min_freq, devfreq->scaling_max_freq);
+	*min_freq = clamp(*min_freq, devfreq->scaling_min_freq, *max_freq);
 }
 EXPORT_SYMBOL(devfreq_get_freq_range);
 
-- 
2.33.0

[PATCH 3/4] PM / devfreq: Remove redundant devfreq_get_freq_range() calling in devfreq_add_device()

[Lifeng Zheng]

The calling of devfreq_get_freq_range() in devfreq_add_device() is
redundant because min_freq and max_freq are never used. Remove it.

Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/devfreq/devfreq.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 2810c84b9f8a..18e3f7e063a4 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -804,7 +804,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
 {
 	struct devfreq *devfreq;
 	struct devfreq_governor *governor;
-	unsigned long min_freq, max_freq;
 	int err = 0;
 
 	if (!dev || !profile || !governor_name) {
@@ -872,8 +871,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
 		goto err_dev;
 	}
 
-	devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
-
 	devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
 	devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
 	if (IS_ERR(devfreq->opp_table))
-- 
2.33.0

[PATCH 4/4] PM / devfreq: Check governor before using governor->name

[Lifeng Zheng]

Commit 96ffcdf239de ("PM / devfreq: Remove redundant governor_name from
struct devfreq") removes governor_name and uses governor->name to replace
it. But devfreq->governor may be NULL and directly using
devfreq->governor->name may cause null pointer exception. Move the check of
governor to before using governor->name.

Fixes: 96ffcdf239de ("PM / devfreq: Remove redundant governor_name from struct devfreq")
Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
---
 drivers/devfreq/devfreq.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 18e3f7e063a4..46f3a8053197 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1376,15 +1376,11 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
 		int ret;
 		struct device *dev = devfreq->dev.parent;
 
+		if (!devfreq->governor)
+			continue;
+
 		if (!strncmp(devfreq->governor->name, governor->name,
 			     DEVFREQ_NAME_LEN)) {
-			/* we should have a devfreq governor! */
-			if (!devfreq->governor) {
-				dev_warn(dev, "%s: Governor %s NOT present\n",
-					 __func__, governor->name);
-				continue;
-				/* Fall through */
-			}
 			ret = devfreq->governor->event_handler(devfreq,
 						DEVFREQ_GOV_STOP, NULL);
 			if (ret) {
-- 
2.33.0

Re: [PATCH 1/4] PM / devfreq: governor: Replace sscanf() with kstrtoul() in set_freq_store()

[David Laight]

On Mon, 21 Apr 2025 11:00:17 +0800
Lifeng Zheng <zhenglifeng1@huawei.com> wrote:

> Replace sscanf() with kstrtoul() in set_freq_store() and check the result
> to avoid invalid input.

Isn't this a UAPI change?

The sscanf() version will ignore trailing characters.
In this case it is actually likely that value might have a trailing "Hz".

	David

> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/devfreq/governor_userspace.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> index d1aa6806b683..175de0c0b50e 100644
> --- a/drivers/devfreq/governor_userspace.c
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -9,6 +9,7 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/devfreq.h>
> +#include <linux/kstrtox.h>
>  #include <linux/pm.h>
>  #include <linux/mutex.h>
>  #include <linux/module.h>
> @@ -39,10 +40,13 @@ static ssize_t set_freq_store(struct device *dev, struct device_attribute *attr,
>  	unsigned long wanted;
>  	int err = 0;
>  
> +	err = kstrtoul(buf, 0, &wanted);
> +	if (err)
> +		return err;
> +
>  	mutex_lock(&devfreq->lock);
>  	data = devfreq->governor_data;
>  
> -	sscanf(buf, "%lu", &wanted);
>  	data->user_frequency = wanted;
>  	data->valid = true;
>  	err = update_devfreq(devfreq);

Re: [PATCH 1/4] PM / devfreq: governor: Replace sscanf() with kstrtoul() in set_freq_store()

[zhenglifeng (A)]

On 2025/4/27 19:17, David Laight wrote:
> On Mon, 21 Apr 2025 11:00:17 +0800
> Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
> 
>> Replace sscanf() with kstrtoul() in set_freq_store() and check the result
>> to avoid invalid input.
> 
> Isn't this a UAPI change?
> 
> The sscanf() version will ignore trailing characters.
> In this case it is actually likely that value might have a trailing "Hz".

I tried to still use sscanf() at first, but checkpatch warned: "Prefer
kstrto<type> to single variable sscanf".

I'm not sure if we should ignore this warning.

> 
> 	David
> 
>>
>> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
>> ---
>>  drivers/devfreq/governor_userspace.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
>> index d1aa6806b683..175de0c0b50e 100644
>> --- a/drivers/devfreq/governor_userspace.c
>> +++ b/drivers/devfreq/governor_userspace.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/device.h>
>>  #include <linux/devfreq.h>
>> +#include <linux/kstrtox.h>
>>  #include <linux/pm.h>
>>  #include <linux/mutex.h>
>>  #include <linux/module.h>
>> @@ -39,10 +40,13 @@ static ssize_t set_freq_store(struct device *dev, struct device_attribute *attr,
>>  	unsigned long wanted;
>>  	int err = 0;
>>  
>> +	err = kstrtoul(buf, 0, &wanted);
>> +	if (err)
>> +		return err;
>> +
>>  	mutex_lock(&devfreq->lock);
>>  	data = devfreq->governor_data;
>>  
>> -	sscanf(buf, "%lu", &wanted);
>>  	data->user_frequency = wanted;
>>  	data->valid = true;
>>  	err = update_devfreq(devfreq);
> 
> 

Re: [PATCH 0/4] PM / devfreq: Some optimizations of devfreq

[zhenglifeng (A)]

Gentle ping.

On 2025/4/21 11:00, Lifeng Zheng wrote:
> 
> Lifeng Zheng (4):
>   PM / devfreq: governor: Replace sscanf() with kstrtoul() in
>     set_freq_store()
>   PM / devfreq: Limit max_freq with scaling_min_freq
>   PM / devfreq: Remove redundant devfreq_get_freq_range() calling in
>     devfreq_add_device()
>   PM / devfreq: Check governor before using governor->name
> 
>  drivers/devfreq/devfreq.c            | 20 +++++---------------
>  drivers/devfreq/governor_userspace.c |  6 +++++-
>  2 files changed, 10 insertions(+), 16 deletions(-)
> 

Re: [PATCH 1/4] PM / devfreq: governor: Replace sscanf() with kstrtoul() in set_freq_store()

[Chanwoo Choi]

Hi,

Applied it. Thanks.

On Mon, Apr 21, 2025 at 12:00 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Replace sscanf() with kstrtoul() in set_freq_store() and check the result
> to avoid invalid input.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/devfreq/governor_userspace.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/governor_userspace.c b/drivers/devfreq/governor_userspace.c
> index d1aa6806b683..175de0c0b50e 100644
> --- a/drivers/devfreq/governor_userspace.c
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -9,6 +9,7 @@
>  #include <linux/slab.h>
>  #include <linux/device.h>
>  #include <linux/devfreq.h>
> +#include <linux/kstrtox.h>
>  #include <linux/pm.h>
>  #include <linux/mutex.h>
>  #include <linux/module.h>
> @@ -39,10 +40,13 @@ static ssize_t set_freq_store(struct device *dev, struct device_attribute *attr,
>         unsigned long wanted;
>         int err = 0;
>
> +       err = kstrtoul(buf, 0, &wanted);
> +       if (err)
> +               return err;
> +
>         mutex_lock(&devfreq->lock);
>         data = devfreq->governor_data;
>
> -       sscanf(buf, "%lu", &wanted);
>         data->user_frequency = wanted;
>         data->valid = true;
>         err = update_devfreq(devfreq);
> --
> 2.33.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 2/4] PM / devfreq: Limit max_freq with scaling_min_freq

[Chanwoo Choi]

Hi,

Applied it. Thanks.

On Mon, Apr 21, 2025 at 12:00 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Limit max_freq in devfreq_get_freq_range() with scaling_min_freq to avoid
> showing an unreachable freq when reading it.
>
> Use macro clamp to simplify code.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/devfreq/devfreq.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 98657d3b9435..2810c84b9f8a 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -152,11 +152,8 @@ void devfreq_get_freq_range(struct devfreq *devfreq,
>                                 (unsigned long)HZ_PER_KHZ * qos_max_freq);
>
>         /* Apply constraints from OPP interface */
> -       *min_freq = max(*min_freq, devfreq->scaling_min_freq);
> -       *max_freq = min(*max_freq, devfreq->scaling_max_freq);
> -
> -       if (*min_freq > *max_freq)
> -               *min_freq = *max_freq;
> +       *max_freq = clamp(*max_freq, devfreq->scaling_min_freq, devfreq->scaling_max_freq);
> +       *min_freq = clamp(*min_freq, devfreq->scaling_min_freq, *max_freq);
>  }
>  EXPORT_SYMBOL(devfreq_get_freq_range);
>
> --
> 2.33.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 3/4] PM / devfreq: Remove redundant devfreq_get_freq_range() calling in devfreq_add_device()

[Chanwoo Choi]

Hi,

Applied it. Thanks

On Mon, Apr 21, 2025 at 12:01 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> The calling of devfreq_get_freq_range() in devfreq_add_device() is
> redundant because min_freq and max_freq are never used. Remove it.
>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/devfreq/devfreq.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 2810c84b9f8a..18e3f7e063a4 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -804,7 +804,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  {
>         struct devfreq *devfreq;
>         struct devfreq_governor *governor;
> -       unsigned long min_freq, max_freq;
>         int err = 0;
>
>         if (!dev || !profile || !governor_name) {
> @@ -872,8 +871,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>                 goto err_dev;
>         }
>
> -       devfreq_get_freq_range(devfreq, &min_freq, &max_freq);
> -
>         devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>         devfreq->opp_table = dev_pm_opp_get_opp_table(dev);
>         if (IS_ERR(devfreq->opp_table))
> --
> 2.33.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH 4/4] PM / devfreq: Check governor before using governor->name

[Chanwoo Choi]

Hi,

Applied it. Thanks.

On Mon, Apr 21, 2025 at 12:01 PM Lifeng Zheng <zhenglifeng1@huawei.com> wrote:
>
> Commit 96ffcdf239de ("PM / devfreq: Remove redundant governor_name from
> struct devfreq") removes governor_name and uses governor->name to replace
> it. But devfreq->governor may be NULL and directly using
> devfreq->governor->name may cause null pointer exception. Move the check of
> governor to before using governor->name.
>
> Fixes: 96ffcdf239de ("PM / devfreq: Remove redundant governor_name from struct devfreq")
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>  drivers/devfreq/devfreq.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 18e3f7e063a4..46f3a8053197 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1376,15 +1376,11 @@ int devfreq_remove_governor(struct devfreq_governor *governor)
>                 int ret;
>                 struct device *dev = devfreq->dev.parent;
>
> +               if (!devfreq->governor)
> +                       continue;
> +
>                 if (!strncmp(devfreq->governor->name, governor->name,
>                              DEVFREQ_NAME_LEN)) {
> -                       /* we should have a devfreq governor! */
> -                       if (!devfreq->governor) {
> -                               dev_warn(dev, "%s: Governor %s NOT present\n",
> -                                        __func__, governor->name);
> -                               continue;
> -                               /* Fall through */
> -                       }
>                         ret = devfreq->governor->event_handler(devfreq,
>                                                 DEVFREQ_GOV_STOP, NULL);
>                         if (ret) {
> --
> 2.33.0
>
>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics