[RFC PATCH 1/1] gpio: Handle NULL pointers gracefully
Pratyush Yadav
p.yadav at ti.com
Mon Jun 8 14:37:29 CEST 2020
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.
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?
> > Signed-off-by: Jean-Jacques Hiblot <jjhiblot at ti.com>
> > Signed-off-by: Pratyush Yadav <p.yadav at ti.com>
> > ---
> > drivers/gpio/Kconfig | 9 ++++
> > drivers/gpio/gpio-uclass.c | 86 ++++++++++++++++++++++++++++++++++----
> > include/asm-generic/gpio.h | 2 +-
> > 3 files changed, 88 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index d87f6cc105..f8b6bcdf44 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -36,6 +36,15 @@ config TPL_DM_GPIO
> > particular GPIOs that they provide. The uclass interface
> > is defined in include/asm-generic/gpio.h.
> >
> > +config GPIO_VALIDATE_DESC
> > + bool "Check if GPIO descriptor is NULL and bail out if it is"
> > + depends on DM_GPIO
> > + default y
> > + help
> > + If a GPIO is optional, the GPIO descriptor is NULL. In that
> > + case, calls should bail out instead of causing NULL pointer
> > + access.
>
> Can you update the gpio function docs in the header file with this info?
Ok.
> > +
> > config GPIO_HOG
> > bool "Enable GPIO hog support"
> > depends on DM_GPIO
> > diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> > index 9eeab22eef..6b97d3aaff 100644
> > --- a/drivers/gpio/gpio-uclass.c
> > +++ b/drivers/gpio/gpio-uclass.c
> > @@ -20,6 +20,25 @@
> >
> > DECLARE_GLOBAL_DATA_PTR;
> >
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > +/*
> > + * This descriptor validation needs to be inserted verbatim into each
> > + * function taking a descriptor, so we need to use a preprocessor
> > + * macro to avoid endless duplication. If the desc is NULL it is an
> > + * optional GPIO and calls should just bail out.
>
> Is the macro defined elsewhere?
It comes from the previous version of this patch. The macro has been
removed in this version. Will fix.
> > + */
> > +static inline int validate_desc(const struct gpio_desc *desc)
> > +{
> > + if (!desc)
> > + return 0;
> > + if (IS_ERR(desc))
> > + return PTR_ERR(desc);
> > + if (!desc->dev)
> > + return -EINVAL;
> > + return 1;
> > +}
>
> #else
> static inline int validate_desc(const struct gpio_desc *desc)
> {
> return 1;
> }
>
> > +#endif
> > +
> > /**
> > * 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?
> > +#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
> > + int ret;
> > +
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > if (!dm_gpio_is_valid(desc))
> > return -ENOENT;
> >
> > @@ -510,6 +545,12 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
> > {
> > int ret;
> >
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > ret = check_reserved(desc, "get_value");
> > if (ret)
> > return ret;
> > @@ -521,6 +562,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
> > {
> > int ret;
> >
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > ret = check_reserved(desc, "set_value");
> > if (ret)
> > return ret;
> > @@ -572,11 +619,21 @@ static int check_dir_flags(ulong flags)
> >
> > static int _dm_gpio_set_dir_flags(struct gpio_desc *desc, ulong flags)
> > {
> > - struct udevice *dev = desc->dev;
> > - struct dm_gpio_ops *ops = gpio_get_ops(dev);
> > - struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> > + struct udevice *dev;
> > + struct dm_gpio_ops *ops;
> > + struct gpio_dev_priv *uc_priv;
> > int ret = 0;
> >
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > + dev = desc->dev;
> > + ops = gpio_get_ops(dev);
> > + uc_priv = dev_get_uclass_priv(dev);
> > +
> > ret = check_dir_flags(flags);
> > if (ret) {
> > dev_dbg(dev,
> > @@ -1043,6 +1100,14 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
> >
> > int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
> > {
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > + int ret;
> > +
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > /* For now, we don't do any checking of dev */
> > return _dm_gpio_free(desc->dev, desc->offset);
> > }
> > @@ -1091,12 +1156,17 @@ static int gpio_renumber(struct udevice *removed_dev)
> >
> > int gpio_get_number(const struct gpio_desc *desc)
> > {
> > - struct udevice *dev = desc->dev;
> > struct gpio_dev_priv *uc_priv;
> >
> > - if (!dev)
> > - return -1;
> > - uc_priv = dev->uclass_priv;
> > +#ifdef CONFIG_GPIO_VALIDATE_DESC
> > + int ret;
> > +
> > + ret = validate_desc(desc);
> > + if (ret <= 0)
> > + return ret;
> > +#endif
> > +
> > + uc_priv = dev_get_uclass_priv(desc->dev);
> >
> > return uc_priv->gpio_base + desc->offset;
> > }
> > 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?
--
Regards,
Pratyush Yadav
Texas Instruments India
More information about the U-Boot
mailing list