gpio: sysfs: fixes and clean ups

PATCH In review Johan Hovold 3 participants 47 messages May 4, 2015 – May 12, 2015

Subsystems (4)

Stable branch Documentation GPIO Generic include/asm header files

lore

Share & tools

lore

Johan Hovold

author

2015-05-04 15:10 UTC #0 lore raw

v2 cover gpio: sysfs: fixes and clean ups

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. [ May already have been applied but not pushed by Linus, but included in v2 for completeness. ]

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

Changes since v1:
- Keep explicit lock-as-irq call in sysfs and add comment that it should be removed once the broken drivers have been fixed (patch 3/23).
- Add comment that the class-device field of struct gpiochip is used by the sysfs interface (patch 6/23).
- Add "sysfs"-infix to gpiochip sysfs registration functions as suggested by Alexandre (patch 7/23).

Johan Hovold (23):
gpio: sysfs: fix memory leaks and device hotplug
gpio: clean up gpiochip_remove
gpio: sysfs: fix redundant lock-as-irq handling
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       | 578 ++++++++++++++++++-------------------
 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, 290 insertions(+), 369 deletions(-)

Signature · 1 line

2.0.5

Johan Hovold

author

2015-05-04 15:10 UTC #1 lore raw

PATCH v2 1/23 gpio: sysfs: fix memory leaks and device hotplug

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 <[email protected]> # v2.6.27: 01cca93a9491

Signed-off-by Johan Hovold <[email protected]>

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

drivers/gpio/gpiolib-sysfs.c +19 -0

index 7722ed53bd65..af3bc7a8033b 100644
@@ -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)

Signature · 1 line

2.0.5

Johan Hovold

author

2015-05-04 15:13 UTC #2 lore raw

PATCH v2 2/23 gpio: clean up gpiochip_remove

In reply to Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib.c +9 -5

index 59eaa23767d8..5a5c208d31c7 100644
@@ -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;
 }

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 07:58 UTC #3 lore raw

Re: [PATCH v2 02/23] gpio: clean up gpiochip_remove

Quoted 4 lines · Johan Hovold

Clean up gpiochip_remove somewhat and only output warning about removing chip with GPIOs requested once.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #4 lore raw

PATCH v2 3/23 gpio: sysfs: fix redundant lock-as-irq handling

In reply to Johan Hovold

Drivers should call gpiochip_lock_as_irq (which prevents the pin direction from being changed) in their irq_request_resources callbacks but some drivers currently fail to do so.

Instead a second, explicit and often redundant call to lock-as-irq is made by the sysfs-interface implementation after an irq has been requested.

Move the explicit call before the irq-request to match the unlock done after the irq is later released. Note that this also fixes an irq leak, should the explicit call ever have failed.

Also add a comment about removing the redundant call once the broken drivers have been fixed.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

drivers/gpio/gpiolib-sysfs.c +15 -7

index af3bc7a8033b..b2b62cc6f9e1 100644
@@ -195,20 +195,28 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 		}
 	}
 
-	ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
-				"gpiolib", value_sd);
+	/*
+	 * FIXME: This should be done in the irq_request_resources callback
+	 *        when the irq is requested, but a few drivers currently fail
+	 *        to do so.
+	 *
+	 *        Remove this redundant call (along with the corresponding
+	 *        unlock) when those drivers have been fixed.
+	 */
+	ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 	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;
-	}
+	ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
+				"gpiolib", value_sd);
+	if (ret < 0)
+		goto err_unlock;
 
 	desc->flags |= gpio_flags;
 	return 0;
 
+err_unlock:
+	gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 free_id:
 	idr_remove(&dirent_idr, id);
 	desc->flags &= GPIO_FLAGS_MASK;

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 07:59 UTC #5 lore raw

Re: [PATCH v2 03/23] gpio: sysfs: fix redundant lock-as-irq handling

Quoted 8 lines · Johan Hovold

Drivers should call gpiochip_lock_as_irq (which prevents the pin direction from being changed) in their irq_request_resources callbacks but some drivers currently fail to do so.
Instead a second, explicit and often redundant call to lock-as-irq is made by the sysfs-interface implementation after an irq has been requested.

Quoted 8 lines

Move the explicit call before the irq-request to match the unlock done after the irq is later released. Note that this also fixes an irq leak, should the explicit call ever have failed.
Also add a comment about removing the redundant call once the broken drivers have been fixed.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #6 lore raw

