* 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).