[PATCH] gpio: Document the 'valid_mask' being internal

[PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

The valid_mask member of the struct gpio_chip is unconditionally written
by the GPIO core at driver registration. Current documentation does not
mention this but just says the valid_mask is used if it's not NULL. This
lured me to try populating it directly in the GPIO driver probe instead
of using the init_valid_mask() callback. It took some retries with
different bitmaps and eventually a bit of code-reading to understand why
the valid_mask was not obeyed. I could've avoided this trial and error if
it was mentioned in the documentation.

Help the next developer who decides to directly populate the valid_mask
in struct gpio_chip by documenting the valid_mask as internal to the
GPIO core.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Alternative approach would be to check whether the valid_mask is NULL at
gpio_chip registration and touch it only if it is NULL. This, however,
might cause problems if any of the existing drivers pass the struct
gpio_chip with uninitialized valid_mask field to the registration. In
order to avoid this I decided to keep current behaviour while
documenting it a bit better.
---
 include/linux/gpio/driver.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270..fe80c65dacb0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -503,7 +503,8 @@ struct gpio_chip {
 	 * @valid_mask:
 	 *
 	 * If not %NULL, holds bitmask of GPIOs which are valid to be used
-	 * from the chip.
+	 * from the chip. Internal to GPIO core. Chip drivers should populate
+	 * init_valid_mask instead.
 	 */
 	unsigned long *valid_mask;
 

base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Linus Walleij]

On Tue, Feb 25, 2025 at 8:01 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> The valid_mask member of the struct gpio_chip is unconditionally written
> by the GPIO core at driver registration. Current documentation does not
> mention this but just says the valid_mask is used if it's not NULL. This
> lured me to try populating it directly in the GPIO driver probe instead
> of using the init_valid_mask() callback. It took some retries with
> different bitmaps and eventually a bit of code-reading to understand why
> the valid_mask was not obeyed. I could've avoided this trial and error if
> it was mentioned in the documentation.
>
> Help the next developer who decides to directly populate the valid_mask
> in struct gpio_chip by documenting the valid_mask as internal to the
> GPIO core.
>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

Ah typical.

>          * If not %NULL, holds bitmask of GPIOs which are valid to be used
> -        * from the chip.
> +        * from the chip. Internal to GPIO core. Chip drivers should populate
> +        * init_valid_mask instead.
>          */
>         unsigned long *valid_mask;

Actually if we want to protect this struct member from being manipulated
outside of gpiolib, we can maybe move it to struct gpio_device in
drivers/gpio/gpiolib.h?

This struct exist for every gpio_chip but is entirely gpiolib-internal.

Then it becomes impossible to do it wrong...

Yours,
Linus Walleij

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 25/02/2025 23:36, Linus Walleij wrote:
> On Tue, Feb 25, 2025 at 8:01 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> 
>> The valid_mask member of the struct gpio_chip is unconditionally written
>> by the GPIO core at driver registration. Current documentation does not
>> mention this but just says the valid_mask is used if it's not NULL. This
>> lured me to try populating it directly in the GPIO driver probe instead
>> of using the init_valid_mask() callback. It took some retries with
>> different bitmaps and eventually a bit of code-reading to understand why
>> the valid_mask was not obeyed. I could've avoided this trial and error if
>> it was mentioned in the documentation.
>>
>> Help the next developer who decides to directly populate the valid_mask
>> in struct gpio_chip by documenting the valid_mask as internal to the
>> GPIO core.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> Ah typical.
> 
>>           * If not %NULL, holds bitmask of GPIOs which are valid to be used
>> -        * from the chip.
>> +        * from the chip. Internal to GPIO core. Chip drivers should populate
>> +        * init_valid_mask instead.
>>           */
>>          unsigned long *valid_mask;
> 
> Actually if we want to protect this struct member from being manipulated
> outside of gpiolib, we can maybe move it to struct gpio_device in
> drivers/gpio/gpiolib.h?
> 
> This struct exist for every gpio_chip but is entirely gpiolib-internal.
> 
> Then it becomes impossible to do it wrong...

True. I can try seeing what it'd require to do that. But ... If there 
are any drivers out there altering the valid_mask _after_ registering 
the driver to the gpio-core ... Then it may be a can of worms and I may 
just keep the lid closed :)

