[U-Boot,v1,1/3] drivers: gpio: Handle gracefully NULL pointers
diff mbox series

Message ID 20191001115130.18886-2-jjhiblot@ti.com
State Under Review
Delegated to: Tom Rini
Headers show
Series
  • gpio: Add a managed API
Related show

Commit Message

Jean-Jacques Hiblot Oct. 1, 2019, 11:51 a.m. UTC
Prepare the way for a managed GPIO API by handling NULL pointers without
crashing nor failing.
VALIDATE_DESC() and validate_desc() come straight from Linux.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---

 drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
 include/asm-generic/gpio.h |  2 +-
 2 files changed, 57 insertions(+), 11 deletions(-)

Comments

Simon Glass Oct. 18, 2019, 8:38 p.m. UTC | #1
Hi Jean-Jacques,

On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
> Prepare the way for a managed GPIO API by handling NULL pointers without
> crashing nor failing.
> VALIDATE_DESC() and validate_desc() come straight from Linux.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>
>  drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
>  include/asm-generic/gpio.h |  2 +-
>  2 files changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 01cfa2f788..63c10f438b 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -18,6 +18,33 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +/*
> + * 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 int validate_desc(const struct gpio_desc *desc, const char *func)
> +{
> +       if (!desc)
> +               return 0;
> +       if (IS_ERR(desc)) {
> +               pr_warn("%s: invalid GPIO (errorpointer)\n", func);
> +               return PTR_ERR(desc);
> +       }
> +       if (!desc->dev) {
> +               pr_warn("%s: invalid GPIO (no device)\n", func);
> +               return -EINVAL;
> +       }
> +       return 1;
> +}
> +
> +#define VALIDATE_DESC(desc) do { \
> +       int __valid = validate_desc(desc, __func__); \
> +       if (__valid <= 0) \
> +               return __valid; \
> +       } while (0)

This adds to code size so should be behind a CONFIG I think.

> +
>  /**
>   * gpio_to_device() - Convert global GPIO number to device, number
>   *
> @@ -269,11 +296,14 @@ 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;
>
> +       VALIDATE_DESC(desc);
> +       dev = desc->dev;
> +
>         uc_priv = dev_get_uclass_priv(dev);
>         if (uc_priv->name[desc->offset])
>                 return -EBUSY;
> @@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func)
>  {
>         struct gpio_dev_priv *uc_priv;
>
> +       VALIDATE_DESC(desc);
> +
>         if (!dm_gpio_is_valid(desc))
>                 return -ENOENT;
>
> @@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
>         int value;
>         int ret;
>
> +       VALIDATE_DESC(desc);
> +
>         ret = check_reserved(desc, "get_value");
>         if (ret)
>                 return ret;
> @@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
>  {
>         int ret;
>
> +       VALIDATE_DESC(desc);
> +
>         ret = check_reserved(desc, "set_value");
>         if (ret)
>                 return ret;
> @@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
>
>  int dm_gpio_get_open_drain(struct gpio_desc *desc)
>  {
> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> +       struct dm_gpio_ops *ops;
>         int ret;
>
> +       VALIDATE_DESC(desc);
> +       ops = gpio_get_ops(desc->dev);
> +
>         ret = check_reserved(desc, "get_open_drain");
>         if (ret)
>                 return ret;
> @@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
>
>  int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
>  {
> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> +       struct dm_gpio_ops *ops;
>         int ret;
>
> +       VALIDATE_DESC(desc);
> +       ops = gpio_get_ops(desc->dev);
> +
>         ret = check_reserved(desc, "set_open_drain");
>         if (ret)
>                 return ret;
> @@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
>
>  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 udevice *dev;
> +       struct dm_gpio_ops *ops;
>         int ret;
>
> +       VALIDATE_DESC(desc);
> +       dev = desc->dev;
> +       ops = gpio_get_ops(dev);
> +
>         ret = check_reserved(desc, "set_dir");
>         if (ret)
>                 return ret;
> @@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc)
>  int gpio_get_value(unsigned gpio)
>  {
>         int ret;
> -

unrelated change?

>         struct gpio_desc desc;
>
>         ret = gpio_to_device(gpio, &desc);
> @@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
>
>  int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
>  {
> +       VALIDATE_DESC(desc);
> +
>         /* For now, we don't do any checking of dev */
>         return _dm_gpio_free(desc->dev, desc->offset);
>  }
> @@ -981,12 +1028,11 @@ 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;
> +       VALIDATE_DESC(desc);

