[RFC PATCH 1/1] gpio: Handle NULL pointers gracefully
Simon Glass
sjg at chromium.org
Mon Jun 1 19:08:58 CEST 2020
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?
>
> 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?
> +
> 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
More information about the U-Boot
mailing list