[U-Boot] [PATCH v1 1/3] drivers: gpio: Handle gracefully NULL pointers

Jean-Jacques Hiblot jjhiblot at ti.com
Mon Oct 21 07:44:18 UTC 2019


On 18/10/2019 22:38, Simon Glass wrote:
> Hi Jean-Jacques,
>
> On Tue, 1 Oct 2019 at 05:51, Jean-Jacques Hiblot <jjhiblot at 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 at 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
>>


More information about the U-Boot mailing list