I think this is pretty opaque. How about writing the code out in full,
with a helper function to do the check. The helper function can return
the error code perhaps.

ret = validate_desc(desc);
if (ret)
   return log_msg_ret("gpio_free", ret);

Regards,
Simon



> +
> +       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 d6cf18744f..ba669b65a9 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -139,7 +139,7 @@ struct gpio_desc {
>   */
>  static inline bool dm_gpio_is_valid(const struct gpio_desc *desc)
>  {
> -       return desc->dev != NULL;
> +       return desc && desc->dev;
>  }
>
>  /**
> --
> 2.17.1
>
Jean-Jacques Hiblot Oct. 21, 2019, 7:44 a.m. UTC | #2
On 18/10/2019 22:38, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>> Prepare the way for a managed GPIO API by handling NULL pointers without
>> crashing nor failing.
>> VALIDATE_DESC() and validate_desc() come straight from Linux.
>>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> ---
>>
>>   drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
>>   include/asm-generic/gpio.h |  2 +-
>>   2 files changed, 57 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>> index 01cfa2f788..63c10f438b 100644
>> --- a/drivers/gpio/gpio-uclass.c
>> +++ b/drivers/gpio/gpio-uclass.c
>> @@ -18,6 +18,33 @@
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> +/*
>> + * 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 int validate_desc(const struct gpio_desc *desc, const char *func)
>> +{
>> +       if (!desc)
>> +               return 0;
>> +       if (IS_ERR(desc)) {
>> +               pr_warn("%s: invalid GPIO (errorpointer)\n", func);
>> +               return PTR_ERR(desc);
>> +       }
>> +       if (!desc->dev) {
>> +               pr_warn("%s: invalid GPIO (no device)\n", func);
>> +               return -EINVAL;
>> +       }
>> +       return 1;
>> +}
>> +
>> +#define VALIDATE_DESC(desc) do { \
>> +       int __valid = validate_desc(desc, __func__); \
>> +       if (__valid <= 0) \
>> +               return __valid; \
>> +       } while (0)
> This adds to code size so should be behind a CONFIG I think.
I'm not sure we really want to keep this out. Most of the added code 
size, will be about the error messages. I would rather remove them (or 
use a debug() or warn_non_spl()
>> +
>>   /**
>>    * gpio_to_device() - Convert global GPIO number to device, number
>>    *
>> @@ -269,11 +296,14 @@ 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;
>>
>> +       VALIDATE_DESC(desc);
>> +       dev = desc->dev;
>> +
>>          uc_priv = dev_get_uclass_priv(dev);
>>          if (uc_priv->name[desc->offset])
>>                  return -EBUSY;
>> @@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func)
>>   {
>>          struct gpio_dev_priv *uc_priv;
>>
>> +       VALIDATE_DESC(desc);
>> +
>>          if (!dm_gpio_is_valid(desc))
>>                  return -ENOENT;
>>
>> @@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
>>          int value;
>>          int ret;
>>
>> +       VALIDATE_DESC(desc);
>> +
>>          ret = check_reserved(desc, "get_value");
>>          if (ret)
>>                  return ret;
>> @@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
>>   {
>>          int ret;
>>
>> +       VALIDATE_DESC(desc);
>> +
>>          ret = check_reserved(desc, "set_value");
>>          if (ret)
>>                  return ret;
>> @@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
>>
>>   int dm_gpio_get_open_drain(struct gpio_desc *desc)
>>   {
>> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
>> +       struct dm_gpio_ops *ops;
>>          int ret;
>>
>> +       VALIDATE_DESC(desc);
>> +       ops = gpio_get_ops(desc->dev);
>> +
>>          ret = check_reserved(desc, "get_open_drain");
>>          if (ret)
>>                  return ret;
>> @@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
>>
>>   int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
>>   {
>> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
>> +       struct dm_gpio_ops *ops;
>>          int ret;
>>
>> +       VALIDATE_DESC(desc);
>> +       ops = gpio_get_ops(desc->dev);
>> +
>>          ret = check_reserved(desc, "set_open_drain");
>>          if (ret)
>>                  return ret;
>> @@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
>>
>>   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 udevice *dev;
>> +       struct dm_gpio_ops *ops;
>>          int ret;
>>
>> +       VALIDATE_DESC(desc);
>> +       dev = desc->dev;
>> +       ops = gpio_get_ops(dev);
>> +
>>          ret = check_reserved(desc, "set_dir");
>>          if (ret)
>>                  return ret;
>> @@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc)
>>   int gpio_get_value(unsigned gpio)
>>   {
>>          int ret;
>> -
> unrelated change?
>
>>          struct gpio_desc desc;
>>
>>          ret = gpio_to_device(gpio, &desc);
>> @@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
>>
>>   int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
>>   {
>> +       VALIDATE_DESC(desc);
>> +
>>          /* For now, we don't do any checking of dev */
>>          return _dm_gpio_free(desc->dev, desc->offset);
>>   }
>> @@ -981,12 +1028,11 @@ 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;
>> +       VALIDATE_DESC(desc);
> I think this is pretty opaque. How about writing the code out in full,
> with a helper function to do the check. The helper function can return
> the error code perhaps.
>
> ret = validate_desc(desc);
> if (ret)
>     return log_msg_ret("gpio_free", ret);

