lkml.org 
[lkml]   [2017]   [Aug]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] devfreq: replace sscanf with kstrtol
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
>>
>>
>>

\
 
 \ /
  Last update: 2017-08-07 14:27    [W:0.098 / U:0.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site