Re: [PATCH 4/4] gpiolib: fix potential crash in gpiod_get() et alia

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, 2013-12-04 at 15:27 +0900, Alex Courbot wrote:
> It seems like your subject line is wrong/truncated.

No, 'et alia' is Latin which means "and others (of any gender)".

> On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
> > Since we may have dev parameter equial to NULL we can't use dev_* macros to
> 
> s/equial/equal.

Thanks, though it seems it'll be not needed in future. ;)

> > print messages. Instead we must switch to plain pr_*.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/gpio/gpiolib.c | 35 +++++++++++++++++++----------------
> >   1 file changed, 19 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 222f544..1517093 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2389,16 +2389,14 @@ static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
> >   			continue;
> >
> >   		chip = find_chip_by_name(p->chip_label);
> > -
> >   		if (!chip) {
> > -			dev_warn(dev, "cannot find GPIO chip %s\n",
> > -				 p->chip_label);
> > +			pr_warn("%s: GPIO chip: cannot find\n", p->chip_label);
> >   			continue;
> >   		}
> >
> >   		if (chip->ngpio <= p->chip_hwnum) {
> > -			dev_warn(dev, "GPIO chip %s has %d GPIOs\n",
> > -				 chip->label, chip->ngpio);
> > +			pr_warn("%s: GPIO chip: has %d GPIOs\n", chip->label,
> > +				chip->ngpio);
> >   			continue;
> >   		}
> 
> I don't think this is needed, the dev_* macros are safe to use with a 
> NULL device (see the definition of __dev_printk in drivers/base/core.c).

Indeed, I had to dive to it.

Anyway, since they are all *_dbg calls would it makes sense to insert
function name in those messages?

> >
> > @@ -2425,7 +2423,7 @@ EXPORT_SYMBOL_GPL(gpiod_get);
> >
> >   /**
> >    * gpiod_get_index - obtain a GPIO from a multi-index GPIO function
> > - * @dev:	GPIO consumer
> > + * @dev:	GPIO consumer, can be NULL for system-global GPIOs

This one is still valid, I think.

> >    * @con_id:	function within the GPIO consumer
> >    * @idx:	index of the GPIO to obtain in the consumer
> >    *
> > @@ -2442,15 +2440,17 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> >   	int status;
> >   	enum gpio_lookup_flags flags = 0;
> >
> > -	dev_dbg(dev, "GPIO lookup for consumer %s\n", con_id);
> > +	pr_debug("%s: GPIO lookup for consumer %s\n", __func__, con_id);
> >
> > -	/* Using device tree? */
> > -	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
> > -		dev_dbg(dev, "using device tree for GPIO lookup\n");
> > -		desc = of_find_gpio(dev, con_id, idx, &flags);
> > -	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
> > -		dev_dbg(dev, "using ACPI for GPIO lookup\n");
> > -		desc = acpi_find_gpio(dev, con_id, idx, &flags);
> > +	if (dev) {
> > +		if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
> > +			pr_debug("%s: using device tree for GPIO lookup\n",
> > +				 __func__);
> > +			desc = of_find_gpio(dev, con_id, idx, &flags);
> > +		} else if (IS_ENABLED(CONFIG_ACPI) && ACPI_HANDLE(dev)) {
> > +			pr_debug("%s: using ACPI for GPIO lookup\n", __func__);
> > +			desc = acpi_find_gpio(dev, con_id, idx, &flags);
> > +		}
> 
> Here I'd argue it is useful to know which device is making the request.

I expected this comment. I have no objection, since I had to mark this
patch as RFC.

> >   	}
> >
> >   	/*
> > @@ -2459,15 +2459,18 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
> >   	 */
> >   	if (!desc || IS_ERR(desc)) {
> >   		struct gpio_desc *pdesc;
> > -		dev_dbg(dev, "using lookup tables for GPIO lookup");
> > +
> > +		pr_debug("%s: using lookup tables for GPIO lookup\n", __func__);
> > +
> >   		pdesc = gpiod_find(dev, con_id, idx, &flags);
> > +
> >   		/* If used as fallback, do not replace the previous error */
> >   		if (!IS_ERR(pdesc) || !desc)
> >   			desc = pdesc;
> >   	}
> >
> >   	if (IS_ERR(desc)) {
> > -		dev_dbg(dev, "lookup for GPIO %s failed\n", con_id);
> > +		pr_debug("%s: lookup for GPIO %s failed\n", __func__, con_id);
> >   		return desc;
> >   	}
> 
> Unless I missed something, I'd suggest to drop this patch.

See above comments. I guess it could be split to at least one patch to
amend commentary and perhaps another one which inserts __func__ into
messages.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy