diff mbox series

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

Message ID 20200529220355.4396-2-p.yadav@ti.com
State New
Delegated to: Tom Rini
Headers show
Series gpio: Handle NULL pointers gracefully | expand

Commit Message

Pratyush Yadav May 29, 2020, 10:03 p.m. UTC
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.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Signed-off-by: Pratyush Yadav <p.yadav@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(-)

--
2.26.2

Comments

Simon Glass June 1, 2020, 5:08 p.m. UTC | #1
Hi Pratyush,

On Fri, 29 May 2020 at 16:04, Pratyush Yadav <p.yadav@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@ti.com>
> Signed-off-by: Pratyush Yadav <p.yadav@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
Pratyush Yadav June 8, 2020, 12:37 p.m. UTC | #2
On 01/06/20 11:08AM, Simon Glass wrote:
> Hi Pratyush,
> 
> On Fri, 29 May 2020 at 16:04, Pratyush Yadav <p.yadav@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@ti.com>
> > Signed-off-by: Pratyush Yadav <p.yadav@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?
Simon Glass June 17, 2020, 3:11 a.m. UTC | #3
Hi Pratyush,

On Mon, 8 Jun 2020 at 06:37, Pratyush Yadav <p.yadav@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@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
diff mbox series

Patch

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.
+
 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.
+ */
+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;
+}
+#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
+	ret = validate_desc(desc);
+	if (ret <= 0)
+		return ret;
+#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;
 }

 /**