[PATCH 00/23] gpio: sysfs: fixes and clean ups

[PATCH 00/23] gpio: sysfs: fixes and clean ups

[Johan Hovold]

These patches fix a number of issues with the gpio sysfs interface,
including

 - fix memory leaks and crashes on device hotplug
 - straighten out the convoluted locking
 - reduce sysfs-interface latencies through more fine-grained locking
 - more clearly separate the sysfs-interface implementation from gpiolib
   core

The first patch is marked for stable and could go into 4.1. 

Unfortunately we can't just kill the gpio sysfs interface, but these
patches will make it more manageable and should allow us to implement a
new user-space interface while maintaining the old one (for a while at
least) without losing our sanity.

Note that there is still a race between chip remove and gpiod_request (and
therefore sysfs export), which needs to be fixed separately (for instance as
part of a generic solution to chip hotplugging).

Johan


Johan Hovold (23):
  gpio: sysfs: fix memory leaks and device hotplug
  gpio: clean up gpiochip_remove
  gpio: sysfs: drop redundant lock-as-irq
  gpio: sysfs: preparatory clean ups
  gpio: sysfs: reduce gpiochip-export locking scope
  gpio: sysfs: clean up chip class-device handling
  gpio: sysfs: rename gpiochip registration functions
  gpio: remove gpiod_sysfs_set_active_low
  gpio: sysfs: use DEVICE_ATTR macros
  gpio: sysfs: release irq after class-device deregistration
  gpio: sysfs: remove redundant export tests
  gpio: sysfs: add gpiod class-device data
  gpio: sysfs: remove redundant gpio-descriptor parameters
  gpio: sysfs: clean up interrupt-interface implementation
  gpio: sysfs: only call irq helper if needed
  gpio: sysfs: split irq allocation and deallocation
  gpio: sysfs: clean up edge_store
  gpio: sysfs: clean up gpiod_export_link locking
  gpio: sysfs: use per-gpio locking
  gpio: sysfs: fix race between gpiod export and unexport
  gpio: sysfs: rename active-low helper
  gpio: sysfs: remove FLAG_SYSFS_DIR
  gpio: sysfs: move irq trigger flags to class-device data

 Documentation/gpio/gpio-legacy.txt |   9 -
 Documentation/gpio/sysfs.txt       |   8 -
 Documentation/zh_CN/gpio.txt       |   8 -
 drivers/gpio/gpiolib-sysfs.c       | 561 +++++++++++++++++--------------------
 drivers/gpio/gpiolib.c             |  18 +-
 drivers/gpio/gpiolib.h             |  16 +-
 include/asm-generic/gpio.h         |   5 -
 include/linux/gpio.h               |   7 -
 include/linux/gpio/consumer.h      |   6 -
 include/linux/gpio/driver.h        |   4 +-
 10 files changed, 273 insertions(+), 369 deletions(-)

-- 
2.0.5

[PATCH 01/23] gpio: sysfs: fix memory leaks and device hotplug

[Johan Hovold]

Unregister GPIOs requested through sysfs at chip remove to avoid leaking
the associated memory and sysfs entries.

The stale sysfs entries prevented the gpio numbers from being exported
when the gpio range was later reused (e.g. at device reconnect).

This also fixes the related module-reference leak.

Note that kernfs makes sure that any on-going sysfs operations finish
before the class devices are unregistered and that further accesses
fail.

The chip exported flag is used to prevent gpiod exports during removal.
This also makes it harder to trigger, but does not fix, the related race
between gpiochip_remove and export_store, which is really a race with
gpiod_request that needs to be addressed separately.

Also note that this would prevent the crashes (e.g. NULL-dereferences)
at reconnect that affects pre-3.18 kernels, as well as use-after-free on
operations on open attribute files on pre-3.14 kernels (prior to
kernfs).

Fixes: d8f388d8dc8d ("gpio: sysfs interface")
Cc: stable <stable@vger.kernel.org>	# v2.6.27: 01cca93a9491
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 7722ed53bd65..af3bc7a8033b 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -551,6 +551,7 @@ static struct class gpio_class = {
  */
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
+	struct gpio_chip	*chip;
 	unsigned long		flags;
 	int			status;
 	const char		*ioname = NULL;
@@ -568,8 +569,16 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		return -EINVAL;
 	}
 
+	chip = desc->chip;
+
 	mutex_lock(&sysfs_lock);
 
+	/* check if chip is being removed */
+	if (!chip || !chip->exported) {
+		status = -ENODEV;
+		goto fail_unlock;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
 	     test_bit(FLAG_EXPORT, &desc->flags)) {
@@ -783,12 +792,15 @@ void gpiochip_unexport(struct gpio_chip *chip)
 {
 	int			status;
 	struct device		*dev;
+	struct gpio_desc *desc;
+	unsigned int i;
 
 	mutex_lock(&sysfs_lock);
 	dev = class_find_device(&gpio_class, NULL, chip, match_export);
 	if (dev) {
 		put_device(dev);
 		device_unregister(dev);
+		/* prevent further gpiod exports */
 		chip->exported = false;
 		status = 0;
 	} else
@@ -797,6 +809,13 @@ void gpiochip_unexport(struct gpio_chip *chip)
 
 	if (status)
 		chip_dbg(chip, "%s: status %d\n", __func__, status);
+
+	/* unregister gpiod class devices owned by sysfs */
+	for (i = 0; i < chip->ngpio; i++) {
+		desc = &chip->desc[i];
+		if (test_and_clear_bit(FLAG_SYSFS, &desc->flags))
+			gpiod_free(desc);
+	}
 }
 
 static int __init gpiolib_sysfs_init(void)
-- 
2.0.5

[PATCH 02/23] gpio: clean up gpiochip_remove

[Johan Hovold]

Clean up gpiochip_remove somewhat and only output warning about removing
chip with GPIOs requested once.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59eaa23767d8..5a5c208d31c7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -325,8 +325,10 @@ static void gpiochip_free_hogs(struct gpio_chip *chip);
  */
 void gpiochip_remove(struct gpio_chip *chip)
 {
+	struct gpio_desc *desc;
 	unsigned long	flags;
 	unsigned	id;
+	bool		requested = false;
 
 	gpiochip_unexport(chip);
 
@@ -339,15 +341,17 @@ void gpiochip_remove(struct gpio_chip *chip)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 	for (id = 0; id < chip->ngpio; id++) {
-		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
-			dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
+		desc = &chip->desc[id];
+		desc->chip = NULL;
+		if (test_bit(FLAG_REQUESTED, &desc->flags))
+			requested = true;
 	}
-	for (id = 0; id < chip->ngpio; id++)
-		chip->desc[id].chip = NULL;
-
 	list_del(&chip->list);
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
+	if (requested)
+		dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
+
 	kfree(chip->desc);
 	chip->desc = NULL;
 }
-- 
2.0.5

[PATCH 03/23] gpio: sysfs: drop redundant lock-as-irq

[Johan Hovold]

Drop redundant lock-as-irq in gpio_setup_irq, which has already been
handled when requesting and releasing the irq (i.e. in the irq chip
irq_request_resources and irq_release_resources callbacks).

Note that we would be leaking the irq in the gpiochip_lock_as_irq error
path if the (second) explicit call ever failed.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index af3bc7a8033b..d87f595a02ce 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -161,7 +161,6 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 	desc->flags &= ~GPIO_TRIGGER_MASK;
 
 	if (!gpio_flags) {
-		gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 		ret = 0;
 		goto free_id;
 	}
@@ -200,12 +199,6 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 	if (ret < 0)
 		goto free_id;
 
-	ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-	if (ret < 0) {
-		gpiod_warn(desc, "failed to flag the GPIO for IRQ\n");
-		goto free_id;
-	}
-
 	desc->flags |= gpio_flags;
 	return 0;
 
-- 
2.0.5

[PATCH 04/23] gpio: sysfs: preparatory clean ups

[Johan Hovold]

Put the recently introduced gpio-chip pointer to some more use in
gpiod_export.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d87f595a02ce..da0300224b4a 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -584,7 +584,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		goto fail_unlock;
 	}
 
