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

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> 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@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.
> 

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