[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