Furthermore, I was not 100% sure the valid_mask was not intended to be 
used directly by the drivers. I hoped you and Bart have an opinion on that.

We can also try:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ca2f58a2cd45..68fc0c6e2ed3 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1047,9 +1047,11 @@ int gpiochip_add_data_with_key(struct gpio_chip 
*gc, void *data,
         if (ret)
                 goto err_cleanup_desc_srcu;

-       ret = gpiochip_init_valid_mask(gc);
-       if (ret)
-               goto err_cleanup_desc_srcu;
+       if (!gc->valid_mask) {
+               ret = gpiochip_init_valid_mask(gc);
+               if (ret)
+                       goto err_cleanup_desc_srcu;
+       }

         for (desc_index = 0; desc_index < gc->ngpio; desc_index++) {
                 struct gpio_desc *desc = &gdev->descs[desc_index];

if you think this is safe enough.

For example the BD79124 driver digs the pin config (GPO or ADC-input) 
during the probe. It needs this to decide which ADC channels to 
register, and also to configure the pinmux. So, the BD79124 could just 
populate the bitmask directly to the valid_mask and omit the 
init_valid_mask() - which might be a tiny simplification in driver. 
Still, I'm not sure if it's worth having two mechanisms to populate 
valid masks - I suppose supporting only the init_valid_mask() might be 
simpler for the gpiolib maintainers ;)

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Linus Walleij]

On Wed, Feb 26, 2025 at 7:09 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 25/02/2025 23:36, Linus Walleij wrote:
> > we can maybe move it to struct gpio_device in
> > drivers/gpio/gpiolib.h?
> >
> > This struct exist for every gpio_chip but is entirely gpiolib-internal.
> >
> > Then it becomes impossible to do it wrong...
>
> True. I can try seeing what it'd require to do that. But ... If there
> are any drivers out there altering the valid_mask _after_ registering
> the driver to the gpio-core ... Then it may be a can of worms and I may
> just keep the lid closed :)

That's easy to check with some git grep valid_mask
and intuition. I think all calls actually changing the valid_mask
are in the init_valid_mask() callback as they should be.

> Furthermore, I was not 100% sure the valid_mask was not intended to be
> used directly by the drivers. I hoped you and Bart have an opinion on that.

Oh it was. First we just had .valid_mask and then it was
manipulated directly.

Then we introduced init_valid_mask() and all users switched over
to using that.

So evolution, not intelligent design...

Yours,
Linus Walleij

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 26/02/2025 12:18, Linus Walleij wrote:
> On Wed, Feb 26, 2025 at 7:09 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>> On 25/02/2025 23:36, Linus Walleij wrote:
>>> we can maybe move it to struct gpio_device in
>>> drivers/gpio/gpiolib.h?
>>>
>>> This struct exist for every gpio_chip but is entirely gpiolib-internal.
>>>
>>> Then it becomes impossible to do it wrong...
>>
>> True. I can try seeing what it'd require to do that. But ... If there
>> are any drivers out there altering the valid_mask _after_ registering
>> the driver to the gpio-core ... Then it may be a can of worms and I may
>> just keep the lid closed :)
> 
> That's easy to check with some git grep valid_mask

True. I just tried. It seems mostly Ok, but...
For example the drivers/gpio/gpio-rcar.c uses the contents of the 
'valid_mask' in it's set_multiple callback to disallow setting the value 
of masked GPIOs.

For uneducated person like me, it feels this check should be done and 
enforced by the gpiolib and not left for untrustworthy driver writers 
like me! (I am working on BD79124 driver and it didn't occur to me I 
should check for the valid_mask in driver :) If gpiolib may call the 
driver's set_multiple() with masked lines - then the bd79124 driver just 
had one unknown bug less :rolleyes:) )

I tried looking at the gpiolib to see how this works... It seems to me:

gpio_chip_set_multiple() does not seem to check for valid_mask. This is 
called from the gpiod_set_array_value_complex() - which gave me a 
headache as it is, as name says, complex. Well, I didn't spot valid_mask 
check but I may have missed a thing or 2...

If someone remembers straight away how this is supposed to work - I 
appreciate any guidance. If not, then I try doing some testing when I 
wire the BD79124 to my board for the next version of the BD79124 series.

