[PATCH 0/4] gpiolib: some fixup patches

[PATCH 0/4] gpiolib: some fixup patches

[Alexandre Courbot]

This short series is a fixup to patch 6/9 of my previous series ("gpiolib: use 
descriptors internally"), which mainly addresses issues raised by Linus W. in 
this patch and a few other minor ones caught while proof-reading gpiolib.

First patch is the most significant. It mostly avoids oopses when passing an 
invalid GPIO to some functions. Also gpio_get/set_value*() now return 0 when 
given an invalid GPIO number instead of oopsing. I tend to think that's an 
improvement, but if it's not please let me know.

Ideally these patches should be melded into the above-mentioned patch since 
they were intended to become a new revision, but I leave that decision to 
Grant's discretion.

This patchset has been thoroughly tested on Tegra 2/Ventana with a GPIO 
backlight driver and the sysfs interface.

Alexandre Courbot (4):
  gpiolib: check descriptors validity before use
  gpiolib: use const parameters when possible
  gpiolib: move comment to right function
  gpiolib: rename local offset variables to "hwgpio"

 drivers/gpio/gpiolib.c | 165 ++++++++++++++++++++++++++-----------------------
 1 file changed, 87 insertions(+), 78 deletions(-)

-- 
1.8.1.3

[PATCH 1/4] gpiolib: check descriptors validity before use

[Alexandre Courbot]

From: Alexandre Courbot <acourbot@nvidia.com>

Some functions dereferenced their GPIO descriptor argument without
checking its validity first, potentially leading to an oops when given
an invalid argument.

This patch also makes gpio_get_value() more resilient when given an
invalid GPIO, returning 0 instead of oopsing.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 64 +++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fff9786..8a2cf9c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -174,7 +174,7 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
 /* caller holds gpio_lock *OR* gpio is marked as requested */
 static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
 {
-	return desc->chip;
+	return desc ? desc->chip : NULL;
 }
 
 struct gpio_chip *gpio_to_chip(unsigned gpio)
@@ -653,7 +653,12 @@ static ssize_t export_store(struct class *class,
 	if (status < 0)
 		goto done;
 
+	status = -EINVAL;
+
 	desc = gpio_to_desc(gpio);
+	/* reject invalid GPIOs */
+	if (!desc)
+		goto done;
 
 	/* No extra locking here; FLAG_SYSFS just signifies that the
 	 * request and export were done by on behalf of userspace, so
@@ -867,8 +872,8 @@ static int gpiod_export_link(struct device *dev, const char *name,
 
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		pr_debug("%s: gpio%d status %d\n", __func__,
+			 desc ? desc_to_gpio(desc) : -1, status);
 
 	return status;
 }
@@ -916,8 +921,8 @@ unlock:
 
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		pr_debug("%s: gpio%d status %d\n", __func__,
+			 desc ? desc_to_gpio(desc) : -1, status);
 
 	return status;
 }
@@ -964,8 +969,8 @@ static void gpiod_unexport(struct gpio_desc *desc)
 	}
 done:
 	if (status)
-		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
-			 status);
+		pr_debug("%s: gpio%d status %d\n", __func__,
+			 desc ? desc_to_gpio(desc) : -1, status);
 }
 
 void gpio_unexport(unsigned gpio)
@@ -1432,8 +1437,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 ? desc_to_gpio(desc) : -1,
-			 label ? : "?", status);
+			 desc ? desc_to_gpio(desc) : -1, label ? : "?", status);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -1655,13 +1659,9 @@ lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status) {
-		int gpio = -1;
-		if (desc)
-			gpio = desc_to_gpio(desc);
+	if (status)
 		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+			__func__, desc ? desc_to_gpio(desc) : -1, status);
 	return status;
 }
 
@@ -1678,6 +1678,9 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 	int			status = -EINVAL;
 	int offset;
 
+	if (!desc)
+		goto fail_unlocked;
+
 	/* Open drain pin should not be driven to 1 */
 	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
 		return gpiod_direction_input(desc);
@@ -1688,8 +1691,6 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!desc)
-		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->set || !chip->direction_output)
 		goto fail;
@@ -1725,13 +1726,10 @@ lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status) {
-		int gpio = -1;
-		if (desc)
-			gpio = desc_to_gpio(desc);
+fail_unlocked:
+	if (status)
 		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+			__func__, desc ? desc_to_gpio(desc) : -1, status);
 	return status;
 }
 
@@ -1776,13 +1774,9 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status) {
-		int gpio = -1;
-		if (desc)
-			gpio = desc_to_gpio(desc);
+	if (status)
 		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+			__func__, desc ? desc_to_gpio(desc) : -1, status);
 
 	return status;
 }
@@ -1830,6 +1824,8 @@ static int gpiod_get_value(struct gpio_desc *desc)
 	int value;
 	int offset;
 
+	if (!desc)
+		return 0;
 	chip = desc->chip;
 	offset = gpio_chip_hwgpio(desc);
 	/* Should be using gpio_get_value_cansleep() */
