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

[PATCH v2 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. [ 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(-)

-- 
2.0.5

[PATCH v2 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. [ 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(-)

-- 
2.0.5

[PATCH v2 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 aeb73ef2955e..9dcd346a20fb 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -308,8 +308,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);
@@ -681,50 +681,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

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

[Alexandre Courbot]

On Tue, May 5, 2015 at 12:10 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. [ 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).

Reiterating my

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

on this very nice series.

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

[Alexandre Courbot]

On Tue, May 5, 2015 at 12:10 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. [ 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).

Reiterating my

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

on this very nice series.

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

[Linus Walleij]

On Mon, May 4, 2015 at 5:10 PM, 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>

GOOD RIDDANCE.

Patch applied.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, May 4, 2015 at 5:10 PM, 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>

GOOD RIDDANCE.

Patch applied.

Yours,
Linus Walleij