> and intuition. I think all calls actually changing the valid_mask
> are in the init_valid_mask() callback as they should be.
> 
>> Furthermore, I was not 100% sure the valid_mask was not intended to be
>> used directly by the drivers. I hoped you and Bart have an opinion on that.
> 
> Oh it was. First we just had .valid_mask and then it was
> manipulated directly.

I still can't decide if hiding the valid_mask is the right thing to do, 
or if we should just respect it if it is set by driver (as it was 
originally intended).

> Then we introduced init_valid_mask() and all users switched over
> to using that.
> 
> So evolution, not intelligent design...

Like anything we actually get done ^_^;

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 26/02/2025 13:42, Matti Vaittinen wrote:
> On 26/02/2025 12:18, Linus Walleij wrote:
>> On Wed, Feb 26, 2025 at 7:09 AM Matti Vaittinen
>> <mazziesaccount@gmail.com> wrote:
>>> On 25/02/2025 23:36, Linus Walleij wrote:
>>>> we can maybe move it to struct gpio_device in
>>>> drivers/gpio/gpiolib.h?
>>>>
>>>> This struct exist for every gpio_chip but is entirely gpiolib-internal.
>>>>
>>>> Then it becomes impossible to do it wrong...
>>>
>>> True. I can try seeing what it'd require to do that. But ... If there
>>> are any drivers out there altering the valid_mask _after_ registering
>>> the driver to the gpio-core ... Then it may be a can of worms and I may
>>> just keep the lid closed :)
>>
>> That's easy to check with some git grep valid_mask
> 
> True. I just tried. It seems mostly Ok, but...
> For example the drivers/gpio/gpio-rcar.c uses the contents of the 
> 'valid_mask' in it's set_multiple callback to disallow setting the value 
> of masked GPIOs.
> 
> For uneducated person like me, it feels this check should be done and 
> enforced by the gpiolib and not left for untrustworthy driver writers 
> like me! (I am working on BD79124 driver and it didn't occur to me I 
> should check for the valid_mask in driver :) If gpiolib may call the 
> driver's set_multiple() with masked lines - then the bd79124 driver just 
> had one unknown bug less :rolleyes:) )
> 
> I tried looking at the gpiolib to see how this works... It seems to me:
> 
> gpio_chip_set_multiple() does not seem to check for valid_mask. This is 
> called from the gpiod_set_array_value_complex() - which gave me a 
> headache as it is, as name says, complex. Well, I didn't spot valid_mask 
> check but I may have missed a thing or 2...
> 
> If someone remembers straight away how this is supposed to work - I 
> appreciate any guidance. If not, then I try doing some testing when I 
> wire the BD79124 to my board for the next version of the BD79124 series.

I did some quick testing. I used:

adc: adc@10 {

...

	channel@0 {
		reg = <0>;
	};
	channel@1 {
		reg = <1>;
	};
	/* ... up to the channel@6. */

	gpio-controller;
};

which left GPIO0 ... GPIO6 masked (pins used for ADC) and only GPIO7 
unmasked.

Then I added:
gpiotst {
	compatible = "rohm,foo-bd72720-gpio";
	rohm,dvs-vsel-gpios = <&adc 5 0>, <&adc 6 0>;
};

and a dummy driver which does:
gpio_array = devm_gpiod_get_array(&pdev->dev, "rohm,dvs-vsel",
				  GPIOD_OUT_LOW);

...

ret = gpiod_set_array_value_cansleep(gpio_array->ndescs,
		gpio_array->desc, gpio_array->info, values);

As a result the bd79124 gpio driver got it's set_multiple called with 
masked pins. (Oh, and I had accidentally prepared to handle this as I 
had added a sanity check for pinmux register in the set_multiple()).

I suppose one can think this is a result of invalid DT and that drivers 
shouldn't need to be prepared for that. But ... After supporting 
customers who try to integrate IC drivers to their products ... I think 
it's still better to be prepared. I definitely wouldn't blame the rcar 
driver authors for their valid_mask sanity check :)

After all this babbling, my point is that having the valid mask visible 
for drivers is useful. Especially because there are cases where the 
'valid_mask' can be directly compared to the 'mask' parameter in the 
set_multiple. It's clear and efficient check, and I could assume the 
set_multiple() is an optimized call, and thus being efficient makes sense.

So... Long story short - I would still suggest keeping the valid_mask 
visible and either taking just the doc update (as was done in the 
original patch) - or skipping the valid_mask initialization in gpiolib 
if driver provides non NULL value.

