Re: [PATCH] gpiolib: of: fix bounds check for valid mask

[Andy Shevchenko]

On Tue, Apr 12, 2022 at 10:13 AM Andrei Lalaev <andrei.lalaev@xxxxxxxxx> wrote:
> On Mon, Apr 11, 2022 at 7:55 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> 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.

-- 
With Best Regards,
Andy Shevchenko