Hi Pratyush, On Mon, 8 Jun 2020 at 06:37, Pratyush Yadav <p.ya...@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.ya...@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.ya...@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