PATCH v2 4/23 gpio: sysfs: preparatory clean ups

In reply to Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/gpio/gpiolib-sysfs.c +4 -4

index b2b62cc6f9e1..658ed28e6d7d 100644
@@ -599,7 +599,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);
 	}

@@ -607,10 +607,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));

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:00 UTC #7 lore raw

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

Quoted 4 lines · Johan Hovold

Put the recently introduced gpio-chip pointer to some more use in gpiod_export.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #8 lore raw

PATCH v2 5/23 gpio: sysfs: reduce gpiochip-export locking scope

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/gpio/gpiolib-sysfs.c +4 -3

index 658ed28e6d7d..d19bf234e878 100644
@@ -779,7 +779,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);

@@ -787,6 +786,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);

@@ -803,17 +804,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:05 UTC #9 lore raw

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

Quoted 4 lines · 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 <[email protected]>

Patch applied with some manual interventions.
Pls check result!

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #10 lore raw

PATCH v2 6/23 gpio: sysfs: clean up chip class-device handling

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 39 +++++++++++++--------------------------
 include/linux/gpio/driver.h  |  4 ++--
 2 files changed, 15 insertions(+), 28 deletions(-)

drivers/gpio/gpiolib-sysfs.c +13 ~~-26~~

index d19bf234e878..767b79adb9a4 100644
@@ -582,7 +582,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;
 	}

@@ -767,7 +767,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,

@@ -783,41 +782,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++) {

@@ -845,7 +832,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;
 
 		/*

include/linux/gpio/driver.h +2 -2

index f1b36593ec9f..2c1e639f66bd 100644
@@ -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 used by sysfs interface (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
 	/*

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:15 UTC #11 lore raw

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

Quoted 11 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #12 lore raw

PATCH v2 7/23 gpio: sysfs: rename gpiochip registration functions

In reply to Johan Hovold

Rename the gpio-chip export/unexport functions to the more descriptive names gpiochip_sysfs_register and gpiochip_sysfs_unregister.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 16 +++++++++-------
 drivers/gpio/gpiolib.c       |  4 ++--
 drivers/gpio/gpiolib.h       |  8 ++++----
 3 files changed, 15 insertions(+), 13 deletions(-)

drivers/gpio/gpiolib-sysfs.c +9 -7

index 767b79adb9a4..aeb73ef2955e 100644
@@ -765,13 +765,14 @@ void gpiod_unexport(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_unexport);
 
-int gpiochip_export(struct gpio_chip *chip)
+int gpiochip_sysfs_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)

@@ -791,7 +792,7 @@ int gpiochip_export(struct gpio_chip *chip)
 	return 0;
 }
 
-void gpiochip_unexport(struct gpio_chip *chip)
+void gpiochip_sysfs_unregister(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc;
 	unsigned int i;

@@ -836,15 +837,16 @@ static int __init gpiolib_sysfs_init(void)
 			continue;
 
 		/*
-		 * TODO we yield gpio_lock here because gpiochip_export()
-		 * acquires a mutex. This is unsafe and needs to be fixed.
+		 * TODO we yield gpio_lock here because
+		 * gpiochip_sysfs_register() acquires a mutex. This is unsafe
+		 * and needs to be fixed.
 		 *
 		 * Also it would be nice to use gpiochip_find() here so we
 		 * can keep gpio_chips local to gpiolib.c, but the yield of
 		 * gpio_lock prevents us from doing this.
 		 */
 		spin_unlock_irqrestore(&gpio_lock, flags);
-		status = gpiochip_export(chip);
+		status = gpiochip_sysfs_register(chip);
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 	spin_unlock_irqrestore(&gpio_lock, flags);

drivers/gpio/gpiolib.c +2 -2

index 5a5c208d31c7..2ce3df3504e6 100644
@@ -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_sysfs_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_sysfs_unregister(chip);
 
 	gpiochip_irqchip_remove(chip);

drivers/gpio/gpiolib.h +4 -4

index 54bc5ec91398..efdd5ded4965 100644
@@ -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_sysfs_register(struct gpio_chip *chip);
+void gpiochip_sysfs_unregister(struct gpio_chip *chip);
 
 #else
 
-static inline int gpiochip_export(struct gpio_chip *chip)
+static inline int gpiochip_sysfs_register(struct gpio_chip *chip)
 {
 	return 0;
 }
 
-static inline void gpiochip_unexport(struct gpio_chip *chip)
+static inline void gpiochip_sysfs_unregister(struct gpio_chip *chip)
 {
 }

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:24 UTC #13 lore raw

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

Quoted 4 lines · Johan Hovold

Rename the gpio-chip export/unexport functions to the more descriptive names gpiochip_sysfs_register and gpiochip_sysfs_unregister.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:10 UTC #14 lore raw

PATCH v2 8/23 gpio: remove gpiod_sysfs_set_active_low

In reply to 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 <[email protected]>

Cc Harry Wei <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc [email protected]

Signed-off-by Johan Hovold <[email protected]>

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

Documentation/gpio/gpio-legacy.txt +0 -9

index 6f83fa965b4b..79ab5648d69b 100644
@@ -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.

Documentation/gpio/sysfs.txt +0 -8

index c2c3a97f8ff7..535b6a8a7a7c 100644
@@ -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.

Documentation/zh_CN/gpio.txt +0 -8

index d5b8f01833f4..bce972521065 100644
@@ -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))支持(上升或下降沿)将会被重新配置来遵循
-这个设置。

