* linux-next: build warning after merge of the suspend tree
@ 2010-02-11 6:10 Stephen Rothwell
2010-02-11 13:11 ` Rafael J. Wysocki
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Rothwell @ 2010-02-11 6:10 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-next, linux-kernel, Alan Stern
[-- Attachment #1: Type: text/plain, Size: 466 bytes --]
Hi Rafael,
After merging the suspend tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:
drivers/usb/core/hcd-pci.c:138: warning: 'wait_for_companions' defined but not used
Introduced by commit ce74abf51a15ab82a9fae6dcd8a9c0038945f10e ("USB:
implement non-tree resume ordering constraints for PCI host controllers").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2010-02-11 6:10 linux-next: build warning after merge of the suspend tree Stephen Rothwell
@ 2010-02-11 13:11 ` Rafael J. Wysocki
2010-02-11 15:32 ` Alan Stern
0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2010-02-11 13:11 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, linux-kernel, Alan Stern
On Thursday 11 February 2010, Stephen Rothwell wrote:
> Hi Rafael,
>
> After merging the suspend tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
>
> drivers/usb/core/hcd-pci.c:138: warning: 'wait_for_companions' defined but not used
>
> Introduced by commit ce74abf51a15ab82a9fae6dcd8a9c0038945f10e ("USB:
> implement non-tree resume ordering constraints for PCI host controllers").
Thanks for the info, I'll fix this up later today in my tree.
Rafael
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2010-02-11 13:11 ` Rafael J. Wysocki
@ 2010-02-11 15:32 ` Alan Stern
0 siblings, 0 replies; 21+ messages in thread
From: Alan Stern @ 2010-02-11 15:32 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Stephen Rothwell, linux-next, linux-kernel
On Thu, 11 Feb 2010, Rafael J. Wysocki wrote:
> On Thursday 11 February 2010, Stephen Rothwell wrote:
> > Hi Rafael,
> >
> > After merging the suspend tree, today's linux-next build (powerpc
> > ppc64_defconfig) produced this warning:
> >
> > drivers/usb/core/hcd-pci.c:138: warning: 'wait_for_companions' defined but not used
> >
> > Introduced by commit ce74abf51a15ab82a9fae6dcd8a9c0038945f10e ("USB:
> > implement non-tree resume ordering constraints for PCI host controllers").
>
> Thanks for the info, I'll fix this up later today in my tree.
Clearly wait_for_companions() needs to be inside "#ifdef
CONFIG_PM_SLEEP ... #endif". In fact, all the new functions added by
the patch should be protected this way, with suitable do-nothing inline
versions provided when CONFIG_PM_SLEEP isn't defined.
Alan Stern
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
@ 2010-02-11 15:32 ` Alan Stern
0 siblings, 0 replies; 21+ messages in thread
From: Alan Stern @ 2010-02-11 15:32 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Stephen Rothwell, linux-next, linux-kernel
On Thu, 11 Feb 2010, Rafael J. Wysocki wrote:
> On Thursday 11 February 2010, Stephen Rothwell wrote:
> > Hi Rafael,
> >
> > After merging the suspend tree, today's linux-next build (powerpc
> > ppc64_defconfig) produced this warning:
> >
> > drivers/usb/core/hcd-pci.c:138: warning: 'wait_for_companions' defined but not used
> >
> > Introduced by commit ce74abf51a15ab82a9fae6dcd8a9c0038945f10e ("USB:
> > implement non-tree resume ordering constraints for PCI host controllers").
>
> Thanks for the info, I'll fix this up later today in my tree.
Clearly wait_for_companions() needs to be inside "#ifdef
CONFIG_PM_SLEEP ... #endif". In fact, all the new functions added by
the patch should be protected this way, with suitable do-nothing inline
versions provided when CONFIG_PM_SLEEP isn't defined.
Alan Stern
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-24 21:30 ` Rafael J. Wysocki
@ 2011-05-26 1:58 ` mark gross
2011-05-26 1:58 ` mark gross
1 sibling, 0 replies; 21+ messages in thread
From: mark gross @ 2011-05-26 1:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: markgross, Stephen Rothwell, linux-next, linux-kernel,
Simon Horman, Linux PM mailing list, Alan Stern
On Tue, May 24, 2011 at 11:30:09PM +0200, Rafael J. Wysocki wrote:
> 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 <markgross@thegnar.org>
> > > >
> > > >
> > > >
> > > > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > > > From: mgross <mgross@cr48>
> > > > 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 <markgross@thegnar.org>
> > > 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 <markgross@thegnar.org>
> > > Acked-by: Simon Horman <horms@verge.net.au>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > > 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 <linux/string.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/init.h>
> > > +#include <linux/kernel.h>
> > >
> > > #include <linux/uaccess.h>
> > >
> > > @@ -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
>
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 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 <rjw@sisk.pl>
> ---
> 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 <linux/string.h>
> #include <linux/platform_device.h>
> #include <linux/init.h>
> +#include <linux/kernel.h>
>
> #include <linux/uaccess.h>
>
> @@ -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);
looks good to me.
Acked-by: markgross <markgross@thegnar.org>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-26 1:58 ` mark gross
@ 2011-05-26 1:58 ` mark gross
1 sibling, 0 replies; 21+ messages in thread
From: mark gross @ 2011-05-26 1:58 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stephen Rothwell, markgross, linux-kernel, linux-next,
Linux PM mailing list
On Tue, May 24, 2011 at 11:30:09PM +0200, Rafael J. Wysocki wrote:
> 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 <markgross@thegnar.org>
> > > >
> > > >
> > > >
> > > > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > > > From: mgross <mgross@cr48>
> > > > 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 <markgross@thegnar.org>
> > > 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 <markgross@thegnar.org>
> > > Acked-by: Simon Horman <horms@verge.net.au>
> > > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > > ---
> > > 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 <linux/string.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/init.h>
> > > +#include <linux/kernel.h>
> > >
> > > #include <linux/uaccess.h>
> > >
> > > @@ -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
>
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> 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 <rjw@sisk.pl>
> ---
> 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 <linux/string.h>
> #include <linux/platform_device.h>
> #include <linux/init.h>
> +#include <linux/kernel.h>
>
> #include <linux/uaccess.h>
>
> @@ -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);
looks good to me.
Acked-by: markgross <markgross@thegnar.org>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-24 6:34 ` mark gross
2011-05-24 21:30 ` Rafael J. Wysocki
@ 2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-26 1:58 ` mark gross
2011-05-26 1:58 ` mark gross
1 sibling, 2 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2011-05-24 21:30 UTC (permalink / raw)
To: markgross
Cc: Stephen Rothwell, linux-next, linux-kernel, Simon Horman,
Linux PM mailing list, Alan Stern
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 <markgross@thegnar.org>
> > >
> > >
> > >
> > > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > > From: mgross <mgross@cr48>
> > > 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 <markgross@thegnar.org>
> > 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 <markgross@thegnar.org>
> > Acked-by: Simon Horman <horms@verge.net.au>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> > 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 <linux/string.h>
> > #include <linux/platform_device.h>
> > #include <linux/init.h>
> > +#include <linux/kernel.h>
> >
> > #include <linux/uaccess.h>
> >
> > @@ -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
---
From: Rafael J. Wysocki <rjw@sisk.pl>
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 <rjw@sisk.pl>
---
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 <linux/string.h>
#include <linux/platform_device.h>
#include <linux/init.h>
+#include <linux/kernel.h>
#include <linux/uaccess.h>
@@ -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);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-24 6:34 ` mark gross
@ 2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-24 21:30 ` Rafael J. Wysocki
1 sibling, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2011-05-24 21:30 UTC (permalink / raw)
To: markgross
Cc: Stephen Rothwell, linux-kernel, linux-next, Linux PM mailing list
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 <markgross@thegnar.org>
> > >
> > >
> > >
> > > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > > From: mgross <mgross@cr48>
> > > 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 <markgross@thegnar.org>
> > 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 <markgross@thegnar.org>
> > Acked-by: Simon Horman <horms@verge.net.au>
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> > 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 <linux/string.h>
> > #include <linux/platform_device.h>
> > #include <linux/init.h>
> > +#include <linux/kernel.h>
> >
> > #include <linux/uaccess.h>
> >
> > @@ -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
---
From: Rafael J. Wysocki <rjw@sisk.pl>
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 <rjw@sisk.pl>
---
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 <linux/string.h>
#include <linux/platform_device.h>
#include <linux/init.h>
+#include <linux/kernel.h>
#include <linux/uaccess.h>
@@ -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);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 19:54 ` Rafael J. Wysocki
@ 2011-05-24 6:34 ` mark gross
2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-24 21:30 ` Rafael J. Wysocki
0 siblings, 2 replies; 21+ messages in thread
From: mark gross @ 2011-05-24 6:34 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: markgross, Stephen Rothwell, linux-next, linux-kernel,
Milton Miller, Simon Horman
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 <markgross@thegnar.org>
> >
> >
> >
> > From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > From: mgross <mgross@cr48>
> > 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 <markgross@thegnar.org>
> 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 <markgross@thegnar.org>
> Acked-by: Simon Horman <horms@verge.net.au>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
> 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 <linux/string.h>
> #include <linux/platform_device.h>
> #include <linux/init.h>
> +#include <linux/kernel.h>
>
> #include <linux/uaccess.h>
>
> @@ -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"
> + 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.
--mark
> + } else {
> return -EINVAL;
> + }
>
> pm_qos_req = filp->private_data;
> pm_qos_update_request(pm_qos_req, value);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 17:42 ` Milton Miller
@ 2011-05-24 6:13 ` mark gross
0 siblings, 0 replies; 21+ messages in thread
From: mark gross @ 2011-05-24 6:13 UTC (permalink / raw)
To: Milton Miller
Cc: mgross, Rafael J. Wysocki, linux-next, linux-kernel,
Randy Dunlap, Stephen Rothwell
On Mon, May 23, 2011 at 12:42:18PM -0500, Milton Miller wrote:
> [ I managed to botch my own email the first time, how embarrassing!
> So I added Randy and updated the paragraph about negative values.
> And then I need a new Message-Id too. ]
I any more resends from you and I'll have to diff your emails to see
what new comments you have come up with.
FWIW Normally when I submit a patch for quick review this is what
happens. I should be more careful. Please note that I didn't start my
subject line with a [patch].
>
> On Mon, 23 May 2011 about 14:18:46 -0000, mgross 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 <markgross@thegnar.org>
> >
>
> (1) This should be in the patch, not the enclosing letter
> (2) Incorrect capitalization
> (3) Incorrect spacing
>
> Please read Documentation/SubmittingPatches again.
Yes I will do that.
>
> >
> >
> > >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > From: mgross <mgross@cr48>
> > 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 *
> >
>
> From should match Signed-off-by:
>
> Please seperate title (subject) and description body
>
> Maybe: pm_qos: strict_strtol takes a long, not s32
>
> strict_strtol takes a pointer to long to store the converted value.
> introduced in xxxx ("change set title here")
>
> So that the reviewers can quickly see if it needs to be backported
> to stable etc.
>
> except read below
>
>
> > ---
> > 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){
>
> Why are you doing an assignment in the if? Why not assign first and
> compare later?
>
> > + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
> > return -EINVAL;
>
> Nit: Some reason not to return -ERANGE if thats what strtol returned?
> While folding the error to -EINVAL is ok, it hides diagnostic informatio
> from the user.
I think EINVAL matches the documentation for this ABI better that
ERANGE.
>
> > }
> > + value = (s32) safe_int;
>
> You call strict checking, which includes overflow checking, but
> only that the value fits in a long. You then defeat that checking
> by casting to int.
The documentation for the ABI says that it has to be a hex value of the
formation 0x12345678 otherwise its not valid. s32 is big enough for
that. I thought about masking for a second and decided this is good
enough.
> It looks like you want strict_strtoint except thats not defined.
> Hoever, the pattern for strict_strto* is kstrto* and kstrtos32 is
> defined ...
hmm, I'll look at the strtos32. That is what I would like.
>
> > } else
> > return -EINVAL;
> >
>
> Oh, and you now may copy 11 characters from userspace into an 11
> character buffer then terminate it by writing the 12th character
> (ascii_value[count == 11]). Except its an 11 character array.
yes, if count = 11 then this code is overwriting by one byte :( I must
have gotten luckly because 11 is an odd number and the compiler padded
it from me. I'll fix that in a future patch.
> The variable is a s32, aparently in native endian if pased in binary
> as 4 bytes. What is the magic to set the value to a negative number
> through the ascii interface? Is yet another character for the -
> required?
No. The ABI documentation is pretty clear about the text format being
simple hex 0x12345678 styled.
--mark
>
>
> I see the string from userspace wasn't properly terminated before
> either. In ed77134bfccf5e75b (PM QOS update), merged in v2.6.35-rc1,
> 11 bytes were copied from user then passed to ssscanf without null
> termination forced. It was updated in 0109c2c48d (PM QoS: Correct
> pr_debug() misuse and improve parameter checks), which was merged in
> 2.6.36-rc4, to change the the function that walks off the string from
> sscanf to strlen. That changelog isn't marked for stable (I didn't
> look if it was sent) but it still isn't force terminated.
>
> happy patching,
> milton
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 14:18 ` mark gross
2011-05-23 15:24 ` Randy Dunlap
2011-05-23 16:21 ` Milton Miller
@ 2011-05-23 19:54 ` Rafael J. Wysocki
2011-05-24 6:34 ` mark gross
2 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2011-05-23 19:54 UTC (permalink / raw)
To: markgross
Cc: Stephen Rothwell, linux-next, linux-kernel, Milton Miller, Simon Horman
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 <markgross@thegnar.org>
>
>
>
> From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@cr48>
> 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?
What about the appended patch instead of your original one?
Rafael
---
From: mark gross <markgross@thegnar.org>
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 <markgross@thegnar.org>
Acked-by: Simon Horman <horms@verge.net.au>
Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
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 <linux/string.h>
#include <linux/platform_device.h>
#include <linux/init.h>
+#include <linux/kernel.h>
#include <linux/uaccess.h>
@@ -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'*/
+ unsigned long int ulval;
+ int ret;
+
+ count = 10;
+ 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);
+ } else {
return -EINVAL;
+ }
pm_qos_req = filp->private_data;
pm_qos_update_request(pm_qos_req, value);
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 16:21 ` Milton Miller
(?)
(?)
@ 2011-05-23 18:57 ` Rafael J. Wysocki
-1 siblings, 0 replies; 21+ messages in thread
From: Rafael J. Wysocki @ 2011-05-23 18:57 UTC (permalink / raw)
To: Milton Miller; +Cc: mgross, linux-next, linux-kernel, Stephen Rothwell
On Monday, May 23, 2011, Milton Miller wrote:
> On Mon, 23 May 2011 about 14:18:46 -0000, mgross 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 <markgross@thegnar.org>
> >
>
> (1) This should be in the patch, not the enclosing letter
That message is for me, actually. I can fold the fix into the patch.
> (2) Incorrect capitalization
Doesn't matter, I can fix it up.
> (3) Incorrect spacing
Likewise.
> Please read Documentation/SubmittingPatches again.
>
> >
> >
> > >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> > From: mgross <mgross@cr48>
> > 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 *
> >
>
> From should match Signed-off-by:
Not necessarily.
> Please seperate title (subject) and description body
Doesn't matter here.
> Maybe: pm_qos: strict_strtol takes a long, not s32
>
> strict_strtol takes a pointer to long to store the converted value.
> introduced in xxxx ("change set title here")
>
> So that the reviewers can quickly see if it needs to be backported
> to stable etc.
>
> except read below
>
>
> > ---
> > 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){
>
> Why are you doing an assignment in the if? Why not assign first and
> compare later?
Because that's what the original code does?
> > + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
> > return -EINVAL;
>
> Nit: Some reason not to return -ERANGE if thats what strtol returned?
> Folding to -EINVAL is ok but hides information.
>
> > }
> > + value = (s32) safe_int;
>
> You call strict checking, which includes overflow checking, but
> only that the value fits in a long. You then defeat that checking
> by casting to int.
That actually is a good point.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 16:21 ` Milton Miller
(?)
@ 2011-05-23 17:42 ` Milton Miller
2011-05-24 6:13 ` mark gross
-1 siblings, 1 reply; 21+ messages in thread
From: Milton Miller @ 2011-05-23 17:42 UTC (permalink / raw)
To: mgross
Cc: Rafael J. Wysocki, linux-next, linux-kernel, Randy Dunlap,
Stephen Rothwell
[ I managed to botch my own email the first time, how embarrassing!
So I added Randy and updated the paragraph about negative values.
And then I need a new Message-Id too. ]
On Mon, 23 May 2011 about 14:18:46 -0000, mgross 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 <markgross@thegnar.org>
>
(1) This should be in the patch, not the enclosing letter
(2) Incorrect capitalization
(3) Incorrect spacing
Please read Documentation/SubmittingPatches again.
>
>
> >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@cr48>
> 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 *
>
>From should match Signed-off-by:
Please seperate title (subject) and description body
Maybe: pm_qos: strict_strtol takes a long, not s32
strict_strtol takes a pointer to long to store the converted value.
introduced in xxxx ("change set title here")
So that the reviewers can quickly see if it needs to be backported
to stable etc.
except read below
> ---
> 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){
Why are you doing an assignment in the if? Why not assign first and
compare later?
> + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
> return -EINVAL;
Nit: Some reason not to return -ERANGE if thats what strtol returned?
While folding the error to -EINVAL is ok, it hides diagnostic informatio
from the user.
> }
> + value = (s32) safe_int;
You call strict checking, which includes overflow checking, but
only that the value fits in a long. You then defeat that checking
by casting to int.
It looks like you want strict_strtoint except thats not defined.
Hoever, the pattern for strict_strto* is kstrto* and kstrtos32 is
defined ...
> } else
> return -EINVAL;
>
Oh, and you now may copy 11 characters from userspace into an 11
character buffer then terminate it by writing the 12th character
(ascii_value[count == 11]). Except its an 11 character array.
The variable is a s32, aparently in native endian if pased in binary
as 4 bytes. What is the magic to set the value to a negative number
through the ascii interface? Is yet another character for the -
required?
I see the string from userspace wasn't properly terminated before
either. In ed77134bfccf5e75b (PM QOS update), merged in v2.6.35-rc1,
11 bytes were copied from user then passed to ssscanf without null
termination forced. It was updated in 0109c2c48d (PM QoS: Correct
pr_debug() misuse and improve parameter checks), which was merged in
2.6.36-rc4, to change the the function that walks off the string from
sscanf to strlen. That changelog isn't marked for stable (I didn't
look if it was sent) but it still isn't force terminated.
happy patching,
milton
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 14:18 ` mark gross
@ 2011-05-23 16:21 ` Milton Miller
2011-05-23 16:21 ` Milton Miller
2011-05-23 19:54 ` Rafael J. Wysocki
2 siblings, 0 replies; 21+ messages in thread
From: Milton Miller @ 2011-05-23 16:21 UTC (permalink / raw)
To: mgross
Cc: Rafael J. Wysocki, linux-next, linux-kernel, mark gross,
Stephen Rothwell
On Mon, 23 May 2011 about 14:18:46 -0000, mgross 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 <markgross@thegnar.org>
>
(1) This should be in the patch, not the enclosing letter
(2) Incorrect capitalization
(3) Incorrect spacing
Please read Documentation/SubmittingPatches again.
>
>
> >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@cr48>
> 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 *
>
>From should match Signed-off-by:
Please seperate title (subject) and description body
Maybe: pm_qos: strict_strtol takes a long, not s32
strict_strtol takes a pointer to long to store the converted value.
introduced in xxxx ("change set title here")
So that the reviewers can quickly see if it needs to be backported
to stable etc.
except read below
> ---
> 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){
Why are you doing an assignment in the if? Why not assign first and
compare later?
> + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
> return -EINVAL;
Nit: Some reason not to return -ERANGE if thats what strtol returned?
Folding to -EINVAL is ok but hides information.
> }
> + value = (s32) safe_int;
You call strict checking, which includes overflow checking, but
only that the value fits in a long. You then defeat that checking
by casting to int.
It looks like you want strict_strtoint except thats not defined.
Hoever, the pattern for strict_strto* is kstrto* and kstrtos32 is
defined ...
> } else
> return -EINVAL;
>
Oh, and you now may copy 11 characters from userspace into an 11
character buffer then terminate it by writing the 12th character
(ascii_value[count == 11]). Except its an 11 character array.
What is the magic to set the value to a negative number? Is
yet another character for the - required?
I see the string from userspace wasn't properly terminated before
either. In ed77134bfccf5e75b (PM QOS update), merged in v2.6.35-rc1,
11 bytes were copied from user then passed to ssscanf without null
termination forced. It was updated in 0109c2c48d (PM QoS: Correct
pr_debug() misuse and improve parameter checks), which was merged in
2.6.36-rc4, to change the the function that walks off the string from
sscanf to strlen. That changelog isn't marked for stable (I didn't
look if it was sent) but it still isn't force terminated.
happy patching,
milton
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
@ 2011-05-23 16:21 ` Milton Miller
0 siblings, 0 replies; 21+ messages in thread
From: Milton Miller @ 2011-05-23 16:21 UTC (permalink / raw)
Cc: Rafael J. Wysocki, linux-next, linux-kernel, mark gross,
Stephen Rothwell
On Mon, 23 May 2011 about 14:18:46 -0000, mgross 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 <markgross@thegnar.org>
>
(1) This should be in the patch, not the enclosing letter
(2) Incorrect capitalization
(3) Incorrect spacing
Please read Documentation/SubmittingPatches again.
>
>
> >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@cr48>
> 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 *
>
>From should match Signed-off-by:
Please seperate title (subject) and description body
Maybe: pm_qos: strict_strtol takes a long, not s32
strict_strtol takes a pointer to long to store the converted value.
introduced in xxxx ("change set title here")
So that the reviewers can quickly see if it needs to be backported
to stable etc.
except read below
> ---
> 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){
Why are you doing an assignment in the if? Why not assign first and
compare later?
> + pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
> return -EINVAL;
Nit: Some reason not to return -ERANGE if thats what strtol returned?
Folding to -EINVAL is ok but hides information.
> }
> + value = (s32) safe_int;
You call strict checking, which includes overflow checking, but
only that the value fits in a long. You then defeat that checking
by casting to int.
It looks like you want strict_strtoint except thats not defined.
Hoever, the pattern for strict_strto* is kstrto* and kstrtos32 is
defined ...
> } else
> return -EINVAL;
>
Oh, and you now may copy 11 characters from userspace into an 11
character buffer then terminate it by writing the 12th character
(ascii_value[count == 11]). Except its an 11 character array.
What is the magic to set the value to a negative number? Is
yet another character for the - required?
I see the string from userspace wasn't properly terminated before
either. In ed77134bfccf5e75b (PM QOS update), merged in v2.6.35-rc1,
11 bytes were copied from user then passed to ssscanf without null
termination forced. It was updated in 0109c2c48d (PM QoS: Correct
pr_debug() misuse and improve parameter checks), which was merged in
2.6.36-rc4, to change the the function that walks off the string from
sscanf to strlen. That changelog isn't marked for stable (I didn't
look if it was sent) but it still isn't force terminated.
happy patching,
milton
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 14:18 ` mark gross
@ 2011-05-23 15:24 ` Randy Dunlap
2011-05-23 16:21 ` Milton Miller
2011-05-23 19:54 ` Rafael J. Wysocki
2 siblings, 0 replies; 21+ messages in thread
From: Randy Dunlap @ 2011-05-23 15:24 UTC (permalink / raw)
To: markgross; +Cc: Stephen Rothwell, Rafael J. Wysocki, linux-next, linux-kernel
On Mon, 23 May 2011 07:18:46 -0700 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 <markgross@thegnar.org>
Not in correct format.
>
> From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@cr48>
what is cr48? conference room 48?
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2011-05-23 5:06 Stephen Rothwell
@ 2011-05-23 14:18 ` mark gross
2011-05-23 15:24 ` Randy Dunlap
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: mark gross @ 2011-05-23 14:18 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: Rafael J. Wysocki, linux-next, linux-kernel, mark gross
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 <markgross@thegnar.org>
>From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
From: mgross <mgross@cr48>
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;
} else
return -EINVAL;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* linux-next: build warning after merge of the suspend tree
@ 2011-05-23 5:06 Stephen Rothwell
2011-05-23 14:18 ` mark gross
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Rothwell @ 2011-05-23 5:06 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-next, linux-kernel, mark gross
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
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").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2010-02-12 11:32 ` Rafael J. Wysocki
@ 2010-02-12 12:25 ` Stephen Rothwell
0 siblings, 0 replies; 21+ messages in thread
From: Stephen Rothwell @ 2010-02-12 12:25 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-next, linux-kernel, Alan Stern
[-- Attachment #1: Type: text/plain, Size: 277 bytes --]
Hi Rafael,
On Fri, 12 Feb 2010 12:32:45 +0100 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>
> Ah, missing "void", sorry about that. Should be fixed now.
Thanks.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: linux-next: build warning after merge of the suspend tree
2010-02-12 4:57 Stephen Rothwell
@ 2010-02-12 11:32 ` Rafael J. Wysocki
2010-02-12 12:25 ` Stephen Rothwell
0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2010-02-12 11:32 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linux-next, linux-kernel, Alan Stern
On Friday 12 February 2010, Stephen Rothwell wrote:
> Hi Rafael,
>
> After merging the suspend tree, today's linux-next build (powerpc
> ppc64_defconfig) produced this warning:
>
> drivers/usb/core/hcd-pci.c:153: warning: return type defaults to 'int'
> drivers/usb/core/hcd-pci.c: In function 'clear_hs_companion':
> drivers/usb/core/hcd-pci.c:153: warning: control reaches end of non-void function
>
> Introduced by commit 708efd3eb95d318442da39ad6e5c01d5c9b85dbf ("USB:
> implement non-tree resume ordering constraints for PCI host controllers").
Ah, missing "void", sorry about that. Should be fixed now.
Rafael
^ permalink raw reply [flat|nested] 21+ messages in thread
* linux-next: build warning after merge of the suspend tree
@ 2010-02-12 4:57 Stephen Rothwell
2010-02-12 11:32 ` Rafael J. Wysocki
0 siblings, 1 reply; 21+ messages in thread
From: Stephen Rothwell @ 2010-02-12 4:57 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-next, linux-kernel, Alan Stern
[-- Attachment #1: Type: text/plain, Size: 599 bytes --]
Hi Rafael,
After merging the suspend tree, today's linux-next build (powerpc
ppc64_defconfig) produced this warning:
drivers/usb/core/hcd-pci.c:153: warning: return type defaults to 'int'
drivers/usb/core/hcd-pci.c: In function 'clear_hs_companion':
drivers/usb/core/hcd-pci.c:153: warning: control reaches end of non-void function
Introduced by commit 708efd3eb95d318442da39ad6e5c01d5c9b85dbf ("USB:
implement non-tree resume ordering constraints for PCI host controllers").
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-05-26 1:58 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-02-11 6:10 linux-next: build warning after merge of the suspend tree Stephen Rothwell
2010-02-11 13:11 ` Rafael J. Wysocki
2010-02-11 15:32 ` Alan Stern
2010-02-11 15:32 ` Alan Stern
2010-02-12 4:57 Stephen Rothwell
2010-02-12 11:32 ` Rafael J. Wysocki
2010-02-12 12:25 ` Stephen Rothwell
2011-05-23 5:06 Stephen Rothwell
2011-05-23 14:18 ` mark gross
2011-05-23 15:24 ` Randy Dunlap
2011-05-23 16:21 ` Milton Miller
2011-05-23 16:21 ` Milton Miller
2011-05-23 17:42 ` Milton Miller
2011-05-24 6:13 ` mark gross
2011-05-23 18:57 ` Rafael J. Wysocki
2011-05-23 19:54 ` Rafael J. Wysocki
2011-05-24 6:34 ` mark gross
2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-24 21:30 ` Rafael J. Wysocki
2011-05-26 1:58 ` mark gross
2011-05-26 1:58 ` mark gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.