| Message ID | Z71qphikHPGB0Yuv@mva-rohm |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: Document the 'valid_mask' being internal | expand |
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
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
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
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
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
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
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
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
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
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;
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(-) base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6