All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] devfreq: replace sscanf with kstrtol
@ 2017-08-04  3:57 gsantosh
  2017-08-04  4:23 ` gsantosh
  2017-08-04 15:12 ` Chanwoo Choi
  0 siblings, 2 replies; 6+ messages in thread
From: gsantosh @ 2017-08-04  3:57 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel,
	Rafael J. Wysocki, gsantosh

Hi,

Adding error checks to devfreq userspace governor, the current 
implementation results in setting wrong
frequency when sscanf returns error.


 From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
 From: Santosh Mardi <gsantosh@codeaurora.org>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol

store_freq function of devfreq userspace governor
executes further, even if error is returned from sscanf,
this will result in setting up wrong frequency value.

The usage for the sscanf is only for single variable so
replace sscanf with kstrtol along with error check to
bail out if any error is returned.

Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
---
  drivers/devfreq/governor_userspace.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/governor_userspace.c 
b/drivers/devfreq/governor_userspace.c
index 77028c2..a84796d 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct 
device_attribute *attr,
  	mutex_lock(&devfreq->lock);
  	data = devfreq->data;

-	sscanf(buf, "%lu", &wanted);
+	err = kstrtol(buf, 0, &wanted);
+	if (err < 0)
+		goto out;
  	data->user_frequency = wanted;
  	data->valid = true;
  	err = update_devfreq(devfreq);
  	if (err == 0)
  		err = count;
+out:
  	mutex_unlock(&devfreq->lock);
  	return err;
  }
-- 

Regards,
Santosh M G.
Qualcomm Innovation Center

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] devfreq: replace sscanf with kstrtol
  2017-08-04  3:57 [PATCH] devfreq: replace sscanf with kstrtol gsantosh
@ 2017-08-04  4:23 ` gsantosh
  2017-08-04 15:12 ` Chanwoo Choi
  1 sibling, 0 replies; 6+ messages in thread
From: gsantosh @ 2017-08-04  4:23 UTC (permalink / raw)
  To: MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel,
	Rafael J. Wysocki, gsantosh

Hi,

Adding error checks to devfreq userspace governor, the current 
implementation results in setting wrong
frequency when sscanf returns error.


 From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
 From: Santosh Mardi <gsantosh@codeaurora.org>
Date: Tue, 25 Jul 2017 18:47:11 +0530
Subject: [PATCH] devfreq: replace sscanf with kstrtol

store_freq function of devfreq userspace governor
executes further, even if error is returned from sscanf,
this will result in setting up wrong frequency value.

The usage for the sscanf is only for single variable so
replace sscanf with kstrtol along with error check to
bail out if any error is returned.

Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
---
  drivers/devfreq/governor_userspace.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/governor_userspace.c 
b/drivers/devfreq/governor_userspace.c
index 77028c2..a84796d 100644
--- a/drivers/devfreq/governor_userspace.c
+++ b/drivers/devfreq/governor_userspace.c
@@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct 
device_attribute *attr,
      mutex_lock(&devfreq->lock);
      data = devfreq->data;

-    sscanf(buf, "%lu", &wanted);
+    err = kstrtol(buf, 0, &wanted);
+    if (err < 0)
+        goto out;
      data->user_frequency = wanted;
      data->valid = true;
      err = update_devfreq(devfreq);
      if (err == 0)
          err = count;
+out:
      mutex_unlock(&devfreq->lock);
      return err;
  }
-- 

Regards,
Santosh M G.
Qualcomm Innovation Center

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: replace sscanf with kstrtol
  2017-08-04  3:57 [PATCH] devfreq: replace sscanf with kstrtol gsantosh
  2017-08-04  4:23 ` gsantosh
@ 2017-08-04 15:12 ` Chanwoo Choi
  2017-08-07  4:47   ` gsantosh
  1 sibling, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2017-08-04 15:12 UTC (permalink / raw)
  To: gsantosh
  Cc: MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel,
	Rafael J. Wysocki, gsantosh

Hi,

On Fri, Aug 4, 2017 at 12:57 PM,  <gsantosh@codeaurora.org> wrote:
> Hi,
>
> Adding error checks to devfreq userspace governor, the current
> implementation results in setting wrong
> frequency when sscanf returns error.
>
>
> From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
> From: Santosh Mardi <gsantosh@codeaurora.org>
> Date: Tue, 25 Jul 2017 18:47:11 +0530
> Subject: [PATCH] devfreq: replace sscanf with kstrtol
>
> store_freq function of devfreq userspace governor
> executes further, even if error is returned from sscanf,
> this will result in setting up wrong frequency value.
>
> The usage for the sscanf is only for single variable so
> replace sscanf with kstrtol along with error check to
> bail out if any error is returned.
>
> Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
> ---
>  drivers/devfreq/governor_userspace.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/devfreq/governor_userspace.c
> b/drivers/devfreq/governor_userspace.c
> index 77028c2..a84796d 100644
> --- a/drivers/devfreq/governor_userspace.c
> +++ b/drivers/devfreq/governor_userspace.c
> @@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct
> device_attribute *attr,
>         mutex_lock(&devfreq->lock);
>         data = devfreq->data;
>
> -       sscanf(buf, "%lu", &wanted);
> +       err = kstrtol(buf, 0, &wanted);
> +       if (err < 0)
> +               goto out;

I think that just you can check the return value as following:
The other point of devfreq already uses the following style
to check the return value of sscanf. I think kstrtol is not necessary.

     err = sscanf(buf, "%lu", &wanted);
     if (err != 1)
          goto out;

And please use the scripts/get_maintainer.pl
in order to prevent the missing of the reviewer.

>         data->user_frequency = wanted;
>         data->valid = true;
>         err = update_devfreq(devfreq);
>         if (err == 0)
>                 err = count;
> +out:
>         mutex_unlock(&devfreq->lock);
>         return err;
>  }
> --
>
> Regards,
> Santosh M G.
> Qualcomm Innovation Center

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: replace sscanf with kstrtol
  2017-08-04 15:12 ` Chanwoo Choi
