[RFC PATCH 1/1] gpio: Handle NULL pointers gracefully

[Simon Glass]

Hi Pratyush,

On Mon, 8 Jun 2020 at 06:37, Pratyush Yadav <p.yadav at ti.com> wrote:
>
> On 01/06/20 11:08AM, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Fri, 29 May 2020 at 16:04, Pratyush Yadav <p.yadav at ti.com> wrote:
> > >
> > > Prepare the way for a managed GPIO API by handling NULL pointers without
> > > crashing or failing. validate_desc() comes from Linux with the prints
> > > removed to reduce code size.
> >
> > Please can you add a little detail as to why this is needed? Are you
> > trying to pass a NULL GPIO descriptor to the function?
>
> Copy-pasting from the cover letter:
>
>   Patch [0] added devm_gpiod_get_index_optional() which would return NULL
>   when when no GPIO was assigned to the requested function. This is
>   convenient for drivers that need to handle optional GPIOs.
>
>   We need to take a stance on who is responsible for the NULL check: the
>   driver or the GPIO core? Do we want to trust drivers to take care of the
>   NULL checks, or do we want to distrust them and make sure they don't
>   send us anything bogus in the GPIO core. Linux does not generally trust
>   drivers and usually verifies anything it gets from them. And FWIW, I see
>   that the clk and phy subsystems in U-Boot also perform checks like this.
>
>   The downside of the checks is of course that they increase code size.
>   They might also slightly decrease performance. The benefit is that we
>   don't burden drivers with taking care of this.

U-Boot has code-size constraints so I would rather rely on automated
testing than run-time checks. Linux has unlimited code size and few
automated tests so it is a different situation.

>   The patch itself is based on a similar patch by Jean-Jacques.
>
>   [0] https://patchwork.ozlabs.org/project/uboot/patch/20200529213808.2815-2-p.yadav@ti.com/
>
> Maybe I should put this in the commit message?

Yes
[..]

> > > +
> > >  /**
> > >   * gpio_desc_init() - Initialize the GPIO descriptor
> > >   *
> > > @@ -303,11 +322,19 @@ int gpio_hog_lookup_name(const char *name, struct gpio_desc **desc)
> > >
> > >  int dm_gpio_request(struct gpio_desc *desc, const char *label)
> > >  {
> > > -       struct udevice *dev = desc->dev;
> > > +       struct udevice *dev;
> > >         struct gpio_dev_priv *uc_priv;
> > >         char *str;
> > >         int ret;
> > >
> > > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> >
> > Please drop the #ifdefs and use the static inline thing from above.
>
> Ok.
>
> > > +       ret = validate_desc(desc);
> > > +       if (ret <= 0)
> > > +               return ret;
> >
> > Here you are returning 0 when you did not successfully request the
> > GPIO.You should return -ENOENT, otherwise callers have no idea what
> > happened and will get confused.
>
> Ok. To be clear, it is ok to return 0 in other places this check is
> done, right?

I'm not a fan of that, but I think you can put the checking in your own layer?

>
> > > +#endif
> > > +
> > > +       dev = desc->dev;
> > > +
> > >         uc_priv = dev_get_uclass_priv(dev);
> > >         if (uc_priv->name[desc->offset])
> > >                 return -EBUSY;
> > > @@ -434,6 +461,14 @@ static int check_reserved(const struct gpio_desc *desc, const char *func)
> > >  {
> > >         struct gpio_dev_priv *uc_priv;
> > >
> > > +#ifdef CONFIG_GPIO_VALIDATE_DESC

if (CONFIG_IS_ENABLED(...))

> > > +       int ret;
> > > +
> > > +       ret = validate_desc(desc);
> > > +       if (ret <= 0)
> > > +               return ret;
> > > +#endif

So if validate_desc() is a static function you can always call it, and
have it do nothing (and return 0) if the CONFIG is not enabled.

[..]

> > > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> > > index e16c2f31d9..46007b1283 100644
> > > --- a/include/asm-generic/gpio.h
> > > +++ b/include/asm-generic/gpio.h
> > > @@ -149,7 +149,7 @@ struct gpio_desc {
> > >   */
> > >  static inline bool dm_gpio_is_valid(const struct gpio_desc *desc)
> > >  {
> > > -       return desc->dev != NULL;
> > > +       return desc && desc->dev;
> >
> > Needs IS_ENABLED(CONFIG_GPIO_VALIDATE_DESC) somewhere here.
>
> Since this already checks for desc->dev != NULL, I figured it would be
> OK to check for desc being valid too before dereferencing it without
> adding a config check. Are you sure we want to make the desc check
> optional here?

Yes because you are changing the meaning here. I am a little worried
as to where this is heading.

Regards,
Simon