@@ -1912,6 +1908,8 @@ static void gpiod_set_value(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip	*chip;
 
+	if (!desc)
+		return;
 	chip = desc->chip;
 	/* Should be using gpio_set_value_cansleep() */
 	WARN_ON(chip->can_sleep);
@@ -1940,6 +1938,8 @@ EXPORT_SYMBOL_GPL(__gpio_set_value);
  */
 static int gpiod_cansleep(struct gpio_desc *desc)
 {
+	if (!desc)
+		return 0;
 	/* only call this on GPIOs that are valid! */
 	return desc->chip->can_sleep;
 }
@@ -1964,6 +1964,8 @@ static int gpiod_to_irq(struct gpio_desc *desc)
 	struct gpio_chip	*chip;
 	int			offset;
 
+	if (!desc)
+		return -EINVAL;
 	chip = desc->chip;
 	offset = gpio_chip_hwgpio(desc);
 	return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
@@ -1987,6 +1989,8 @@ static int gpiod_get_value_cansleep(struct gpio_desc *desc)
 	int offset;
 
 	might_sleep_if(extra_checks);
+	if (!desc)
+		return 0;
 	chip = desc->chip;
 	offset = gpio_chip_hwgpio(desc);
 	value = chip->get ? chip->get(chip, offset) : 0;
@@ -2005,6 +2009,8 @@ static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
 	struct gpio_chip	*chip;
 
 	might_sleep_if(extra_checks);
+	if (!desc)
+		return;
 	chip = desc->chip;
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
 	if (test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
-- 
1.8.1.3

[PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

From: Alexandre Courbot <acourbot@nvidia.com>

Constify descriptor parameter of gpiod_* functions for those that
should obviously not modify it. This includes value or direction get,
cansleep, and IRQ number query.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8a2cf9c..ad6df6b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -88,13 +88,14 @@ static int gpiod_request(struct gpio_desc *desc, const char *label);
 static void gpiod_free(struct gpio_desc *desc);
 static int gpiod_direction_input(struct gpio_desc *desc);
 static int gpiod_direction_output(struct gpio_desc *desc, int value);
+static int gpiod_get_direction(const struct gpio_desc *desc);
 static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce);
-static int gpiod_get_value_cansleep(struct gpio_desc *desc);
+static int gpiod_get_value_cansleep(const struct gpio_desc *desc);
 static void gpiod_set_value_cansleep(struct gpio_desc *desc, int value);
-static int gpiod_get_value(struct gpio_desc *desc);
+static int gpiod_get_value(const struct gpio_desc *desc);
 static void gpiod_set_value(struct gpio_desc *desc, int value);
-static int gpiod_cansleep(struct gpio_desc *desc);
-static int gpiod_to_irq(struct gpio_desc *desc);
+static int gpiod_cansleep(const struct gpio_desc *desc);
+static int gpiod_to_irq(const struct gpio_desc *desc);
 static int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
 static int gpiod_export_link(struct device *dev, const char *name,
 			     struct gpio_desc *desc);
@@ -172,7 +173,7 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
 }
 
 /* caller holds gpio_lock *OR* gpio is marked as requested */
-static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
+static struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
 	return desc ? desc->chip : NULL;
 }
@@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
 }
 
 /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
-static int gpiod_get_direction(struct gpio_desc *desc)
+static int gpiod_get_direction(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	unsigned		offset;
@@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
 	if (status > 0) {
 		/* GPIOF_DIR_IN, or other positive */
 		status = 1;
-		clear_bit(FLAG_IS_OUT, &desc->flags);
+		/* FLAG_IS_OUT is just a cache of the result of get_direction(),
+		 * so it does not affect constness per se */
+		clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
 	}
 	if (status == 0) {
 		/* GPIOF_DIR_OUT */
-		set_bit(FLAG_IS_OUT, &desc->flags);
+		set_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
 	}
 	return status;
 }
@@ -263,7 +266,7 @@ static DEFINE_MUTEX(sysfs_lock);
 static ssize_t gpio_direction_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -1818,7 +1821,7 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
  * It returns the zero or nonzero value provided by the associated
  * gpio_chip.get() method; or zero if no such method is provided.
  */
