[PATCH 0/4] gpiolib: clean up and potential bug fix

[PATCH 0/4] gpiolib: clean up and potential bug fix

[Andy Shevchenko]

The patch series mostly addressing messaging format and fix for one potential
bug. It depends on recent Alex's patch [1] and Mika's patchset [2].

Compile tested and tested on Medfield with SFI. If Mika would have time, he
might test it tomorrow on newer machines.

[1] http://www.spinics.net/lists/kernel/msg1645907.html
[2] http://www.spinics.net/lists/linux-acpi/msg47572.html

Andy Shevchenko (4):
  gpiolib: unify pr_* messages format
  gpiolib: introduce chip_* to print with chip->label prefix
  gpiolib: convert gpiod_lookup description to kernel-doc
  gpiolib: fix potential crash in gpiod_get() et alia

 drivers/gpio/gpiolib.c      | 148 ++++++++++++++++++++++++--------------------
 include/linux/gpio/driver.h |  26 +++-----
 2 files changed, 89 insertions(+), 85 deletions(-)

-- 
1.8.4.4

[PATCH 1/4] gpiolib: unify pr_* messages format

[Andy Shevchenko]

This patch includes following amendments:
 1) use "?" as a label when last is not defined in gpiod_*;
 2) whenever it's possilbe gpiod_* are used;
 3) print a function name, if it's already used in other messages.

Additionally it fixes an indentation in few places.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 72 ++++++++++++++++++++++++--------------------------
 1 file changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f7af309..9ed6f19 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -86,36 +86,36 @@ static int gpiod_request(struct gpio_desc *desc, const char *label);
 static void gpiod_free(struct gpio_desc *desc);
 
 #ifdef CONFIG_DEBUG_FS
-#define gpiod_emerg(desc, fmt, ...)			                \
-	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label, \
+#define gpiod_emerg(desc, fmt, ...)					       \
+	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
                  ##__VA_ARGS__)
