* re: gpio / ACPI: Add support for ACPI GPIO operation regions @ 2014-03-28 8:37 Dan Carpenter 2014-03-31 12:05 ` Mika Westerberg 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-03-28 8:37 UTC (permalink / raw) To: mika.westerberg; +Cc: linux-gpio Hello Mika Westerberg, The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO operation regions" from Mar 14, 2014, leads to the following static checker warning: drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? drivers/gpio/gpiolib-acpi.c 451 if (function == ACPI_WRITE) 452 gpiod_set_raw_value(desc, !!((1 << i) & *value)); 453 else 454 *value |= gpiod_get_raw_value(desc) << i; 455 } "value" is u64 pointer but gpiod_get_raw_value() is an int so this could lead to wrapping bugs. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gpio / ACPI: Add support for ACPI GPIO operation regions 2014-03-28 8:37 gpio / ACPI: Add support for ACPI GPIO operation regions Dan Carpenter @ 2014-03-31 12:05 ` Mika Westerberg 2014-03-31 12:11 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Mika Westerberg @ 2014-03-31 12:05 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-gpio On Fri, Mar 28, 2014 at 11:37:32AM +0300, Dan Carpenter wrote: > Hello Mika Westerberg, > > The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO > operation regions" from Mar 14, 2014, leads to the following static > checker warning: > > drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() > warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? Thanks for the report. However, I'm not able to reproduce this warning with sparse. How did you get this? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gpio / ACPI: Add support for ACPI GPIO operation regions 2014-03-31 12:05 ` Mika Westerberg @ 2014-03-31 12:11 ` Dan Carpenter 2014-03-31 13:45 ` Mika Westerberg 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-03-31 12:11 UTC (permalink / raw) To: Mika Westerberg; +Cc: linux-gpio On Mon, Mar 31, 2014 at 03:05:25PM +0300, Mika Westerberg wrote: > On Fri, Mar 28, 2014 at 11:37:32AM +0300, Dan Carpenter wrote: > > Hello Mika Westerberg, > > > > The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO > > operation regions" from Mar 14, 2014, leads to the following static > > checker warning: > > > > drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() > > warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? > > Thanks for the report. However, I'm not able to reproduce this warning with > sparse. How did you get this? It's not a Sparse warning. It's some unreleased stuff (too many false positives). I sort through the warnings manually and send the ones which seem valid. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gpio / ACPI: Add support for ACPI GPIO operation regions 2014-03-31 12:11 ` Dan Carpenter @ 2014-03-31 13:45 ` Mika Westerberg 2014-03-31 13:51 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Mika Westerberg @ 2014-03-31 13:45 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-gpio On Mon, Mar 31, 2014 at 03:11:33PM +0300, Dan Carpenter wrote: > On Mon, Mar 31, 2014 at 03:05:25PM +0300, Mika Westerberg wrote: > > On Fri, Mar 28, 2014 at 11:37:32AM +0300, Dan Carpenter wrote: > > > Hello Mika Westerberg, > > > > > > The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO > > > operation regions" from Mar 14, 2014, leads to the following static > > > checker warning: > > > > > > drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() > > > warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? > > > > Thanks for the report. However, I'm not able to reproduce this warning with > > sparse. How did you get this? > > It's not a Sparse warning. It's some unreleased stuff (too many false > positives). I sort through the warnings manually and send the ones > which seem valid. I see. What do you think about the patch below? I have to admit that this kind of stuff is in my "gray" area of understanding. diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index d5be56fe689e..401add28933f 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -451,7 +451,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address, if (function == ACPI_WRITE) gpiod_set_raw_value(desc, !!((1 << i) & *value)); else - *value |= gpiod_get_raw_value(desc) << i; + *value |= (u64)gpiod_get_raw_value(desc) << i; } out: ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: gpio / ACPI: Add support for ACPI GPIO operation regions 2014-03-31 13:45 ` Mika Westerberg @ 2014-03-31 13:51 ` Dan Carpenter 2014-03-31 14:04 ` Mika Westerberg 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-03-31 13:51 UTC (permalink / raw) To: Mika Westerberg; +Cc: linux-gpio On Mon, Mar 31, 2014 at 04:45:57PM +0300, Mika Westerberg wrote: > On Mon, Mar 31, 2014 at 03:11:33PM +0300, Dan Carpenter wrote: > > On Mon, Mar 31, 2014 at 03:05:25PM +0300, Mika Westerberg wrote: > > > On Fri, Mar 28, 2014 at 11:37:32AM +0300, Dan Carpenter wrote: > > > > Hello Mika Westerberg, > > > > > > > > The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO > > > > operation regions" from Mar 14, 2014, leads to the following static > > > > checker warning: > > > > > > > > drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() > > > > warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? > > > > > > Thanks for the report. However, I'm not able to reproduce this warning with > > > sparse. How did you get this? > > > > It's not a Sparse warning. It's some unreleased stuff (too many false > > positives). I sort through the warnings manually and send the ones > > which seem valid. > > I see. > > What do you think about the patch below? I have to admit that this kind of > stuff is in my "gray" area of understanding. It looks good to me. The question, I guess is can pin_table_length ever be more than 31. gpiod_get_raw_value() returns and int of 0-1 so if "i" is 31 then *value is an unexpected number because the shift is undefined (it wraps around in GCC). If it can't go higher than 31 then the existing code is fine. (We never use the upper 32 bits of *value). But your patch silences a valid looking static checker warning. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: gpio / ACPI: Add support for ACPI GPIO operation regions 2014-03-31 13:51 ` Dan Carpenter @ 2014-03-31 14:04 ` Mika Westerberg 0 siblings, 0 replies; 6+ messages in thread From: Mika Westerberg @ 2014-03-31 14:04 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-gpio On Mon, Mar 31, 2014 at 04:51:24PM +0300, Dan Carpenter wrote: > On Mon, Mar 31, 2014 at 04:45:57PM +0300, Mika Westerberg wrote: > > On Mon, Mar 31, 2014 at 03:11:33PM +0300, Dan Carpenter wrote: > > > On Mon, Mar 31, 2014 at 03:05:25PM +0300, Mika Westerberg wrote: > > > > On Fri, Mar 28, 2014 at 11:37:32AM +0300, Dan Carpenter wrote: > > > > > Hello Mika Westerberg, > > > > > > > > > > The patch 473ed7be0da0: "gpio / ACPI: Add support for ACPI GPIO > > > > > operation regions" from Mar 14, 2014, leads to the following static > > > > > checker warning: > > > > > > > > > > drivers/gpio/gpiolib-acpi.c:454 acpi_gpio_adr_space_handler() > > > > > warn: should 'gpiod_get_raw_value(desc) << i' be a 64 bit type? > > > > > > > > Thanks for the report. However, I'm not able to reproduce this warning with > > > > sparse. How did you get this? > > > > > > It's not a Sparse warning. It's some unreleased stuff (too many false > > > positives). I sort through the warnings manually and send the ones > > > which seem valid. > > > > I see. > > > > What do you think about the patch below? I have to admit that this kind of > > stuff is in my "gray" area of understanding. > > It looks good to me. The question, I guess is can pin_table_length ever > be more than 31. gpiod_get_raw_value() returns and int of 0-1 so > if "i" is 31 then *value is an unexpected number because the shift is > undefined (it wraps around in GCC). It can be larger than 31 so I guess I'll submit this patch to LinusW. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-03-31 13:56 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-03-28 8:37 gpio / ACPI: Add support for ACPI GPIO operation regions Dan Carpenter 2014-03-31 12:05 ` Mika Westerberg 2014-03-31 12:11 ` Dan Carpenter 2014-03-31 13:45 ` Mika Westerberg 2014-03-31 13:51 ` Dan Carpenter 2014-03-31 14:04 ` Mika Westerberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).