From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753485AbdHGM0p (ORCPT ); Mon, 7 Aug 2017 08:26:45 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:52788 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753128AbdHGM0l (ORCPT ); Mon, 7 Aug 2017 08:26:41 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Date: Mon, 07 Aug 2017 17:56:40 +0530 From: gsantosh@codeaurora.org To: Chanwoo Choi Cc: cwchoi00@gmail.com, MyungJoo Ham , Kyungmin Park , linux-pm@vger.kernel.org, linux-kernel , "Rafael J. Wysocki" , gsantosh@qti.qualcomm.com, skannan@quicinc.com Subject: Re: [PATCH] devfreq: replace sscanf with kstrtol In-Reply-To: <5987F9CC.1080105@samsung.com> References: <5987F9CC.1080105@samsung.com> Message-ID: User-Agent: Roundcube Webmail/1.2.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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, 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 >>>> 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 >>>> --- >>>> 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 (maintainer:DEVICE FREQUENCY > (DEVFREQ)) > Kyungmin Park (maintainer:DEVICE FREQUENCY > (DEVFREQ)) > Chanwoo Choi (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 >> >> >>