drivers/gpio/gpiolib-sysfs.c +2 ~~-46~~

60 lines not on this page · raw message

include/asm-generic/gpio.h +0 -5

index 9bb0d11729c9..40ec1433f05d 100644
@@ -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));

include/linux/gpio.h +0 -7

index ab81339a8590..d12b5d566e4b 100644
@@ -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 */

include/linux/gpio/consumer.h +0 -6

index 3a7c9ffd5ab9..09a7fb0062a6 100644
@@ -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)
 {
 }

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:27 UTC #15 lore raw

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

Quoted 17 lines · 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.

Quoted 7 lines

Cc Jonathan Corbet <[email protected]>
Cc Harry Wei <[email protected]>
Cc Arnd Bergmann <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]
Signed-off-by Johan Hovold <[email protected]>

GOOD RIDDANCE.

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #16 lore raw

PATCH v2 9/23 gpio: sysfs: use DEVICE_ATTR macros

In reply to Johan Hovold

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

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

drivers/gpio/gpiolib-sysfs.c +18 ~~-25~~

index 9dcd346a20fb..a78dabd4035b 100644
@@ -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)
 {

@@ -237,7 +233,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);

@@ -264,7 +260,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);

@@ -291,8 +287,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)

@@ -319,7 +314,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);

@@ -338,7 +333,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);

@@ -360,9 +355,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)

@@ -410,32 +403,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,

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:28 UTC #17 lore raw

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

Quoted 4 lines · Johan Hovold

Use DEVICE_ATTR_RO and DEVICE_ATTR_RW rather than specifying masks and callbacks directly.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #18 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/gpio/gpiolib-sysfs.c +5 -1

index a78dabd4035b..e4b079eec9bc 100644
@@ -695,7 +695,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

@@ -706,6 +705,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);
 	}

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:29 UTC #19 lore raw

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

Quoted 7 lines · 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.

Quoted 5 lines

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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:10 UTC #20 lore raw

PATCH v2 11/23 gpio: sysfs: remove redundant export tests

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 74 ++++++++++++++------------------------------
 1 file changed, 23 insertions(+), 51 deletions(-)

drivers/gpio/gpiolib-sysfs.c +23 ~~-51~~

index e4b079eec9bc..10baf31efe74 100644
@@ -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);

@@ -237,23 +226,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);

@@ -275,13 +259,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);

@@ -322,10 +302,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);

@@ -338,18 +315,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:30 UTC #21 lore raw

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

Quoted 7 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #22 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 62 ++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 16 deletions(-)

drivers/gpio/gpiolib-sysfs.c +46 ~~-16~~

index 10baf31efe74..097e7e539c9d 100644
@@ -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);

@@ -225,7 +234,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;

@@ -247,7 +257,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;

@@ -297,7 +308,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);

@@ -313,7 +325,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;

@@ -333,7 +346,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);

@@ -525,6 +539,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;

@@ -549,7 +564,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);

@@ -561,7 +576,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 &&

@@ -571,33 +586,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;
 }
 
 /**

@@ -653,6 +680,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;

@@ -676,6 +704,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

@@ -683,6 +712,7 @@ void gpiod_unexport(struct gpio_desc *desc)
 		 */
 		gpio_setup_irq(desc, dev, 0);
 		put_device(dev);
