Re: [PATCH 4/4] gpiolib: fix potential crash in gpiod_get() et alia
From: Alex Courbot <acourbot@nvidia.com>
Date: 2013-12-04 06:27:11
It seems like your subject line is wrong/truncated. 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.
quoted hunk ↗ jump to hunk
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).
quoted hunk ↗ jump to hunk
@@ -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 * @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.
quoted hunk ↗ jump to hunk
} /*@@ -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. Alex.