-#define gpiod_crit(desc, fmt, ...)			                \
-	pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_crit(desc, fmt, ...)					       \
+	pr_crit("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                  ##__VA_ARGS__)
-#define gpiod_err(desc, fmt, ...)				        \
-	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,   \
+#define gpiod_err(desc, fmt, ...)					       \
+	pr_err("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",  \
                  ##__VA_ARGS__)
-#define gpiod_warn(desc, fmt, ...)				        \
-	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_warn(desc, fmt, ...)					       \
+	pr_warn("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                  ##__VA_ARGS__)
-#define gpiod_info(desc, fmt, ...)				        \
-	pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label,  \
+#define gpiod_info(desc, fmt, ...)					       \
+	pr_info("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?", \
                 ##__VA_ARGS__)
-#define gpiod_dbg(desc, fmt, ...)				   \
-	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label, \
+#define gpiod_dbg(desc, fmt, ...)					       \
+	pr_debug("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
                  ##__VA_ARGS__)
 #else
-#define gpiod_emerg(desc, fmt, ...)			           \
+#define gpiod_emerg(desc, fmt, ...)					\
 	pr_emerg("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_crit(desc, fmt, ...)			           \
+#define gpiod_crit(desc, fmt, ...)					\
 	pr_crit("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_err(desc, fmt, ...)				   \
+#define gpiod_err(desc, fmt, ...)					\
 	pr_err("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_warn(desc, fmt, ...)				   \
+#define gpiod_warn(desc, fmt, ...)					\
 	pr_warn("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_info(desc, fmt, ...)				   \
+#define gpiod_info(desc, fmt, ...)					\
 	pr_info("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
-#define gpiod_dbg(desc, fmt, ...)				   \
+#define gpiod_dbg(desc, fmt, ...)					\
 	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
 #endif
 
@@ -188,7 +188,8 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
 	if (WARN(test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0,
 			"autorequest GPIO-%d\n", gpio)) {
 		if (!try_module_get(chip->owner)) {
-			pr_err("GPIO-%d: module can't be gotten \n", gpio);
+			gpiod_err(desc, "%s: module can't be gotten\n",
+					__func__);
 			clear_bit(FLAG_REQUESTED, &desc->flags);
 			/* lose */
 			return -EIO;
@@ -809,8 +810,8 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
 	     test_bit(FLAG_EXPORT, &desc->flags)) {
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		pr_debug("%s: gpio %d unavailable (requested=%d, exported=%d)\n",
-				__func__, desc_to_gpio(desc),
+		gpiod_dbg(desc, "%s: unavailable (requested=%d, exported=%d)\n",
+				__func__,
 				test_bit(FLAG_REQUESTED, &desc->flags),
 				test_bit(FLAG_EXPORT, &desc->flags));
 		status = -EPERM;
@@ -858,8 +859,7 @@ fail_unregister_device:
 	device_unregister(dev);
 fail_unlock:
 	mutex_unlock(&sysfs_lock);
-	pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-		 status);
+	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiod_export);
@@ -907,8 +907,7 @@ int gpiod_export_link(struct device *dev, const char *name,
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 
 	return status;
 }
@@ -952,8 +951,7 @@ unlock:
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 
 	return status;
 }
@@ -995,8 +993,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 	}
 
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
@@ -1223,7 +1220,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (status)
 		goto fail;
 
-	pr_debug("gpiochip_add: registered GPIOs %d to %d on device: %s\n",
+	pr_debug("%s: registered GPIOs %d to %d on device: %s\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
 		chip->label ? : "generic");
 
@@ -1233,7 +1230,7 @@ unlock:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 fail:
 	/* failures here can mean systems won't boot... */
-	pr_err("gpiochip_add: gpios %d..%d (%s) failed to register\n",
+	pr_err("%s: GPIOs %d..%d (%s) failed to register\n", __func__,
 		chip->base, chip->base + chip->ngpio - 1,
 		chip->label ? : "generic");
 	return status;
@@ -1502,8 +1499,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 	}
 done:
 	if (status)
-		pr_debug("_gpio_request: gpio-%d (%s) status %d\n",
-			 desc_to_gpio(desc), label ? : "?", status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -1704,7 +1700,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (!chip->get || !chip->direction_input) {
 		gpiod_warn(desc,
 			"%s: missing get() or direction_input() operations\n",
-			 __func__);
+			__func__);
 		return -EIO;
 	}
 
@@ -1724,7 +1720,8 @@ int gpiod_direction_input(struct gpio_desc *desc)
 	if (status) {
 		status = chip->request(chip, offset);
 		if (status < 0) {
-			gpiod_dbg(desc, "chip request fail, %d\n", status);
+			gpiod_dbg(desc, "%s: chip request fail, %d\n",
+					__func__, status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
@@ -1742,7 +1739,7 @@ lose:
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	if (status)
-		gpiod_dbg(desc, "%s status %d\n", __func__, status);
+		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_input);
@@ -1809,7 +1806,8 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
 	if (status) {
 		status = chip->request(chip, offset);
 		if (status < 0) {
-			gpiod_dbg(desc, "chip request fail, %d\n", status);
+			gpiod_dbg(desc, "%s: chip request fail, %d\n",
+					__func__, status);
 			/* and it's not available to anyone else ...
 			 * gpio_request() is the fully clean solution.
 			 */
-- 
1.8.4.4

[PATCH 2/4] gpiolib: introduce chip_* to print with chip->label prefix

[Andy Shevchenko]

In several places we are printing messages with prefix based on chip->label.
Introduced macros help us to do this easier and in uniform way.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 9ed6f19..222f544 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -85,6 +85,8 @@ static DEFINE_IDR(dirent_idr);
 static int gpiod_request(struct gpio_desc *desc, const char *label);
 static void gpiod_free(struct gpio_desc *desc);
 
+/* With descriptor prefix */
+
 #ifdef CONFIG_DEBUG_FS
 #define gpiod_emerg(desc, fmt, ...)					       \
 	pr_emerg("gpio-%d (%s): " fmt, desc_to_gpio(desc), desc->label ? : "?",\
@@ -119,6 +121,21 @@ static void gpiod_free(struct gpio_desc *desc);
 	pr_debug("gpio-%d: " fmt, desc_to_gpio(desc), ##__VA_ARGS__)
 #endif
 
+/* With chip prefix */
+
+#define chip_emerg(chip, fmt, ...)					\
+	pr_emerg("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_crit(chip, fmt, ...)					\
+	pr_crit("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_err(chip, fmt, ...)					\
+	pr_err("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_warn(chip, fmt, ...)					\
+	pr_warn("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_info(chip, fmt, ...)					\
+	pr_info("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+#define chip_dbg(chip, fmt, ...)					\
+	pr_debug("%s: GPIO chip: " fmt, chip->label, ##__VA_ARGS__)
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 #ifdef CONFIG_DEBUG_FS
@@ -1032,8 +1049,7 @@ static int gpiochip_export(struct gpio_chip *chip)
 			chip->desc[gpio++].chip = NULL;
 		spin_unlock_irqrestore(&gpio_lock, flags);
 
-		pr_debug("%s: chip %s status %d\n", __func__,
-				chip->label, status);
+		chip_dbg(chip, "%s: status %d\n", __func__, status);
 	}
 
 	return status;
@@ -1056,8 +1072,7 @@ static void gpiochip_unexport(struct gpio_chip *chip)
 	mutex_unlock(&sysfs_lock);
 
 	if (status)
-		pr_debug("%s: chip %s status %d\n", __func__,
-				chip->label, status);
+		chip_dbg(chip, "%s: status %d\n", __func__, status);
 }
 
 static int __init gpiolib_sysfs_init(void)
@@ -1339,8 +1354,7 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
-		pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
-				chip->label);
+		chip_err(chip, "failed to allocate pin ranges\n");
 		return -ENOMEM;
 	}
 
@@ -1361,9 +1375,8 @@ int gpiochip_add_pingroup_range(struct gpio_chip *chip,
 
 	pinctrl_add_gpio_range(pctldev, &pin_range->range);
 
-	pr_debug("GPIO chip %s: created GPIO range %d->%d ==> %s PINGRP %s\n",
-		 chip->label, gpio_offset,
-		 gpio_offset + pin_range->range.npins - 1,
+	chip_dbg(chip, "created GPIO range %d->%d ==> %s PINGRP %s\n",
+		 gpio_offset, gpio_offset + pin_range->range.npins - 1,
 		 pinctrl_dev_get_devname(pctldev), pin_group);
 
 	list_add_tail(&pin_range->node, &chip->pin_ranges);
@@ -1390,8 +1403,7 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 
 	pin_range = kzalloc(sizeof(*pin_range), GFP_KERNEL);
 	if (!pin_range) {
-		pr_err("%s: GPIO chip: failed to allocate pin ranges\n",
-				chip->label);
+		chip_err(chip, "failed to allocate pin ranges\n");
 		return -ENOMEM;
 	}
 
@@ -1406,13 +1418,12 @@ int gpiochip_add_pin_range(struct gpio_chip *chip, const char *pinctl_name,
 			&pin_range->range);
 	if (IS_ERR(pin_range->pctldev)) {
 		ret = PTR_ERR(pin_range->pctldev);
-		pr_err("%s: GPIO chip: could not create pin range\n",
-		       chip->label);
+		chip_err(chip, "could not create pin range\n");
 		kfree(pin_range);
 		return ret;
 	}
-	pr_debug("GPIO chip %s: created GPIO range %d->%d ==> %s PIN %d->%d\n",
-		 chip->label, gpio_offset, gpio_offset + npins - 1,
+	chip_dbg(chip, "created GPIO range %d->%d ==> %s PIN %d->%d\n",
+		 gpio_offset, gpio_offset + npins - 1,
 		 pinctl_name,
 		 pin_offset, pin_offset + npins - 1);
 
-- 
1.8.4.4

[PATCH 3/4] gpiolib: convert gpiod_lookup description to kernel-doc

[Andy Shevchenko]

The patch moves description of the fields to the top of struct definition and
converts them to the kernel-doc format.

There is no functional change.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 7193f43..64f2c38 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -136,29 +136,21 @@ enum gpio_lookup_flags {
 };
 
 /**
- * Lookup table for associating GPIOs to specific devices and functions using
- * platform data.
+ * struct gpiod_lookup - lookup table
+ * @chip_label: name of the chip the GPIO belongs to
+ * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO
+ * @con_id: name of the GPIO from the device's point of view
+ * @idx: index of the GPIO in case several GPIOs share the same name
+ * @flags: mask of GPIO_* values
+ *
+ * gpiod_lookup is a lookup table for associating GPIOs to specific devices and
+ * functions using platform data.
  */
 struct gpiod_lookup {
-	/*
-	 * name of the chip the GPIO belongs to
-	 */
 	const char *chip_label;
-	/*
-	 * hardware number (i.e. relative to the chip) of the GPIO
-	 */
 	u16 chip_hwnum;
-	/*
-	 * name of the GPIO from the device's point of view
-	 */
 	const char *con_id;
-	/*
-	 * index of the GPIO in case several GPIOs share the same name
-	 */
 	unsigned int idx;
-	/*
-	 * mask of GPIO_* values
-	 */
 	enum gpio_lookup_flags flags;
 };
 
-- 
1.8.4.4

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

[Andy Shevchenko]

Since we may have dev parameter equial to NULL we can't use dev_* macros to
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;
 		}
 
@@ -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);
+		}
 	}
 
 	/*
@@ -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;
 	}
 
-- 
1.8.4.4

Re: [PATCH 3/4] gpiolib: convert gpiod_lookup description to kernel-doc

[Alex Courbot]

On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
> The patch moves description of the fields to the top of struct definition and
> converts them to the kernel-doc format.
>
> There is no functional change.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 1/4] gpiolib: unify pr_* messages format

[Alex Courbot]

On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
> This patch includes following amendments:
>   1) use "?" as a label when last is not defined in gpiod_*;

"when last is not defined" -> "when none is defined" ?

>   2) whenever it's possilbe gpiod_* are used;

s/possilbe/possible

>   3) print a function name, if it's already used in other messages.
>
> Additionally it fixes an indentation in few places.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Nice cleanup. I wonder if we should not make the GPIO label mandatory in 
some future patch and always use it in error messages (and also build it 
from dev_name(dev) and con_id in gpiod_get()).

One thing I wonder though: are you *absolutely* sure that none of the 
gpiod_*() log functions you are using can be called with a NULL desc? If 
you are not, it might be worth to make them more robust to this case 
that would otherwise make the kernel crash.

Might be even better if we could get rid of these desc_to_gpio() calls 
and display something like "desc->chip->label[gpio_chip_hwgpio(desc)]" 
in the log instead of "gpio-desc_to_gpio(desc)".

(last suggestion is just a suggestion though, I'm ok with this patch as 
long as you can guarantee none of the desc_to_gpio(desc) calls will crash)

Alex.

Re: [PATCH 1/4] gpiolib: unify pr_* messages format

[Alex Courbot]

On 12/04/2013 03:15 PM, Alex Courbot wrote:
> On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
>> This patch includes following amendments:
>>   1) use "?" as a label when last is not defined in gpiod_*;
>
> "when last is not defined" -> "when none is defined" ?
>
>>   2) whenever it's possilbe gpiod_* are used;
>
> s/possilbe/possible
>
>>   3) print a function name, if it's already used in other messages.
>>
>> Additionally it fixes an indentation in few places.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>
>
> Nice cleanup. I wonder if we should not make the GPIO label mandatory in
> some future patch and always use it in error messages (and also build it
> from dev_name(dev) and con_id in gpiod_get()).
>
> One thing I wonder though: are you *absolutely* sure that none of the
> gpiod_*() log functions you are using can be called with a NULL desc? If
> you are not, it might be worth to make them more robust to this case
> that would otherwise make the kernel crash.

Looking twice, all the call sites you replaced already used 
desc_to_gpio() or have a valid descriptor around, so please ignore that 
comment.

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 2/4] gpiolib: introduce chip_* to print with chip->label prefix

[Alex Courbot]

On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
> In several places we are printing messages with prefix based on chip->label.
> Introduced macros help us to do this easier and in uniform way.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks, that's going to be useful indeed.

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

[Alex Courbot]

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.

> 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).

>
> @@ -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.

>   	}
>
>   	/*
> @@ -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.

Re: [PATCH 1/4] gpiolib: unify pr_* messages format

[Andy Shevchenko]

On Wed, 2013-12-04 at 15:15 +0900, Alex Courbot wrote:

Thanks for the comments, my answers below.

> On 12/04/2013 04:03 AM, Andy Shevchenko wrote:
> > This patch includes following amendments:
> >   1) use "?" as a label when last is not defined in gpiod_*;
> 
> "when last is not defined" -> "when none is defined" ?

I was implying "when the last one is not defined". I will amend it.

> >   2) whenever it's possilbe gpiod_* are used;
> 
> s/possilbe/possible

Got it.

> 
> >   3) print a function name, if it's already used in other messages.
> >
> > Additionally it fixes an indentation in few places.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks!

> 
> Nice cleanup. I wonder if we should not make the GPIO label mandatory in 
> some future patch and always use it in error messages (and also build it 
> from dev_name(dev) and con_id in gpiod_get()).

If maintainers want this we could do it later.

> One thing I wonder though: are you *absolutely* sure that none of the 
> gpiod_*() log functions you are using can be called with a NULL desc? If 
> you are not, it might be worth to make them more robust to this case 
> that would otherwise make the kernel crash.

Like you already noticed I did changes only in places where we have desc
is not NULL.

> Might be even better if we could get rid of these desc_to_gpio() calls 
> and display something like "desc->chip->label[gpio_chip_hwgpio(desc)]" 
> in the log instead of "gpio-desc_to_gpio(desc)".
> 
> (last suggestion is just a suggestion though, I'm ok with this patch as 
> long as you can guarantee none of the desc_to_gpio(desc) calls will crash)

I propose to do this later as well. At least by this patch we're
decreasing amount of those calls.

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

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

[Andy Shevchenko]

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