+		kfree(data);
 	}
 
 	if (status)

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:31 UTC #23 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #24 lore raw

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

In reply to Johan Hovold

Remove redundant gpio-descriptor parameters from sysfs_set_active_low and gpio_setup_irq.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

drivers/gpio/gpiolib-sysfs.c +11 ~~-13~~

index 097e7e539c9d..0bc959fbcf23 100644
@@ -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;

@@ -257,8 +258,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;

@@ -270,7 +269,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;

@@ -280,9 +279,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)

@@ -298,8 +298,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;

@@ -325,8 +325,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;

@@ -334,7 +332,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);

@@ -710,7 +708,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);
 	}

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:32 UTC #25 lore raw

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

Quoted 4 lines · Johan Hovold

Remove redundant gpio-descriptor parameters from sysfs_set_active_low and gpio_setup_irq.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #26 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 54 +++++++++++++++-----------------------------
 drivers/gpio/gpiolib.h       |  3 ---
 2 files changed, 18 insertions(+), 39 deletions(-)

drivers/gpio/gpiolib-sysfs.c +18 ~~-36~~

index 0bc959fbcf23..bccba406fc22 100644
@@ -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,17 +146,15 @@ 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) {
 		gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 		ret = 0;
-		goto free_id;
+		goto free_kn;
 	}
 
 	irq_flags = IRQF_SHARED;

@@ -169,25 +165,12 @@ 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;
-		}
 	}
 
 	/*

@@ -200,10 +183,10 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 	 */
 	ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 	if (ret < 0)
-		goto free_id;
+		goto free_kn;
 
 	ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
-				"gpiolib", value_sd);
+				"gpiolib", data);
 	if (ret < 0)
 		goto err_unlock;

@@ -212,12 +195,11 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 
 err_unlock:
 	gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-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;
 }

drivers/gpio/gpiolib.h +0 -3

index efdd5ded4965..4a857da698b2 100644
@@ -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;

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:34 UTC #27 lore raw

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

Quoted 5 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:10 UTC #28 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib-sysfs.c +14 -5

index bccba406fc22..201b757ad0db 100644
@@ -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;

@@ -240,6 +237,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;

@@ -249,12 +249,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;

@@ -690,7 +698,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);
 	}

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:35 UTC #29 lore raw

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

Quoted 6 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #30 lore raw

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

In reply to Johan Hovold

Add separate helper functions for irq request and free.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 74 ++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

drivers/gpio/gpiolib-sysfs.c +37 ~~-37~~

index 201b757ad0db..d9b3faa01fee 100644
@@ -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,27 +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) {
-		gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-		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))

@@ -162,14 +156,6 @@ 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;
-		}
-	}
-
 	/*
 	 * FIXME: This should be done in the irq_request_resources callback
 	 *        when the irq is requested, but a few drivers currently fail

@@ -180,27 +166,36 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
 	 */
 	ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 	if (ret < 0)
-		goto free_kn;
+		goto err_put_kn;
 
-	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 err_unlock;
 
 	desc->flags |= gpio_flags;
+
 	return 0;
 
 err_unlock:
 	gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-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);
+	gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
+	sysfs_put(data->value_kn);
+}
+
 static const struct {
 	const char *name;
 	unsigned long flags;

@@ -240,7 +235,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++)

@@ -258,9 +253,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);

@@ -288,8 +288,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;

@@ -699,7 +699,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);
 	}

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:36 UTC #31 lore raw

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

Quoted 3 lines · Johan Hovold

Add separate helper functions for irq request and free.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:10 UTC #32 lore raw

PATCH v2 17/23 gpio: sysfs: clean up edge_store

In reply to Johan Hovold

Remove goto from success path.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/gpio/gpiolib-sysfs.c +7 -5

index d9b3faa01fee..1161a46618dd 100644
@@ -236,14 +236,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:37 UTC #33 lore raw

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

Quoted 3 lines · Johan Hovold

Remove goto from success path.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #34 lore raw

PATCH v2 18/23 gpio: sysfs: clean up gpiod_export_link locking

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

drivers/gpio/gpiolib-sysfs.c +8 ~~-20~~

index 1161a46618dd..682e4d34999c 100644
@@ -631,34 +631,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:38 UTC #35 lore raw

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

Quoted 5 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #36 lore raw

PATCH v2 19/23 gpio: sysfs: use per-gpio locking

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 52 +++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 18 deletions(-)

