| Message ID | 20220411063324.98542-1-andrei.lalaev@emlid.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: of: fix bounds check for valid mask | expand |
On Mon, Apr 11, 2022 at 8:36 AM Andrei Lalaev <andrei.lalaev@emlid.com> wrote: > > Use "greater" instead of "greater or equal" when performs bounds check > to make sure that GPIOS are in available range. Previous implementation > skipped ranges which include last GPIO in range. > > Signed-off-by: Andrei Lalaev <andrei.lalaev@emlid.com> > --- > drivers/gpio/gpiolib-of.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > index ae1ce319cd78..7e5e51d49d09 100644 > --- a/drivers/gpio/gpiolib-of.c > +++ b/drivers/gpio/gpiolib-of.c > @@ -910,7 +910,7 @@ static void of_gpiochip_init_valid_mask(struct gpio_chip *chip) > i, &start); > of_property_read_u32_index(np, "gpio-reserved-ranges", > i + 1, &count); > - if (start >= chip->ngpio || start + count >= chip->ngpio) > + if (start >= chip->ngpio || start + count > chip->ngpio) > continue; > > bitmap_clear(chip->valid_mask, start, count); > -- > 2.25.1 > Queued for fixes, good catch. Thanks Bart
On Mon, Apr 11, 2022 at 12:57 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote: > > Use "greater" instead of "greater or equal" when performs bounds check > to make sure that GPIOS are in available range. Previous implementation the available > skipped ranges which include last GPIO in range. the last Should it have a Fixes tag? OTOH, the current implementation suggests that we have start,end rather than start,count. What does documentation tell about it? Does it need to be fixed?
Thanks for the grammar comments. > Should it have a Fixes tag? Sure, thanks, I will resend with a Fixes tag and without grammar errors. > What does documentation tell about it? Documentation (devicetree/bindings/gpio/gpio.txt line 152) tells that "This property indicates the start and size of the GPIOs that can't be used." And the example (line 178) at the same file shows that the second element of a tuple is the count: "gpio-reserved-ranges = <0 4>, <12 2>;" > Does it need to be fixed? I think so, because the current implementation doesn't reserve some GPIO ranges. For example, we have 20 GPIOs and we want to reserve GPIOs from 14 to 19. In this case the "reserved-ranges" looks like "<14 6>" but the "of_gpiochip_init_valid_mask" drops the range and this is not expected behavior.
On Mon, Apr 11, 2022 at 5:46 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote: ... > > What does documentation tell about it? > > Documentation (devicetree/bindings/gpio/gpio.txt line 152) tells that > "This property indicates the start and size of the GPIOs that can't be used." > And the example (line 178) at the same file shows that the second element of > a tuple is the count: "gpio-reserved-ranges = <0 4>, <12 2>;" > > > Does it need to be fixed? This question was related to the documentation contents. > I think so, because the current implementation doesn't reserve some GPIO ranges. > For example, we have 20 GPIOs and we want to reserve GPIOs from 14 to 19. > In this case the "reserved-ranges" looks like "<14 6>" but the > "of_gpiochip_init_valid_mask" drops the range and this is not expected behavior. On top of that, it would be nice to be sure that at least all current in-kernel users (meaning all DTS provided so far by the kernel) do interpret it as start,size. Otherwise this will be an (unacceptable) ABI change and hence documentation would need to be fixed with variable names in the code.
On Mon, Apr 11, 2022 at 7:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > On top of that, it would be nice to be sure that at least all current > in-kernel users (meaning all DTS provided so far by the kernel) do > interpret it as start,size. I checked the DTS and only 36 of them use "gpio-reserved-ranges". The 33 of them use tuple with the second element that is less than the first. So it means that the maintainers interpreted it as "start,size". And only 3 of them use one tuple with the second element is greater than the first. The list of this DTS: 1. arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi AngeloGioacchino Del Regno added it in the commit 9da65e441d4d ("arm64: dts: qcom: Add support for SONY Xperia X Performance / XZ / XZs (msm8996, Tone platform)") But in another commit 8b36c824b9a77 ("arm64: dts: qcom: sdm630-xperia-nile: Add all RPM and fixed regulators") he clearly interpreted it as "start,size" because the second element is less than the first. 2. arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts The same maintainer as for the previous DTS. He added "reserved-range" in the commit 122d2c5f31b6e ("arm64: dts: qcom: Add support for MSM8998 F(x)tec Pro1 QX1000") 3. arch/arm64/boot/dts/qcom/sa8155p-adp.dts Bhupesh Sharma added it in the commit 12dd4ebda47ab ("arm64: dts: qcom: Fix usb entries for SA8155p adp board") Should I ask these maintainers how they interpreted this property? > Otherwise this will be an (unacceptable) ABI change and hence documentation > would need to be fixed with variable names in the code I want you to notice that "of_gpiochip_init_valid_mask" uses "bitmap_clear" that clears "nbits" bits starting from the "start". So it can't be interpreted as "start,end". That's why I think everyone interprets it as "start,size" because it works like "start,size" and the documentation tells it is "start,size".
On Mon, Apr 11, 2022 at 2:17 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Apr 11, 2022 at 12:57 PM Andrei Lalaev <andrei.lalaev@emlid.com> wrote: > > > > Use "greater" instead of "greater or equal" when performs bounds check > > to make sure that GPIOS are in available range. Previous implementation > > the available > > > skipped ranges which include last GPIO in range. > > the last > > Should it have a Fixes tag? > > OTOH, the current implementation suggests that we have start,end > rather than start,count. What does documentation tell about it? Does > it need to be fixed? > > -- > With Best Regards, > Andy Shevchenko Thanks Andy, I rushed this one. Backing it out. Bart
On Tue, Apr 12, 2022 at 10:13 AM Andrei Lalaev <andrei.lalaev@emlid.com> wrote: > On Mon, Apr 11, 2022 at 7:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > On top of that, it would be nice to be sure that at least all current > > in-kernel users (meaning all DTS provided so far by the kernel) do > > interpret it as start,size. > > I checked the DTS and only 36 of them use "gpio-reserved-ranges". > The 33 of them use tuple with the second element that is less than the first. > So it means that the maintainers interpreted it as "start,size". > > And only 3 of them use one tuple with the second element is greater than the first. > The list of this DTS: > 1. arch/arm64/boot/dts/qcom/msm8996-sony-xperia-tone.dtsi > AngeloGioacchino Del Regno added it in the commit 9da65e441d4d > ("arm64: dts: qcom: Add support for SONY Xperia X Performance / XZ / XZs (msm8996, Tone platform)") > But in another commit 8b36c824b9a77 ("arm64: dts: qcom: sdm630-xperia-nile: Add all RPM and fixed regulators") > he clearly interpreted it as "start,size" because the second element > is less than the first. > > 2. arch/arm64/boot/dts/qcom/msm8998-fxtec-pro1.dts > The same maintainer as for the previous DTS. > He added "reserved-range" in the commit 122d2c5f31b6e > ("arm64: dts: qcom: Add support for MSM8998 F(x)tec Pro1 QX1000") > > 3. arch/arm64/boot/dts/qcom/sa8155p-adp.dts > Bhupesh Sharma added it in the commit 12dd4ebda47ab > ("arm64: dts: qcom: Fix usb entries for SA8155p adp board") > > Should I ask these maintainers how they interpreted this property? Ideally it would be nice to have their responses. Meanwhile... > > Otherwise this will be an (unacceptable) ABI change and hence documentation > > would need to be fixed with variable names in the code > > I want you to notice that "of_gpiochip_init_valid_mask" uses "bitmap_clear" > that clears "nbits" bits starting from the "start". So it can't be interpreted > as "start,end". That's why I think everyone interprets it as "start,size" because > it works like "start,size" and the documentation tells it is "start,size". ...meanwhile try to compress above into a few sentences and put it to the commit message explaining that the de facto use of the values seems as start,size and questioned DTSes have been asked for an explanation from the current maintainers. I'm now 99% confident that your patch is correct. Thanks for doing all this analysis.
On Tue, Apr 12, 2022 at 11:28 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Mon, Apr 11, 2022 at 2:17 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: ... > > OTOH, the current implementation suggests that we have start,end > > rather than start,count. What does documentation tell about it? Does > > it need to be fixed? > Thanks Andy, I rushed this one. Backing it out. With the last analysis by Andrei I think the patch is correct. I suggest waiting for v2 with a better commit message.
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index ae1ce319cd78..7e5e51d49d09 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -910,7 +910,7 @@ static void of_gpiochip_init_valid_mask(struct gpio_chip *chip) i, &start); of_property_read_u32_index(np, "gpio-reserved-ranges", i + 1, &count); - if (start >= chip->ngpio || start + count >= chip->ngpio) + if (start >= chip->ngpio || start + count > chip->ngpio) continue; bitmap_clear(chip->valid_mask, start, count);
Use "greater" instead of "greater or equal" when performs bounds check to make sure that GPIOS are in available range. Previous implementation skipped ranges which include last GPIO in range. Signed-off-by: Andrei Lalaev <andrei.lalaev@emlid.com> --- drivers/gpio/gpiolib-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)