The helper is there already. I could remove macro usage, or maybe rename 
the macro as VALIDATE_DESC_OR_EXIT().

JJ

>
> Regards,
> Simon
>
>
>
>> +
>> +       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 d6cf18744f..ba669b65a9 100644
>> --- a/include/asm-generic/gpio.h
>> +++ b/include/asm-generic/gpio.h
>> @@ -139,7 +139,7 @@ struct gpio_desc {
>>    */
>>   static inline bool dm_gpio_is_valid(const struct gpio_desc *desc)
>>   {
>> -       return desc->dev != NULL;
>> +       return desc && desc->dev;
>>   }
>>
>>   /**
>> --
>> 2.17.1
>>
Simon Glass Oct. 21, 2019, 10:53 p.m. UTC | #3
Hi Jean-Jacques,

On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>
>
> On 18/10/2019 22:38, Simon Glass wrote:
> > Hi Jean-Jacques,
> >
> > On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
> >> Prepare the way for a managed GPIO API by handling NULL pointers without
> >> crashing nor failing.
> >> VALIDATE_DESC() and validate_desc() come straight from Linux.
> >>
> >> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> >> ---
> >>
> >>   drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
> >>   include/asm-generic/gpio.h |  2 +-
> >>   2 files changed, 57 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> >> index 01cfa2f788..63c10f438b 100644
> >> --- a/drivers/gpio/gpio-uclass.c
> >> +++ b/drivers/gpio/gpio-uclass.c
> >> @@ -18,6 +18,33 @@
> >>
> >>   DECLARE_GLOBAL_DATA_PTR;
> >>
> >> +/*
> >> + * 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 int validate_desc(const struct gpio_desc *desc, const char *func)
> >> +{
> >> +       if (!desc)
> >> +               return 0;
> >> +       if (IS_ERR(desc)) {
> >> +               pr_warn("%s: invalid GPIO (errorpointer)\n", func);
> >> +               return PTR_ERR(desc);
> >> +       }
> >> +       if (!desc->dev) {
> >> +               pr_warn("%s: invalid GPIO (no device)\n", func);
> >> +               return -EINVAL;
> >> +       }
> >> +       return 1;
> >> +}
> >> +
> >> +#define VALIDATE_DESC(desc) do { \
> >> +       int __valid = validate_desc(desc, __func__); \
> >> +       if (__valid <= 0) \
> >> +               return __valid; \
> >> +       } while (0)
> > This adds to code size so should be behind a CONFIG I think.
> I'm not sure we really want to keep this out. Most of the added code
> size, will be about the error messages. I would rather remove them (or
> use a debug() or warn_non_spl()

You should probably do that anyway.

But these checks do add to code size, and we should be careful not to
have unnecessary checks in the final firmware. We can enable them
during development, but don't want the code bloated with lots of
pointless checks.

> >> +
> >>   /**
> >>    * gpio_to_device() - Convert global GPIO number to device, number
> >>    *
> >> @@ -269,11 +296,14 @@ 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;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +       dev = desc->dev;
> >> +
> >>          uc_priv = dev_get_uclass_priv(dev);
> >>          if (uc_priv->name[desc->offset])
> >>                  return -EBUSY;
> >> @@ -400,6 +430,8 @@ static int check_reserved(const struct gpio_desc *desc, const char *func)
> >>   {
> >>          struct gpio_dev_priv *uc_priv;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +
> >>          if (!dm_gpio_is_valid(desc))
> >>                  return -ENOENT;
> >>
> >> @@ -468,6 +500,8 @@ int dm_gpio_get_value(const struct gpio_desc *desc)
> >>          int value;
> >>          int ret;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +
> >>          ret = check_reserved(desc, "get_value");
> >>          if (ret)
> >>                  return ret;
> >> @@ -481,6 +515,8 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
> >>   {
> >>          int ret;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +
> >>          ret = check_reserved(desc, "set_value");
> >>          if (ret)
> >>                  return ret;
> >> @@ -493,9 +529,12 @@ int dm_gpio_set_value(const struct gpio_desc *desc, int value)
> >>
> >>   int dm_gpio_get_open_drain(struct gpio_desc *desc)
> >>   {
> >> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> >> +       struct dm_gpio_ops *ops;
> >>          int ret;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +       ops = gpio_get_ops(desc->dev);
> >> +
> >>          ret = check_reserved(desc, "get_open_drain");
> >>          if (ret)
> >>                  return ret;
> >> @@ -508,9 +547,12 @@ int dm_gpio_get_open_drain(struct gpio_desc *desc)
> >>
> >>   int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
> >>   {
> >> -       struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
> >> +       struct dm_gpio_ops *ops;
> >>          int ret;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +       ops = gpio_get_ops(desc->dev);
> >> +
> >>          ret = check_reserved(desc, "set_open_drain");
> >>          if (ret)
> >>                  return ret;
> >> @@ -525,10 +567,14 @@ int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
> >>
> >>   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 udevice *dev;
> >> +       struct dm_gpio_ops *ops;
> >>          int ret;
> >>
> >> +       VALIDATE_DESC(desc);
> >> +       dev = desc->dev;
> >> +       ops = gpio_get_ops(dev);
> >> +
> >>          ret = check_reserved(desc, "set_dir");
> >>          if (ret)
> >>                  return ret;
> >> @@ -570,7 +616,6 @@ int dm_gpio_set_dir(struct gpio_desc *desc)
> >>   int gpio_get_value(unsigned gpio)
> >>   {
> >>          int ret;
> >> -
> > unrelated change?
> >
> >>          struct gpio_desc desc;
> >>
> >>          ret = gpio_to_device(gpio, &desc);
> >> @@ -933,6 +978,8 @@ int gpio_get_list_count(struct udevice *dev, const char *list_name)
> >>
> >>   int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
> >>   {
> >> +       VALIDATE_DESC(desc);
> >> +
> >>          /* For now, we don't do any checking of dev */
> >>          return _dm_gpio_free(desc->dev, desc->offset);
> >>   }
> >> @@ -981,12 +1028,11 @@ 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;
> >> +       VALIDATE_DESC(desc);
> > I think this is pretty opaque. How about writing the code out in full,
> > with a helper function to do the check. The helper function can return
> > the error code perhaps.
> >
> > ret = validate_desc(desc);
> > if (ret)
> >     return log_msg_ret("gpio_free", ret);
>
> The helper is there already. I could remove macro usage, or maybe rename
> the macro as VALIDATE_DESC_OR_EXIT().