@ 2017-08-07  4:47   ` gsantosh
  2017-08-07  5:25     ` Chanwoo Choi
  0 siblings, 1 reply; 6+ messages in thread
From: gsantosh @ 2017-08-07  4:47 UTC (permalink / raw)
  To: cwchoi00, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel,
	Rafael J. Wysocki
  Cc: gsantosh, skannan

On 2017-08-04 20:42, Chanwoo Choi wrote:
> Hi,
> 
> On Fri, Aug 4, 2017 at 12:57 PM,  <gsantosh@codeaurora.org> wrote:
>> Hi,
>> 
>> Adding error checks to devfreq userspace governor, the current
>> implementation results in setting wrong
>> frequency when sscanf returns error.
>> 
>> 
>> From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
>> From: Santosh Mardi <gsantosh@codeaurora.org>
>> Date: Tue, 25 Jul 2017 18:47:11 +0530
>> Subject: [PATCH] devfreq: replace sscanf with kstrtol
>> 
>> store_freq function of devfreq userspace governor
>> executes further, even if error is returned from sscanf,
>> this will result in setting up wrong frequency value.
>> 
>> The usage for the sscanf is only for single variable so
>> replace sscanf with kstrtol along with error check to
>> bail out if any error is returned.
>> 
>> Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
>> ---
>>  drivers/devfreq/governor_userspace.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/devfreq/governor_userspace.c
>> b/drivers/devfreq/governor_userspace.c
>> index 77028c2..a84796d 100644
>> --- a/drivers/devfreq/governor_userspace.c
>> +++ b/drivers/devfreq/governor_userspace.c
>> @@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, 
>> struct
>> device_attribute *attr,
>>         mutex_lock(&devfreq->lock);
>>         data = devfreq->data;
>> 
>> -       sscanf(buf, "%lu", &wanted);
>> +       err = kstrtol(buf, 0, &wanted);
>> +       if (err < 0)
>> +               goto out;
> 
> I think that just you can check the return value as following:
> The other point of devfreq already uses the following style
> to check the return value of sscanf. I think kstrtol is not necessary.
> 
>      err = sscanf(buf, "%lu", &wanted);
>      if (err != 1)
>           goto out;
> 

[Santosh] - I Agree we need to have this error check as mentioned by you 
if we are scanning an arrary from the sscanf,
but in the above code we are only scanning one variable and there is a 
rule in the checkpatch scripts, not to use sscanf if it is a single 
variable, So I need to replace sscanf to strtol

I have added all the mails I got as output from 
scripts/get_maintainer.pl scripts in this mail.


