From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752015AbdHGFZg (ORCPT ); Mon, 7 Aug 2017 01:25:36 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:51552 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751686AbdHGFZe (ORCPT ); Mon, 7 Aug 2017 01:25:34 -0400 X-AuditID: b6c32a45-f79466d000002ac6-d4-5987f9cb0896 MIME-version: 1.0 Content-transfer-encoding: 8BIT Content-type: text/plain; charset="UTF-8" Message-id: <5987F9CC.1080105@samsung.com> Date: Mon, 07 Aug 2017 14:25:32 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: gsantosh@codeaurora.org, cwchoi00@gmail.com, MyungJoo Ham , Kyungmin Park , linux-pm@vger.kernel.org, linux-kernel , "Rafael J. Wysocki" Cc: gsantosh@qti.qualcomm.com, skannan@quicinc.com Subject: Re: [PATCH] devfreq: replace sscanf with kstrtol In-reply-to: X-Brightmail-Tracker: H4sIAAAAAAAAA01Se0hTcRjtd+/cvUaz20r7GGXr0oMMl5t7XCutyPRSEevxR/Zg3eZlW7pN dmdkb8V0raaJSLGEzB5mVpaZ9DA0s6f0oDCCcj0UexsklRFW265R/53vcM73OHwkLj8uVZA2 h5t3ObhsWjpc0nR9mi6+40dxRkJ9tYHpvTGd+dreIGHOBr7gzL2CjwTz+HKllOn3tSPmWf4J KdN94hPB/KzyRcyNZB+X+DD2kr+LYI80v8PY6opexJY1b2dLGk8itr8h1kis4mdbeS6Tdyl5 h9mZaXNYkulFy03zTTp9gjpencQYaKWDs/PJdOpiY3yaLTu4GK3cyGXnBikjJwj0jJTZLmeu m1danYI7mV6tVmtU6gSDSqPRqLSJa2dqdEHJOt56pqhQktMybtOumkJsJ7oW40WRJFBaqDvY hUQcAw8D9VIvGk7KqYsIrpy9isSiH4Fvdy3hRWTYca2GEfnTCOpedBAht4waBQPlAUlIg1MT oP1RVojGqWlw4Uo9IeoDCBr3vMJEfRzc7e2UhrCEmgwlPUciQlga5FvePg3zI6mJ0DnQHd4u mloJlw59CzcaQxVgUP/sByFO0MDhutd4CI+mkuBCaWcYR1Ip8N5TJgkZgLpPwOCD50i8YDw0 tOLiyanw6/dDTMSj4f2tRkLECtjj8ROitxjB+crAUCMPghe1+RGiKhHuevMxcYso8FwfHIpI Bp4iuShhofRY99CwedDk7ZOIUezFIHC4WroPKf3/pef/l57/v/SqEH4SxfA5gt3CC5octUrg 7EKuw6IyO+0NKPyjcQsuogP3F7chikT0CFmrvzhDHsFtFPLsbQhInB4js9uClCyTy9vMu5wm V242L7QhXTD8MlwRbXYGP97hNqm1SQlavV5j0DPBzxsr29H0ZKWcsnBuPovnc3jXXx9GRip2 IlfUFMWsVvQ2euQhVWDNlvLmNwPn5EbDiK7lijzPfmXFNnqqrHLJ66x+xaCNI6p1t9f3aFsX LOvTD0u/4zN/f3V8wsSq2oW32AOKHbFHjUtNPRtupm31v3y04jPTp7VmKGd9SC+oGXbMcsqc GTvn+f6r+4zjm4rLJ7WQ6Xt3//yV2EFLBCunjsNdAvcH+Qd+1rkDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrHIsWRmVeSWpSXmKPExsVy+t9jQd3TP9sjDZbfFrJ4dlTb4uuRTSwW G+59YrY42/SG3eLyrjlsFp97jzBa3G5cwWbxeMVbdovfC3pZHTg9Lvf1MnnsnHWX3WPxnpdM HoumPmP0mLinzqNvyypGj8+b5ALYo9xsMlITU1KLFFLzkvNTMvPSbZVCQ9x0LZQU8hJzU22V InR9Q4KUFMoSc0qBPCMDNODgHOAerKRvl+CWsa6thaVgv0xF6/IWpgbGg2JdjBwcEgImEgeX W3QxcgKZYhIX7q1nA7GFBFYzSjzbmwVi8woISvyYfI8FpJxZQF7iyKVskDCzgLrEpHmLmLsY uYDKHzBKfFu1iBWiXkvi1LOrYHNYBFQl+p4sBouzAcX3v7gBFucXUJS4+uMxI8hMUYEIie4T lSBzRARamCR+vulihFhgJLFw9SNmEFtYwFJia/9VqGU9TBJzH08EK+IUsJN41TGRZQKj4Cwk t85CuHUWklsXMDKvYhRLLSjOTc8tNiow0itOzC0uzUvXS87P3cQIjMtth7UCdjA2nYs+xCjA wajEw3tgVnukEGtiWXFl7iFGCQ5mJRHe3EygEG9KYmVValF+fFFpTmrxIUZToFcnMkuJJucD U0ZeSbyhiaWRiYGZmaGRgbGZkjjvhMAvEUIC6YklqdmpqQWpRTB9TBycUg2MQlxLFpzZGPru atuBmY9b+adz8UQbH2SJMJ3dyh7KE/xjjUNawuHZr/y3TX/FYhzzuLWFY33gBbFzPjsaI9ax tHWyV74/k/Xss3QMd+2SU12H2OQ/aVmt05OKnHSPWeLJWZEbvGfm6bfvXsPf4fyKl3U+g9P7 hznK6xxya34f75y5rnLm0XWzlFiKMxINtZiLihMBRf+Mh+ECAAA= X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170807052531epcas2p2864f7abb269d8cd2390fce55c9aae00e X-Msg-Generator: CA X-Sender-IP: 182.195.42.80 X-Local-Sender: =?UTF-8?B?7LWc7LCs7JqwG1RpemVuIFBsYXRmb3JtIExhYihTL1fshLw=?= =?UTF-8?B?7YSwKRvsgrzshLHsoITsnpAbU2VuaW9yIEVuZ2luZWVy?= X-Global-Sender: =?UTF-8?B?Q2hhbndvbyBDaG9pG1RpemVuIFBsYXRmb3JtIExhYi4bU2Ft?= =?UTF-8?B?c3VuZyBFbGVjdHJvbmljcxtTZW5pb3IgRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG1RFTEUbQzEwVjgxMTE=?= CMS-TYPE: 102P DLP-Filter: Pass X-CFilter-Loop: Reflected X-CMS-RootMailID: 20170807044753epcas3p25273149fa9751d026f4dce5aa48d21f8 X-RootMTR: 20170807044753epcas3p25273149fa9751d026f4dce5aa48d21f8 References: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > 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) > > >> 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