Hi Pratyush, On Fri, 29 May 2020 at 16:04, Pratyush Yadav <p.ya...@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? > > Signed-off-by: Jean-Jacques Hiblot <jjhib...@ti.com> > Signed-off-by: Pratyush Yadav <p.ya...@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? > + > 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? > + */ > +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. > + 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. > +#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. Regards, Simon