> And please use the scripts/get_maintainer.pl
> in order to prevent the missing of the reviewer.
> 
>>         data->user_frequency = wanted;
>>         data->valid = true;
>>         err = update_devfreq(devfreq);
>>         if (err == 0)
>>                 err = count;
>> +out:
>>         mutex_unlock(&devfreq->lock);
>>         return err;
>>  }
>> --
>> 
>> Regards,
>> Santosh M G.
>> Qualcomm Innovation Center

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: replace sscanf with kstrtol
  2017-08-07  4:47   ` gsantosh
@ 2017-08-07  5:25     ` Chanwoo Choi
  2017-08-07 12:26       ` gsantosh
  0 siblings, 1 reply; 6+ messages in thread
From: Chanwoo Choi @ 2017-08-07  5:25 UTC (permalink / raw)
  To: gsantosh, cwchoi00, MyungJoo Ham, Kyungmin Park, linux-pm,
	linux-kernel, Rafael J. Wysocki
  Cc: gsantosh, skannan

Hi,

On 2017년 08월 07일 13:47, gsantosh@codeaurora.org wrote:
> On 2017-08-04 20:42, Chanwoo Choi wrote:
>> Hi,
>>
>> On Fri, Aug 4, 2017 at 12:57 PM,  <gsantosh@codeaurora.org> wrote:
>>> Hi,
>>>
>>> Adding error checks to devfreq userspace governor, the current
>>> implementation results in setting wrong
>>> frequency when sscanf returns error.
>>>
>>>
>>> From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 2001
>>> From: Santosh Mardi <gsantosh@codeaurora.org>
>>> Date: Tue, 25 Jul 2017 18:47:11 +0530
>>> Subject: [PATCH] devfreq: replace sscanf with kstrtol
>>>
>>> store_freq function of devfreq userspace governor
>>> executes further, even if error is returned from sscanf,
>>> this will result in setting up wrong frequency value.
>>>
>>> The usage for the sscanf is only for single variable so
>>> replace sscanf with kstrtol along with error check to
>>> bail out if any error is returned.
>>>
>>> Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
>>> ---
>>>  drivers/devfreq/governor_userspace.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/devfreq/governor_userspace.c
>>> b/drivers/devfreq/governor_userspace.c
>>> index 77028c2..a84796d 100644
>>> --- a/drivers/devfreq/governor_userspace.c
>>> +++ b/drivers/devfreq/governor_userspace.c
>>> @@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, struct
>>> device_attribute *attr,
>>>         mutex_lock(&devfreq->lock);
>>>         data = devfreq->data;
>>>
>>> -       sscanf(buf, "%lu", &wanted);
>>> +       err = kstrtol(buf, 0, &wanted);
>>> +       if (err < 0)
>>> +               goto out;
>>
>> I think that just you can check the return value as following:
>> The other point of devfreq already uses the following style
>> to check the return value of sscanf. I think kstrtol is not necessary.
>>
>>      err = sscanf(buf, "%lu", &wanted);
>>      if (err != 1)
>>           goto out;
>>
> 
> [Santosh] - I Agree we need to have this error check as mentioned by you if we are scanning an arrary from the sscanf,
> but in the above code we are only scanning one variable and there is a rule in the checkpatch scripts, not to use sscanf if it is a single variable, So I need to replace sscanf to strtol

IMHO, even if checkpatch shows the warning about sscanf,
I'd like you to use 'sscanf' in order to maintain
the consistency and readability when handling the sscanf.

For example, drivers/devfreq/devfreq.c and drivers/cpufreq/cpufreq.c
have the same warnings on many points.

> 
> I have added all the mails I got as output from scripts/get_maintainer.pl scripts in this mail.

Maybe, you missed including me (reviewer) to cc list.

MyungJoo Ham <myungjoo.ham@samsung.com> (maintainer:DEVICE FREQUENCY (DEVFREQ))
Kyungmin Park <kyungmin.park@samsung.com> (maintainer:DEVICE FREQUENCY (DEVFREQ))
Chanwoo Choi <cw00.choi@samsung.com> (reviewer:DEVICE FREQUENCY (DEVFREQ))
linux-pm@vger.kernel.org (open list:DEVICE FREQUENCY (DEVFREQ))
linux-kernel@vger.kernel.org (open list)

> 
> 
>> And please use the scripts/get_maintainer.pl
>> in order to prevent the missing of the reviewer.
>>
>>>         data->user_frequency = wanted;
>>>         data->valid = true;
>>>         err = update_devfreq(devfreq);
>>>         if (err == 0)
>>>                 err = count;
>>> +out:
>>>         mutex_unlock(&devfreq->lock);
>>>         return err;
>>>  }
>>> -- 
>>>
>>> Regards,
>>> Santosh M G.
>>> Qualcomm Innovation Center
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] devfreq: replace sscanf with kstrtol
  2017-08-07  5:25     ` Chanwoo Choi