What do you think?

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Linus Walleij]

On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
> On 26/02/2025 12:18, Linus Walleij wrote:


> > That's easy to check with some git grep valid_mask
>
> True. I just tried. It seems mostly Ok, but...
> For example the drivers/gpio/gpio-rcar.c uses the contents of the
> 'valid_mask' in it's set_multiple callback to disallow setting the value
> of masked GPIOs.
>
> For uneducated person like me, it feels this check should be done and
> enforced by the gpiolib and not left for untrustworthy driver writers
> like me! (I am working on BD79124 driver and it didn't occur to me I
> should check for the valid_mask in driver :) If gpiolib may call the
> driver's set_multiple() with masked lines - then the bd79124 driver just
> had one unknown bug less :rolleyes:) )

Yeah that should be done in gpiolib.

And I think it is, gpiolib will not allow you to request a line
that is not valid AFAIK.

This check in rcar is just overzealous and can probably be
removed. Geert what do you say?

Yours,
Linus Walleij

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Linus Walleij]

On Thu, Feb 27, 2025 at 9:24 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:

> I did some quick testing. I used:
(...)
> which left GPIO0 ... GPIO6 masked (pins used for ADC) and only GPIO7
> unmasked.
>
> Then I added:
> gpiotst {
>         compatible = "rohm,foo-bd72720-gpio";
>         rohm,dvs-vsel-gpios = <&adc 5 0>, <&adc 6 0>;
> };
>
> and a dummy driver which does:
> gpio_array = devm_gpiod_get_array(&pdev->dev, "rohm,dvs-vsel",
>                                   GPIOD_OUT_LOW);
>
> ...
>
> ret = gpiod_set_array_value_cansleep(gpio_array->ndescs,
>                 gpio_array->desc, gpio_array->info, values);
>
> As a result the bd79124 gpio driver got it's set_multiple called with
> masked pins. (Oh, and I had accidentally prepared to handle this as I
> had added a sanity check for pinmux register in the set_multiple()).

But... how did you mask of the pins 0..5 in valid_mask in this
example?

If this is device tree, I would expect that at least you set up
gpio-reserved-ranges = <0 5>; which will initialize the valid_mask.

You still need to tell the gpiolib that they are taken for other
purposes somehow.

I think devm_gpiod_get_array() should have failed in that case.

The call graph should look like this:

devm_gpiod_get_array()
    gpiod_get_array()
        gpiod_get_index(0...n)
            gpiod_find_and_request()
                gpiod_request()
                    gpiod_request_commit()
                        gpiochip_line_is_valid()

And gpiochip_line_is_valid() looks like this:

bool gpiochip_line_is_valid(const struct gpio_chip *gc,
                unsigned int offset)
{
    /* No mask means all valid */
    if (likely(!gc->valid_mask))
        return true;
    return test_bit(offset, gc->valid_mask);
}

So why is this not working?

Yours,
Linus Walleij

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 28/02/2025 10:23, Linus Walleij wrote:
> On Thu, Feb 27, 2025 at 9:24 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> 
>> I did some quick testing. I used:
> (...)
>> which left GPIO0 ... GPIO6 masked (pins used for ADC) and only GPIO7
>> unmasked.
>>
>> Then I added:
>> gpiotst {
>>          compatible = "rohm,foo-bd72720-gpio";
>>          rohm,dvs-vsel-gpios = <&adc 5 0>, <&adc 6 0>;
>> };
>>
>> and a dummy driver which does:
>> gpio_array = devm_gpiod_get_array(&pdev->dev, "rohm,dvs-vsel",
>>                                    GPIOD_OUT_LOW);
>>
>> ...
>>
>> ret = gpiod_set_array_value_cansleep(gpio_array->ndescs,
>>                  gpio_array->desc, gpio_array->info, values);
>>
>> As a result the bd79124 gpio driver got it's set_multiple called with
>> masked pins. (Oh, and I had accidentally prepared to handle this as I
>> had added a sanity check for pinmux register in the set_multiple()).
> 
> But... how did you mask of the pins 0..5 in valid_mask in this
> example?

I will double-check this soon, but the BD79124 driver should use the 
init_valid_mask() to set all ADC channels 'invalid'. I believe I did 
print the gc->valid_mask in my test-run (0x80) and had the 
set_multiple() called with mask 0x60.

