From patchwork Tue May 24 21:30:09 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rafael Wysocki X-Patchwork-Id: 813782 Received: from smtp1.linux-foundation.org (smtp1.linux-foundation.org [140.211.169.13]) by demeter1.kernel.org (8.14.4/8.14.3) with ESMTP id p4OLX8fh031965 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=FAIL) for ; Tue, 24 May 2011 21:33:28 GMT Received: from daredevil.linux-foundation.org (localhost [127.0.0.1]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p4OLUHZa005079; Tue, 24 May 2011 14:30:45 -0700 Received: from ogre.sisk.pl (ogre.sisk.pl [217.79.144.158]) by smtp1.linux-foundation.org (8.14.2/8.13.5/Debian-3ubuntu1.1) with ESMTP id p4OLTkp5004989 for ; Tue, 24 May 2011 14:29:48 -0700 Received: from localhost (localhost.localdomain [127.0.0.1]) by ogre.sisk.pl (Postfix) with ESMTP id 934681B0103; Tue, 24 May 2011 23:18:14 +0200 (CEST) Received: from ogre.sisk.pl ([127.0.0.1]) by localhost (ogre.sisk.pl [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 25407-09; Tue, 24 May 2011 23:17:53 +0200 (CEST) Received: from ferrari.rjw.lan (220-bem-13.acn.waw.pl [82.210.184.220]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ogre.sisk.pl (Postfix) with ESMTP id D0D5E1AFE16; Tue, 24 May 2011 23:17:53 +0200 (CEST) From: "Rafael J. Wysocki" To: markgross@thegnar.org Date: Tue, 24 May 2011 23:30:09 +0200 User-Agent: KMail/1.13.6 (Linux/2.6.39+; KDE/4.6.0; x86_64; ; ) References: <20110523150636.ccd83777.sfr@canb.auug.org.au> <201105232154.52344.rjw@sisk.pl> <20110524063424.GC18075@gvim.org> In-Reply-To: <20110524063424.GC18075@gvim.org> MIME-Version: 1.0 Message-Id: <201105242330.09491.rjw@sisk.pl> X-Virus-Scanned: amavisd-new at ogre.sisk.pl using MkS_Vir for Linux Received-SPF: pass (localhost is always allowed.) X-Spam-Status: No, hits=-3.935 required=5 tests=AWL, BAYES_00, OSDL_HEADER_SUBJECT_BRACKETED X-Spam-Checker-Version: SpamAssassin 3.2.4-osdl_revision__1.47__ X-MIMEDefang-Filter: lf$Revision: 1.188 $ X-Scanned-By: MIMEDefang 2.63 on 140.211.169.21 Cc: Stephen Rothwell , linux-kernel@vger.kernel.org, linux-next@vger.kernel.org, Linux PM mailing list Subject: Re: [linux-pm] linux-next: build warning after merge of the suspend tree X-BeenThere: linux-pm@lists.linux-foundation.org X-Mailman-Version: 2.1.9 Precedence: list List-Id: Linux power management List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter1.kernel.org [140.211.167.41]); Tue, 24 May 2011 21:33:28 +0000 (UTC) On Tuesday, May 24, 2011, mark gross wrote: > On Mon, May 23, 2011 at 09:54:52PM +0200, Rafael J. Wysocki wrote: > > On Monday, May 23, 2011, mark gross wrote: > > > On Mon, May 23, 2011 at 03:06:36PM +1000, Stephen Rothwell wrote: > > > > Hi Rafael, > > > > > > > > After merging the suspend tree, today's linux-next build (i386 defconfig > > > > among others) produced this warning: > > > > > > > > kernel/pm_qos_params.c: In function 'pm_qos_power_write': > > > > kernel/pm_qos_params.c:420: warning: passing argument 3 of 'kstrtol' from incompatible pointer type > > > > include/linux/kernel.h:210: note: expected 'long int *' but argument is of type 's32 *' > > > > > > > > Intreoduced by commit 365daa955e03 ("PM: Correct PM QOS's user mode > > > > interface to work with ascii input per"). > > > > > > Gah! I'm sorry about that. > > > > > > attached is a fix. > > > > > > > > > --mark > > > > > > signed-off-by:markgross > > > > > > > > > > > > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001 > > > From: mgross > > > Date: Mon, 23 May 2011 07:14:09 -0700 > > > Subject: [PATCH] clean up a compile time warning in the use of strict_strtol but that was > > > passing an s32 * when it should be passing a long * > > > > > > --- > > > kernel/pm_qos_params.c | 6 ++++-- > > > 1 files changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c > > > index d61ecf3..dd37c56 100644 > > > --- a/kernel/pm_qos_params.c > > > +++ b/kernel/pm_qos_params.c > > > @@ -405,6 +405,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf, > > > size_t count, loff_t *f_pos) > > > { > > > s32 value; > > > + long safe_int; > > > int x; > > > char ascii_value[11]; > > > struct pm_qos_request_list *pm_qos_req; > > > @@ -417,10 +418,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf, > > > ascii_value[count] = 0; > > > if (copy_from_user(ascii_value, buf, count)) > > > return -EFAULT; > > > - if ((x=strict_strtol(ascii_value, 16, &value)) != 0){ > > > - pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x); > > > + if ((x=strict_strtol(ascii_value, 16, &safe_int)) != 0){ > > > + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x); > > > return -EINVAL; > > > } > > > + value = (s32) safe_int; > > > > Well, this doesn't seem quite right. > > > > > } else > > > return -EINVAL; > > > > Besides, if count == 11, there we'll write beyond ascii_value[], right? > > right. That is a bug. > > > > > What about the appended patch instead of your original one? > Thank you! > > > Rafael > > > > > > --- > > From: mark gross > > Subject: PM: Correct PM QOS's user mode interface to work with ascii input per > > > > What is in the kernel docs. Writing a string to the ABI from user mode > > comes in 2 flavors. One with and one without a '\n' at the end. This > > change accepts both. > > > > # echo 0x12345678 > /dev/cpu_dma_latency > > > > and > > > > # echo -n 0x12345678 > /dev/cpu_dma_latency > > > > now both work. > > > > [rjw: Fixed up array bounds checking and type casting.] > > > > Signed-off-by: mark gross > > Acked-by: Simon Horman > > Signed-off-by: Rafael J. Wysocki > > --- > > kernel/pm_qos_params.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > Index: linux-2.6/kernel/pm_qos_params.c > > =================================================================== > > --- linux-2.6.orig/kernel/pm_qos_params.c > > +++ linux-2.6/kernel/pm_qos_params.c > > @@ -40,6 +40,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -404,24 +405,30 @@ static ssize_t pm_qos_power_write(struct > > size_t count, loff_t *f_pos) > > { > > s32 value; > > - int x; > > char ascii_value[11]; > > struct pm_qos_request_list *pm_qos_req; > > > > if (count == sizeof(s32)) { > > if (copy_from_user(&value, buf, sizeof(s32))) > > return -EFAULT; > > - } else if (count == 11) { /* len('0x12345678/0') */ > > - if (copy_from_user(ascii_value, buf, 11)) > > + } else if (count >= 10) { /* '0x12345678' or '0x12345678\n'*/ > Did you mean <= 11 here? I know that Alan Stern wanted to see the ABI > adjusted to accept shorter hex string that the full 10 char string. > > FWIW I'm ok with doing that. > > > + unsigned long int ulval; > > + int ret; > > + > > + count = 10; > > I kind of wanted to return an error to the user if they violate the > documented format for the hex string. With this code you get a silent > truncation of the lower digits if the string is "0x123456789abcdef" That's correct, but with the previous code there will be a truncation is "echo -n 0x123456789". If we don't want that, we should check if the last character is a '\n'. > > + ascii_value[count] = 0; > > + if (copy_from_user(ascii_value, buf, count)) > > return -EFAULT; > > - if (strlen(ascii_value) != 10) > > - return -EINVAL; > > - x = sscanf(ascii_value, "%x", &value); > > - if (x != 1) > > + > > + ret = strict_strtoul(ascii_value, 16, &ulval); > > + if (ret){ > > + pr_debug("%s, 0x%lx, 0x%x\n", ascii_value, ulval, ret); > > return -EINVAL; > > - pr_debug("%s, %d, 0x%x\n", ascii_value, x, value); > > - } else > > + } > > + value = (s32)lower_32_bits(ulval); > This shouldn't be needed if you are constrained to a hex format of > "0x12345678" but this is more readable. lower_32_bits is a good idea. OK Appended is another patch that adds the '\n' check and reworks the code a bit (so that it will accept strings like "0x1" too). Thanks, Rafael Acked-by: markgross --- From: Rafael J. Wysocki Subject: PM: Fix PM QOS's user mode interface to work with ASCII input Make pm_qos_power_write() accept values passed to it in the ASCII hex format either with or without an ending newline. Signed-off-by: Rafael J. Wysocki --- kernel/pm_qos_params.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) Index: linux-2.6/kernel/pm_qos_params.c =================================================================== --- linux-2.6.orig/kernel/pm_qos_params.c +++ linux-2.6/kernel/pm_qos_params.c @@ -40,6 +40,7 @@ #include #include #include +#include #include @@ -404,24 +405,36 @@ static ssize_t pm_qos_power_write(struct size_t count, loff_t *f_pos) { s32 value; - int x; - char ascii_value[11]; struct pm_qos_request_list *pm_qos_req; if (count == sizeof(s32)) { if (copy_from_user(&value, buf, sizeof(s32))) return -EFAULT; - } else if (count == 11) { /* len('0x12345678/0') */ - if (copy_from_user(ascii_value, buf, 11)) + } else if (count <= 11) { /* ASCII perhaps? */ + char ascii_value[11]; + unsigned long int ulval; + int ret; + + if (copy_from_user(ascii_value, buf, count)) return -EFAULT; - if (strlen(ascii_value) != 10) - return -EINVAL; - x = sscanf(ascii_value, "%x", &value); - if (x != 1) + + if (count > 10) { + if (ascii_value[10] == '\n') + ascii_value[10] = '\0'; + else + return -EINVAL; + } else { + ascii_value[count] = '\0'; + } + ret = strict_strtoul(ascii_value, 16, &ulval); + if (ret) { + pr_debug("%s, 0x%lx, 0x%x\n", ascii_value, ulval, ret); return -EINVAL; - pr_debug("%s, %d, 0x%x\n", ascii_value, x, value); - } else + } + value = (s32)lower_32_bits(ulval); + } else { return -EINVAL; + } pm_qos_req = filp->private_data; pm_qos_update_request(pm_qos_req, value);