Re: [RFC PATCH 1/1] gpio: Handle NULL pointers gracefully

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