I need to rewind _my_ stack as I already switched to work with BD79104 
instead :) So, please give me couple of hours...

> If this is device tree, I would expect that at least you set up
> gpio-reserved-ranges = <0 5>; which will initialize the valid_mask.
> 
> You still need to tell the gpiolib that they are taken for other
> purposes somehow.
> 
> I think devm_gpiod_get_array() should have failed in that case.
> 
> The call graph should look like this:
> 
> devm_gpiod_get_array()
>      gpiod_get_array()
>          gpiod_get_index(0...n)
>              gpiod_find_and_request()
>                  gpiod_request()
>                      gpiod_request_commit()
>                          gpiochip_line_is_valid()

I remember trying to follow that call stack in the code. The beginning 
of it seems same, but for some reason I didn't end up in the 
gpiochip_line_is_valid(). This, however, requires confirmation :)

> 
> And gpiochip_line_is_valid() looks like this:
> 
> bool gpiochip_line_is_valid(const struct gpio_chip *gc,
>                  unsigned int offset)
> {
>      /* No mask means all valid */
>      if (likely(!gc->valid_mask))
>          return true;
>      return test_bit(offset, gc->valid_mask);
> }
> 
> So why is this not working?

couple of hours please, couple of hours ;)

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

CC: Geert (because, I think he was asked about the Rcar GPIO check before).

On 28/02/2025 10:23, Linus Walleij wrote:
> On Thu, Feb 27, 2025 at 9:24 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
 > >> I did some quick testing. I used:
> (...)
>> which left GPIO0 ... GPIO6 masked (pins used for ADC) and only GPIO7
>> unmasked.
>>
>> Then I added:
>> gpiotst {
>>          compatible = "rohm,foo-bd72720-gpio";
>>          rohm,dvs-vsel-gpios = <&adc 5 0>, <&adc 6 0>;
>> };
>>
>> and a dummy driver which does:
>> gpio_array = devm_gpiod_get_array(&pdev->dev, "rohm,dvs-vsel",
>>                                    GPIOD_OUT_LOW);
>>
>> ...
>>
>> ret = gpiod_set_array_value_cansleep(gpio_array->ndescs,
>>                  gpio_array->desc, gpio_array->info, values);
>>
>> As a result the bd79124 gpio driver got it's set_multiple called with
>> masked pins. (Oh, and I had accidentally prepared to handle this as I
>> had added a sanity check for pinmux register in the set_multiple()).
> 
> But... how did you mask of the pins 0..5 in valid_mask in this
> example?
> 
> If this is device tree, I would expect that at least you set up
> gpio-reserved-ranges = <0 5>; which will initialize the valid_mask.
> 
> You still need to tell the gpiolib that they are taken for other
> purposes somehow.
> 
> I think devm_gpiod_get_array() should have failed in that case.
> 
> The call graph should look like this:
> 
> devm_gpiod_get_array()
>      gpiod_get_array()
>          gpiod_get_index(0...n)
>              gpiod_find_and_request()
>                  gpiod_request()
>                      gpiod_request_commit()

Here in my setup the guard.gc->request == NULL. Thus the code never goes 
to the branch with the validation. And just before you ask me why the 
guard.gc->request is NULL - what do you call a blind bambi? :)
  - No idea.

>                          gpiochip_line_is_valid()

Eg, This is never called.

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Geert Uytterhoeven]

Hi Linus,

CC Biju

On Fri, 28 Feb 2025 at 09:07, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
> > On 26/02/2025 12:18, Linus Walleij wrote:
> > > That's easy to check with some git grep valid_mask
> >
> > True. I just tried. It seems mostly Ok, but...
> > For example the drivers/gpio/gpio-rcar.c uses the contents of the
> > 'valid_mask' in it's set_multiple callback to disallow setting the value
> > of masked GPIOs.
> >
> > For uneducated person like me, it feels this check should be done and
> > enforced by the gpiolib and not left for untrustworthy driver writers
> > like me! (I am working on BD79124 driver and it didn't occur to me I
> > should check for the valid_mask in driver :) If gpiolib may call the
> > driver's set_multiple() with masked lines - then the bd79124 driver just
> > had one unknown bug less :rolleyes:) )
>
> Yeah that should be done in gpiolib.
>
> And I think it is, gpiolib will not allow you to request a line
> that is not valid AFAIK.

