[PATCH v2 0/4] gpiolib: some fixup patches

[PATCH v2 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 tested on Tegra 2/Ventana with a GPIO backlight driver
and the sysfs interface.

Changes from v1:
- as suggested by Ryan Mallon, add a dedicated control flow for signaling
  invalid GPIOs to avoid meaningless error messages and ugly ternary operators
- use pr_warn instead of pr_debug to signal invalid GPIOs
- do descriptors validity check before acquiring any lock

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 | 213 +++++++++++++++++++++++++++----------------------
 1 file changed, 117 insertions(+), 96 deletions(-)

-- 
1.8.1.3

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

[Alexandre Courbot]

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 silently crashing.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Cc: Ryan Mallon <rmallon@gmail.com>
---
 drivers/gpio/gpiolib.c | 112 ++++++++++++++++++++++++++++---------------------
 1 file changed, 65 insertions(+), 47 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index fff9786..1a8a7a8 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)
@@ -654,6 +654,11 @@ static ssize_t export_store(struct class *class,
 		goto done;
 
 	desc = gpio_to_desc(gpio);
+	/* reject invalid GPIOs */
+	if (!desc) {
+		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
+		return -EINVAL;
+	}
 
 	/* No extra locking here; FLAG_SYSFS just signifies that the
 	 * request and export were done by on behalf of userspace, so
@@ -690,12 +695,14 @@ static ssize_t unexport_store(struct class *class,
 	if (status < 0)
 		goto done;
 
-	status = -EINVAL;
-
 	desc = gpio_to_desc(gpio);
 	/* reject bogus commands (gpio_unexport ignores them) */
-	if (!desc)
-		goto done;
+	if (!desc) {
+		pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
+		return -EINVAL;
+	}
+
+	status = -EINVAL;
 
 	/* No extra locking here; FLAG_SYSFS just signifies that the
 	 * request and export were done by on behalf of userspace, so
@@ -846,8 +853,10 @@ static int gpiod_export_link(struct device *dev, const char *name,
 {
 	int			status = -EINVAL;
 
-	if (!desc)
-		goto done;
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
 
 	mutex_lock(&sysfs_lock);
 
@@ -865,7 +874,6 @@ static int gpiod_export_link(struct device *dev, const char *name,
 
 	mutex_unlock(&sysfs_lock);
 
-done:
 	if (status)
 		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
 			 status);
@@ -896,8 +904,10 @@ static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
 	struct device		*dev = NULL;
 	int			status = -EINVAL;
 
-	if (!desc)
-		goto done;
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
 
 	mutex_lock(&sysfs_lock);
 
@@ -914,7 +924,6 @@ static int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
 unlock:
 	mutex_unlock(&sysfs_lock);
 
-done:
 	if (status)
 		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
 			 status);
@@ -940,8 +949,8 @@ static void gpiod_unexport(struct gpio_desc *desc)
 	struct device		*dev = NULL;
 
 	if (!desc) {
-		status = -EINVAL;
-		goto done;
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return;
 	}
 
 	mutex_lock(&sysfs_lock);
@@ -962,7 +971,7 @@ static void gpiod_unexport(struct gpio_desc *desc)
 		device_unregister(dev);
 		put_device(dev);
 	}
-done:
+
 	if (status)
 		pr_debug("%s: gpio%d status %d\n", __func__, desc_to_gpio(desc),
 			 status);
@@ -1384,12 +1393,13 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 	int			status = -EPROBE_DEFER;
 	unsigned long		flags;
 
-	spin_lock_irqsave(&gpio_lock, flags);
-
 	if (!desc) {
-		status = -EINVAL;
-		goto done;
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
 	}
+
+	spin_lock_irqsave(&gpio_lock, flags);
+
 	chip = desc->chip;
 	if (chip == NULL)
 		goto done;
@@ -1432,8 +1442,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_to_gpio(desc), label ? : "?", status);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 	return status;
 }
@@ -1616,10 +1625,13 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 	int			status = -EINVAL;
 	int			offset;
 
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!desc)
-		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->get || !chip->direction_input)
 		goto fail;
@@ -1655,13 +1667,9 @@ lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status) {
-		int gpio = -1;
-		if (desc)
-			gpio = desc_to_gpio(desc);
-		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+	if (status)
+		pr_debug("%s: gpio-%d status %d\n", __func__,
+			 desc_to_gpio(desc), status);
 	return status;
 }
 
@@ -1678,6 +1686,11 @@ static int gpiod_direction_output(struct gpio_desc *desc, int value)
 	int			status = -EINVAL;
 	int offset;
 
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
+
 	/* 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 +1701,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 +1736,9 @@ lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
-	if (status) {
-		int gpio = -1;
-		if (desc)
-			gpio = desc_to_gpio(desc);
-		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+	if (status)
+		pr_debug("%s: gpio-%d status %d\n", __func__,
+			 desc_to_gpio(desc), status);
 	return status;
 }
 
@@ -1753,10 +1760,13 @@ static int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
 	int			status = -EINVAL;
 	int			offset;
 
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	if (!desc)
-		goto fail;
 	chip = desc->chip;
 	if (!chip || !chip->set || !chip->set_debounce)
 		goto fail;
@@ -1776,13 +1786,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);
-		pr_debug("%s: gpio-%d status %d\n",
-			__func__, gpio, status);
-	}
+	if (status)
+		pr_debug("%s: gpio-%d status %d\n", __func__,
+			 desc_to_gpio(desc), status);
 
 	return status;
 }
@@ -1830,6 +1836,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 +1920,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 +1950,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 +1976,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 +2001,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 +2021,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]

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 1a8a7a8..a33bfc2 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);
@@ -1830,7 +1833,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;
@@ -1948,7 +1951,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;
@@ -1971,7 +1974,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;
@@ -1994,7 +1997,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]

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 a33bfc2..c2534d6 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]

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 c2534d6..6e38dfd 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;
@@ -756,7 +756,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) {
@@ -787,9 +787,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",
@@ -1596,7 +1596,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];
@@ -1626,7 +1626,7 @@ static int gpiod_direction_input(struct gpio_desc *desc)
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 	int			status = -EINVAL;
-	int			offset;
+	int			hwgpio;
 
 	if (!desc) {
 		pr_warn("%s: invalid GPIO\n", __func__);
@@ -1648,9 +1648,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);
@@ -1661,7 +1661,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);
 
@@ -1687,7 +1687,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) {
 		pr_warn("%s: invalid GPIO\n", __func__);
@@ -1717,9 +1717,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);
@@ -1730,7 +1730,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);
@@ -1761,7 +1761,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;
 
 	if (!desc) {
 		pr_warn("%s: invalid GPIO\n", __func__);
@@ -1784,8 +1784,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);
@@ -1837,15 +1837,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;
 }
@@ -1866,14 +1866,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);
 	}
@@ -1893,14 +1893,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);
 	}
@@ -1977,13 +1977,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)
@@ -2001,14 +2001,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

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Alexandre Courbot]

Grant, will you be able to include these for 3.9? They fix code that
you merged recently, so I'd be glad if they could be squashed into the
patch mentioned in the description.

Thanks,
Alex.


On Fri, Feb 15, 2013 at 2:46 PM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 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 tested on Tegra 2/Ventana with a GPIO backlight driver
> and the sysfs interface.
>
> Changes from v1:
> - as suggested by Ryan Mallon, add a dedicated control flow for signaling
>   invalid GPIOs to avoid meaningless error messages and ugly ternary operators
> - use pr_warn instead of pr_debug to signal invalid GPIOs
> - do descriptors validity check before acquiring any lock
>
> 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 | 213 +++++++++++++++++++++++++++----------------------
>  1 file changed, 117 insertions(+), 96 deletions(-)
>
> --
> 1.8.1.3
>

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

[Grant Likely]

On Fri, 15 Feb 2013 14:46:14 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 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 silently crashing.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Ryan Mallon <rmallon@gmail.com>

Applied, thanks.

g.

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

[Grant Likely]

On Fri, 15 Feb 2013 14:46:15 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 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>

Applied, thanks.

g.

> ---
>  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 1a8a7a8..a33bfc2 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);
> @@ -1830,7 +1833,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;
> @@ -1948,7 +1951,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;
> @@ -1971,7 +1974,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;
> @@ -1994,7 +1997,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
> 

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

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

[Grant Likely]

On Fri, 15 Feb 2013 14:46:17 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
> Their value being obtained by gpio_chip_hwgpio(), this better reflects
> their use.
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

I'm not going to bother with this one. It is churn on things that are
only local variables with no functional change. I would be more inclined
if it was something globably visable, but locals are easy to grok.

g.

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Grant Likely]

On Fri, 22 Feb 2013 11:19:44 +0900, Alexandre Courbot <gnurou@gmail.com> wrote:
> Grant, will you be able to include these for 3.9? They fix code that
> you merged recently, so I'd be glad if they could be squashed into the
> patch mentioned in the description.

They won't get squashed in because the tree is already composed and in
linux-next. I don't rebase unless I really need to. I've applied them
and I'll probably push to Linus since they are effectively bug fixes of
a sort and the merge window hasn't closed yet.

g.

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Alexandre Courbot]

On Wed, Feb 27, 2013 at 2:53 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Fri, 22 Feb 2013 11:19:44 +0900, Alexandre Courbot <gnurou@gmail.com> wrote:
>> Grant, will you be able to include these for 3.9? They fix code that
>> you merged recently, so I'd be glad if they could be squashed into the
>> patch mentioned in the description.
>
> They won't get squashed in because the tree is already composed and in
> linux-next. I don't rebase unless I really need to. I've applied them
> and I'll probably push to Linus since they are effectively bug fixes of
> a sort and the merge window hasn't closed yet.

That's fine too - as long as the patches with side-effects are merged.
That will allow me to continue going forward with GPIO, thanks!

Alex.

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Grant Likely]

On Wed, Feb 27, 2013 at 1:22 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Feb 27, 2013 at 2:53 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Fri, 22 Feb 2013 11:19:44 +0900, Alexandre Courbot <gnurou@gmail.com> wrote:
>>> Grant, will you be able to include these for 3.9? They fix code that
>>> you merged recently, so I'd be glad if they could be squashed into the
>>> patch mentioned in the description.
>>
>> They won't get squashed in because the tree is already composed and in
>> linux-next. I don't rebase unless I really need to. I've applied them
>> and I'll probably push to Linus since they are effectively bug fixes of
>> a sort and the merge window hasn't closed yet.
>
> That's fine too - as long as the patches with side-effects are merged.
> That will allow me to continue going forward with GPIO, thanks!

While you're working on that, I'd like you to keep the following in
mind. I'm getting concerned with the level of overhead that the gpio
access routines are incuring. They're doing a lot of checks right now
when with GPIOs we want it to be as fast as possible for the case of
mmio gpios. (i2c and spi gpios are always going to be slow, so I'm not
so concerned here). gpio_get, gpio_set and gpio_direction all need to
be fast.

Basically, I think the model should be that if you've got a gpio_desc
pointer, then you've got a valid gpio. A lot of the checks that are
currently performed in the gpiod_ versions of functions can be moved
to the gpio_ versions where a lookup has to be performed anyway. For
example, right now gpiod_direction_output() is 61 lines long. Madness!
 :-)

I've been playing with an idea of pulling in some basic MMIO gpio
access directly into gpiolib so that when appropriate gpiolib itself
can have a fast path for doing the register access and shadow register
management.

g.

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Alexandre Courbot]

On Wed, Feb 27, 2013 at 4:52 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> While you're working on that, I'd like you to keep the following in
> mind. I'm getting concerned with the level of overhead that the gpio
> access routines are incuring. They're doing a lot of checks right now
> when with GPIOs we want it to be as fast as possible for the case of
> mmio gpios. (i2c and spi gpios are always going to be slow, so I'm not
> so concerned here). gpio_get, gpio_set and gpio_direction all need to
> be fast.
>
> Basically, I think the model should be that if you've got a gpio_desc
> pointer, then you've got a valid gpio.

Yes, that's what I had in mind as well. Since the only way to obtain a
GPIO descriptor is through the gpiod_get() function(s), we can safely
issues descriptors are valid. There should be no gpiod_is_valid()
function.

> A lot of the checks that are
> currently performed in the gpiod_ versions of functions can be moved
> to the gpio_ versions where a lookup has to be performed anyway. For
> example, right now gpiod_direction_output() is 61 lines long. Madness!
>  :-)

There is certainly room to optimize things and avoid redundant tests
in the legacy GPIO API. Will be worth having a look at this once we
have a clean gpiod implementation.

> I've been playing with an idea of pulling in some basic MMIO gpio
> access directly into gpiolib so that when appropriate gpiolib itself
> can have a fast path for doing the register access and shadow register
> management.

Appealing. :)

Alex.

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Linus Walleij]

On Wed, Feb 27, 2013 at 8:52 AM, Grant Likely <grant.likely@secretlab.ca> wrote:

> I've been playing with an idea of pulling in some basic MMIO gpio
> access directly into gpiolib so that when appropriate gpiolib itself
> can have a fast path for doing the register access and shadow register
> management.

Shadow registers: isn't that what regmap usually does? Or
what are you referring to here?

But fastpath MMIO in the gpiolib is an appealing thought,
given that this was a design impetus in the early history of
the subsystem.

Yours,
Linus Walleij

Re: [PATCH v2 0/4] gpiolib: some fixup patches

[Grant Likely]

On Thu, Feb 28, 2013 at 6:21 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Feb 27, 2013 at 8:52 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>
>> I've been playing with an idea of pulling in some basic MMIO gpio
>> access directly into gpiolib so that when appropriate gpiolib itself
>> can have a fast path for doing the register access and shadow register
>> management.
>
> Shadow registers: isn't that what regmap usually does? Or
> what are you referring to here?

Yes, regmap does shadow registers, but that is another layer of
indirection that would nullify the advantage of a fast path.

> But fastpath MMIO in the gpiolib is an appealing thought,
> given that this was a design impetus in the early history of
> the subsystem.

Indeed. I've worked on many a platform that struggled to bitbang fast
enough. A lot of the time it came down to too much indirection and
mucking about when it came to actually manipulating the GPIO. I would
like do some experiments in ways to implement a fast path and see what
the code generation looks like. Any indirection is expensive so the
trick is to minimize the number of dereferences and callbacks
involved. Unfortunately there are several common access modes that I
think should be supported, the most important being single data
register vs. set/clr and direction registers with different
polarities. Also, should the fast path only do 8-bit accesses, or can
16, 32 and 64 bit banks be supported?

g.