drivers/gpio/gpiolib-sysfs.c +34 ~~-18~~

index 682e4d34999c..1bb05aa33a84 100644
@@ -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);

@@ -185,6 +193,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);

@@ -215,7 +227,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;

@@ -225,7 +237,8 @@ static ssize_t edge_show(struct device *dev,
 		}
 	}
 
-	mutex_unlock(&sysfs_lock);
+	mutex_unlock(&data->mutex);
+
 	return status;
 }

@@ -248,7 +261,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;

@@ -265,12 +278,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);

@@ -304,12 +318,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;
 }

@@ -317,16 +331,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;
 }

@@ -583,6 +598,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])

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:39 UTC #37 lore raw

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

Quoted 14 lines · 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.

Quoted 6 lines

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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:10 UTC #38 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 51 ++++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

drivers/gpio/gpiolib-sysfs.c +26 ~~-25~~

index 1bb05aa33a84..1540f7d60f06 100644
@@ -674,9 +674,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__);

@@ -685,33 +684,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:42 UTC #39 lore raw

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

Quoted 9 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #40 lore raw

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

In reply to Johan Hovold

Rename active-low helper using common prefix.

Also remove unnecessary manipulation of value argument.

Signed-off-by Johan Hovold <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpio/gpiolib-sysfs.c +2 -2

index 1540f7d60f06..06372c7c822c 100644
@@ -285,7 +285,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;

@@ -339,7 +339,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);

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:43 UTC #41 lore raw

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

Quoted 5 lines · Johan Hovold

Rename active-low helper using common prefix.
Also remove unnecessary manipulation of value argument.
Signed-off-by Johan Hovold <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:13 UTC #42 lore raw

PATCH v2 22/23 gpio: sysfs: remove FLAG_SYSFS_DIR

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 15 +++++++--------
 drivers/gpio/gpiolib.h       |  1 -
 2 files changed, 7 insertions(+), 9 deletions(-)

drivers/gpio/gpiolib-sysfs.c +7 -8

index 06372c7c822c..9047b2d556a0 100644
@@ -16,6 +16,8 @@ struct gpiod_data {
 	struct mutex mutex;
 	struct kernfs_node *value_kn;
 	int irq;
+
+	bool direction_can_change;
 };
 
 /*

@@ -354,7 +356,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)

@@ -583,12 +585,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);

@@ -599,6 +595,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])

@@ -693,7 +693,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);

drivers/gpio/gpiolib.h +0 -1

index 4a857da698b2..34eea33e482c 100644
@@ -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))

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:44 UTC #43 lore raw

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

Quoted 6 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Johan Hovold

author

2015-05-04 15:14 UTC #44 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/gpio/gpiolib-sysfs.c | 47 ++++++++++++++++++++++----------------------
 drivers/gpio/gpiolib.h       |  4 ----
 2 files changed, 24 insertions(+), 27 deletions(-)

drivers/gpio/gpiolib-sysfs.c +24 ~~-23~~

index 9047b2d556a0..b57ed8e55ab5 100644
@@ -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;

@@ -183,7 +189,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
 	if (ret < 0)
 		goto err_unlock;
 
-	desc->flags |= gpio_flags;
+	data->irq_flags = flags;
 
 	return 0;

@@ -204,7 +210,7 @@ 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);
 	gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
 	sysfs_put(data->value_kn);

@@ -212,28 +218,25 @@ static void gpio_sysfs_free_irq(struct device *dev)
 
 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;
 		}

@@ -248,8 +251,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;

@@ -265,12 +267,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) {

@@ -292,6 +294,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;

@@ -302,12 +305,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;

@@ -700,7 +701,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);

drivers/gpio/gpiolib.h +0 -4

index 34eea33e482c..42c2ede96545 100644
@@ -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;
 };

Signature · 1 line

2.0.5

Linus Walleij

2015-05-12 08:45 UTC #45 lore raw

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

Quoted 8 lines · 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 <[email protected]>

Patch applied.

Yours,
Linus Walleij

Alexandre Courbot

Reviewed-by on the v2 series

2015-05-12 06:37 UTC #46 lore raw

In reply to Johan Hovold

Quoted 13 lines · 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. [ May already have been applied but not pushed by Linus, but included in v2 for completeness. ]

Quoted 8 lines

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

Reiterating my

Reviewed-by Alexandre Courbot <[email protected]>

on this very nice series.