Correct, since commit 3789f5acb9bbe088 ("gpiolib: Avoid calling
chip->request() for unused gpios") by Biju.

> This check in rcar is just overzealous and can probably be
> removed. Geert what do you say?

I looked at the history, and the related discussion.  It was actually
Biju who added the valid_mask check to gpio_rcar_set_multiple()
(triggering the creation of commit 3789f5acb9bbe088), and I just copied
that when adding gpio_rcar_get_multiple().
His v2[1] had checks in both the .request() and .set_multiple()
callbacks, but it's possible he added the latter first, and didn't
realize that became unneeded after adding the former.  The final version
v3[2] retained only the check in .set_multiple(), as by that time the
common gpiod_request_commit() had gained a check.

While .set_multiple() takes hardware offsets, not gpio_desc pointers,
these do originate from an array of gpio_desc pointers, so all of them
must have been requested properly.
We never exposed set_multiple() with raw GPIO numbers to users, right?
So I agree the check is probably not needed.

[1] https://lore.kernel.org/linux-renesas-soc/1533219087-33695-2-git-send-email-biju.das@bp.renesas.com
[2] https://lore.kernel.org/linux-renesas-soc/1533628626-26503-2-git-send-email-biju.das@bp.renesas.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 28/02/2025 11:28, Matti Vaittinen wrote:
> 
> CC: Geert (because, I think he was asked about the Rcar GPIO check before).
> 
> On 28/02/2025 10:23, Linus Walleij wrote:
>> On Thu, Feb 27, 2025 at 9:24 AM Matti Vaittinen
>> <mazziesaccount@gmail.com> wrote:

>> The call graph should look like this:
>>
>> devm_gpiod_get_array()
>>      gpiod_get_array()
>>          gpiod_get_index(0...n)
>>              gpiod_find_and_request()
>>                  gpiod_request()
>>                      gpiod_request_commit()
> 
> Here in my setup the guard.gc->request == NULL. Thus the code never goes 
> to the branch with the validation. And just before you ask me why the 
> guard.gc->request is NULL - what do you call a blind bambi? :)
>   - No idea.

Oh, I suppose the 'guard.gc' is just the chip structure. So, these 
validity checks are only applied if the gc provides the request 
callback? As far as I understand, the request callback is optional, and 
thus the validity check for GPIOs may be omitted.

> 
>>                          gpiochip_line_is_valid()
> 
> Eg, This is never called.
> 

Yours,
	-- Matti

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

On 28/02/2025 11:42, Matti Vaittinen wrote:
> On 28/02/2025 11:28, Matti Vaittinen wrote:
>>
>> CC: Geert (because, I think he was asked about the Rcar GPIO check 
>> before).
>>
>> On 28/02/2025 10:23, Linus Walleij wrote:
>>> On Thu, Feb 27, 2025 at 9:24 AM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
> 
>>> The call graph should look like this:
>>>
>>> devm_gpiod_get_array()
>>>      gpiod_get_array()
>>>          gpiod_get_index(0...n)
>>>              gpiod_find_and_request()
>>>                  gpiod_request()
>>>                      gpiod_request_commit()
>>
>> Here in my setup the guard.gc->request == NULL. Thus the code never 
>> goes to the branch with the validation. And just before you ask me why 
>> the guard.gc->request is NULL - what do you call a blind bambi? :)
>>   - No idea.
> 
> Oh, I suppose the 'guard.gc' is just the chip structure. So, these 
> validity checks are only applied if the gc provides the request 
> callback? As far as I understand, the request callback is optional, and 
> thus the validity check for GPIOs may be omitted.
> 
>>
>>>                          gpiochip_line_is_valid()

Would something like this be appropriate? It seems to work "on my 
machine" :) Do you see any unwanted side-effects?

+++ b/drivers/gpio/gpiolib.c
@@ -2315,6 +2315,10 @@ static int gpiod_request_commit(struct gpio_desc 
*desc, const char *label)
         if (!guard.gc)
                 return -ENODEV;

+       offset = gpio_chip_hwgpio(desc);
+       if (!gpiochip_line_is_valid(guard.gc, offset))
+               return -EINVAL;
+
         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
                 return -EBUSY;