@ 2017-08-07 12:26       ` gsantosh
  0 siblings, 0 replies; 6+ messages in thread
From: gsantosh @ 2017-08-07 12:26 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: cwchoi00, MyungJoo Ham, Kyungmin Park, linux-pm, linux-kernel,
	Rafael J. Wysocki, gsantosh, skannan

On 2017-08-07 10:55, Chanwoo Choi wrote:
> Hi,
> 
> On 2017년 08월 07일 13:47, gsantosh@codeaurora.org wrote:
>> On 2017-08-04 20:42, Chanwoo Choi wrote:
>>> Hi,
>>> 
>>> On Fri, Aug 4, 2017 at 12:57 PM,  <gsantosh@codeaurora.org> wrote:
>>>> Hi,
>>>> 
>>>> Adding error checks to devfreq userspace governor, the current
>>>> implementation results in setting wrong
>>>> frequency when sscanf returns error.
>>>> 
>>>> 
>>>> From 12e0a347addd70529b2c378299b27b65f0766f99 Mon Sep 17 00:00:00 
>>>> 2001
>>>> From: Santosh Mardi <gsantosh@codeaurora.org>
>>>> Date: Tue, 25 Jul 2017 18:47:11 +0530
>>>> Subject: [PATCH] devfreq: replace sscanf with kstrtol
>>>> 
>>>> store_freq function of devfreq userspace governor
>>>> executes further, even if error is returned from sscanf,
>>>> this will result in setting up wrong frequency value.
>>>> 
>>>> The usage for the sscanf is only for single variable so
>>>> replace sscanf with kstrtol along with error check to
>>>> bail out if any error is returned.
>>>> 
>>>> Signed-off-by: Santosh Mardi <gsantosh@codeaurora.org>
>>>> ---
>>>>  drivers/devfreq/governor_userspace.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/drivers/devfreq/governor_userspace.c
>>>> b/drivers/devfreq/governor_userspace.c
>>>> index 77028c2..a84796d 100644
>>>> --- a/drivers/devfreq/governor_userspace.c
>>>> +++ b/drivers/devfreq/governor_userspace.c
>>>> @@ -53,12 +53,15 @@ static ssize_t store_freq(struct device *dev, 
>>>> struct
>>>> device_attribute *attr,
>>>>         mutex_lock(&devfreq->lock);
>>>>         data = devfreq->data;
>>>> 
>>>> -       sscanf(buf, "%lu", &wanted);
>>>> +       err = kstrtol(buf, 0, &wanted);
>>>> +       if (err < 0)
>>>> +               goto out;
>>> 
>>> I think that just you can check the return value as following:
>>> The other point of devfreq already uses the following style
>>> to check the return value of sscanf. I think kstrtol is not 
>>> necessary.
>>> 
>>>      err = sscanf(buf, "%lu", &wanted);
>>>      if (err != 1)
>>>           goto out;
>>> 
>> 
>> [Santosh] - I Agree we need to have this error check as mentioned by 
>> you if we are scanning an arrary from the sscanf,
>> but in the above code we are only scanning one variable and there is a 
>> rule in the checkpatch scripts, not to use sscanf if it is a single 
>> variable, So I need to replace sscanf to strtol
> 
> IMHO, even if checkpatch shows the warning about sscanf,
> I'd like you to use 'sscanf' in order to maintain
> the consistency and readability when handling the sscanf.
> 
> For example, drivers/devfreq/devfreq.c and drivers/cpufreq/cpufreq.c
> have the same warnings on many points.

[Santosh] - Thanks, will change the patch to add the error check for 
sscanf and bail out.

> 
>> 
>> I have added all the mails I got as output from 
>> scripts/get_maintainer.pl scripts in this mail.
> 
> Maybe, you missed including me (reviewer) to cc list.
> 
> MyungJoo Ham <myungjoo.ham@samsung.com> (maintainer:DEVICE FREQUENCY 
> (DEVFREQ))
> Kyungmin Park <kyungmin.park@samsung.com> (maintainer:DEVICE FREQUENCY
> (DEVFREQ))
> Chanwoo Choi <cw00.choi@samsung.com> (reviewer:DEVICE FREQUENCY 
> (DEVFREQ))
> linux-pm@vger.kernel.org (open list:DEVICE FREQUENCY (DEVFREQ))
> linux-kernel@vger.kernel.org (open list)
> 

[Santosh] - Sorry, May be I am using bit older script file, will cross 
check this in future.

>> 
>> 
>>> And please use the scripts/get_maintainer.pl
>>> in order to prevent the missing of the reviewer.
>>> 
>>>>         data->user_frequency = wanted;
>>>>         data->valid = true;
>>>>         err = update_devfreq(devfreq);
>>>>         if (err == 0)
>>>>                 err = count;
>>>> +out:
>>>>         mutex_unlock(&devfreq->lock);
>>>>         return err;
>>>>  }
>>>> --
>>>> 
>>>> Regards,
>>>> Santosh M G.
>>>> Qualcomm Innovation Center
>> 
>> 
>> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-08-07 12:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-04  3:57 [PATCH] devfreq: replace sscanf with kstrtol gsantosh
2017-08-04  4:23 ` gsantosh
2017-08-04 15:12 ` Chanwoo Choi
2017-08-07  4:47   ` gsantosh
2017-08-07  5:25     ` Chanwoo Choi
2017-08-07 12:26       ` gsantosh

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.