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

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Linus,

CC Biju

On Fri, 28 Feb 2025 at 09:07, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
> <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxxxxxxx
[2] https://lore.kernel.org/linux-renesas-soc/1533628626-26503-2-git-send-email-biju.das@xxxxxxxxxxxxxx

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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