-static int gpiod_get_value(struct gpio_desc *desc)
+static int gpiod_get_value(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
@@ -1936,7 +1939,7 @@ EXPORT_SYMBOL_GPL(__gpio_set_value);
  * This is used directly or indirectly to implement gpio_cansleep().  It
  * returns nonzero if access reading or writing the GPIO value can sleep.
  */
-static int gpiod_cansleep(struct gpio_desc *desc)
+static int gpiod_cansleep(const struct gpio_desc *desc)
 {
 	if (!desc)
 		return 0;
@@ -1959,7 +1962,7 @@ EXPORT_SYMBOL_GPL(__gpio_cansleep);
  * It returns the number of the IRQ signaled by this (input) GPIO,
  * or a negative errno.
  */
-static int gpiod_to_irq(struct gpio_desc *desc)
+static int gpiod_to_irq(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int			offset;
@@ -1982,7 +1985,7 @@ EXPORT_SYMBOL_GPL(__gpio_to_irq);
  * Common examples include ones connected to I2C or SPI chips.
  */
 
-static int gpiod_get_value_cansleep(struct gpio_desc *desc)
+static int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
-- 
1.8.1.3

[PATCH 3/4] gpiolib: move comment to right function

[Alexandre Courbot]

From: Alexandre Courbot <acourbot@nvidia.com>

This comment applies to gpio_to_chip(), not gpiod_to_chip().

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index ad6df6b..d8aa1a0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -172,12 +172,12 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
 	return 0;
 }
 
-/* caller holds gpio_lock *OR* gpio is marked as requested */
 static struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
 	return desc ? desc->chip : NULL;
 }
 
+/* caller holds gpio_lock *OR* gpio is marked as requested */
 struct gpio_chip *gpio_to_chip(unsigned gpio)
 {
 	return gpiod_to_chip(gpio_to_desc(gpio));
-- 
1.8.1.3

[PATCH 4/4] gpiolib: rename local offset variables to "hwgpio"

[Alexandre Courbot]

From: Alexandre Courbot <acourbot@nvidia.com>

Their value being obtained by gpio_chip_hwgpio(), this better reflects
their use.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib.c | 70 +++++++++++++++++++++++++-------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d8aa1a0..42838c2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -72,7 +72,7 @@ struct gpio_desc {
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
-#define GPIO_OFFSET_VALID(chip, offset) (offset >= 0 && offset < chip->ngpio)
+#define GPIO_HWNUM_VALID(chip, hwgpio) (hwgpio >= 0 && hwgpio < chip->ngpio)
 
 static LIST_HEAD(gpio_chips);
 
@@ -211,16 +211,16 @@ static int gpiochip_find_base(int ngpio)
 static int gpiod_get_direction(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
-	unsigned		offset;
+	unsigned		hwgpio;
 	int			status = -EINVAL;
 
 	chip = gpiod_to_chip(desc);
-	offset = gpio_chip_hwgpio(desc);
+	hwgpio = gpio_chip_hwgpio(desc);
 
 	if (!chip->get_direction)
 		return status;
 
-	status = chip->get_direction(chip, offset);
+	status = chip->get_direction(chip, hwgpio);
 	if (status > 0) {
 		/* GPIOF_DIR_IN, or other positive */
 		status = 1;
@@ -754,7 +754,7 @@ static int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	int			status;
 	const char		*ioname = NULL;
 	struct device		*dev;
-	int			offset;
+	int			hwgpio;
 
 	/* can't export until sysfs is available ... */
 	if (!gpio_class.p) {
@@ -785,9 +785,9 @@ static int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		direction_may_change = false;
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
-	offset = gpio_chip_hwgpio(desc);
-	if (desc->chip->names && desc->chip->names[offset])
-		ioname = desc->chip->names[offset];
+	hwgpio = gpio_chip_hwgpio(desc);
+	if (desc->chip->names && desc->chip->names[hwgpio])
+		ioname = desc->chip->names[hwgpio];
 
 	dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
 			    desc, ioname ? ioname : "gpio%u",
@@ -1591,7 +1591,7 @@ const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
 {
 	struct gpio_desc *desc;
 
-	if (!GPIO_OFFSET_VALID(chip, offset))
+	if (!GPIO_HWNUM_VALID(chip, offset))
 		return NULL;
 
 	desc = &chip->desc[offset];
@@ -1621,7 +1621,7 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
-	int			offset;
+	int			hwgpio;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -1640,9 +1640,9 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 
 	might_sleep_if(chip->can_sleep);
 
-	offset = gpio_chip_hwgpio(desc);
+	hwgpio = gpio_chip_hwgpio(desc);
 	if (status) {
-		status = chip->request(chip, offset);
+		status = chip->request(chip, hwgpio);
 		if (status < 0) {
 			pr_debug("GPIO-%d: chip request fail, %d\n",
 				desc_to_gpio(desc), status);
@@ -1653,7 +1653,7 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 		}
 	}
 
-	status = chip->direction_input(chip, offset);
+	status = chip->direction_input(chip, hwgpio);
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
 
@@ -1679,7 +1679,7 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
-	int offset;
+	int			hwgpio;
 
 	if (!desc)
 		goto fail_unlocked;
@@ -1707,9 +1707,9 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 
 	might_sleep_if(chip->can_sleep);
 
-	offset = gpio_chip_hwgpio(desc);
+	hwgpio = gpio_chip_hwgpio(desc);
 	if (status) {
-		status = chip->request(chip, offset);
+		status = chip->request(chip, hwgpio);
 		if (status < 0) {
 			pr_debug("GPIO-%d: chip request fail, %d\n",
 				desc_to_gpio(desc), status);
@@ -1720,7 +1720,7 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 		}
 	}
 
-	status = chip->direction_output(chip, offset, value);
+	status = chip->direction_output(chip, hwgpio, value);
 	if (status == 0)
 		set_bit(FLAG_IS_OUT, &desc->flags);
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
@@ -1752,7 +1752,7 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
-	int			offset;
+	int			hwgpio;
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
@@ -1772,8 +1772,8 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 
 	might_sleep_if(chip->can_sleep);
 
-	offset = gpio_chip_hwgpio(desc);
-	return chip->set_debounce(chip, offset, debounce);
+	hwgpio = gpio_chip_hwgpio(desc);
+	return chip->set_debounce(chip, hwgpio, debounce);
 
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
@@ -1825,15 +1825,15 @@ static int gpiod_get_value(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
-	int offset;
+	int hwgpio;
 
 	if (!desc)
 		return 0;
 	chip = desc->chip;
-	offset = gpio_chip_hwgpio(desc);
+	hwgpio = gpio_chip_hwgpio(desc);
 	/* Should be using gpio_get_value_cansleep() */
 	WARN_ON(chip->can_sleep);
-	value = chip->get ? chip->get(chip, offset) : 0;
+	value = chip->get ? chip->get(chip, hwgpio) : 0;
 	trace_gpio_value(desc_to_gpio(desc), 1, value);
 	return value;
 }
@@ -1854,14 +1854,14 @@ static void _gpio_set_open_drain_value(struct gpio_desc *desc, int value)
 {
 	int err = 0;
 	struct gpio_chip *chip = desc->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int hwgpio = gpio_chip_hwgpio(desc);
 
 	if (value) {
-		err = chip->direction_input(chip, offset);
+		err = chip->direction_input(chip, hwgpio);
 		if (!err)
 			clear_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		err = chip->direction_output(chip, offset, 0);
+		err = chip->direction_output(chip, hwgpio, 0);
 		if (!err)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	}
@@ -1881,14 +1881,14 @@ static void _gpio_set_open_source_value(struct gpio_desc *desc, int value)
 {
 	int err = 0;
 	struct gpio_chip *chip = desc->chip;
-	int offset = gpio_chip_hwgpio(desc);
+	int hwgpio = gpio_chip_hwgpio(desc);
 
 	if (value) {
-		err = chip->direction_output(chip, offset, 1);
+		err = chip->direction_output(chip, hwgpio, 1);
 		if (!err)
 			set_bit(FLAG_IS_OUT, &desc->flags);
 	} else {
-		err = chip->direction_input(chip, offset);
+		err = chip->direction_input(chip, hwgpio);
 		if (!err)
 			clear_bit(FLAG_IS_OUT, &desc->flags);
 	}
@@ -1965,13 +1965,13 @@ EXPORT_SYMBOL_GPL(__gpio_cansleep);
 static int gpiod_to_irq(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
-	int			offset;
+	int			hwgpio;
 
 	if (!desc)
 		return -EINVAL;
 	chip = desc->chip;
-	offset = gpio_chip_hwgpio(desc);
-	return chip->to_irq ? chip->to_irq(chip, offset) : -ENXIO;
+	hwgpio = gpio_chip_hwgpio(desc);
+	return chip->to_irq ? chip->to_irq(chip, hwgpio) : -ENXIO;
 }
 
 int __gpio_to_irq(unsigned gpio)
@@ -1989,14 +1989,14 @@ static int gpiod_get_value_cansleep(const struct gpio_desc *desc)
 {
 	struct gpio_chip	*chip;
 	int value;
-	int offset;
+	int hwgpio;
 
 	might_sleep_if(extra_checks);
 	if (!desc)
 		return 0;
 	chip = desc->chip;
-	offset = gpio_chip_hwgpio(desc);
-	value = chip->get ? chip->get(chip, offset) : 0;
+	hwgpio = gpio_chip_hwgpio(desc);
+	value = chip->get ? chip->get(chip, hwgpio) : 0;
 	trace_gpio_value(desc_to_gpio(desc), 1, value);
 	return value;
 }
-- 
1.8.1.3

[PATCH 4/4] gpiolib: rename local offset variables to "hwgpio"

[Alexandre Courbot]

Delivered-To: gnurou@gmail.com
Received: by 10.64.96.232 with SMTP id dv8csp15271ieb;
        Sun, 10 Feb 2013 05:48:07 -0800 (PST)
X-Received: by 10.68.0.39 with SMTP id 7mr11102539pbb.124.1360504086445;
        Sun, 10 Feb 2013 05:48:06 -0800 (PST)
Return-Path: <acourbot@nvidia.com>
Received: from hqemgate04.nvidia.com (hqemgate04.nvidia.com. [216.228.121.35])
        by mx.google.com with ESMTPS id qj7si3931281pbb.322.2013.02.10.05.48.06
        (version=TLSv1 cipher=RC4-SHA bits=128/128);
        Sun, 10 Feb 2013 05:48:06 -0800 (PST)
Received-SPF: pass (google.com: best guess record for domain of acourbot@nvidia.com designates 216.228.121.35 as permitted sender) client-ip=216.228.121.35;
Authentication-Results: mx.google.com;
       spf=pass (google.com: best guess record for domain of acourbot@nvidia.com designates 216.228.121.35 as permitted sender) smtp.mail=acourbot@nvidia.com
Received: from hqnvupgp05.nvidia.com (Not Verified[216.228.121.13]) by hqemgate04.nvidia.com
	id <B5117a50b0000>; Sun, 10 Feb 2013 05:47:56 -0800
Received: from hqemhub01.nvidia.com ([172.17.108.22])
  by hqnvupgp05.nvidia.com (PGP Universal service);
  Sun, 10 Feb 2013 05:48:05 -0800
X-PGP-Universal: processed;
	by hqnvupgp05.nvidia.com on Sun, 10 Feb 2013 05:48:05 -0800
Received: from [10.18.131.155] (172.20.144.16) by hqemhub01.nvidia.com
 (172.20.150.30) with Microsoft SMTP Server id 8.3.297.1; Sun, 10 Feb 2013
 05:48:05 -0800
Message-ID: <5117A5AB.5090702@nvidia.com>
Date: Sun, 10 Feb 2013 22:50:35 +0900
From: Alex Courbot <acourbot@nvidia.com>
Organization: NVIDIA
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130109 Thunderbird/17.0.2
MIME-Version: 1.0
To: <gnurou@gmail.com>
Subject: Re: [grant:gpio/next 10/16] gpiolib.c:undefined reference to `gpiod_unexport'
References: <51166e1c.NRfQN8VgI14zuNwf%fengguang.wu@intel.com> <CACxGe6tAEX7o9dxNTXtwKwzZHmEteKAS+7uy4j-DTX_2V583tg@mail.gmail.com>
In-Reply-To: <CACxGe6tAEX7o9dxNTXtwKwzZHmEteKAS+7uy4j-DTX_2V583tg@mail.gmail.com>
Return-Path: acourbot@nvidia.com
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit

On 02/10/2013 01:34 AM, Grant Likely wrote:
> Alex, this is broken when the sysfs interface isn't enabled. Can you
> send a fixup patch?
>
> g.
>
> On Sat, Feb 9, 2013 at 3:41 PM, kbuild test robot
> <fengguang.wu@intel.com> wrote:
>> tree:   git://git.secretlab.ca/git/linux-2.6.git gpio/next
>> head:   8a307b35962e42de0f998c6029e8851c61eadb4e
>> commit: 5bb47609e8167d733786cb781ada29536385635c [10/16] gpiolib: use descriptors internally
>> config: i386-randconfig-b040 (attached as .config)
>>
>> All error/warnings:
>>
>>     drivers/built-in.o: In function `gpiod_free':
>>>> gpiolib.c:(.text+0xda3): undefined reference to `gpiod_unexport'
>>     drivers/built-in.o: In function `gpio_request_one':
>>>> (.text+0x147f): undefined reference to `gpiod_export'

Oops, apologies. Here follows the fix. Please meld it into the culprit 
patch to fix all warnings and errors when sysfs is not compiled in.

Thanks,
Alex.

---
  drivers/gpio/gpiolib.c | 21 +++++++++++++++++++++
  1 file changed, 21 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e6e597c..b5a71c5 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1085,6 +1085,27 @@ static inline void gpiochip_unexport(struct 
gpio_chip *chip)
  {
  }

+static inline int gpiod_export(struct gpio_desc *desc,
+                              bool direction_may_change)
+{
+       return -ENOSYS;
+}
+
+static inline int gpiod_export_link(struct device *dev, const char *name,
+                                   struct gpio_desc *desc)
+{
+       return -ENOSYS;
+}
+
+static inline int gpiod_sysfs_set_active_low(struct gpio_desc *desc, 
int value)
+{
+       return -ENOSYS;
+}
+
+static inline void gpiod_unexport(struct gpio_desc *desc)
+{
+}
+
  #endif /* CONFIG_GPIO_SYSFS */

  /*
-- 
1.8.1.3

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH 1/4] gpiolib: check descriptors validity before use

[Ryan Mallon]

On 13/02/13 18:02, Alexandre Courbot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> Some functions dereferenced their GPIO descriptor argument without
> checking its validity first, potentially leading to an oops when given
> an invalid argument.
> 
> This patch also makes gpio_get_value() more resilient when given an
> invalid GPIO, returning 0 instead of oopsing.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 64 +++++++++++++++++++++++++++-----------------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index fff9786..8a2cf9c 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -174,7 +174,7 @@ static int gpio_ensure_requested(struct gpio_desc *desc)
>  /* caller holds gpio_lock *OR* gpio is marked as requested */
>  static struct gpio_chip *gpiod_to_chip(struct gpio_desc *desc)
>  {
> -	return desc->chip;
> +	return desc ? desc->chip : NULL;
>  }
>  
>  struct gpio_chip *gpio_to_chip(unsigned gpio)
> @@ -653,7 +653,12 @@ static ssize_t export_store(struct class *class,
>  	if (status < 0)
>  		goto done;
>  
> +	status = -EINVAL;
> +
>  	desc = gpio_to_desc(gpio);
> +	/* reject invalid GPIOs */
> +	if (!desc)
> +		goto done;
>  
>  	/* No extra locking here; FLAG_SYSFS just signifies that the
>  	 * request and export were done by on behalf of userspace, so
> @@ -867,8 +872,8 @@ static int gpiod_export_link(struct device *dev, const char *name,
>  
>  done:
>  	if (status)
> -		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
> -			 status);
> +		pr_debug("%s: gpio%d status %d\n", __func__,
> +			 desc ? desc_to_gpio(desc) : -1, status);

Is it really useful to use the same pr_debug for the error case? Why not do:

	desc = gpio_to_desc(gpio);
	if (!desc) {
		pr_debug("%s - Invalid gpio %d\n", __func__, gpio);
		return -EINVAL;
	}
	
	...

At this point desc is known valid, though you could just use the gpio
number that was passed in (assuming that it is always the same as
desc_to_gpio).

	pr_debug("%s: gpio%d status %d\n", __func__,
		 desc_to_gpio(desc), status);
	return status;

That provides more information (the original gpio number and the reason
for the -EINVAL) if the gpio is not valid, and removes the ugly ternary
operator from the pr_debug. Same goes for the other functions.

~Ryan

Re: [PATCH 4/4] gpiolib: rename local offset variables to "hwgpio"

[Ryan Mallon]

On 13/02/13 18:03, Alexandre Courbot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> Their value being obtained by gpio_chip_hwgpio(), this better reflects
> their use.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 70 +++++++++++++++++++++++++-------------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d8aa1a0..42838c2 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -72,7 +72,7 @@ struct gpio_desc {
>  };
>  static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>  
> -#define GPIO_OFFSET_VALID(chip, offset) (offset >= 0 && offset < chip->ngpio)
> +#define GPIO_HWNUM_VALID(chip, hwgpio) (hwgpio >= 0 && hwgpio < chip->ngpio)

Nitpicky - Is it accurate to call these hardware numbers? Don't some of
the platforms remap the gpio numbers? These numbers may not match
against the platform's datasheet for example.

~Ryan

Re: [PATCH 1/4] gpiolib: check descriptors validity before use

[Alexandre Courbot]

On Thu, Feb 14, 2013 at 7:49 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> Is it really useful to use the same pr_debug for the error case? Why not do:
>
>         desc = gpio_to_desc(gpio);
>         if (!desc) {
>                 pr_debug("%s - Invalid gpio %d\n", __func__, gpio);
>                 return -EINVAL;
>         }
>
>         ...
>
> At this point desc is known valid, though you could just use the gpio
> number that was passed in (assuming that it is always the same as
> desc_to_gpio).
>
>         pr_debug("%s: gpio%d status %d\n", __func__,
>                  desc_to_gpio(desc), status);
>         return status;
>
> That provides more information (the original gpio number and the reason
> for the -EINVAL) if the gpio is not valid, and removes the ugly ternary
> operator from the pr_debug. Same goes for the other functions.

That's mainly a style issue - I tried to preserve the code's behavior
as much as possible with these patches. But indeed the invalid GPIO
case is special, and on second thought identifying invalid GPIOs are
"gpio-1" is confusing. One could also say horrible.

Guess this deserves to be fixed in a v2. Thanks.

Alex.

Re: [PATCH 4/4] gpiolib: rename local offset variables to "hwgpio"

[Alexandre Courbot]

On Thu, Feb 14, 2013 at 7:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> Nitpicky - Is it accurate to call these hardware numbers? Don't some of
> the platforms remap the gpio numbers? These numbers may not match
> against the platform's datasheet for example.

I'm following a suggestion by Grant and Linus W. here. This is closer
to existing convention in e.g. irqdomain.

Also at that level the GPIO numbers are local to the GPIO controller,
i.e. they are not supposed to be the GPIO numbers used by consumers
(or the datasheet for that instance).

Alex.

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Uwe Kleine-König]

Hello,

On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
> From: Alexandre Courbot <acourbot@nvidia.com>
> 
> Constify descriptor parameter of gpiod_* functions for those that
> should obviously not modify it. This includes value or direction get,
> cansleep, and IRQ number query.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
v3.9-rc2.

>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 8a2cf9c..ad6df6b 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
>  }
>  
>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
> -static int gpiod_get_direction(struct gpio_desc *desc)
> +static int gpiod_get_direction(const struct gpio_desc *desc)
>  {
>  	struct gpio_chip	*chip;
>  	unsigned		offset;
> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
>  	if (status > 0) {
>  		/* GPIOF_DIR_IN, or other positive */
>  		status = 1;
> -		clear_bit(FLAG_IS_OUT, &desc->flags);
> +		/* FLAG_IS_OUT is just a cache of the result of get_direction(),
> +		 * so it does not affect constness per se */
> +		clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
I think this is worse than not having gpiod_get_direction take a const
descriptor.
I don't know an example for this particular case where this could break,
but usually casting away a const is bad and can break in various ways.

Maybe even drop the public function gpiod_get_direction completely.
As of next-20141114 there are only two users of this function (apart
from drivers/gpio/gpiolib*) and both are wrong (as discussed in
http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
>> From: Alexandre Courbot <acourbot@nvidia.com>
>>
>> Constify descriptor parameter of gpiod_* functions for those that
>> should obviously not modify it. This includes value or direction get,
>> cansleep, and IRQ number query.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
> v3.9-rc2.
>
>>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
>>  1 file changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 8a2cf9c..ad6df6b 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
>>  }
>>
>>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
>> -static int gpiod_get_direction(struct gpio_desc *desc)
>> +static int gpiod_get_direction(const struct gpio_desc *desc)
>>  {
>>       struct gpio_chip        *chip;
>>       unsigned                offset;
>> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
>>       if (status > 0) {
>>               /* GPIOF_DIR_IN, or other positive */
>>               status = 1;
>> -             clear_bit(FLAG_IS_OUT, &desc->flags);
>> +             /* FLAG_IS_OUT is just a cache of the result of get_direction(),
>> +              * so it does not affect constness per se */
>> +             clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
> I think this is worse than not having gpiod_get_direction take a const
> descriptor.

Yeah, this kind of sucks but ultimately gpiod_get_direction() is
without side-effect to the caller and the const parameter helps
highlighting that fact.

> I don't know an example for this particular case where this could break,
> but usually casting away a const is bad and can break in various ways.
>
> Maybe even drop the public function gpiod_get_direction completely.
> As of next-20141114 there are only two users of this function (apart
> from drivers/gpio/gpiolib*) and both are wrong (as discussed in
> http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).

Or maybe gpiod_direction_*() should register the last set direction to
be used by gpiod_get_direction() if the controller does not implement
the get_direction op? On the other hand callers can also track the
direction themselves (since they presumably set it), so this function
is really only useful for users who want to read the *hardware* state
of a GPIO. Which might still be a useful feature?

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Uwe Kleine-König]

Hello Alexandre,

On Wed, Nov 19, 2014 at 05:33:42PM +0900, Alexandre Courbot wrote:
> On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hello,
> >
> > On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
> >> From: Alexandre Courbot <acourbot@nvidia.com>
> >>
> >> Constify descriptor parameter of gpiod_* functions for those that
> >> should obviously not modify it. This includes value or direction get,
> >> cansleep, and IRQ number query.
> >>
> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> > This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
> > v3.9-rc2.
> >
> >>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
> >>  1 file changed, 16 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index 8a2cf9c..ad6df6b 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
> >>  }
> >>
> >>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
> >> -static int gpiod_get_direction(struct gpio_desc *desc)
> >> +static int gpiod_get_direction(const struct gpio_desc *desc)
> >>  {
> >>       struct gpio_chip        *chip;
> >>       unsigned                offset;
> >> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
> >>       if (status > 0) {
> >>               /* GPIOF_DIR_IN, or other positive */
> >>               status = 1;
> >> -             clear_bit(FLAG_IS_OUT, &desc->flags);
> >> +             /* FLAG_IS_OUT is just a cache of the result of get_direction(),
> >> +              * so it does not affect constness per se */
> >> +             clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
> > I think this is worse than not having gpiod_get_direction take a const
> > descriptor.
> 
> Yeah, this kind of sucks but ultimately gpiod_get_direction() is
> without side-effect to the caller and the const parameter helps
> highlighting that fact.
Does that mean you want to keep the const and take the risk that it
might introduce some hard to debug problem when the compiler changes its
optimisation strategies? I hope you don't.

> > I don't know an example for this particular case where this could break,
> > but usually casting away a const is bad and can break in various ways.
> >
> > Maybe even drop the public function gpiod_get_direction completely.
> > As of next-20141114 there are only two users of this function (apart
> > from drivers/gpio/gpiolib*) and both are wrong (as discussed in
> > http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).
> 
> Or maybe gpiod_direction_*() should register the last set direction to
> be used by gpiod_get_direction() if the controller does not implement
> the get_direction op? On the other hand callers can also track the
> direction themselves (since they presumably set it), so this function
> is really only useful for users who want to read the *hardware* state
> of a GPIO. Which might still be a useful feature?
I think it's useful to be able to check how a given gpio is configured
in hardware. So you really want to look into the register and not cache
the direction in gpiod_direction_*.
And I'd say having /sys/kernel/debug/gpio is good enough. So IMHO there
is no reason to hide gpiod_get_direction from general misuse.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

On Wed, Nov 19, 2014 at 5:44 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Alexandre,
>
> On Wed, Nov 19, 2014 at 05:33:42PM +0900, Alexandre Courbot wrote:
>> On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > Hello,
>> >
>> > On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
>> >> From: Alexandre Courbot <acourbot@nvidia.com>
>> >>
>> >> Constify descriptor parameter of gpiod_* functions for those that
>> >> should obviously not modify it. This includes value or direction get,
>> >> cansleep, and IRQ number query.
>> >>
>> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> > This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
>> > v3.9-rc2.
>> >
>> >>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
>> >>  1 file changed, 16 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> >> index 8a2cf9c..ad6df6b 100644
>> >> --- a/drivers/gpio/gpiolib.c
>> >> +++ b/drivers/gpio/gpiolib.c
>> >> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
>> >>  }
>> >>
>> >>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
>> >> -static int gpiod_get_direction(struct gpio_desc *desc)
>> >> +static int gpiod_get_direction(const struct gpio_desc *desc)
>> >>  {
>> >>       struct gpio_chip        *chip;
>> >>       unsigned                offset;
>> >> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
>> >>       if (status > 0) {
>> >>               /* GPIOF_DIR_IN, or other positive */
>> >>               status = 1;
>> >> -             clear_bit(FLAG_IS_OUT, &desc->flags);
>> >> +             /* FLAG_IS_OUT is just a cache of the result of get_direction(),
>> >> +              * so it does not affect constness per se */
>> >> +             clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
>> > I think this is worse than not having gpiod_get_direction take a const
>> > descriptor.
>>
>> Yeah, this kind of sucks but ultimately gpiod_get_direction() is
>> without side-effect to the caller and the const parameter helps
>> highlighting that fact.
> Does that mean you want to keep the const and take the risk that it
> might introduce some hard to debug problem when the compiler changes its
> optimisation strategies? I hope you don't.

Nope, I would like to keep that const but correctness is more
important than prototype aesthetics. I will fix that.

>> > I don't know an example for this particular case where this could break,
>> > but usually casting away a const is bad and can break in various ways.
>> >
>> > Maybe even drop the public function gpiod_get_direction completely.
>> > As of next-20141114 there are only two users of this function (apart
>> > from drivers/gpio/gpiolib*) and both are wrong (as discussed in
>> > http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).
>>
>> Or maybe gpiod_direction_*() should register the last set direction to
>> be used by gpiod_get_direction() if the controller does not implement
>> the get_direction op? On the other hand callers can also track the
>> direction themselves (since they presumably set it), so this function
>> is really only useful for users who want to read the *hardware* state
>> of a GPIO. Which might still be a useful feature?
> I think it's useful to be able to check how a given gpio is configured
> in hardware. So you really want to look into the register and not cache
> the direction in gpiod_direction_*.
> And I'd say having /sys/kernel/debug/gpio is good enough. So IMHO there
> is no reason to hide gpiod_get_direction from general misuse.

Mmm so are you suggesting that we should keep gpiod_get_direction() in the end?

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Uwe Kleine-König]

Hello Alexandre,

On Wed, Nov 19, 2014 at 05:57:46PM +0900, Alexandre Courbot wrote:
> On Wed, Nov 19, 2014 at 5:44 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Hello Alexandre,
> >
> > On Wed, Nov 19, 2014 at 05:33:42PM +0900, Alexandre Courbot wrote:
> >> On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > Hello,
> >> >
> >> > On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
> >> >> From: Alexandre Courbot <acourbot@nvidia.com>
> >> >>
> >> >> Constify descriptor parameter of gpiod_* functions for those that
> >> >> should obviously not modify it. This includes value or direction get,
> >> >> cansleep, and IRQ number query.
> >> >>
> >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> >> > This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
> >> > v3.9-rc2.
> >> >
> >> >>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
> >> >>  1 file changed, 16 insertions(+), 13 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> >> index 8a2cf9c..ad6df6b 100644
> >> >> --- a/drivers/gpio/gpiolib.c
> >> >> +++ b/drivers/gpio/gpiolib.c
> >> >> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
> >> >>  }
> >> >>
> >> >>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
> >> >> -static int gpiod_get_direction(struct gpio_desc *desc)
> >> >> +static int gpiod_get_direction(const struct gpio_desc *desc)
> >> >>  {
> >> >>       struct gpio_chip        *chip;
> >> >>       unsigned                offset;
> >> >> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
> >> >>       if (status > 0) {
> >> >>               /* GPIOF_DIR_IN, or other positive */
> >> >>               status = 1;
> >> >> -             clear_bit(FLAG_IS_OUT, &desc->flags);
> >> >> +             /* FLAG_IS_OUT is just a cache of the result of get_direction(),
> >> >> +              * so it does not affect constness per se */
> >> >> +             clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
> >> > I think this is worse than not having gpiod_get_direction take a const
> >> > descriptor.
> >>
> >> Yeah, this kind of sucks but ultimately gpiod_get_direction() is
> >> without side-effect to the caller and the const parameter helps
> >> highlighting that fact.
> > Does that mean you want to keep the const and take the risk that it
> > might introduce some hard to debug problem when the compiler changes its
> > optimisation strategies? I hope you don't.
> 
> Nope, I would like to keep that const but correctness is more
> important than prototype aesthetics. I will fix that.
Fine.

> 
> >> > I don't know an example for this particular case where this could break,
> >> > but usually casting away a const is bad and can break in various ways.
> >> >
> >> > Maybe even drop the public function gpiod_get_direction completely.
> >> > As of next-20141114 there are only two users of this function (apart
> >> > from drivers/gpio/gpiolib*) and both are wrong (as discussed in
> >> > http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).
> >>
> >> Or maybe gpiod_direction_*() should register the last set direction to
> >> be used by gpiod_get_direction() if the controller does not implement
> >> the get_direction op? On the other hand callers can also track the
> >> direction themselves (since they presumably set it), so this function
> >> is really only useful for users who want to read the *hardware* state
> >> of a GPIO. Which might still be a useful feature?
> > I think it's useful to be able to check how a given gpio is configured
> > in hardware. So you really want to look into the register and not cache
> > the direction in gpiod_direction_*.
> > And I'd say having /sys/kernel/debug/gpio is good enough. So IMHO there
> > is no reason to hide gpiod_get_direction from general misuse.
> 
> Mmm so are you suggesting that we should keep gpiod_get_direction() in the end?
I'd make gpiod_get_direction static and only use it to fill in
/sys/kernel/debug/gpio.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

 On Wed, Nov 19, 2014 at 6:02 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Alexandre,
>
> On Wed, Nov 19, 2014 at 05:57:46PM +0900, Alexandre Courbot wrote:
>> On Wed, Nov 19, 2014 at 5:44 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > Hello Alexandre,
>> >
>> > On Wed, Nov 19, 2014 at 05:33:42PM +0900, Alexandre Courbot wrote:
>> >> On Mon, Nov 17, 2014 at 6:09 PM, Uwe Kleine-König
>> >> <u.kleine-koenig@pengutronix.de> wrote:
>> >> > Hello,
>> >> >
>> >> > On Wed, Feb 13, 2013 at 04:03:00PM +0900, Alexandre Courbot wrote:
>> >> >> From: Alexandre Courbot <acourbot@nvidia.com>
>> >> >>
>> >> >> Constify descriptor parameter of gpiod_* functions for those that
>> >> >> should obviously not modify it. This includes value or direction get,
>> >> >> cansleep, and IRQ number query.
>> >> >>
>> >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> >> > This patch is applied as def634338d3ffb32fbe9b0a2d70cc24ef909cd4f since
>> >> > v3.9-rc2.
>> >> >
>> >> >>  drivers/gpio/gpiolib.c | 29 ++++++++++++++++-------------
>> >> >>  1 file changed, 16 insertions(+), 13 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> >> >> index 8a2cf9c..ad6df6b 100644
>> >> >> --- a/drivers/gpio/gpiolib.c
>> >> >> +++ b/drivers/gpio/gpiolib.c
>> >> >> @@ -207,7 +208,7 @@ static int gpiochip_find_base(int ngpio)
>> >> >>  }
>> >> >>
>> >> >>  /* caller ensures gpio is valid and requested, chip->get_direction may sleep  */
>> >> >> -static int gpiod_get_direction(struct gpio_desc *desc)
>> >> >> +static int gpiod_get_direction(const struct gpio_desc *desc)
>> >> >>  {
>> >> >>       struct gpio_chip        *chip;
>> >> >>       unsigned                offset;
>> >> >> @@ -223,11 +224,13 @@ static int gpiod_get_direction(struct gpio_desc *desc)
>> >> >>       if (status > 0) {
>> >> >>               /* GPIOF_DIR_IN, or other positive */
>> >> >>               status = 1;
>> >> >> -             clear_bit(FLAG_IS_OUT, &desc->flags);
>> >> >> +             /* FLAG_IS_OUT is just a cache of the result of get_direction(),
>> >> >> +              * so it does not affect constness per se */
>> >> >> +             clear_bit(FLAG_IS_OUT, &((struct gpio_desc *)desc)->flags);
>> >> > I think this is worse than not having gpiod_get_direction take a const
>> >> > descriptor.
>> >>
>> >> Yeah, this kind of sucks but ultimately gpiod_get_direction() is
>> >> without side-effect to the caller and the const parameter helps
>> >> highlighting that fact.
>> > Does that mean you want to keep the const and take the risk that it
>> > might introduce some hard to debug problem when the compiler changes its
>> > optimisation strategies? I hope you don't.
>>
>> Nope, I would like to keep that const but correctness is more
>> important than prototype aesthetics. I will fix that.
> Fine.
>
>>
>> >> > I don't know an example for this particular case where this could break,
>> >> > but usually casting away a const is bad and can break in various ways.
>> >> >
>> >> > Maybe even drop the public function gpiod_get_direction completely.
>> >> > As of next-20141114 there are only two users of this function (apart
>> >> > from drivers/gpio/gpiolib*) and both are wrong (as discussed in
>> >> > http://thread.gmane.org/gmane.linux.serial/16808/focus=4783).
>> >>
>> >> Or maybe gpiod_direction_*() should register the last set direction to
>> >> be used by gpiod_get_direction() if the controller does not implement
>> >> the get_direction op? On the other hand callers can also track the
>> >> direction themselves (since they presumably set it), so this function
>> >> is really only useful for users who want to read the *hardware* state
>> >> of a GPIO. Which might still be a useful feature?
>> > I think it's useful to be able to check how a given gpio is configured
>> > in hardware. So you really want to look into the register and not cache
>> > the direction in gpiod_direction_*.
>> > And I'd say having /sys/kernel/debug/gpio is good enough. So IMHO there
>> > is no reason to hide gpiod_get_direction from general misuse.
>>
>> Mmm so are you suggesting that we should keep gpiod_get_direction() in the end?
> I'd make gpiod_get_direction static and only use it to fill in
> /sys/kernel/debug/gpio.

That's very tempting. I see only atmel_serial.c using this function,
and there is no gpio_get_direction() declared anywhere so no user of
this either. I'm not sure what I was thinking when I decided to export
it?

So yeah, if you can fix the current users, I agree with your proposal.

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Uwe Kleine-König]

Hello Alexandre,

On Wed, Nov 19, 2014 at 06:07:50PM +0900, Alexandre Courbot wrote:
>  On Wed, Nov 19, 2014 at 6:02 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > I'd make gpiod_get_direction static and only use it to fill in
> > /sys/kernel/debug/gpio.
> 
> That's very tempting. I see only atmel_serial.c using this function,
> and there is no gpio_get_direction() declared anywhere so no user of
> this either. I'm not sure what I was thinking when I decided to export
> it?
In next there is also drivers/tty/serial/mxs-auart.c.
 
Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Alexandre Courbot]

On Wed, Nov 19, 2014 at 7:09 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Alexandre,
>
> On Wed, Nov 19, 2014 at 06:07:50PM +0900, Alexandre Courbot wrote:
>>  On Wed, Nov 19, 2014 at 6:02 PM, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> > I'd make gpiod_get_direction static and only use it to fill in
>> > /sys/kernel/debug/gpio.
>>
>> That's very tempting. I see only atmel_serial.c using this function,
>> and there is no gpio_get_direction() declared anywhere so no user of
>> this either. I'm not sure what I was thinking when I decided to export
>> it?
> In next there is also drivers/tty/serial/mxs-auart.c.

Ok. I think we can get rid of gpiod_get_direction() if
serial_mctrl_gpio implements its own way to query the direction (maybe
using a bitmap in the mctrl_gpios and a mctrl_gpio_get_direction()
function that queries that map). Using gpiod_get_direction() is just
too unreliable.

Janusz, since your change for mxs-auart is in -next, would you mind
amending it to do this? Then we could do the same for atmel_serial and
remove gpiod_get_direction() from the public GPIO interface. This
function would do more harm than good anyway.

Thanks!

Re: [PATCH 2/4] gpiolib: use const parameters when possible

[Janusz Użycki]

Hello,

W dniu 2014-11-25 o 08:37, Alexandre Courbot pisze:
> On Wed, Nov 19, 2014 at 7:09 PM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Hello Alexandre,
>>
>> On Wed, Nov 19, 2014 at 06:07:50PM +0900, Alexandre Courbot wrote:
>>>   On Wed, Nov 19, 2014 at 6:02 PM, Uwe Kleine-König
>>> <u.kleine-koenig@pengutronix.de> wrote:
>>>> I'd make gpiod_get_direction static and only use it to fill in
>>>> /sys/kernel/debug/gpio.
>>> That's very tempting. I see only atmel_serial.c using this function,
>>> and there is no gpio_get_direction() declared anywhere so no user of
>>> this either. I'm not sure what I was thinking when I decided to export
>>> it?
>> In next there is also drivers/tty/serial/mxs-auart.c.
> Ok. I think we can get rid of gpiod_get_direction() if
> serial_mctrl_gpio implements its own way to query the direction (maybe
> using a bitmap in the mctrl_gpios and a mctrl_gpio_get_direction()
> function that queries that map). Using gpiod_get_direction() is just
> too unreliable.

good idea

>
> Janusz, since your change for mxs-auart is in -next, would you mind
> amending it to do this? Then we could do the same for atmel_serial and
> remove gpiod_get_direction() from the public GPIO interface. This
> function would do more harm than good anyway.

After a discussion I prepared RFC patch set to avoid the function.
You will find it here [1]:
("[RFC PATCH 1/2] serial: mctrl-gpio: Add irqs helpers for input lines")
("[RFC PATCH 2/2] serial: mxs-auart: use helpers for gpio irqs")
and the function was moved for debug only:
("[PATCH v3] gpio: mxs: implement get_direction callback")

Affected drivers by the patch set in the next are:
- atmel_serial
- mxs-auart
- clps711x (very basic usage)
The RFC patch set removes the inconvenient function from mxs-auart as 
example.
I didn't prepared a patch for atmel_serial because I wait for comments about
mctrl_gpio_is_gpio() function to differentiate if line is GPIO or native in
enable/disable_ms() callbacks.
I also can't test it for other devices than mxs (mx28).

[1] http://www.spinics.net/lists/arm-kernel/msg378444.html

best regards
Janusz