Re: [PATCH] devfreq: replace sscanf with kstrtol

[gsantosh@codeaurora.org]

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