-	if (desc->chip->direction_input && desc->chip->direction_output &&
+	if (chip->direction_input && chip->direction_output &&
 			direction_may_change) {
 		set_bit(FLAG_SYSFS_DIR, &desc->flags);
 	}
@@ -592,10 +592,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	offset = gpio_chip_hwgpio(desc);
-	if (desc->chip->names && desc->chip->names[offset])
-		ioname = desc->chip->names[offset];
+	if (chip->names && chip->names[offset])
+		ioname = chip->names[offset];
 
-	dev = device_create_with_groups(&gpio_class, desc->chip->dev,
+	dev = device_create_with_groups(&gpio_class, chip->dev,
 					MKDEV(0, 0), desc, gpio_groups,
 					ioname ? ioname : "gpio%u",
 					desc_to_gpio(desc));
-- 
2.0.5

[PATCH 05/23] gpio: sysfs: reduce gpiochip-export locking scope

[Johan Hovold]

Reduce scope of sysfs_lock protection during chip export and unexport,
which is only needed to prevent gpiod (re-)exports during chip removal.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index da0300224b4a..c433f075f349 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -764,7 +764,6 @@ int gpiochip_export(struct gpio_chip *chip)
 		return 0;
 
 	/* use chip->base for the ID; it's already known to be unique */
-	mutex_lock(&sysfs_lock);
 	dev = device_create_with_groups(&gpio_class, chip->dev, MKDEV(0, 0),
 					chip, gpiochip_groups,
 					"gpiochip%d", chip->base);
@@ -772,6 +771,8 @@ int gpiochip_export(struct gpio_chip *chip)
 		status = PTR_ERR(dev);
 	else
 		status = 0;
+
+	mutex_lock(&sysfs_lock);
 	chip->exported = (status == 0);
 	mutex_unlock(&sysfs_lock);
 
@@ -788,17 +789,17 @@ void gpiochip_unexport(struct gpio_chip *chip)
 	struct gpio_desc *desc;
 	unsigned int i;
 
-	mutex_lock(&sysfs_lock);
 	dev = class_find_device(&gpio_class, NULL, chip, match_export);
 	if (dev) {
 		put_device(dev);
 		device_unregister(dev);
 		/* prevent further gpiod exports */
+		mutex_lock(&sysfs_lock);
 		chip->exported = false;
+		mutex_unlock(&sysfs_lock);
 		status = 0;
 	} else
 		status = -ENODEV;
-	mutex_unlock(&sysfs_lock);
 
 	if (status)
 		chip_dbg(chip, "%s: status %d\n", __func__, status);
-- 
2.0.5

[PATCH 06/23] gpio: sysfs: clean up chip class-device handling

[Johan Hovold]

Clean gpio-chip class device registration and deregistration.

The class device is registered when a gpio-chip is added (or from
gpiolib_sysfs_init post-core init call), and deregistered when the chip
is removed.

Store the class device in struct gpio_chip directly rather than do a
class-device lookup on deregistration. This also removes the need for
the exported flag.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 39 +++++++++++++--------------------------
 include/linux/gpio/driver.h  |  4 ++--
 2 files changed, 15 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c433f075f349..2f8bdce792f6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	mutex_lock(&sysfs_lock);
 
 	/* check if chip is being removed */
-	if (!chip || !chip->exported) {
+	if (!chip || !chip->cdev) {
 		status = -ENODEV;
 		goto fail_unlock;
 	}
@@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
 
 int gpiochip_export(struct gpio_chip *chip)
 {
-	int		status;
 	struct device	*dev;
 
 	/* Many systems register gpio chips for SOC support very early,
@@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
 					chip, gpiochip_groups,
 					"gpiochip%d", chip->base);
 	if (IS_ERR(dev))
-		status = PTR_ERR(dev);
-	else
-		status = 0;
+		return PTR_ERR(dev);
 
 	mutex_lock(&sysfs_lock);
-	chip->exported = (status == 0);
+	chip->cdev = dev;
 	mutex_unlock(&sysfs_lock);
 
-	if (status)
-		chip_dbg(chip, "%s: status %d\n", __func__, status);
-
-	return status;
+	return 0;
 }
 
 void gpiochip_unexport(struct gpio_chip *chip)
 {
-	int			status;
-	struct device		*dev;
 	struct gpio_desc *desc;
 	unsigned int i;
 
-	dev = class_find_device(&gpio_class, NULL, chip, match_export);
-	if (dev) {
-		put_device(dev);
-		device_unregister(dev);
-		/* prevent further gpiod exports */
-		mutex_lock(&sysfs_lock);
-		chip->exported = false;
-		mutex_unlock(&sysfs_lock);
-		status = 0;
-	} else
-		status = -ENODEV;
+	if (!chip->cdev)
+		return;
 
-	if (status)
-		chip_dbg(chip, "%s: status %d\n", __func__, status);
+	device_unregister(chip->cdev);
+
+	/* prevent further gpiod exports */
+	mutex_lock(&sysfs_lock);
+	chip->cdev = NULL;
+	mutex_unlock(&sysfs_lock);
 
 	/* unregister gpiod class devices owned by sysfs */
 	for (i = 0; i < chip->ngpio; i++) {
@@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
 	 */
 	spin_lock_irqsave(&gpio_lock, flags);
 	list_for_each_entry(chip, &gpio_chips, list) {
-		if (chip->exported)
+		if (chip->cdev)
 			continue;
 
 		/*
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index f1b36593ec9f..8c26855fc6ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -20,6 +20,7 @@ struct seq_file;
  * struct gpio_chip - abstract a GPIO controller
  * @label: for diagnostics
  * @dev: optional device providing the GPIOs
+ * @cdev: class device (may be NULL)
  * @owner: helps prevent removal of modules exporting active GPIOs
  * @list: links gpio_chips together for traversal
  * @request: optional hook for chip-specific activation, such as
@@ -57,7 +58,6 @@ struct seq_file;
  *	implies that if the chip supports IRQs, these IRQs need to be threaded
  *	as the chip access may sleep when e.g. reading out the IRQ status
  *	registers.
- * @exported: flags if the gpiochip is exported for use from sysfs. Private.
  * @irq_not_threaded: flag must be set if @can_sleep is set but the
  *	IRQs don't need to be threaded
  *
@@ -74,6 +74,7 @@ struct seq_file;
 struct gpio_chip {
 	const char		*label;
 	struct device		*dev;
+	struct device		*cdev;
 	struct module		*owner;
 	struct list_head        list;
 
@@ -109,7 +110,6 @@ struct gpio_chip {
 	const char		*const *names;
 	bool			can_sleep;
 	bool			irq_not_threaded;
-	bool			exported;
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 	/*
-- 
2.0.5

[PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Johan Hovold]

Rename the gpio-chip export/unexport functions to the more descriptive
names gpiochip_register and gpiochip_unregister.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 13 +++++++------
 drivers/gpio/gpiolib.c       |  4 ++--
 drivers/gpio/gpiolib.h       |  8 ++++----
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2f8bdce792f6..31434c5a90ef 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -750,13 +750,14 @@ void gpiod_unexport(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
-int gpiochip_export(struct gpio_chip *chip)
+int gpiochip_register(struct gpio_chip *chip)
 {
 	struct device	*dev;
 
-	/* Many systems register gpio chips for SOC support very early,
+	/*
+	 * Many systems add gpio chips for SOC support very early,
 	 * before driver model support is available.  In those cases we
-	 * export this later, in gpiolib_sysfs_init() ... here we just
+	 * register later, in gpiolib_sysfs_init() ... here we just
 	 * verify that _some_ field of gpio_class got initialized.
 	 */
 	if (!gpio_class.p)
@@ -776,7 +777,7 @@ int gpiochip_export(struct gpio_chip *chip)
 	return 0;
 }
 
-void gpiochip_unexport(struct gpio_chip *chip)
+void gpiochip_unregister(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc;
 	unsigned int i;
@@ -821,7 +822,7 @@ static int __init gpiolib_sysfs_init(void)
 			continue;
 
 		/*
-		 * TODO we yield gpio_lock here because gpiochip_export()
+		 * TODO we yield gpio_lock here because gpiochip_register()
 		 * acquires a mutex. This is unsafe and needs to be fixed.
 		 *
 		 * Also it would be nice to use gpiochip_find() here so we
@@ -829,7 +830,7 @@ static int __init gpiolib_sysfs_init(void)
 		 * gpio_lock prevents us from doing this.
 		 */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = gpiochip_export(chip);
+		status = gpiochip_register(chip);
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 	spin_unlock_irqrestore(&gpio_lock, flags);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5a5c208d31c7..fefc36211205 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -285,7 +285,7 @@ int gpiochip_add(struct gpio_chip *chip)
 	of_gpiochip_add(chip);
 	acpi_gpiochip_add(chip);
 
-	status = gpiochip_export(chip);
+	status = gpiochip_register(chip);
 	if (status)
 		goto err_remove_chip;
 
@@ -330,7 +330,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 	unsigned	id;
 	bool		requested = false;
 
-	gpiochip_unexport(chip);
+	gpiochip_unregister(chip);
 
 	gpiochip_irqchip_remove(chip);
 
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 54bc5ec91398..69458a022c90 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -149,17 +149,17 @@ static int __maybe_unused gpio_chip_hwgpio(const struct gpio_desc *desc)
 
 #ifdef CONFIG_GPIO_SYSFS
 
-int gpiochip_export(struct gpio_chip *chip);
-void gpiochip_unexport(struct gpio_chip *chip);
+int gpiochip_register(struct gpio_chip *chip);
+void gpiochip_unregister(struct gpio_chip *chip);
 
 #else
 
-static inline int gpiochip_export(struct gpio_chip *chip)
+static inline int gpiochip_register(struct gpio_chip *chip)
 {
 	return 0;
 }
 
-static inline void gpiochip_unexport(struct gpio_chip *chip)
+static inline void gpiochip_unregister(struct gpio_chip *chip)
 {
 }
 
-- 
2.0.5

[PATCH 08/23] gpio: remove gpiod_sysfs_set_active_low

[Johan Hovold]

Remove gpiod_sysfs_set_active_low (and gpio_sysfs_set_active_low) which
allowed code to change the polarity of a gpio line even after it had
been exported through sysfs.

Drivers should not care, and generally does not know, about gpio-line
polarity which is a hardware feature that needs to be described by
firmware.

It is currently possible to define gpio-line polarity in device-tree and
acpi firmware or using platform data. Userspace can also change the
polarity through sysfs.

Note that drivers using the legacy gpio interface could still use
GPIOF_ACTIVE_LOW to change the polarity before exporting the gpio.

There are no in-kernel users of this interface.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Harry Wei <harryxiyou@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@zh-kernel.org
Cc: linux-arch@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 Documentation/gpio/gpio-legacy.txt |  9 -------
 Documentation/gpio/sysfs.txt       |  8 -------
 Documentation/zh_CN/gpio.txt       |  8 -------
 drivers/gpio/gpiolib-sysfs.c       | 48 ++------------------------------------
 include/asm-generic/gpio.h         |  5 ----
 include/linux/gpio.h               |  7 ------
 include/linux/gpio/consumer.h      |  6 -----
 7 files changed, 2 insertions(+), 89 deletions(-)

diff --git a/Documentation/gpio/gpio-legacy.txt b/Documentation/gpio/gpio-legacy.txt
index 6f83fa965b4b..79ab5648d69b 100644
--- a/Documentation/gpio/gpio-legacy.txt
+++ b/Documentation/gpio/gpio-legacy.txt
@@ -751,9 +751,6 @@ requested using gpio_request():
 	int gpio_export_link(struct device *dev, const char *name,
 		unsigned gpio)
 
-	/* change the polarity of a GPIO node in sysfs */
-	int gpio_sysfs_set_active_low(unsigned gpio, int value);
-
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpio_export().  The driver can control whether the
 signal direction may change.  This helps drivers prevent userspace code
@@ -767,9 +764,3 @@ After the GPIO has been exported, gpio_export_link() allows creating
 symlinks from elsewhere in sysfs to the GPIO sysfs node.  Drivers can
 use this to provide the interface under their own device in sysfs with
 a descriptive name.
-
-Drivers can use gpio_sysfs_set_active_low() to hide GPIO line polarity
-differences between boards from user space.  This only affects the
-sysfs interface.  Polarity change can be done both before and after
-gpio_export(), and previously enabled poll(2) support for either
-rising or falling edge will be reconfigured to follow this setting.
diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index c2c3a97f8ff7..535b6a8a7a7c 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -132,9 +132,6 @@ requested using gpio_request():
 	int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc);
 
-	/* change the polarity of a GPIO node in sysfs */
-	int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
-
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpiod_export(). The driver can control whether the
 signal direction may change. This helps drivers prevent userspace code
@@ -148,8 +145,3 @@ After the GPIO has been exported, gpiod_export_link() allows creating
 symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
 use this to provide the interface under their own device in sysfs with
 a descriptive name.
-
-Drivers can use gpiod_sysfs_set_active_low() to hide GPIO line polarity
-differences between boards from user space. Polarity change can be done both
-before and after gpiod_export(), and previously enabled poll(2) support for
-either rising or falling edge will be reconfigured to follow this setting.
diff --git a/Documentation/zh_CN/gpio.txt b/Documentation/zh_CN/gpio.txt
index d5b8f01833f4..bce972521065 100644
--- a/Documentation/zh_CN/gpio.txt
+++ b/Documentation/zh_CN/gpio.txt
@@ -638,9 +638,6 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
 	int gpio_export_link(struct device *dev, const char *name,
 		unsigned gpio)
 
-	/* 改变 sysfs 中的一个 GPIO 节点的极性 */
-	int gpio_sysfs_set_active_low(unsigned gpio, int value);
-
 在一个内核驱动申请一个 GPIO 之后，它可以通过 gpio_export()使其在 sysfs
 接口中可见。该驱动可以控制信号方向是否可修改。这有助于防止用户空间代码无意间
 破坏重要的系统状态。
@@ -651,8 +648,3 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
 在 GPIO 被导出之后，gpio_export_link()允许在 sysfs 文件系统的任何地方
 创建一个到这个 GPIO sysfs 节点的符号链接。这样驱动就可以通过一个描述性的
 名字，在 sysfs 中他们所拥有的设备下提供一个(到这个 GPIO sysfs 节点的)接口。
-
-驱动可以使用 gpio_sysfs_set_active_low() 来在用户空间隐藏电路板之间
-GPIO 线的极性差异。这个仅对 sysfs 接口起作用。极性的改变可以在 gpio_export()
-前后进行,且之前使能的轮询操作(poll(2))支持(上升或下降沿)将会被重新配置来遵循
-这个设置。
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 31434c5a90ef..8a95a954f514 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
 	/* reconfigure poll(2) support if enabled on one edge only */
-	if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
-				!!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
+	if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
+				!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
 		unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
 
 		gpio_setup_irq(desc, dev, 0);
@@ -666,50 +666,6 @@ int gpiod_export_link(struct device *dev, const char *name,
 EXPORT_SYMBOL_GPL(gpiod_export_link);
 
 /**
- * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value
- * @gpio: gpio to change
- * @value: non-zero to use active low, i.e. inverted values
- *
- * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute.
- * The GPIO does not have to be exported yet.  If poll(2) support has
- * been enabled for either rising or falling edge, it will be
- * reconfigured to follow the new polarity.
- *
- * Returns zero on success, else an error.
- */
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
-{
-	struct device		*dev = NULL;
-	int			status = -EINVAL;
-
-	if (!desc) {
-		pr_warn("%s: invalid GPIO\n", __func__);
-		return -EINVAL;
-	}
-
-	mutex_lock(&sysfs_lock);
-
-	if (test_bit(FLAG_EXPORT, &desc->flags)) {
-		dev = class_find_device(&gpio_class, NULL, desc, match_export);
-		if (dev == NULL) {
-			status = -ENODEV;
-			goto unlock;
-		}
-	}
-
-	status = sysfs_set_active_low(desc, dev, value);
-	put_device(dev);
-unlock:
-	mutex_unlock(&sysfs_lock);
-
-	if (status)
-		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
-
-	return status;
-}
-EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low);
-
-/**
  * gpiod_unexport - reverse effect of gpio_export()
  * @gpio: gpio to make unavailable
  *
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 9bb0d11729c9..40ec1433f05d 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -128,11 +128,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
 	return gpiod_export_link(dev, name, gpio_to_desc(gpio));
 }
 
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
-	return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value);
-}
-
 static inline void gpio_unexport(unsigned gpio)
 {
 	gpiod_unexport(gpio_to_desc(gpio));
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index ab81339a8590..d12b5d566e4b 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -196,13 +196,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
 	return -EINVAL;
 }
 
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
-	/* GPIO can never have been requested */
-	WARN_ON(1);
-	return -EINVAL;
-}
-
 static inline void gpio_unexport(unsigned gpio)
 {
 	/* GPIO can never have been exported */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 3a7c9ffd5ab9..09a7fb0062a6 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -449,7 +449,6 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
 int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc);
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
 void gpiod_unexport(struct gpio_desc *desc);
 
 #else  /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
@@ -466,11 +465,6 @@ static inline int gpiod_export_link(struct device *dev, const char *name,
 	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)
 {
 }
-- 
2.0.5

[PATCH 09/23] gpio: sysfs: use DEVICE_ATTR macros

[Johan Hovold]

Use DEVICE_ATTR_RO and DEVICE_ATTR_RW rather than specifying masks and
callbacks directly.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 8a95a954f514..9d9f1f1a2417 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -38,7 +38,7 @@ static DEFINE_MUTEX(sysfs_lock);
  *        /edge configuration
  */
 
-static ssize_t gpio_direction_show(struct device *dev,
+static ssize_t direction_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -59,7 +59,7 @@ static ssize_t gpio_direction_show(struct device *dev,
 	return status;
 }
 
-static ssize_t gpio_direction_store(struct device *dev,
+static ssize_t direction_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -81,11 +81,9 @@ static ssize_t gpio_direction_store(struct device *dev,
 	mutex_unlock(&sysfs_lock);
 	return status ? : size;
 }
+static DEVICE_ATTR_RW(direction);
 
-static /* const */ DEVICE_ATTR(direction, 0644,
-		gpio_direction_show, gpio_direction_store);
-
-static ssize_t gpio_value_show(struct device *dev,
+static ssize_t value_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -102,7 +100,7 @@ static ssize_t gpio_value_show(struct device *dev,
 	return status;
 }
 
-static ssize_t gpio_value_store(struct device *dev,
+static ssize_t value_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -127,9 +125,7 @@ static ssize_t gpio_value_store(struct device *dev,
 	mutex_unlock(&sysfs_lock);
 	return status;
 }
-
-static DEVICE_ATTR(value, 0644,
-		gpio_value_show, gpio_value_store);
+static DEVICE_ATTR_RW(value);
 
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
@@ -222,7 +218,7 @@ static const struct {
 	{ "both",    BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
 };
 
-static ssize_t gpio_edge_show(struct device *dev,
+static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	const struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -249,7 +245,7 @@ static ssize_t gpio_edge_show(struct device *dev,
 	return status;
 }
 
-static ssize_t gpio_edge_store(struct device *dev,
+static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -276,8 +272,7 @@ found:
 
 	return status;
 }
-
-static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
+static DEVICE_ATTR_RW(edge);
 
 static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 				int value)
@@ -304,7 +299,7 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 	return status;
 }
 
-static ssize_t gpio_active_low_show(struct device *dev,
+static ssize_t active_low_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	const struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -323,7 +318,7 @@ static ssize_t gpio_active_low_show(struct device *dev,
 	return status;
 }
 
-static ssize_t gpio_active_low_store(struct device *dev,
+static ssize_t active_low_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
@@ -345,9 +340,7 @@ static ssize_t gpio_active_low_store(struct device *dev,
 
 	return status ? : size;
 }
-
-static DEVICE_ATTR(active_low, 0644,
-		gpio_active_low_show, gpio_active_low_store);
+static DEVICE_ATTR_RW(active_low);
 
 static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			       int n)
@@ -395,32 +388,32 @@ static const struct attribute_group *gpio_groups[] = {
  *   /ngpio ... matching gpio_chip.ngpio
  */
 
-static ssize_t chip_base_show(struct device *dev,
+static ssize_t base_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	const struct gpio_chip	*chip = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%d\n", chip->base);
 }
-static DEVICE_ATTR(base, 0444, chip_base_show, NULL);
+static DEVICE_ATTR_RO(base);
 
-static ssize_t chip_label_show(struct device *dev,
+static ssize_t label_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	const struct gpio_chip	*chip = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%s\n", chip->label ? : "");
 }
-static DEVICE_ATTR(label, 0444, chip_label_show, NULL);
+static DEVICE_ATTR_RO(label);
 
-static ssize_t chip_ngpio_show(struct device *dev,
+static ssize_t ngpio_show(struct device *dev,
 			       struct device_attribute *attr, char *buf)
 {
 	const struct gpio_chip	*chip = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", chip->ngpio);
 }
-static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL);
+static DEVICE_ATTR_RO(ngpio);
 
 static struct attribute *gpiochip_attrs[] = {
 	&dev_attr_base.attr,
-- 
2.0.5

[PATCH 10/23] gpio: sysfs: release irq after class-device deregistration

[Johan Hovold]

Make sure to release any irq only after the class device has been
deregistered.

This avoids a race between gpiod_unexport and edge_store, where an irq
could be allocated just before the gpio class device is deregistered
without relying on FLAG_EXPORT and the global sysfs lock.

Note that there is no need to hold the sysfs lock when releasing the irq
after the class device is gone as kernfs will prevent further attribute
operations.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 9d9f1f1a2417..d896b6fa7fe8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -680,7 +680,6 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 		dev = class_find_device(&gpio_class, NULL, desc, match_export);
 		if (dev) {
-			gpio_setup_irq(desc, dev, 0);
 			clear_bit(FLAG_SYSFS_DIR, &desc->flags);
 			clear_bit(FLAG_EXPORT, &desc->flags);
 		} else
@@ -691,6 +690,11 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 	if (dev) {
 		device_unregister(dev);
+		/*
+		 * Release irq after deregistration to prevent race with
+		 * edge_store.
+		 */
+		gpio_setup_irq(desc, dev, 0);
 		put_device(dev);
 	}
 
-- 
2.0.5

[PATCH 11/23] gpio: sysfs: remove redundant export tests

[Johan Hovold]

The attribute operations will never be called for an unregistered device
so remove redundant checks for FLAG_EXPORT.

Note that kernfs will also guarantee that any active sysfs operation has
finished before the attribute is removed during deregistration.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 74 ++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 51 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d896b6fa7fe8..b77f29cd1603 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -46,14 +46,10 @@ static ssize_t direction_show(struct device *dev,
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags)) {
-		status = -EIO;
-	} else {
-		gpiod_get_direction(desc);
-		status = sprintf(buf, "%s\n",
+	gpiod_get_direction(desc);
+	status = sprintf(buf, "%s\n",
 			test_bit(FLAG_IS_OUT, &desc->flags)
 				? "out" : "in");
-	}
 
 	mutex_unlock(&sysfs_lock);
 	return status;
@@ -67,9 +63,7 @@ static ssize_t direction_store(struct device *dev,
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else if (sysfs_streq(buf, "high"))
+	if (sysfs_streq(buf, "high"))
 		status = gpiod_direction_output_raw(desc, 1);
 	else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
 		status = gpiod_direction_output_raw(desc, 0);
@@ -91,10 +85,7 @@ static ssize_t value_show(struct device *dev,
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else
-		status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
+	status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
 
 	mutex_unlock(&sysfs_lock);
 	return status;
@@ -108,11 +99,9 @@ static ssize_t value_store(struct device *dev,
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else if (!test_bit(FLAG_IS_OUT, &desc->flags))
+	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
 		status = -EPERM;
-	else {
+	} else {
 		long		value;
 
 		status = kstrtol(buf, 0, &value);
@@ -222,23 +211,18 @@ static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	const struct gpio_desc	*desc = dev_get_drvdata(dev);
-	ssize_t			status;
+	unsigned long mask;
+	ssize_t	status = 0;
+	int i;
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else {
-		int i;
-
-		status = 0;
-		for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
-			if ((desc->flags & GPIO_TRIGGER_MASK)
-					== trigger_types[i].flags) {
-				status = sprintf(buf, "%s\n",
-						 trigger_types[i].name);
-				break;
-			}
+	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
+		mask = desc->flags & GPIO_TRIGGER_MASK;
+		if (mask == trigger_types[i].flags) {
+			status = sprintf(buf, "%s\n", trigger_types[i].name);
+			break;
+		}
 	}
 
 	mutex_unlock(&sysfs_lock);
@@ -260,13 +244,9 @@ static ssize_t edge_store(struct device *dev,
 found:
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else {
-		status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
-		if (!status)
-			status = size;
-	}
+	status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+	if (!status)
+		status = size;
 
 	mutex_unlock(&sysfs_lock);
 
@@ -307,10 +287,7 @@ static ssize_t active_low_show(struct device *dev,
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags))
-		status = -EIO;
-	else
-		status = sprintf(buf, "%d\n",
+	status = sprintf(buf, "%d\n",
 				!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
 
 	mutex_unlock(&sysfs_lock);
@@ -323,18 +300,13 @@ static ssize_t active_low_store(struct device *dev,
 {
 	struct gpio_desc	*desc = dev_get_drvdata(dev);
 	ssize_t			status;
+	long			value;
 
 	mutex_lock(&sysfs_lock);
 
-	if (!test_bit(FLAG_EXPORT, &desc->flags)) {
-		status = -EIO;
-	} else {
-		long		value;
-
-		status = kstrtol(buf, 0, &value);
-		if (status == 0)
-			status = sysfs_set_active_low(desc, dev, value != 0);
-	}
+	status = kstrtol(buf, 0, &value);
+	if (status == 0)
+		status = sysfs_set_active_low(desc, dev, value != 0);
 
 	mutex_unlock(&sysfs_lock);
 
-- 
2.0.5

[PATCH 12/23] gpio: sysfs: add gpiod class-device data

[Johan Hovold]

Add gpiod class-device data.

This is a first step in getting rid of the insane gpio-descriptor flag
overloading, backward irq-interface implementation, and course grained
sysfs-interface locking (a single static mutex for every operation on
all exported gpios in a system).

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 62 ++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b77f29cd1603..4cc90f7ce14b 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -6,9 +6,14 @@
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/kdev_t.h>
+#include <linux/slab.h>
 
 #include "gpiolib.h"
 
+struct gpiod_data {
+	struct gpio_desc *desc;
+};
+
 static DEFINE_IDR(dirent_idr);
 
 
@@ -41,7 +46,8 @@ static DEFINE_MUTEX(sysfs_lock);
 static ssize_t direction_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -58,7 +64,8 @@ static ssize_t direction_show(struct device *dev,
 static ssize_t direction_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -80,7 +87,8 @@ static DEVICE_ATTR_RW(direction);
 static ssize_t value_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -94,7 +102,8 @@ static ssize_t value_show(struct device *dev,
 static ssize_t value_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -210,7 +219,8 @@ static const struct {
 static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	unsigned long mask;
 	ssize_t	status = 0;
 	int i;
@@ -232,7 +242,8 @@ static ssize_t edge_show(struct device *dev,
 static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 	int			i;
 
@@ -282,7 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 static ssize_t active_low_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
-	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
 	mutex_lock(&sysfs_lock);
@@ -298,7 +310,8 @@ static ssize_t active_low_show(struct device *dev,
 static ssize_t active_low_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 	long			value;
 
@@ -318,7 +331,8 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 			       int n)
 {
 	struct device *dev = container_of(kobj, struct device, kobj);
-	struct gpio_desc *desc = dev_get_drvdata(dev);
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
 	umode_t mode = attr->mode;
 	bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
 
@@ -510,6 +524,7 @@ static struct class gpio_class = {
 int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 {
 	struct gpio_chip	*chip;
+	struct gpiod_data	*data;
 	unsigned long		flags;
 	int			status;
 	const char		*ioname = NULL;
@@ -534,7 +549,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	/* check if chip is being removed */
 	if (!chip || !chip->cdev) {
 		status = -ENODEV;
-		goto fail_unlock;
+		goto err_unlock;
 	}
 
 	spin_lock_irqsave(&gpio_lock, flags);
@@ -546,7 +561,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 				test_bit(FLAG_REQUESTED, &desc->flags),
 				test_bit(FLAG_EXPORT, &desc->flags));
 		status = -EPERM;
-		goto fail_unlock;
+		goto err_unlock;
 	}
 
 	if (chip->direction_input && chip->direction_output &&
@@ -556,33 +571,45 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		status = -ENOMEM;
+		goto err_unlock;
+	}
+
+	data->desc = desc;
+
 	offset = gpio_chip_hwgpio(desc);
 	if (chip->names && chip->names[offset])
 		ioname = chip->names[offset];
 
 	dev = device_create_with_groups(&gpio_class, chip->dev,
-					MKDEV(0, 0), desc, gpio_groups,
+					MKDEV(0, 0), data, gpio_groups,
 					ioname ? ioname : "gpio%u",
 					desc_to_gpio(desc));
 	if (IS_ERR(dev)) {
 		status = PTR_ERR(dev);
-		goto fail_unlock;
+		goto err_free_data;
 	}
 
 	set_bit(FLAG_EXPORT, &desc->flags);
 	mutex_unlock(&sysfs_lock);
 	return 0;
 
-fail_unlock:
+err_free_data:
+	kfree(data);
+err_unlock:
 	mutex_unlock(&sysfs_lock);
 	gpiod_dbg(desc, "%s: status %d\n", __func__, status);
 	return status;
 }
 EXPORT_SYMBOL_GPL(gpiod_export);
 
-static int match_export(struct device *dev, const void *data)
+static int match_export(struct device *dev, const void *desc)
 {
-	return dev_get_drvdata(dev) == data;
+	struct gpiod_data *data = dev_get_drvdata(dev);
+
+	return data->desc == desc;
 }
 
 /**
@@ -638,6 +665,7 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
  */
 void gpiod_unexport(struct gpio_desc *desc)
 {
+	struct gpiod_data	*data;
 	int			status = 0;
 	struct device		*dev = NULL;
 
@@ -661,6 +689,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 	mutex_unlock(&sysfs_lock);
 
 	if (dev) {
+		data = dev_get_drvdata(dev);
 		device_unregister(dev);
 		/*
 		 * Release irq after deregistration to prevent race with
@@ -668,6 +697,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 */
 		gpio_setup_irq(desc, dev, 0);
 		put_device(dev);
+		kfree(data);
 	}
 
 	if (status)
-- 
2.0.5

[PATCH 13/23] gpio: sysfs: remove redundant gpio-descriptor parameters

[Johan Hovold]

Remove redundant gpio-descriptor parameters from sysfs_set_active_low and
gpio_setup_irq.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 4cc90f7ce14b..bc4f192c0aa3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -133,9 +133,10 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
-static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
-		unsigned long gpio_flags)
+static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 {
+	struct gpiod_data	*data = dev_get_drvdata(dev);
+	struct gpio_desc	*desc = data->desc;
 	struct kernfs_node	*value_sd;
 	unsigned long		irq_flags;
 	int			ret, irq, id;
@@ -242,8 +243,6 @@ static ssize_t edge_show(struct device *dev,
 static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 	int			i;
 
@@ -255,7 +254,7 @@ static ssize_t edge_store(struct device *dev,
 found:
 	mutex_lock(&sysfs_lock);
 
-	status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+	status = gpio_setup_irq(dev, trigger_types[i].flags);
 	if (!status)
 		status = size;
 
@@ -265,9 +264,10 @@ found:
 }
 static DEVICE_ATTR_RW(edge);
 
-static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
-				int value)
+static int sysfs_set_active_low(struct device *dev, int value)
 {
+	struct gpiod_data	*data = dev_get_drvdata(dev);
+	struct gpio_desc	*desc = data->desc;
 	int			status = 0;
 
 	if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
@@ -283,8 +283,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
 				!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
 		unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
 
-		gpio_setup_irq(desc, dev, 0);
-		status = gpio_setup_irq(desc, dev, trigger_flags);
+		gpio_setup_irq(dev, 0);
+		status = gpio_setup_irq(dev, trigger_flags);
 	}
 
 	return status;
@@ -310,8 +310,6 @@ static ssize_t active_low_show(struct device *dev,
 static ssize_t active_low_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
-	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 	long			value;
 
@@ -319,7 +317,7 @@ static ssize_t active_low_store(struct device *dev,
 
 	status = kstrtol(buf, 0, &value);
 	if (status == 0)
-		status = sysfs_set_active_low(desc, dev, value != 0);
+		status = sysfs_set_active_low(dev, value != 0);
 
 	mutex_unlock(&sysfs_lock);
 
@@ -695,7 +693,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 * Release irq after deregistration to prevent race with
 		 * edge_store.
 		 */
-		gpio_setup_irq(desc, dev, 0);
+		gpio_setup_irq(dev, 0);
 		put_device(dev);
 		kfree(data);
 	}
-- 
2.0.5

[PATCH 14/23] gpio: sysfs: clean up interrupt-interface implementation

[Johan Hovold]

Store the value sysfs entry in the gpiod data rather than in a global
table accessed through an index stored in the overloaded gpio-descriptor
flag field.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 54 +++++++++++++++-----------------------------
 drivers/gpio/gpiolib.h       |  3 ---
 2 files changed, 18 insertions(+), 39 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index bc4f192c0aa3..8041c1728231 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -12,11 +12,9 @@
 
 struct gpiod_data {
 	struct gpio_desc *desc;
+	struct kernfs_node *value_kn;
 };
 
-static DEFINE_IDR(dirent_idr);
-
-
 /* lock protects against unexport_gpio() being called while
  * sysfs files are active.
  */
@@ -127,9 +125,10 @@ static DEVICE_ATTR_RW(value);
 
 static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 {
-	struct kernfs_node	*value_sd = priv;
+	struct gpiod_data *data = priv;
+
+	sysfs_notify_dirent(data->value_kn);
 
-	sysfs_notify_dirent(value_sd);
 	return IRQ_HANDLED;
 }
 
@@ -137,9 +136,8 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
 	struct gpio_desc	*desc = data->desc;
-	struct kernfs_node	*value_sd;
 	unsigned long		irq_flags;
-	int			ret, irq, id;
+	int			ret, irq;
 
 	if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
 		return 0;
@@ -148,16 +146,14 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 	if (irq < 0)
 		return -EIO;
 
-	id = desc->flags >> ID_SHIFT;
-	value_sd = idr_find(&dirent_idr, id);
-	if (value_sd)
-		free_irq(irq, value_sd);
+	if (data->value_kn)
+		free_irq(irq, data);
 
 	desc->flags &= ~GPIO_TRIGGER_MASK;
 
 	if (!gpio_flags) {
 		ret = 0;
-		goto free_id;
+		goto free_kn;
 	}
 
 	irq_flags = IRQF_SHARED;
@@ -168,41 +164,27 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
 
-	if (!value_sd) {
-		value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
-		if (!value_sd) {
+	if (!data->value_kn) {
+		data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+		if (!data->value_kn) {
 			ret = -ENODEV;
 			goto err_out;
 		}
-
-		ret = idr_alloc(&dirent_idr, value_sd, 1, 0, GFP_KERNEL);
-		if (ret < 0)
-			goto free_sd;
-		id = ret;
-
-		desc->flags &= GPIO_FLAGS_MASK;
-		desc->flags |= (unsigned long)id << ID_SHIFT;
-
-		if (desc->flags >> ID_SHIFT != id) {
-			ret = -ERANGE;
-			goto free_id;
-		}
 	}
 
 	ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
-				"gpiolib", value_sd);
+				"gpiolib", data);
 	if (ret < 0)
-		goto free_id;
+		goto free_kn;
 
 	desc->flags |= gpio_flags;
 	return 0;
 
-free_id:
-	idr_remove(&dirent_idr, id);
-	desc->flags &= GPIO_FLAGS_MASK;
-free_sd:
-	if (value_sd)
-		sysfs_put(value_sd);
+free_kn:
+	if (data->value_kn) {
+		sysfs_put(data->value_kn);
+		data->value_kn = NULL;
+	}
 err_out:
 	return ret;
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 69458a022c90..4deb71d2bd01 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -90,9 +90,6 @@ struct gpio_desc {
 #define FLAG_SYSFS_DIR	10	/* show sysfs direction attribute */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 
-#define ID_SHIFT	16	/* add new flags before this one */
-
-#define GPIO_FLAGS_MASK		((1 << ID_SHIFT) - 1)
 #define GPIO_TRIGGER_MASK	(BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
 
 	const char		*label;
-- 
2.0.5

[PATCH 15/23] gpio: sysfs: only call irq helper if needed

[Johan Hovold]

Only call irq helper if actually reconfiguring interrupt state.

This is a preparatory step in introducing separate gpio-irq request and
free functions.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 8041c1728231..bd22de806182 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,9 +139,6 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 	unsigned long		irq_flags;
 	int			ret, irq;
 
-	if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
-		return 0;
-
 	irq = gpiod_to_irq(desc);
 	if (irq < 0)
 		return -EIO;
@@ -225,6 +222,9 @@ static ssize_t edge_show(struct device *dev,
 static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
+	unsigned long flags;
 	ssize_t			status;
 	int			i;
 
@@ -234,12 +234,20 @@ static ssize_t edge_store(struct device *dev,
 	return -EINVAL;
 
 found:
+	flags = trigger_types[i].flags;
+
 	mutex_lock(&sysfs_lock);
 
-	status = gpio_setup_irq(dev, trigger_types[i].flags);
+	if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+		status = size;
+		goto out_unlock;
+	}
+
+	status = gpio_setup_irq(dev, flags);
 	if (!status)
 		status = size;
 
+out_unlock:
 	mutex_unlock(&sysfs_lock);
 
 	return status;
@@ -675,7 +683,8 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 * Release irq after deregistration to prevent race with
 		 * edge_store.
 		 */
-		gpio_setup_irq(dev, 0);
+		if (desc->flags & GPIO_TRIGGER_MASK)
+			gpio_setup_irq(dev, 0);
 		put_device(dev);
 		kfree(data);
 	}
-- 
2.0.5

[PATCH 16/23] gpio: sysfs: split irq allocation and deallocation

[Johan Hovold]

Add separate helper functions for irq request and free.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 72 ++++++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index bd22de806182..323272569292 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -13,6 +13,7 @@
 struct gpiod_data {
 	struct gpio_desc *desc;
 	struct kernfs_node *value_kn;
+	int irq;
 };
 
 /* lock protects against unexport_gpio() being called while
@@ -132,26 +133,20 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
-static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
+static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
 	struct gpio_desc	*desc = data->desc;
 	unsigned long		irq_flags;
-	int			ret, irq;
+	int			ret;
 
-	irq = gpiod_to_irq(desc);
-	if (irq < 0)
+	data->irq = gpiod_to_irq(desc);
+	if (data->irq < 0)
 		return -EIO;
 
-	if (data->value_kn)
-		free_irq(irq, data);
-
-	desc->flags &= ~GPIO_TRIGGER_MASK;
-
-	if (!gpio_flags) {
-		ret = 0;
-		goto free_kn;
-	}
+	data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+	if (!data->value_kn)
+		return -ENODEV;
 
 	irq_flags = IRQF_SHARED;
 	if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
@@ -161,31 +156,31 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
 
-	if (!data->value_kn) {
-		data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
-		if (!data->value_kn) {
-			ret = -ENODEV;
-			goto err_out;
-		}
-	}
-
-	ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
+	ret = request_any_context_irq(data->irq, gpio_sysfs_irq, irq_flags,
 				"gpiolib", data);
 	if (ret < 0)
-		goto free_kn;
+		goto err_put_kn;
 
 	desc->flags |= gpio_flags;
+
 	return 0;
 
-free_kn:
-	if (data->value_kn) {
-		sysfs_put(data->value_kn);
-		data->value_kn = NULL;
-	}
-err_out:
+err_put_kn:
+	sysfs_put(data->value_kn);
+
 	return ret;
 }
 
+static void gpio_sysfs_free_irq(struct device *dev)
+{
+	struct gpiod_data *data = dev_get_drvdata(dev);
+	struct gpio_desc *desc = data->desc;
+
+	desc->flags &= ~GPIO_TRIGGER_MASK;
+	free_irq(data->irq, data);
+	sysfs_put(data->value_kn);
+}
+
 static const struct {
 	const char *name;
 	unsigned long flags;
@@ -225,7 +220,7 @@ static ssize_t edge_store(struct device *dev,
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
 	unsigned long flags;
-	ssize_t			status;
+	ssize_t	status = size;
 	int			i;
 
 	for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
@@ -243,9 +238,14 @@ found:
 		goto out_unlock;
 	}
 
-	status = gpio_setup_irq(dev, flags);
-	if (!status)
-		status = size;
+	if (desc->flags & GPIO_TRIGGER_MASK)
+		gpio_sysfs_free_irq(dev);
+
+	if (flags) {
+		status = gpio_sysfs_request_irq(dev, flags);
+		if (!status)
+			status = size;
+	}
 
 out_unlock:
 	mutex_unlock(&sysfs_lock);
@@ -273,8 +273,8 @@ static int sysfs_set_active_low(struct device *dev, int value)
 				!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
 		unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
 
-		gpio_setup_irq(dev, 0);
-		status = gpio_setup_irq(dev, trigger_flags);
+		gpio_sysfs_free_irq(dev);
+		status = gpio_sysfs_request_irq(dev, trigger_flags);
 	}
 
 	return status;
@@ -684,7 +684,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 * edge_store.
 		 */
 		if (desc->flags & GPIO_TRIGGER_MASK)
-			gpio_setup_irq(dev, 0);
+			gpio_sysfs_free_irq(dev);
 		put_device(dev);
 		kfree(data);
 	}
-- 
2.0.5

[PATCH 17/23] gpio: sysfs: clean up edge_store

[Johan Hovold]

Remove goto from success path.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 323272569292..761e1644cff1 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -221,14 +221,16 @@ static ssize_t edge_store(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	unsigned long flags;
 	ssize_t	status = size;
-	int			i;
+	int i;
 
-	for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
 		if (sysfs_streq(trigger_types[i].name, buf))
-			goto found;
-	return -EINVAL;
+			break;
+	}
+
+	if (i == ARRAY_SIZE(trigger_types))
+		return -EINVAL;
 
-found:
 	flags = trigger_types[i].flags;
 
 	mutex_lock(&sysfs_lock);
-- 
2.0.5

[PATCH 18/23] gpio: sysfs: clean up gpiod_export_link locking

[Johan Hovold]

Drop unnecessary locking from gpiod_export_link. If the class device has
not already been unregistered, class_find_device returns the ref-counted
class device so there's no need for locking.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 761e1644cff1..d9265b4609a3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -616,34 +616,22 @@ static int match_export(struct device *dev, const void *desc)
 int gpiod_export_link(struct device *dev, const char *name,
 		      struct gpio_desc *desc)
 {
-	int			status = -EINVAL;
+	struct device *cdev;
+	int ret;
 
 	if (!desc) {
 		pr_warn("%s: invalid GPIO\n", __func__);
 		return -EINVAL;
 	}
 
-	mutex_lock(&sysfs_lock);
-
-	if (test_bit(FLAG_EXPORT, &desc->flags)) {
-		struct device *tdev;
-
-		tdev = class_find_device(&gpio_class, NULL, desc, match_export);
-		if (tdev != NULL) {
-			status = sysfs_create_link(&dev->kobj, &tdev->kobj,
-						name);
-			put_device(tdev);
-		} else {
-			status = -ENODEV;
-		}
-	}
-
-	mutex_unlock(&sysfs_lock);
+	cdev = class_find_device(&gpio_class, NULL, desc, match_export);
+	if (!cdev)
+		return -ENODEV;
 
-	if (status)
-		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+	ret = sysfs_create_link(&dev->kobj, &cdev->kobj, name);
+	put_device(cdev);
 
-	return status;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(gpiod_export_link);
 
-- 
2.0.5

[PATCH 19/23] gpio: sysfs: use per-gpio locking

[Johan Hovold]

Add a per-gpio mutex to serialise attribute operations rather than use
one global mutex for all gpios and chips.

Having a single global lock for all gpios in a system adds unnecessary
latency to the sysfs interface, and especially when having gpio
controllers connected over slow buses.

Now that the global gpio-sysfs interrupt table is gone and with per-gpio
data in place, we can easily switch to using a more fine-grained locking
scheme.

Keep the global mutex to serialise the global (class) operations of gpio
export and unexport and chip removal.

Also document the locking assumptions made.

Note that this is also needed to fix a race between gpiod_export and
gpiod_unexport.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 52 +++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d9265b4609a3..19c4351021d9 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -12,12 +12,15 @@
 
 struct gpiod_data {
 	struct gpio_desc *desc;
+
+	struct mutex mutex;
 	struct kernfs_node *value_kn;
 	int irq;
 };
 
-/* lock protects against unexport_gpio() being called while
- * sysfs files are active.
+/*
+ * Lock to serialise gpiod export and unexport, and prevent re-export of
+ * gpiod whose chip is being unregistered.
  */
 static DEFINE_MUTEX(sysfs_lock);
 
@@ -49,14 +52,15 @@ static ssize_t direction_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	gpiod_get_direction(desc);
 	status = sprintf(buf, "%s\n",
 			test_bit(FLAG_IS_OUT, &desc->flags)
 				? "out" : "in");
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status;
 }
 
@@ -67,7 +71,7 @@ static ssize_t direction_store(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	if (sysfs_streq(buf, "high"))
 		status = gpiod_direction_output_raw(desc, 1);
@@ -78,7 +82,8 @@ static ssize_t direction_store(struct device *dev,
 	else
 		status = -EINVAL;
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status ? : size;
 }
 static DEVICE_ATTR_RW(direction);
@@ -90,11 +95,12 @@ static ssize_t value_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status;
 }
 
@@ -105,7 +111,7 @@ static ssize_t value_store(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
 		status = -EPERM;
@@ -119,7 +125,8 @@ static ssize_t value_store(struct device *dev,
 		}
 	}
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status;
 }
 static DEVICE_ATTR_RW(value);
@@ -133,6 +140,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
+/* Caller holds gpiod-data mutex. */
 static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
@@ -171,6 +179,10 @@ err_put_kn:
 	return ret;
 }
 
+/*
+ * Caller holds gpiod-data mutex (unless called after class-device
+ * deregistration).
+ */
 static void gpio_sysfs_free_irq(struct device *dev)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
@@ -200,7 +212,7 @@ static ssize_t edge_show(struct device *dev,
 	ssize_t	status = 0;
 	int i;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
 		mask = desc->flags & GPIO_TRIGGER_MASK;
@@ -210,7 +222,8 @@ static ssize_t edge_show(struct device *dev,
 		}
 	}
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status;
 }
 
@@ -233,7 +246,7 @@ static ssize_t edge_store(struct device *dev,
 
 	flags = trigger_types[i].flags;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
 		status = size;
@@ -250,12 +263,13 @@ static ssize_t edge_store(struct device *dev,
 	}
 
 out_unlock:
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
 
 	return status;
 }
 static DEVICE_ATTR_RW(edge);
 
+/* Caller holds gpiod-data mutex. */
 static int sysfs_set_active_low(struct device *dev, int value)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
@@ -289,12 +303,12 @@ static ssize_t active_low_show(struct device *dev,
 	struct gpio_desc *desc = data->desc;
 	ssize_t			status;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	status = sprintf(buf, "%d\n",
 				!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
 
 	return status;
 }
@@ -302,16 +316,17 @@ static ssize_t active_low_show(struct device *dev,
 static ssize_t active_low_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
+	struct gpiod_data	*data = dev_get_drvdata(dev);
 	ssize_t			status;
 	long			value;
 
-	mutex_lock(&sysfs_lock);
+	mutex_lock(&data->mutex);
 
 	status = kstrtol(buf, 0, &value);
 	if (status == 0)
 		status = sysfs_set_active_low(dev, value != 0);
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
 
 	return status ? : size;
 }
@@ -568,6 +583,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 	}
 
 	data->desc = desc;
+	mutex_init(&data->mutex);
 
 	offset = gpio_chip_hwgpio(desc);
 	if (chip->names && chip->names[offset])
-- 
2.0.5

[PATCH 20/23] gpio: sysfs: fix race between gpiod export and unexport

[Johan Hovold]

Make sure to deregister the class device (and release the irq) while
holding the sysfs lock in gpio_unexport to prevent racing with
gpio_export.

Note that this requires the recently introduced per-gpio locking to
avoid a deadlock with the kernfs active protection when waiting for the
attribute operations to drain during deregistration.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 51 ++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 19c4351021d9..5eee3d0b3a81 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -659,9 +659,8 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
  */
 void gpiod_unexport(struct gpio_desc *desc)
 {
-	struct gpiod_data	*data;
-	int			status = 0;
-	struct device		*dev = NULL;
+	struct gpiod_data *data;
+	struct device *dev;
 
 	if (!desc) {
 		pr_warn("%s: invalid GPIO\n", __func__);
@@ -670,33 +669,35 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 	mutex_lock(&sysfs_lock);
 
-	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		goto err_unlock;
 
-		dev = class_find_device(&gpio_class, NULL, desc, match_export);
-		if (dev) {
-			clear_bit(FLAG_SYSFS_DIR, &desc->flags);
-			clear_bit(FLAG_EXPORT, &desc->flags);
-		} else
-			status = -ENODEV;
-	}
+	dev = class_find_device(&gpio_class, NULL, desc, match_export);
+	if (!dev)
+		goto err_unlock;
+
+	data = dev_get_drvdata(dev);
+
+	clear_bit(FLAG_SYSFS_DIR, &desc->flags);
+	clear_bit(FLAG_EXPORT, &desc->flags);
+
+	device_unregister(dev);
+
+	/*
+	 * Release irq after deregistration to prevent race with edge_store.
+	 */
+	if (desc->flags & GPIO_TRIGGER_MASK)
+		gpio_sysfs_free_irq(dev);
 
 	mutex_unlock(&sysfs_lock);
 
-	if (dev) {
-		data = dev_get_drvdata(dev);
-		device_unregister(dev);
-		/*
-		 * Release irq after deregistration to prevent race with
-		 * edge_store.
-		 */
-		if (desc->flags & GPIO_TRIGGER_MASK)
-			gpio_sysfs_free_irq(dev);
-		put_device(dev);
-		kfree(data);
-	}
+	put_device(dev);
+	kfree(data);
 
-	if (status)
-		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+	return;
+
+err_unlock:
+	mutex_unlock(&sysfs_lock);
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
-- 
2.0.5

[PATCH 21/23] gpio: sysfs: rename active-low helper

[Johan Hovold]

Rename active-low helper using common prefix.

Also remove unnecessary manipulation of value argument.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 5eee3d0b3a81..00609e197f50 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -270,7 +270,7 @@ out_unlock:
 static DEVICE_ATTR_RW(edge);
 
 /* Caller holds gpiod-data mutex. */
-static int sysfs_set_active_low(struct device *dev, int value)
+static int gpio_sysfs_set_active_low(struct device *dev, int value)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
 	struct gpio_desc	*desc = data->desc;
@@ -324,7 +324,7 @@ static ssize_t active_low_store(struct device *dev,
 
 	status = kstrtol(buf, 0, &value);
 	if (status == 0)
-		status = sysfs_set_active_low(dev, value != 0);
+		status = gpio_sysfs_set_active_low(dev, value);
 
 	mutex_unlock(&data->mutex);
 
-- 
2.0.5

[PATCH 22/23] gpio: sysfs: remove FLAG_SYSFS_DIR

[Johan Hovold]

Remove FLAG_SYSFS_DIR, which is sysfs-interface specific, and store it
in the class-device data instead.

Note that the flag is only used during export.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 15 +++++++--------
 drivers/gpio/gpiolib.h       |  1 -
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 00609e197f50..27a8c84fc241 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -16,6 +16,8 @@ struct gpiod_data {
 	struct mutex mutex;
 	struct kernfs_node *value_kn;
 	int irq;
+
+	bool direction_can_change;
 };
 
 /*
@@ -339,7 +341,7 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
 	struct gpiod_data *data = dev_get_drvdata(dev);
 	struct gpio_desc *desc = data->desc;
 	umode_t mode = attr->mode;
-	bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
+	bool show_direction = data->direction_can_change;
 
 	if (attr == &dev_attr_direction.attr) {
 		if (!show_direction)
@@ -568,12 +570,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 		status = -EPERM;
 		goto err_unlock;
 	}
-
-	if (chip->direction_input && chip->direction_output &&
-			direction_may_change) {
-		set_bit(FLAG_SYSFS_DIR, &desc->flags);
-	}
-
 	spin_unlock_irqrestore(&gpio_lock, flags);
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -584,6 +580,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
 
 	data->desc = desc;
 	mutex_init(&data->mutex);
+	if (chip->direction_input && chip->direction_output)
+		data->direction_can_change = direction_may_change;
+	else
+		data->direction_can_change = false;
 
 	offset = gpio_chip_hwgpio(desc);
 	if (chip->names && chip->names[offset])
@@ -678,7 +678,6 @@ void gpiod_unexport(struct gpio_desc *desc)
 
 	data = dev_get_drvdata(dev);
 
-	clear_bit(FLAG_SYSFS_DIR, &desc->flags);
 	clear_bit(FLAG_EXPORT, &desc->flags);
 
 	device_unregister(dev);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 4deb71d2bd01..591257a4359a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -87,7 +87,6 @@ struct gpio_desc {
 #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
-#define FLAG_SYSFS_DIR	10	/* show sysfs direction attribute */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 
 #define GPIO_TRIGGER_MASK	(BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
-- 
2.0.5

[PATCH 23/23] gpio: sysfs: move irq trigger flags to class-device data

[Johan Hovold]

Move irq trigger flags, which as sysfs-interface specific, to the class
device data.

This avoids accessing the gpio-descriptor flags field using non-atomic
operations without any locking, and allows for a more clear separation
of the sysfs interface from gpiolib core.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpio/gpiolib-sysfs.c | 48 ++++++++++++++++++++++----------------------
 drivers/gpio/gpiolib.h       |  4 ----
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 27a8c84fc241..8786b8b79ce6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -10,12 +10,18 @@
 
 #include "gpiolib.h"
 
+#define GPIO_IRQF_TRIGGER_FALLING	BIT(0)
+#define GPIO_IRQF_TRIGGER_RISING	BIT(1)
+#define GPIO_IRQF_TRIGGER_BOTH		(GPIO_IRQF_TRIGGER_FALLING | \
+					 GPIO_IRQF_TRIGGER_RISING)
+
 struct gpiod_data {
 	struct gpio_desc *desc;
 
 	struct mutex mutex;
 	struct kernfs_node *value_kn;
 	int irq;
+	unsigned char irq_flags;
 
 	bool direction_can_change;
 };
@@ -143,7 +149,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 }
 
 /* Caller holds gpiod-data mutex. */
-static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
+static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
 {
 	struct gpiod_data	*data = dev_get_drvdata(dev);
 	struct gpio_desc	*desc = data->desc;
@@ -159,10 +165,10 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
 		return -ENODEV;
 
 	irq_flags = IRQF_SHARED;
-	if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
+	if (flags & GPIO_IRQF_TRIGGER_FALLING)
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
-	if (test_bit(FLAG_TRIG_RISE, &gpio_flags))
+	if (flags & GPIO_IRQF_TRIGGER_RISING)
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
 
@@ -171,7 +177,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
 	if (ret < 0)
 		goto err_put_kn;
 
-	desc->flags |= gpio_flags;
+	data->irq_flags = flags;
 
 	return 0;
 
@@ -188,37 +194,33 @@ err_put_kn:
 static void gpio_sysfs_free_irq(struct device *dev)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
 
-	desc->flags &= ~GPIO_TRIGGER_MASK;
+	data->irq_flags = 0;
 	free_irq(data->irq, data);
 	sysfs_put(data->value_kn);
 }
 
 static const struct {
 	const char *name;
-	unsigned long flags;
+	unsigned char flags;
 } trigger_types[] = {
 	{ "none",    0 },
-	{ "falling", BIT(FLAG_TRIG_FALL) },
-	{ "rising",  BIT(FLAG_TRIG_RISE) },
-	{ "both",    BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
+	{ "falling", GPIO_IRQF_TRIGGER_FALLING },
+	{ "rising",  GPIO_IRQF_TRIGGER_RISING },
+	{ "both",    GPIO_IRQF_TRIGGER_BOTH },
 };
 
 static ssize_t edge_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
-	unsigned long mask;
 	ssize_t	status = 0;
 	int i;
 
 	mutex_lock(&data->mutex);
 
 	for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
-		mask = desc->flags & GPIO_TRIGGER_MASK;
-		if (mask == trigger_types[i].flags) {
+		if (data->irq_flags == trigger_types[i].flags) {
 			status = sprintf(buf, "%s\n", trigger_types[i].name);
 			break;
 		}
@@ -233,8 +235,7 @@ static ssize_t edge_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t size)
 {
 	struct gpiod_data *data = dev_get_drvdata(dev);
-	struct gpio_desc *desc = data->desc;
-	unsigned long flags;
+	unsigned char flags;
 	ssize_t	status = size;
 	int i;
 
@@ -250,12 +251,12 @@ static ssize_t edge_store(struct device *dev,
 
 	mutex_lock(&data->mutex);
 
-	if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+	if (flags == data->irq_flags) {
 		status = size;
 		goto out_unlock;
 	}
 
-	if (desc->flags & GPIO_TRIGGER_MASK)
+	if (data->irq_flags)
 		gpio_sysfs_free_irq(dev);
 
 	if (flags) {
@@ -277,6 +278,7 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
 	struct gpiod_data	*data = dev_get_drvdata(dev);
 	struct gpio_desc	*desc = data->desc;
 	int			status = 0;
+	unsigned int		flags = data->irq_flags;
 
 	if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
 		return 0;
@@ -287,12 +289,10 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
 	/* reconfigure poll(2) support if enabled on one edge only */
-	if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
-				!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
-		unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
-
+	if (flags == GPIO_IRQF_TRIGGER_FALLING ||
+					flags == GPIO_IRQF_TRIGGER_RISING) {
 		gpio_sysfs_free_irq(dev);
-		status = gpio_sysfs_request_irq(dev, trigger_flags);
+		status = gpio_sysfs_request_irq(dev, flags);
 	}
 
 	return status;
@@ -685,7 +685,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 	/*
 	 * Release irq after deregistration to prevent race with edge_store.
 	 */
-	if (desc->flags & GPIO_TRIGGER_MASK)
+	if (data->irq_flags)
 		gpio_sysfs_free_irq(dev);
 
 	mutex_unlock(&sysfs_lock);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 591257a4359a..e048be7d1f5e 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -81,16 +81,12 @@ struct gpio_desc {
 #define FLAG_IS_OUT	1
 #define FLAG_EXPORT	2	/* protected by sysfs_lock */
 #define FLAG_SYSFS	3	/* exported via /sys/class/gpio/control */
-#define FLAG_TRIG_FALL	4	/* trigger on falling edge */
-#define FLAG_TRIG_RISE	5	/* trigger on rising edge */
 #define FLAG_ACTIVE_LOW	6	/* value has active low */
 #define FLAG_OPEN_DRAIN	7	/* Gpio is open drain type */
 #define FLAG_OPEN_SOURCE 8	/* Gpio is open source type */
 #define FLAG_USED_AS_IRQ 9	/* GPIO is connected to an IRQ */
 #define FLAG_IS_HOGGED	11	/* GPIO is hogged */
 
-#define GPIO_TRIGGER_MASK	(BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
-
 	const char		*label;
 };
 
-- 
2.0.5

Re: [PATCH 08/23] gpio: remove gpiod_sysfs_set_active_low

[Alexandre Courbot]

On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> Remove gpiod_sysfs_set_active_low (and gpio_sysfs_set_active_low) which
> allowed code to change the polarity of a gpio line even after it had
> been exported through sysfs.
>
> Drivers should not care, and generally does not know, about gpio-line
> polarity which is a hardware feature that needs to be described by
> firmware.
>
> It is currently possible to define gpio-line polarity in device-tree and
> acpi firmware or using platform data. Userspace can also change the
> polarity through sysfs.
>
> Note that drivers using the legacy gpio interface could still use
> GPIOF_ACTIVE_LOW to change the polarity before exporting the gpio.
>
> There are no in-kernel users of this interface.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Harry Wei <harryxiyou@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@zh-kernel.org
> Cc: linux-arch@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  Documentation/gpio/gpio-legacy.txt |  9 -------
>  Documentation/gpio/sysfs.txt       |  8 -------
>  Documentation/zh_CN/gpio.txt       |  8 -------
>  drivers/gpio/gpiolib-sysfs.c       | 48 ++------------------------------------
>  include/asm-generic/gpio.h         |  5 ----
>  include/linux/gpio.h               |  7 ------
>  include/linux/gpio/consumer.h      |  6 -----
>  7 files changed, 2 insertions(+), 89 deletions(-)
>
> diff --git a/Documentation/gpio/gpio-legacy.txt b/Documentation/gpio/gpio-legacy.txt
> index 6f83fa965b4b..79ab5648d69b 100644
> --- a/Documentation/gpio/gpio-legacy.txt
> +++ b/Documentation/gpio/gpio-legacy.txt
> @@ -751,9 +751,6 @@ requested using gpio_request():
>         int gpio_export_link(struct device *dev, const char *name,
>                 unsigned gpio)
>
> -       /* change the polarity of a GPIO node in sysfs */
> -       int gpio_sysfs_set_active_low(unsigned gpio, int value);
> -
>  After a kernel driver requests a GPIO, it may only be made available in
>  the sysfs interface by gpio_export().  The driver can control whether the
>  signal direction may change.  This helps drivers prevent userspace code
> @@ -767,9 +764,3 @@ After the GPIO has been exported, gpio_export_link() allows creating
>  symlinks from elsewhere in sysfs to the GPIO sysfs node.  Drivers can
>  use this to provide the interface under their own device in sysfs with
>  a descriptive name.
> -
> -Drivers can use gpio_sysfs_set_active_low() to hide GPIO line polarity
> -differences between boards from user space.  This only affects the
> -sysfs interface.  Polarity change can be done both before and after
> -gpio_export(), and previously enabled poll(2) support for either
> -rising or falling edge will be reconfigured to follow this setting.
> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
> index c2c3a97f8ff7..535b6a8a7a7c 100644
> --- a/Documentation/gpio/sysfs.txt
> +++ b/Documentation/gpio/sysfs.txt
> @@ -132,9 +132,6 @@ requested using gpio_request():
>         int gpiod_export_link(struct device *dev, const char *name,
>                       struct gpio_desc *desc);
>
> -       /* change the polarity of a GPIO node in sysfs */
> -       int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
> -
>  After a kernel driver requests a GPIO, it may only be made available in
>  the sysfs interface by gpiod_export(). The driver can control whether the
>  signal direction may change. This helps drivers prevent userspace code
> @@ -148,8 +145,3 @@ After the GPIO has been exported, gpiod_export_link() allows creating
>  symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
>  use this to provide the interface under their own device in sysfs with
>  a descriptive name.
> -
> -Drivers can use gpiod_sysfs_set_active_low() to hide GPIO line polarity
> -differences between boards from user space. Polarity change can be done both
> -before and after gpiod_export(), and previously enabled poll(2) support for
> -either rising or falling edge will be reconfigured to follow this setting.
> diff --git a/Documentation/zh_CN/gpio.txt b/Documentation/zh_CN/gpio.txt
> index d5b8f01833f4..bce972521065 100644
> --- a/Documentation/zh_CN/gpio.txt
> +++ b/Documentation/zh_CN/gpio.txt
> @@ -638,9 +638,6 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
>         int gpio_export_link(struct device *dev, const char *name,
>                 unsigned gpio)
>
> -       /* 改变 sysfs 中的一个 GPIO 节点的极性 */
> -       int gpio_sysfs_set_active_low(unsigned gpio, int value);
> -
>  在一个内核驱动申请一个 GPIO 之后，它可以通过 gpio_export()使其在 sysfs
>  接口中可见。该驱动可以控制信号方向是否可修改。这有助于防止用户空间代码无意间
>  破坏重要的系统状态。
> @@ -651,8 +648,3 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
>  在 GPIO 被导出之后，gpio_export_link()允许在 sysfs 文件系统的任何地方
>  创建一个到这个 GPIO sysfs 节点的符号链接。这样驱动就可以通过一个描述性的
>  名字，在 sysfs 中他们所拥有的设备下提供一个(到这个 GPIO sysfs 节点的)接口。
> -
> -驱动可以使用 gpio_sysfs_set_active_low() 来在用户空间隐藏电路板之间
> -GPIO 线的极性差异。这个仅对 sysfs 接口起作用。极性的改变可以在 gpio_export()
> -前后进行,且之前使能的轮询操作(poll(2))支持(上升或下降沿)将会被重新配置来遵循
> -这个设置。
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 31434c5a90ef..8a95a954f514 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
>                 clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
>
>         /* reconfigure poll(2) support if enabled on one edge only */
> -       if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> -                               !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
> +       if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> +                               !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {

This change seems to be unrelated to this patch...

Otherwise, I agree and good riddance!

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Alexandre Courbot]

On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> Rename the gpio-chip export/unexport functions to the more descriptive
> names gpiochip_register and gpiochip_unregister.

Since these functions are related to sysfs, wouldn't
gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
sounds better to me) be even more descriptive?

The renaming should probably also cover the non-static gpiod_*
functions of gpiolib-sysfs.c which are equally ambiguous. Basically
anything non-static from gpiolib-sysfs.c should have that prefix.

Re: [PATCH 06/23] gpio: sysfs: clean up chip class-device handling

[Alexandre Courbot]

On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> Clean gpio-chip class device registration and deregistration.
>
> The class device is registered when a gpio-chip is added (or from
> gpiolib_sysfs_init post-core init call), and deregistered when the chip
> is removed.
>
> Store the class device in struct gpio_chip directly rather than do a
> class-device lookup on deregistration. This also removes the need for
> the exported flag.
>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/gpio/gpiolib-sysfs.c | 39 +++++++++++++--------------------------
>  include/linux/gpio/driver.h  |  4 ++--
>  2 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index c433f075f349..2f8bdce792f6 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
>         mutex_lock(&sysfs_lock);
>
>         /* check if chip is being removed */
> -       if (!chip || !chip->exported) {
> +       if (!chip || !chip->cdev) {
>                 status = -ENODEV;
>                 goto fail_unlock;
>         }
> @@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
>
>  int gpiochip_export(struct gpio_chip *chip)
>  {
> -       int             status;
>         struct device   *dev;
>
>         /* Many systems register gpio chips for SOC support very early,
> @@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
>                                         chip, gpiochip_groups,
>                                         "gpiochip%d", chip->base);
>         if (IS_ERR(dev))
> -               status = PTR_ERR(dev);
> -       else
> -               status = 0;
> +               return PTR_ERR(dev);
>
>         mutex_lock(&sysfs_lock);
> -       chip->exported = (status == 0);
> +       chip->cdev = dev;
>         mutex_unlock(&sysfs_lock);
>
> -       if (status)
> -               chip_dbg(chip, "%s: status %d\n", __func__, status);
> -
> -       return status;
> +       return 0;
>  }
>
>  void gpiochip_unexport(struct gpio_chip *chip)
>  {
> -       int                     status;
> -       struct device           *dev;
>         struct gpio_desc *desc;
>         unsigned int i;
>
> -       dev = class_find_device(&gpio_class, NULL, chip, match_export);
> -       if (dev) {
> -               put_device(dev);
> -               device_unregister(dev);
> -               /* prevent further gpiod exports */
> -               mutex_lock(&sysfs_lock);
> -               chip->exported = false;
> -               mutex_unlock(&sysfs_lock);
> -               status = 0;
> -       } else
> -               status = -ENODEV;
> +       if (!chip->cdev)
> +               return;
>
> -       if (status)
> -               chip_dbg(chip, "%s: status %d\n", __func__, status);
> +       device_unregister(chip->cdev);
> +
> +       /* prevent further gpiod exports */
> +       mutex_lock(&sysfs_lock);
> +       chip->cdev = NULL;
> +       mutex_unlock(&sysfs_lock);
>
>         /* unregister gpiod class devices owned by sysfs */
>         for (i = 0; i < chip->ngpio; i++) {
> @@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
>          */
>         spin_lock_irqsave(&gpio_lock, flags);
>         list_for_each_entry(chip, &gpio_chips, list) {
> -               if (chip->exported)
> +               if (chip->cdev)
>                         continue;
>
>                 /*
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index f1b36593ec9f..8c26855fc6ec 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -20,6 +20,7 @@ struct seq_file;
>   * struct gpio_chip - abstract a GPIO controller
>   * @label: for diagnostics
>   * @dev: optional device providing the GPIOs
> + * @cdev: class device (may be NULL)

Maybe a comment explaining that this field is non-NULL when a chip is
exported would be useful to understand how it is used in the code?

Re: [PATCH 00/23] gpio: sysfs: fixes and clean ups

[Alexandre Courbot]

On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> These patches fix a number of issues with the gpio sysfs interface,
> including
>
>  - fix memory leaks and crashes on device hotplug
>  - straighten out the convoluted locking
>  - reduce sysfs-interface latencies through more fine-grained locking
>  - more clearly separate the sysfs-interface implementation from gpiolib
>    core
>
> The first patch is marked for stable and could go into 4.1.
>
> Unfortunately we can't just kill the gpio sysfs interface, but these
> patches will make it more manageable and should allow us to implement a
> new user-space interface while maintaining the old one (for a while at
> least) without losing our sanity.
>
> Note that there is still a race between chip remove and gpiod_request (and
> therefore sysfs export), which needs to be fixed separately (for instance as
> part of a generic solution to chip hotplugging).

Thanks a lot for this great series. To my embarrassment I could not
find anything big to say about it. It simply fixes some of the biggest
issues with the sysfs interface and makes things much more
maintainable.

I made a few minor comments, but globally I like this very much.

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

Re: [PATCH 08/23] gpio: remove gpiod_sysfs_set_active_low

[Johan Hovold]

On Mon, Apr 27, 2015 at 12:54:15PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> > Remove gpiod_sysfs_set_active_low (and gpio_sysfs_set_active_low) which
> > allowed code to change the polarity of a gpio line even after it had
> > been exported through sysfs.
> >
> > Drivers should not care, and generally does not know, about gpio-line
> > polarity which is a hardware feature that needs to be described by
> > firmware.
> >
> > It is currently possible to define gpio-line polarity in device-tree and
> > acpi firmware or using platform data. Userspace can also change the
> > polarity through sysfs.
> >
> > Note that drivers using the legacy gpio interface could still use
> > GPIOF_ACTIVE_LOW to change the polarity before exporting the gpio.
> >
> > There are no in-kernel users of this interface.
> >
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Harry Wei <harryxiyou@gmail.com>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: linux-doc@vger.kernel.org
> > Cc: linux-kernel@zh-kernel.org
> > Cc: linux-arch@vger.kernel.org
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---

> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index 31434c5a90ef..8a95a954f514 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
> >                 clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> >
> >         /* reconfigure poll(2) support if enabled on one edge only */
> > -       if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> > -                               !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
> > +       if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> > +                               !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
> 
> This change seems to be unrelated to this patch...

This helper is now only called from the attribute operation and dev
will never be NULL.

On the other hand, it was never called with a NULL argument before this
patch either (the test has always been bogus). Let me know if you prefer
this bit to be a separate patch.

> Otherwise, I agree and good riddance!

Thanks,
Johan

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Johan Hovold]

On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> > Rename the gpio-chip export/unexport functions to the more descriptive
> > names gpiochip_register and gpiochip_unregister.
> 
> Since these functions are related to sysfs, wouldn't
> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> sounds better to me) be even more descriptive?

I'm trying to get rid of the made up notion of "exporting" things. What
we are doing is to register devices with driver core, and that involves
a representation is sysfs.

Eventually, a gpio chip should always be registered with driver core and
this is not directly related to the (by then hopefully legacy)
sysfs-interface.

> The renaming should probably also cover the non-static gpiod_*
> functions of gpiolib-sysfs.c which are equally ambiguous. Basically
> anything non-static from gpiolib-sysfs.c should have that prefix.

This would be a different change, and some of those functions are also
part of the consumer API.

Johan

Re: [PATCH 08/23] gpio: remove gpiod_sysfs_set_active_low

[Alexandre Courbot]

On Mon, Apr 27, 2015 at 5:16 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Apr 27, 2015 at 12:54:15PM +0900, Alexandre Courbot wrote:
>> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
>> > Remove gpiod_sysfs_set_active_low (and gpio_sysfs_set_active_low) which
>> > allowed code to change the polarity of a gpio line even after it had
>> > been exported through sysfs.
>> >
>> > Drivers should not care, and generally does not know, about gpio-line
>> > polarity which is a hardware feature that needs to be described by
>> > firmware.
>> >
>> > It is currently possible to define gpio-line polarity in device-tree and
>> > acpi firmware or using platform data. Userspace can also change the
>> > polarity through sysfs.
>> >
>> > Note that drivers using the legacy gpio interface could still use
>> > GPIOF_ACTIVE_LOW to change the polarity before exporting the gpio.
>> >
>> > There are no in-kernel users of this interface.
>> >
>> > Cc: Jonathan Corbet <corbet@lwn.net>
>> > Cc: Harry Wei <harryxiyou@gmail.com>
>> > Cc: Arnd Bergmann <arnd@arndb.de>
>> > Cc: linux-doc@vger.kernel.org
>> > Cc: linux-kernel@zh-kernel.org
>> > Cc: linux-arch@vger.kernel.org
>> > Signed-off-by: Johan Hovold <johan@kernel.org>
>> > ---
>
>> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> > index 31434c5a90ef..8a95a954f514 100644
>> > --- a/drivers/gpio/gpiolib-sysfs.c
>> > +++ b/drivers/gpio/gpiolib-sysfs.c
>> > @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
>> >                 clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
>> >
>> >         /* reconfigure poll(2) support if enabled on one edge only */
>> > -       if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
>> > -                               !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
>> > +       if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
>> > +                               !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
>>
>> This change seems to be unrelated to this patch...
>
> This helper is now only called from the attribute operation and dev
> will never be NULL.
>
> On the other hand, it was never called with a NULL argument before this
> patch either (the test has always been bogus). Let me know if you prefer
> this bit to be a separate patch.

Nope, after reading your explanation I understand why you want to have
this here. Thanks for clarifying!

Re: [PATCH 06/23] gpio: sysfs: clean up chip class-device handling

[Johan Hovold]

On Mon, Apr 27, 2015 at 12:54:41PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> > Clean gpio-chip class device registration and deregistration.
> >
> > The class device is registered when a gpio-chip is added (or from
> > gpiolib_sysfs_init post-core init call), and deregistered when the chip
> > is removed.
> >
> > Store the class device in struct gpio_chip directly rather than do a
> > class-device lookup on deregistration. This also removes the need for
> > the exported flag.
> >
> > Signed-off-by: Johan Hovold <johan@kernel.org>

> > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> > index f1b36593ec9f..8c26855fc6ec 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -20,6 +20,7 @@ struct seq_file;
> >   * struct gpio_chip - abstract a GPIO controller
> >   * @label: for diagnostics
> >   * @dev: optional device providing the GPIOs
> > + * @cdev: class device (may be NULL)
> 
> Maybe a comment explaining that this field is non-NULL when a chip is
> exported would be useful to understand how it is used in the code?

I've added comments where the field is used. I didn't want to get into
explaining sysfs implementation details in the header file, but the "may
be NULL" is there as a hint to actually look at the code.

And a gpio chip will always be registered with driver core (rather than
"exported" ;) ) until it is removed. [ Currently we also allow for
"late" registration, though. ]

This is related to the issue discussed in my last mail, and again the
plan is to let chip registration be used for more than the sysfs
interface.

Johan

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Alexandre Courbot]

On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
>> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
>> > Rename the gpio-chip export/unexport functions to the more descriptive
>> > names gpiochip_register and gpiochip_unregister.
>>
>> Since these functions are related to sysfs, wouldn't
>> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
>> sounds better to me) be even more descriptive?
>
> I'm trying to get rid of the made up notion of "exporting" things. What
> we are doing is to register devices with driver core, and that involves
> a representation is sysfs.
>
> Eventually, a gpio chip should always be registered with driver core and
> this is not directly related to the (by then hopefully legacy)
> sysfs-interface.

I understand and agree, but even after your patch series, registration
of a gpio chip with the driver core is still dependent on the
CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
and either always register GPIO chips (effectively moving the call to
device_create into gpiolib.c) and only keep the legacy bits in
gpiolib-sysfs.c?

We would then only enable the legacy sysfs interface if
CONFIG_GPIO_SYSFS is set, but the gpiochip nodes would still appear as
long as core sysfs support is compiled in.

>> The renaming should probably also cover the non-static gpiod_*
>> functions of gpiolib-sysfs.c which are equally ambiguous. Basically
>> anything non-static from gpiolib-sysfs.c should have that prefix.
>
> This would be a different change, and some of those functions are also
> part of the consumer API.

That could be another patch. I don't mind if an exported function name
changes for consistency as long as all in-kernel users are updated as
well.

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Johan Hovold]

On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> >> > Rename the gpio-chip export/unexport functions to the more descriptive
> >> > names gpiochip_register and gpiochip_unregister.
> >>
> >> Since these functions are related to sysfs, wouldn't
> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> >> sounds better to me) be even more descriptive?
> >
> > I'm trying to get rid of the made up notion of "exporting" things. What
> > we are doing is to register devices with driver core, and that involves
> > a representation is sysfs.
> >
> > Eventually, a gpio chip should always be registered with driver core and
> > this is not directly related to the (by then hopefully legacy)
> > sysfs-interface.
> 
> I understand and agree, but even after your patch series, registration
> of a gpio chip with the driver core is still dependent on the
> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
> and either always register GPIO chips (effectively moving the call to
> device_create into gpiolib.c) and only keep the legacy bits in
> gpiolib-sysfs.c?

That is the plan yes, but there's only so much I can do in one series.
;) The current crazy sysfs API also prevents the decoupling of the sysfs
interface from chip device registration.

Johan

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Alexandre Courbot]

On Mon, Apr 27, 2015 at 6:05 PM, Johan Hovold <johan@kernel.org> wrote:
> On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
>> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <johan@kernel.org> wrote:
>> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
>> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
>> >> > Rename the gpio-chip export/unexport functions to the more descriptive
>> >> > names gpiochip_register and gpiochip_unregister.
>> >>
>> >> Since these functions are related to sysfs, wouldn't
>> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
>> >> sounds better to me) be even more descriptive?
>> >
>> > I'm trying to get rid of the made up notion of "exporting" things. What
>> > we are doing is to register devices with driver core, and that involves
>> > a representation is sysfs.
>> >
>> > Eventually, a gpio chip should always be registered with driver core and
>> > this is not directly related to the (by then hopefully legacy)
>> > sysfs-interface.
>>
>> I understand and agree, but even after your patch series, registration
>> of a gpio chip with the driver core is still dependent on the
>> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
>> and either always register GPIO chips (effectively moving the call to
>> device_create into gpiolib.c) and only keep the legacy bits in
>> gpiolib-sysfs.c?
>
> That is the plan yes, but there's only so much I can do in one series.
> ;) The current crazy sysfs API also prevents the decoupling of the sysfs
> interface from chip device registration.

Sounds good then. This patch series is great anyway, so if Linus has
nothing against it I hope we can merge it quickly.

Re: [PATCH 07/23] gpio: sysfs: rename gpiochip registration functions

[Johan Hovold]

On Tue, Apr 28, 2015 at 12:27:16PM +0900, Alexandre Courbot wrote:
> On Mon, Apr 27, 2015 at 6:05 PM, Johan Hovold <johan@kernel.org> wrote:
> > On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
> >> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <johan@kernel.org> wrote:
> >> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> >> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <johan@kernel.org> wrote:
> >> >> > Rename the gpio-chip export/unexport functions to the more descriptive
> >> >> > names gpiochip_register and gpiochip_unregister.
> >> >>
> >> >> Since these functions are related to sysfs, wouldn't
> >> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> >> >> sounds better to me) be even more descriptive?
> >> >
> >> > I'm trying to get rid of the made up notion of "exporting" things. What
> >> > we are doing is to register devices with driver core, and that involves
> >> > a representation is sysfs.
> >> >
> >> > Eventually, a gpio chip should always be registered with driver core and
> >> > this is not directly related to the (by then hopefully legacy)
> >> > sysfs-interface.
> >>
> >> I understand and agree, but even after your patch series, registration
> >> of a gpio chip with the driver core is still dependent on the
> >> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
> >> and either always register GPIO chips (effectively moving the call to
> >> device_create into gpiolib.c) and only keep the legacy bits in
> >> gpiolib-sysfs.c?
> >
> > That is the plan yes, but there's only so much I can do in one series.
> > ;) The current crazy sysfs API also prevents the decoupling of the sysfs
> > interface from chip device registration.
> 
> Sounds good then. This patch series is great anyway, so if Linus has
> nothing against it I hope we can merge it quickly.

Thanks for the review.

Johan

Re: [PATCH 01/23] gpio: sysfs: fix memory leaks and device hotplug

[Linus Walleij]

On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <johan@kernel.org> wrote:

> Unregister GPIOs requested through sysfs at chip remove to avoid leaking
> the associated memory and sysfs entries.
>
> The stale sysfs entries prevented the gpio numbers from being exported
> when the gpio range was later reused (e.g. at device reconnect).
>
> This also fixes the related module-reference leak.
>
> Note that kernfs makes sure that any on-going sysfs operations finish
> before the class devices are unregistered and that further accesses
> fail.
>
> The chip exported flag is used to prevent gpiod exports during removal.
> This also makes it harder to trigger, but does not fix, the related race
> between gpiochip_remove and export_store, which is really a race with
> gpiod_request that needs to be addressed separately.
>
> Also note that this would prevent the crashes (e.g. NULL-dereferences)
> at reconnect that affects pre-3.18 kernels, as well as use-after-free on
> operations on open attribute files on pre-3.14 kernels (prior to
> kernfs).
>
> Fixes: d8f388d8dc8d ("gpio: sysfs interface")
> Cc: stable <stable@vger.kernel.org>     # v2.6.27: 01cca93a9491
> Signed-off-by: Johan Hovold <johan@kernel.org>

Patch applied for fixes.

I worry a bit about what userspaces do out there, but they
cannot reasonably have behaviours tied to in-flight removal
of GPIO chips, that would be bizarre.

Yours,
Linus Walleij

Re: [PATCH 03/23] gpio: sysfs: drop redundant lock-as-irq

[Linus Walleij]

On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <johan@kernel.org> wrote:

> Drop redundant lock-as-irq in gpio_setup_irq, which has already been
> handled when requesting and releasing the irq (i.e. in the irq chip
> irq_request_resources and irq_release_resources callbacks).

Well we would hope they all do that. And I hope for the vast majority
that is true, but there is a TODO to go over all gpiochip drivers
(some which are elsewhere in the kernel than drivers/gpio) and
make sure they actually do so.

Right now it's a bit arbitrary if so happens, and in not marked by
the driver as IRQ then this kicks in and provides an additional
protection.

But maybe that's overzealous, what do people say?

Yours,
Linus Walleij

Re: [PATCH 01/23] gpio: sysfs: fix memory leaks and device hotplug

[Johan Hovold]

On Wed, Apr 29, 2015 at 11:44:18PM +0200, Linus Walleij wrote:
> On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > Unregister GPIOs requested through sysfs at chip remove to avoid leaking
> > the associated memory and sysfs entries.
> >
> > The stale sysfs entries prevented the gpio numbers from being exported
> > when the gpio range was later reused (e.g. at device reconnect).
> >
> > This also fixes the related module-reference leak.
> >
> > Note that kernfs makes sure that any on-going sysfs operations finish
> > before the class devices are unregistered and that further accesses
> > fail.
> >
> > The chip exported flag is used to prevent gpiod exports during removal.
> > This also makes it harder to trigger, but does not fix, the related race
> > between gpiochip_remove and export_store, which is really a race with
> > gpiod_request that needs to be addressed separately.
> >
> > Also note that this would prevent the crashes (e.g. NULL-dereferences)
> > at reconnect that affects pre-3.18 kernels, as well as use-after-free on
> > operations on open attribute files on pre-3.14 kernels (prior to
> > kernfs).
> >
> > Fixes: d8f388d8dc8d ("gpio: sysfs interface")
> > Cc: stable <stable@vger.kernel.org>     # v2.6.27: 01cca93a9491
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Patch applied for fixes.
> 
> I worry a bit about what userspaces do out there, but they
> cannot reasonably have behaviours tied to in-flight removal
> of GPIO chips, that would be bizarre.

You shouldn't worry too much; even before this patch userspace would see
an -ENODEV when accessing an open sysfs attribute file of a disconnected
device as kernfs would orphan the file -- only now without the associated
leaks and crashes. ;)

Johan

Re: [PATCH 03/23] gpio: sysfs: drop redundant lock-as-irq

[Johan Hovold]

On Wed, Apr 29, 2015 at 11:48:57PM +0200, Linus Walleij wrote:
> On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <johan@kernel.org> wrote:
> 
> > Drop redundant lock-as-irq in gpio_setup_irq, which has already been
> > handled when requesting and releasing the irq (i.e. in the irq chip
> > irq_request_resources and irq_release_resources callbacks).
> 
> Well we would hope they all do that. And I hope for the vast majority
> that is true, but there is a TODO to go over all gpiochip drivers
> (some which are elsewhere in the kernel than drivers/gpio) and
> make sure they actually do so.
> 
> Right now it's a bit arbitrary if so happens, and in not marked by
> the driver as IRQ then this kicks in and provides an additional
> protection.
> 
> But maybe that's overzealous, what do people say?

No, you're right. The drivers that fail to do this needs to be fixed,
but the "redundant" lock-as-irq in the sysfs interface should not be
removed before that.

I'll respin the series and add it back with a comment explaining why
gpiochip_lock_as_irq is currently called twice.

Thanks,
Johan