gpio / ACPI: Add support for ACPI GPIO operation regions

DISCUSSION Dan Carpenter 2 participants 6 messages Mar 28, 2014 – Mar 31, 2014 GPIO

Share & tools

Dan Carpenter

author

2014-03-28 08:37 UTC #0 lore raw

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

Mika Westerberg

2014-03-31 11:58 UTC #1 lore raw

Quoted 8 lines · Dan Carpenter

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?

Dan Carpenter

author

2014-03-31 12:11 UTC #2 lore raw

Quoted 7 lines · Mika Westerberg

Quoted 6 lines · Dan Carpenter
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:

Quoted 5 lines

Quoted 2 lines
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

Mika Westerberg

2014-03-31 13:39 UTC #3 lore raw

Quoted 11 lines · Dan Carpenter

Quoted 10 lines · Mika Westerberg
Quoted 8 lines · Dan Carpenter
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?

Quoted 6 lines

Quoted 2 lines
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.

drivers/gpio/gpiolib-acpi.c +1 -1

index d5be56fe689e..401add28933f 100644
@@ -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:

Dan Carpenter

author

2014-03-31 13:51 UTC #4 lore raw

Quoted 15 lines · Mika Westerberg

Quoted 14 lines · Dan Carpenter
Quoted 12 lines · Mika Westerberg
Quoted 8 lines · Dan Carpenter
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?

Quoted 8 lines

Quoted 3 lines
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

Mika Westerberg

2014-03-31 13:56 UTC #5 lore raw

Quoted 22 lines · Dan Carpenter

Quoted 21 lines · Mika Westerberg
Quoted 11 lines · Dan Carpenter
Quoted 10 lines · Mika Westerberg
Quoted 8 lines · Dan Carpenter
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?
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 2 lines
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.