@@ -2323,11 +2327,7 @@ static int gpiod_request_commit(struct gpio_desc 
*desc, const char *label)
          */

         if (guard.gc->request) {
-               offset = gpio_chip_hwgpio(desc);
-               if (gpiochip_line_is_valid(guard.gc, offset))
-                       ret = guard.gc->request(guard.gc, offset);
-               else
-                       ret = -EINVAL;
+               ret = guard.gc->request(guard.gc, offset);
                 if (ret)
                         goto out_clear_bit;
         }

I can craft a formal patch if this seems reasonable.

Yours,
	-- Matti

RE: [PATCH] gpio: Document the 'valid_mask' being internal

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 28 February 2025 09:32
> Subject: Re: [PATCH] gpio: Document the 'valid_mask' being internal
> 
> Hi Linus,
> 
> CC Biju
> 
> On Fri, 28 Feb 2025 at 09:07, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
> > <mazziesaccount@gmail.com> wrote:
> > > On 26/02/2025 12:18, Linus Walleij wrote:
> > > > That's easy to check with some git grep valid_mask
> > >
> > > True. I just tried. It seems mostly Ok, but...
> > > For example the drivers/gpio/gpio-rcar.c uses the contents of the
> > > 'valid_mask' in it's set_multiple callback to disallow setting the
> > > value of masked GPIOs.
> > >
> > > For uneducated person like me, it feels this check should be done
> > > and enforced by the gpiolib and not left for untrustworthy driver
> > > writers like me! (I am working on BD79124 driver and it didn't occur
> > > to me I should check for the valid_mask in driver :) If gpiolib may
> > > call the driver's set_multiple() with masked lines - then the
> > > bd79124 driver just had one unknown bug less :rolleyes:) )
> >
> > Yeah that should be done in gpiolib.
> >
> > And I think it is, gpiolib will not allow you to request a line that
> > is not valid AFAIK.
> 
> Correct, since commit 3789f5acb9bbe088 ("gpiolib: Avoid calling
> chip->request() for unused gpios") by Biju.
> 
> > This check in rcar is just overzealous and can probably be removed.
> > Geert what do you say?
> 
> I looked at the history, and the related discussion.  It was actually Biju who added the valid_mask
> check to gpio_rcar_set_multiple() (triggering the creation of commit 3789f5acb9bbe088), and I just
> copied that when adding gpio_rcar_get_multiple().
> His v2[1] had checks in both the .request() and .set_multiple() callbacks, but it's possible he added
> the latter first, and didn't realize that became unneeded after adding the former.  The final version
> v3[2] retained only the check in .set_multiple(), as by that time the common gpiod_request_commit()
> had gained a check.
> 
> While .set_multiple() takes hardware offsets, not gpio_desc pointers, these do originate from an array
> of gpio_desc pointers, so all of them must have been requested properly.
> We never exposed set_multiple() with raw GPIO numbers to users, right?
> So I agree the check is probably not needed.
> 

I agree, when the code is mainlined at that time set_multiple() has some draw backs and hence
the check is added to take care of GPIO holes.

Cheers,
Biju
  

Re: [PATCH] gpio: Document the 'valid_mask' being internal

[Matti Vaittinen]

Hi dee Ho peeps (Biju, Geert, Linus and all)

On 28/02/2025 12:28, Biju Das wrote:
> Hi Geert,
> 
>> -----Original Message-----
>> From: Geert Uytterhoeven <geert@linux-m68k.org>
>> On Fri, 28 Feb 2025 at 09:07, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
>>> <mazziesaccount@gmail.com> wrote:
>>>> On 26/02/2025 12:18, Linus Walleij wrote:


> I agree, when the code is mainlined at that time set_multiple() has some draw backs and hence
> the check is added to take care of GPIO holes.

If I don't read it wrong, rcar GPIO supports some input enabling "en 
masse" during the probe. It seems to me the gpio_rcar_enable_inputs() 
does also need the valid GPIOs information - I suppose some of the GPIOs 
may have been masked in the device-tree, and those shouldn't be enabled.

It feels counter productive to hide the valid_mask - and do some 
device-tree parsing in the driver(s) which may need it.

I suppose we can still hide the valid_mask in struct gpio_device as 
suggested - but then we should probably create a getter for it in the 
gpiolib.

Or does someone see a way around needing the valid_mask in the 
gpio_rcar_enable_inputs() ?

Have a nice weekend!

Yours,
	-- Matti