I't the implicit return that I am not keen on. Is it needed? I don't
see that sort of thing in U-Boot at present.

Regards,
Simon
Jean-Jacques Hiblot Oct. 22, 2019, 10:26 a.m. UTC | #4
On 22/10/2019 00:53, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Mon, 21 Oct 2019 at 01:45, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>
>> On 18/10/2019 22:38, Simon Glass wrote:
>>> Hi Jean-Jacques,
>>>
>>> On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot@ti.com> wrote:
>>>> Prepare the way for a managed GPIO API by handling NULL pointers without
>>>> crashing nor failing.
>>>> VALIDATE_DESC() and validate_desc() come straight from Linux.
>>>>
>>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>>> ---
>>>>
>>>>    drivers/gpio/gpio-uclass.c | 66 ++++++++++++++++++++++++++++++++------
>>>>    include/asm-generic/gpio.h |  2 +-
>>>>    2 files changed, 57 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
>>>> index 01cfa2f788..63c10f438b 100644
>>>> --- a/drivers/gpio/gpio-uclass.c
>>>> +++ b/drivers/gpio/gpio-uclass.c
>>>> @@ -18,6 +18,33 @@
>>>>
>>>>    DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> +/*
>>>> + * 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 int validate_desc(const struct gpio_desc *desc, const char *func)
>>>> +{
>>>> +       if (!desc)
>>>> +               return 0;
>>>> +       if (IS_ERR(desc)) {
>>>> +               pr_warn("%s: invalid GPIO (errorpointer)\n", func);
>>>> +               return PTR_ERR(desc);
>>>> +       }
>>>> +       if (!desc->dev) {
>>>> +               pr_warn("%s: invalid GPIO (no device)\n", func);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +       return 1;
>>>> +}
>>>> +
>>>> +#define VALIDATE_DESC(desc) do { \
>>>> +       int __valid = validate_desc(desc, __func__); \
>>>> +       if (__valid <= 0) \
>>>> +               return __valid; \
>>>> +       } while (0)
>>> This adds to code size so should be behind a CONFIG I think.
>> I'm not sure we really want to keep this out. Most of the added code
>> size, will be about the error messages. I would rather remove them (or
>> use a debug() or warn_non_spl()
> You should probably do that anyway.
>
> But these checks do add to code size, and we should be careful not to
> have unnecessary checks in the final firmware. We can enable them
> during development, but don't want the code bloated with lots of
> pointless checks.

I see, but I don't agree that this is pointless. It allows for optional 
GPIO support.

Practically speaking, in u-boot, that will probably be used only by the 
users of the managed API: devm_gpiod_get_optional() and 
devm_gpiod_get_index_optional()


>
>>>> +
>>>>    /**
>>>> I't the implicit return that I am not keen on. Is it needed? I don't
>>>> see that sort of thing in U-Boot at present.

Well it bothered me too at first when I looked at how it was done in 
linux. But I got used to it pretty quickly :)

I'll remove this in the v2


Thanks for the review.


JJ

>>>>
>>>> Regards,
>>>> Simon
>>>>

Patch
diff mbox series

diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 01cfa2f788..63c10f438b 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -18,6 +18,33 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
+/*
+ * 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 int validate_desc(const struct gpio_desc *desc, const char *func)
+{
+	if (!desc)
+		return 0;
+	if (IS_ERR(desc)) {
+		pr_warn("%s: invalid GPIO (errorpointer)\n", func);
+		return PTR_ERR(desc);
+	}
+	if (!desc->dev) {
+		pr_warn("%s: invalid GPIO (no device)\n", func);
+		return -EINVAL;
+	}
+	return 1;
+}
+
+#define VALIDATE_DESC(desc) do { \
+	int __valid = validate_desc(desc, __func__); \
+	if (__valid <= 0) \
+		return __valid; \
+	} while (0)
+
 /**
  * gpio_to_device() - Convert global GPIO number to device, number
  *
@@ -269,11 +296,14 @@  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;
 
+	VALIDATE_DESC(desc);
+	dev = desc->dev;
+
 	uc_priv = dev_get_uclass_priv(dev);
 	if (uc_priv->name[desc->offset])
 		return -EBUSY;
@@ -400,6 +430,8 @@  static int check_reserved(const struct gpio_desc *desc, const char *func)
 {
 	struct gpio_dev_priv *uc_priv;
 
+	VALIDATE_DESC(desc);
+
 	if (!dm_gpio_is_valid(desc))
 		return -ENOENT;
 
@@ -468,6 +500,8 @@  int dm_gpio_get_value(const struct gpio_desc *desc)
 	int value;
 	int ret;
 
+	VALIDATE_DESC(desc);
+
 	ret = check_reserved(desc, "get_value");
 	if (ret)
 		return ret;
@@ -481,6 +515,8 @@  int dm_gpio_set_value(const struct gpio_desc *desc, int value)
 {
 	int ret;
 
+	VALIDATE_DESC(desc);
+
 	ret = check_reserved(desc, "set_value");
 	if (ret)
 		return ret;
@@ -493,9 +529,12 @@  int dm_gpio_set_value(const struct gpio_desc *desc, int value)
 
 int dm_gpio_get_open_drain(struct gpio_desc *desc)
 {
-	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	struct dm_gpio_ops *ops;
 	int ret;
 
+	VALIDATE_DESC(desc);
+	ops = gpio_get_ops(desc->dev);
+
 	ret = check_reserved(desc, "get_open_drain");
 	if (ret)
 		return ret;
@@ -508,9 +547,12 @@  int dm_gpio_get_open_drain(struct gpio_desc *desc)
 
 int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
 {
-	struct dm_gpio_ops *ops = gpio_get_ops(desc->dev);
+	struct dm_gpio_ops *ops;
 	int ret;
 
+	VALIDATE_DESC(desc);
+	ops = gpio_get_ops(desc->dev);
+
 	ret = check_reserved(desc, "set_open_drain");
 	if (ret)
 		return ret;
@@ -525,10 +567,14 @@  int dm_gpio_set_open_drain(struct gpio_desc *desc, int value)
 
 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 udevice *dev;
+	struct dm_gpio_ops *ops;
 	int ret;
 
+	VALIDATE_DESC(desc);
+	dev = desc->dev;
+	ops = gpio_get_ops(dev);
+
 	ret = check_reserved(desc, "set_dir");
 	if (ret)
 		return ret;
@@ -570,7 +616,6 @@  int dm_gpio_set_dir(struct gpio_desc *desc)
 int gpio_get_value(unsigned gpio)
 {
 	int ret;
-
 	struct gpio_desc desc;
 
 	ret = gpio_to_device(gpio, &desc);
@@ -933,6 +978,8 @@  int gpio_get_list_count(struct udevice *dev, const char *list_name)
 
 int dm_gpio_free(struct udevice *dev, struct gpio_desc *desc)
 {
+	VALIDATE_DESC(desc);
+
 	/* For now, we don't do any checking of dev */
 	return _dm_gpio_free(desc->dev, desc->offset);
 }
@@ -981,12 +1028,11 @@  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;
+	VALIDATE_DESC(desc);
+
+	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 d6cf18744f..ba669b65a9 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -139,7 +139,7 @@  struct gpio_desc {
  */
 static inline bool dm_gpio_is_valid(const struct gpio_desc *desc)
 {
-	return desc->dev != NULL;
+	return desc && desc->dev;
 }
 
 /**