These patches fix a number of issues with the gpio sysfs interface,
including
- fix memory leaks and crashes on device hotplug
- straighten out the convoluted locking
- reduce sysfs-interface latencies through more fine-grained locking
- more clearly separate the sysfs-interface implementation from gpiolib
core
The first patch is marked for stable and could go into 4.1.
Unfortunately we can't just kill the gpio sysfs interface, but these
patches will make it more manageable and should allow us to implement a
new user-space interface while maintaining the old one (for a while at
least) without losing our sanity.
Note that there is still a race between chip remove and gpiod_request (and
therefore sysfs export), which needs to be fixed separately (for instance as
part of a generic solution to chip hotplugging).
Johan
Johan Hovold (23):
gpio: sysfs: fix memory leaks and device hotplug
gpio: clean up gpiochip_remove
gpio: sysfs: drop redundant lock-as-irq
gpio: sysfs: preparatory clean ups
gpio: sysfs: reduce gpiochip-export locking scope
gpio: sysfs: clean up chip class-device handling
gpio: sysfs: rename gpiochip registration functions
gpio: remove gpiod_sysfs_set_active_low
gpio: sysfs: use DEVICE_ATTR macros
gpio: sysfs: release irq after class-device deregistration
gpio: sysfs: remove redundant export tests
gpio: sysfs: add gpiod class-device data
gpio: sysfs: remove redundant gpio-descriptor parameters
gpio: sysfs: clean up interrupt-interface implementation
gpio: sysfs: only call irq helper if needed
gpio: sysfs: split irq allocation and deallocation
gpio: sysfs: clean up edge_store
gpio: sysfs: clean up gpiod_export_link locking
gpio: sysfs: use per-gpio locking
gpio: sysfs: fix race between gpiod export and unexport
gpio: sysfs: rename active-low helper
gpio: sysfs: remove FLAG_SYSFS_DIR
gpio: sysfs: move irq trigger flags to class-device data
Documentation/gpio/gpio-legacy.txt | 9 -
Documentation/gpio/sysfs.txt | 8 -
Documentation/zh_CN/gpio.txt | 8 -
drivers/gpio/gpiolib-sysfs.c | 561 +++++++++++++++++--------------------
drivers/gpio/gpiolib.c | 18 +-
drivers/gpio/gpiolib.h | 16 +-
include/asm-generic/gpio.h | 5 -
include/linux/gpio.h | 7 -
include/linux/gpio/consumer.h | 6 -
include/linux/gpio/driver.h | 4 +-
10 files changed, 273 insertions(+), 369 deletions(-)
--
2.0.5
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(+)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 7722ed53bd65..af3bc7a8033b 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -551,6 +551,7 @@ static struct class gpio_class = {
*/
int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
{
+ struct gpio_chip *chip;
unsigned long flags;
int status;
const char *ioname = NULL;
@@ -568,8 +569,16 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
return -EINVAL;
}
+ chip = desc->chip;
+
mutex_lock(&sysfs_lock);
+ /* check if chip is being removed */
+ if (!chip || !chip->exported) {
+ status = -ENODEV;
+ goto fail_unlock;
+ }
+
spin_lock_irqsave(&gpio_lock, flags);
if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
test_bit(FLAG_EXPORT, &desc->flags)) {
@@ -783,12 +792,15 @@ void gpiochip_unexport(struct gpio_chip *chip)
{
int status;
struct device *dev;
+ struct gpio_desc *desc;
+ unsigned int i;
mutex_lock(&sysfs_lock);
dev = class_find_device(&gpio_class, NULL, chip, match_export);
if (dev) {
put_device(dev);
device_unregister(dev);
+ /* prevent further gpiod exports */
chip->exported = false;
status = 0;
} else
@@ -797,6 +809,13 @@ void gpiochip_unexport(struct gpio_chip *chip)
if (status)
chip_dbg(chip, "%s: status %d\n", __func__, status);
+
+ /* unregister gpiod class devices owned by sysfs */
+ for (i = 0; i < chip->ngpio; i++) {
+ desc = &chip->desc[i];
+ if (test_and_clear_bit(FLAG_SYSFS, &desc->flags))
+ gpiod_free(desc);
+ }
}
static int __init gpiolib_sysfs_init(void)
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 59eaa23767d8..5a5c208d31c7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -325,8 +325,10 @@ static void gpiochip_free_hogs(struct gpio_chip *chip);
*/
void gpiochip_remove(struct gpio_chip *chip)
{
+ struct gpio_desc *desc;
unsigned long flags;
unsigned id;
+ bool requested = false;
gpiochip_unexport(chip);
@@ -339,15 +341,17 @@ void gpiochip_remove(struct gpio_chip *chip)
spin_lock_irqsave(&gpio_lock, flags);
for (id = 0; id < chip->ngpio; id++) {
- if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags))
- dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
+ desc = &chip->desc[id];
+ desc->chip = NULL;
+ if (test_bit(FLAG_REQUESTED, &desc->flags))
+ requested = true;
}
- for (id = 0; id < chip->ngpio; id++)
- chip->desc[id].chip = NULL;
-
list_del(&chip->list);
spin_unlock_irqrestore(&gpio_lock, flags);
+ if (requested)
+ dev_crit(chip->dev, "REMOVING GPIOCHIP WITH GPIOS STILL REQUESTED\n");
+
kfree(chip->desc);
chip->desc = NULL;
}
--
2.0.5
Drop redundant lock-as-irq in gpio_setup_irq, which has already been
handled when requesting and releasing the irq (i.e. in the irq chip
irq_request_resources and irq_release_resources callbacks).
Note that we would be leaking the irq in the gpiochip_lock_as_irq error
path if the (second) explicit call ever failed.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index af3bc7a8033b..d87f595a02ce 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -161,7 +161,6 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
desc->flags &= ~GPIO_TRIGGER_MASK;
if (!gpio_flags) {
- gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
ret = 0;
goto free_id;
}
@@ -200,12 +199,6 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
if (ret < 0)
goto free_id;
- ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
- if (ret < 0) {
- gpiod_warn(desc, "failed to flag the GPIO for IRQ\n");
- goto free_id;
- }
-
desc->flags |= gpio_flags;
return 0;
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d87f595a02ce..da0300224b4a 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -584,7 +584,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
goto fail_unlock;
}
- if (desc->chip->direction_input && desc->chip->direction_output &&
+ if (chip->direction_input && chip->direction_output &&
direction_may_change) {
set_bit(FLAG_SYSFS_DIR, &desc->flags);
}
@@ -592,10 +592,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
spin_unlock_irqrestore(&gpio_lock, flags);
offset = gpio_chip_hwgpio(desc);
- if (desc->chip->names && desc->chip->names[offset])
- ioname = desc->chip->names[offset];
+ if (chip->names && chip->names[offset])
+ ioname = chip->names[offset];
- dev = device_create_with_groups(&gpio_class, desc->chip->dev,
+ dev = device_create_with_groups(&gpio_class, chip->dev,
MKDEV(0, 0), desc, gpio_groups,
ioname ? ioname : "gpio%u",
desc_to_gpio(desc));
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index da0300224b4a..c433f075f349 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -764,7 +764,6 @@ int gpiochip_export(struct gpio_chip *chip)
return 0;
/* use chip->base for the ID; it's already known to be unique */
- mutex_lock(&sysfs_lock);
dev = device_create_with_groups(&gpio_class, chip->dev, MKDEV(0, 0),
chip, gpiochip_groups,
"gpiochip%d", chip->base);
@@ -772,6 +771,8 @@ int gpiochip_export(struct gpio_chip *chip)
status = PTR_ERR(dev);
else
status = 0;
+
+ mutex_lock(&sysfs_lock);
chip->exported = (status == 0);
mutex_unlock(&sysfs_lock);
@@ -788,17 +789,17 @@ void gpiochip_unexport(struct gpio_chip *chip)
struct gpio_desc *desc;
unsigned int i;
- mutex_lock(&sysfs_lock);
dev = class_find_device(&gpio_class, NULL, chip, match_export);
if (dev) {
put_device(dev);
device_unregister(dev);
/* prevent further gpiod exports */
+ mutex_lock(&sysfs_lock);
chip->exported = false;
+ mutex_unlock(&sysfs_lock);
status = 0;
} else
status = -ENODEV;
- mutex_unlock(&sysfs_lock);
if (status)
chip_dbg(chip, "%s: status %d\n", __func__, status);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c433f075f349..2f8bdce792f6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
mutex_lock(&sysfs_lock);
/* check if chip is being removed */
- if (!chip || !chip->exported) {
+ if (!chip || !chip->cdev) {
status = -ENODEV;
goto fail_unlock;
}
@@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
int gpiochip_export(struct gpio_chip *chip)
{
- int status;
struct device *dev;
/* Many systems register gpio chips for SOC support very early,
@@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
chip, gpiochip_groups,
"gpiochip%d", chip->base);
if (IS_ERR(dev))
- status = PTR_ERR(dev);
- else
- status = 0;
+ return PTR_ERR(dev);
mutex_lock(&sysfs_lock);
- chip->exported = (status == 0);
+ chip->cdev = dev;
mutex_unlock(&sysfs_lock);
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
-
- return status;
+ return 0;
}
void gpiochip_unexport(struct gpio_chip *chip)
{
- int status;
- struct device *dev;
struct gpio_desc *desc;
unsigned int i;
- dev = class_find_device(&gpio_class, NULL, chip, match_export);
- if (dev) {
- put_device(dev);
- device_unregister(dev);
- /* prevent further gpiod exports */
- mutex_lock(&sysfs_lock);
- chip->exported = false;
- mutex_unlock(&sysfs_lock);
- status = 0;
- } else
- status = -ENODEV;
+ if (!chip->cdev)
+ return;
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
+ device_unregister(chip->cdev);
+
+ /* prevent further gpiod exports */
+ mutex_lock(&sysfs_lock);
+ chip->cdev = NULL;
+ mutex_unlock(&sysfs_lock);
/* unregister gpiod class devices owned by sysfs */
for (i = 0; i < chip->ngpio; i++) {
@@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
*/
spin_lock_irqsave(&gpio_lock, flags);
list_for_each_entry(chip, &gpio_chips, list) {
- if (chip->exported)
+ if (chip->cdev)
continue;
/*
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index f1b36593ec9f..8c26855fc6ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -20,6 +20,7 @@ struct seq_file;
* struct gpio_chip - abstract a GPIO controller
* @label: for diagnostics
* @dev: optional device providing the GPIOs
+ * @cdev: class device (may be NULL)
* @owner: helps prevent removal of modules exporting active GPIOs
* @list: links gpio_chips together for traversal
* @request: optional hook for chip-specific activation, such as
@@ -57,7 +58,6 @@ struct seq_file;
* implies that if the chip supports IRQs, these IRQs need to be threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
- * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -74,6 +74,7 @@ struct seq_file;
struct gpio_chip {
const char *label;
struct device *dev;
+ struct device *cdev;
struct module *owner;
struct list_head list;
@@ -109,7 +110,6 @@ struct gpio_chip {
const char *const *names;
bool can_sleep;
bool irq_not_threaded;
- bool exported;
#ifdef CONFIG_GPIOLIB_IRQCHIP
/*
--
2.0.5
Rename the gpio-chip export/unexport functions to the more descriptive
names gpiochip_register and gpiochip_unregister.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 13 +++++++------
drivers/gpio/gpiolib.c | 4 ++--
drivers/gpio/gpiolib.h | 8 ++++----
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 2f8bdce792f6..31434c5a90ef 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -750,13 +750,14 @@ void gpiod_unexport(struct gpio_desc *desc)
}
EXPORT_SYMBOL_GPL(gpiod_unexport);
-int gpiochip_export(struct gpio_chip *chip)
+int gpiochip_register(struct gpio_chip *chip)
{
struct device *dev;
- /* Many systems register gpio chips for SOC support very early,
+ /*
+ * Many systems add gpio chips for SOC support very early,
* before driver model support is available. In those cases we
- * export this later, in gpiolib_sysfs_init() ... here we just
+ * register later, in gpiolib_sysfs_init() ... here we just
* verify that _some_ field of gpio_class got initialized.
*/
if (!gpio_class.p)
@@ -776,7 +777,7 @@ int gpiochip_export(struct gpio_chip *chip)
return 0;
}
-void gpiochip_unexport(struct gpio_chip *chip)
+void gpiochip_unregister(struct gpio_chip *chip)
{
struct gpio_desc *desc;
unsigned int i;
@@ -821,7 +822,7 @@ static int __init gpiolib_sysfs_init(void)
continue;
/*
- * TODO we yield gpio_lock here because gpiochip_export()
+ * TODO we yield gpio_lock here because gpiochip_register()
* acquires a mutex. This is unsafe and needs to be fixed.
*
* Also it would be nice to use gpiochip_find() here so we
@@ -829,7 +830,7 @@ static int __init gpiolib_sysfs_init(void)
* gpio_lock prevents us from doing this.
*/
spin_unlock_irqrestore(&gpio_lock, flags);
- status = gpiochip_export(chip);
+ status = gpiochip_register(chip);
spin_lock_irqsave(&gpio_lock, flags);
}
spin_unlock_irqrestore(&gpio_lock, flags);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5a5c208d31c7..fefc36211205 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -285,7 +285,7 @@ int gpiochip_add(struct gpio_chip *chip)
of_gpiochip_add(chip);
acpi_gpiochip_add(chip);
- status = gpiochip_export(chip);
+ status = gpiochip_register(chip);
if (status)
goto err_remove_chip;
@@ -330,7 +330,7 @@ void gpiochip_remove(struct gpio_chip *chip)
unsigned id;
bool requested = false;
- gpiochip_unexport(chip);
+ gpiochip_unregister(chip);
gpiochip_irqchip_remove(chip);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 54bc5ec91398..69458a022c90 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -149,17 +149,17 @@ static int __maybe_unused gpio_chip_hwgpio(const struct gpio_desc *desc)
#ifdef CONFIG_GPIO_SYSFS
-int gpiochip_export(struct gpio_chip *chip);
-void gpiochip_unexport(struct gpio_chip *chip);
+int gpiochip_register(struct gpio_chip *chip);
+void gpiochip_unregister(struct gpio_chip *chip);
#else
-static inline int gpiochip_export(struct gpio_chip *chip)
+static inline int gpiochip_register(struct gpio_chip *chip)
{
return 0;
}
-static inline void gpiochip_unexport(struct gpio_chip *chip)
+static inline void gpiochip_unregister(struct gpio_chip *chip)
{
}
--
2.0.5
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]
Cc: [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(-)
diff --git a/Documentation/gpio/gpio-legacy.txt b/Documentation/gpio/gpio-legacy.txt
index 6f83fa965b4b..79ab5648d69b 100644
--- a/Documentation/gpio/gpio-legacy.txt
+++ b/Documentation/gpio/gpio-legacy.txt
@@ -751,9 +751,6 @@ requested using gpio_request():
int gpio_export_link(struct device *dev, const char *name,
unsigned gpio)
- /* change the polarity of a GPIO node in sysfs */
- int gpio_sysfs_set_active_low(unsigned gpio, int value);
-
After a kernel driver requests a GPIO, it may only be made available in
the sysfs interface by gpio_export(). The driver can control whether the
signal direction may change. This helps drivers prevent userspace code
@@ -767,9 +764,3 @@ After the GPIO has been exported, gpio_export_link() allows creating
symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
use this to provide the interface under their own device in sysfs with
a descriptive name.
-
-Drivers can use gpio_sysfs_set_active_low() to hide GPIO line polarity
-differences between boards from user space. This only affects the
-sysfs interface. Polarity change can be done both before and after
-gpio_export(), and previously enabled poll(2) support for either
-rising or falling edge will be reconfigured to follow this setting.
diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
index c2c3a97f8ff7..535b6a8a7a7c 100644
--- a/Documentation/gpio/sysfs.txt
+++ b/Documentation/gpio/sysfs.txt
@@ -132,9 +132,6 @@ requested using gpio_request():
int gpiod_export_link(struct device *dev, const char *name,
struct gpio_desc *desc);
- /* change the polarity of a GPIO node in sysfs */
- int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
-
After a kernel driver requests a GPIO, it may only be made available in
the sysfs interface by gpiod_export(). The driver can control whether the
signal direction may change. This helps drivers prevent userspace code
@@ -148,8 +145,3 @@ After the GPIO has been exported, gpiod_export_link() allows creating
symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
use this to provide the interface under their own device in sysfs with
a descriptive name.
-
-Drivers can use gpiod_sysfs_set_active_low() to hide GPIO line polarity
-differences between boards from user space. Polarity change can be done both
-before and after gpiod_export(), and previously enabled poll(2) support for
-either rising or falling edge will be reconfigured to follow this setting.
diff --git a/Documentation/zh_CN/gpio.txt b/Documentation/zh_CN/gpio.txt
index d5b8f01833f4..bce972521065 100644
--- a/Documentation/zh_CN/gpio.txt
+++ b/Documentation/zh_CN/gpio.txt
@@ -638,9 +638,6 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
int gpio_export_link(struct device *dev, const char *name,
unsigned gpio)
- /* 改变 sysfs 中的一个 GPIO 节点的极性 */
- int gpio_sysfs_set_active_low(unsigned gpio, int value);
-
在一个内核驱动申请一个 GPIO 之后，它可以通过 gpio_export()使其在 sysfs
接口中可见。该驱动可以控制信号方向是否可修改。这有助于防止用户空间代码无意间
破坏重要的系统状态。
@@ -651,8 +648,3 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
在 GPIO 被导出之后，gpio_export_link()允许在 sysfs 文件系统的任何地方
创建一个到这个 GPIO sysfs 节点的符号链接。这样驱动就可以通过一个描述性的
名字，在 sysfs 中他们所拥有的设备下提供一个(到这个 GPIO sysfs 节点的)接口。
-
-驱动可以使用 gpio_sysfs_set_active_low() 来在用户空间隐藏电路板之间
-GPIO 线的极性差异。这个仅对 sysfs 接口起作用。极性的改变可以在 gpio_export()
-前后进行,且之前使能的轮询操作(poll(2))支持(上升或下降沿)将会被重新配置来遵循
-这个设置。
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 31434c5a90ef..8a95a954f514 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
/* reconfigure poll(2) support if enabled on one edge only */
- if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
- !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
+ if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
+ !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
gpio_setup_irq(desc, dev, 0);
@@ -666,50 +666,6 @@ int gpiod_export_link(struct device *dev, const char *name,
EXPORT_SYMBOL_GPL(gpiod_export_link);
/**
- * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value
- * @gpio: gpio to change
- * @value: non-zero to use active low, i.e. inverted values
- *
- * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute.
- * The GPIO does not have to be exported yet. If poll(2) support has
- * been enabled for either rising or falling edge, it will be
- * reconfigured to follow the new polarity.
- *
- * Returns zero on success, else an error.
- */
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
-{
- struct device *dev = NULL;
- int status = -EINVAL;
-
- if (!desc) {
- pr_warn("%s: invalid GPIO\n", __func__);
- return -EINVAL;
- }
-
- mutex_lock(&sysfs_lock);
-
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
- dev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (dev == NULL) {
- status = -ENODEV;
- goto unlock;
- }
- }
-
- status = sysfs_set_active_low(desc, dev, value);
- put_device(dev);
-unlock:
- mutex_unlock(&sysfs_lock);
-
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
-
- return status;
-}
-EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low);
-
-/**
* gpiod_unexport - reverse effect of gpio_export()
* @gpio: gpio to make unavailable
*
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 9bb0d11729c9..40ec1433f05d 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -128,11 +128,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
return gpiod_export_link(dev, name, gpio_to_desc(gpio));
}
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
- return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value);
-}
-
static inline void gpio_unexport(unsigned gpio)
{
gpiod_unexport(gpio_to_desc(gpio));
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index ab81339a8590..d12b5d566e4b 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -196,13 +196,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
return -EINVAL;
}
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
- /* GPIO can never have been requested */
- WARN_ON(1);
- return -EINVAL;
-}
-
static inline void gpio_unexport(unsigned gpio)
{
/* GPIO can never have been exported */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 3a7c9ffd5ab9..09a7fb0062a6 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -449,7 +449,6 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
int gpiod_export_link(struct device *dev, const char *name,
struct gpio_desc *desc);
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
void gpiod_unexport(struct gpio_desc *desc);
#else /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
@@ -466,11 +465,6 @@ static inline int gpiod_export_link(struct device *dev, const char *name,
return -ENOSYS;
}
-static inline int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
-{
- return -ENOSYS;
-}
-
static inline void gpiod_unexport(struct gpio_desc *desc)
{
}
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 8a95a954f514..9d9f1f1a2417 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -38,7 +38,7 @@ static DEFINE_MUTEX(sysfs_lock);
* /edge configuration
*/
-static ssize_t gpio_direction_show(struct device *dev,
+static ssize_t direction_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -59,7 +59,7 @@ static ssize_t gpio_direction_show(struct device *dev,
return status;
}
-static ssize_t gpio_direction_store(struct device *dev,
+static ssize_t direction_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -81,11 +81,9 @@ static ssize_t gpio_direction_store(struct device *dev,
mutex_unlock(&sysfs_lock);
return status ? : size;
}
+static DEVICE_ATTR_RW(direction);
-static /* const */ DEVICE_ATTR(direction, 0644,
- gpio_direction_show, gpio_direction_store);
-
-static ssize_t gpio_value_show(struct device *dev,
+static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -102,7 +100,7 @@ static ssize_t gpio_value_show(struct device *dev,
return status;
}
-static ssize_t gpio_value_store(struct device *dev,
+static ssize_t value_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -127,9 +125,7 @@ static ssize_t gpio_value_store(struct device *dev,
mutex_unlock(&sysfs_lock);
return status;
}
-
-static DEVICE_ATTR(value, 0644,
- gpio_value_show, gpio_value_store);
+static DEVICE_ATTR_RW(value);
static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
{
@@ -222,7 +218,7 @@ static const struct {
{ "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
};
-static ssize_t gpio_edge_show(struct device *dev,
+static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -249,7 +245,7 @@ static ssize_t gpio_edge_show(struct device *dev,
return status;
}
-static ssize_t gpio_edge_store(struct device *dev,
+static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -276,8 +272,7 @@ found:
return status;
}
-
-static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
+static DEVICE_ATTR_RW(edge);
static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
int value)
@@ -304,7 +299,7 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
return status;
}
-static ssize_t gpio_active_low_show(struct device *dev,
+static ssize_t active_low_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -323,7 +318,7 @@ static ssize_t gpio_active_low_show(struct device *dev,
return status;
}
-static ssize_t gpio_active_low_store(struct device *dev,
+static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -345,9 +340,7 @@ static ssize_t gpio_active_low_store(struct device *dev,
return status ? : size;
}
-
-static DEVICE_ATTR(active_low, 0644,
- gpio_active_low_show, gpio_active_low_store);
+static DEVICE_ATTR_RW(active_low);
static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
@@ -395,32 +388,32 @@ static const struct attribute_group *gpio_groups[] = {
* /ngpio ... matching gpio_chip.ngpio
*/
-static ssize_t chip_base_show(struct device *dev,
+static ssize_t base_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%d\n", chip->base);
}
-static DEVICE_ATTR(base, 0444, chip_base_show, NULL);
+static DEVICE_ATTR_RO(base);
-static ssize_t chip_label_show(struct device *dev,
+static ssize_t label_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%s\n", chip->label ? : "");
}
-static DEVICE_ATTR(label, 0444, chip_label_show, NULL);
+static DEVICE_ATTR_RO(label);
-static ssize_t chip_ngpio_show(struct device *dev,
+static ssize_t ngpio_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%u\n", chip->ngpio);
}
-static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL);
+static DEVICE_ATTR_RO(ngpio);
static struct attribute *gpiochip_attrs[] = {
&dev_attr_base.attr,
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 9d9f1f1a2417..d896b6fa7fe8 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -680,7 +680,6 @@ void gpiod_unexport(struct gpio_desc *desc)
dev = class_find_device(&gpio_class, NULL, desc, match_export);
if (dev) {
- gpio_setup_irq(desc, dev, 0);
clear_bit(FLAG_SYSFS_DIR, &desc->flags);
clear_bit(FLAG_EXPORT, &desc->flags);
} else
@@ -691,6 +690,11 @@ void gpiod_unexport(struct gpio_desc *desc)
if (dev) {
device_unregister(dev);
+ /*
+ * Release irq after deregistration to prevent race with
+ * edge_store.
+ */
+ gpio_setup_irq(desc, dev, 0);
put_device(dev);
}
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d896b6fa7fe8..b77f29cd1603 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -46,14 +46,10 @@ static ssize_t direction_show(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags)) {
- status = -EIO;
- } else {
- gpiod_get_direction(desc);
- status = sprintf(buf, "%s\n",
+ gpiod_get_direction(desc);
+ status = sprintf(buf, "%s\n",
test_bit(FLAG_IS_OUT, &desc->flags)
? "out" : "in");
- }
mutex_unlock(&sysfs_lock);
return status;
@@ -67,9 +63,7 @@ static ssize_t direction_store(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else if (sysfs_streq(buf, "high"))
+ if (sysfs_streq(buf, "high"))
status = gpiod_direction_output_raw(desc, 1);
else if (sysfs_streq(buf, "out") || sysfs_streq(buf, "low"))
status = gpiod_direction_output_raw(desc, 0);
@@ -91,10 +85,7 @@ static ssize_t value_show(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else
- status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
+ status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
mutex_unlock(&sysfs_lock);
return status;
@@ -108,11 +99,9 @@ static ssize_t value_store(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else if (!test_bit(FLAG_IS_OUT, &desc->flags))
+ if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
status = -EPERM;
- else {
+ } else {
long value;
status = kstrtol(buf, 0, &value);
@@ -222,23 +211,18 @@ static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
- ssize_t status;
+ unsigned long mask;
+ ssize_t status = 0;
+ int i;
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else {
- int i;
-
- status = 0;
- for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
- if ((desc->flags & GPIO_TRIGGER_MASK)
- == trigger_types[i].flags) {
- status = sprintf(buf, "%s\n",
- trigger_types[i].name);
- break;
- }
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
+ mask = desc->flags & GPIO_TRIGGER_MASK;
+ if (mask == trigger_types[i].flags) {
+ status = sprintf(buf, "%s\n", trigger_types[i].name);
+ break;
+ }
}
mutex_unlock(&sysfs_lock);
@@ -260,13 +244,9 @@ static ssize_t edge_store(struct device *dev,
found:
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else {
- status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
- if (!status)
- status = size;
- }
+ status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ if (!status)
+ status = size;
mutex_unlock(&sysfs_lock);
@@ -307,10 +287,7 @@ static ssize_t active_low_show(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else
- status = sprintf(buf, "%d\n",
+ status = sprintf(buf, "%d\n",
!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
mutex_unlock(&sysfs_lock);
@@ -323,18 +300,13 @@ static ssize_t active_low_store(struct device *dev,
{
struct gpio_desc *desc = dev_get_drvdata(dev);
ssize_t status;
+ long value;
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags)) {
- status = -EIO;
- } else {
- long value;
-
- status = kstrtol(buf, 0, &value);
- if (status == 0)
- status = sysfs_set_active_low(desc, dev, value != 0);
- }
+ status = kstrtol(buf, 0, &value);
+ if (status == 0)
+ status = sysfs_set_active_low(desc, dev, value != 0);
mutex_unlock(&sysfs_lock);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index b77f29cd1603..4cc90f7ce14b 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -6,9 +6,14 @@
#include <linux/gpio/driver.h>
#include <linux/interrupt.h>
#include <linux/kdev_t.h>
+#include <linux/slab.h>
#include "gpiolib.h"
+struct gpiod_data {
+ struct gpio_desc *desc;
+};
+
static DEFINE_IDR(dirent_idr);
@@ -41,7 +46,8 @@ static DEFINE_MUTEX(sysfs_lock);
static ssize_t direction_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -58,7 +64,8 @@ static ssize_t direction_show(struct device *dev,
static ssize_t direction_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -80,7 +87,8 @@ static DEVICE_ATTR_RW(direction);
static ssize_t value_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -94,7 +102,8 @@ static ssize_t value_show(struct device *dev,
static ssize_t value_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -210,7 +219,8 @@ static const struct {
static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- const struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
unsigned long mask;
ssize_t status = 0;
int i;
@@ -232,7 +242,8 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
int i;
@@ -282,7 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
static ssize_t active_low_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- const struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -298,7 +310,8 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
long value;
@@ -318,7 +331,8 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
struct device *dev = container_of(kobj, struct device, kobj);
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
umode_t mode = attr->mode;
bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
@@ -510,6 +524,7 @@ static struct class gpio_class = {
int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
{
struct gpio_chip *chip;
+ struct gpiod_data *data;
unsigned long flags;
int status;
const char *ioname = NULL;
@@ -534,7 +549,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
/* check if chip is being removed */
if (!chip || !chip->cdev) {
status = -ENODEV;
- goto fail_unlock;
+ goto err_unlock;
}
spin_lock_irqsave(&gpio_lock, flags);
@@ -546,7 +561,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
test_bit(FLAG_REQUESTED, &desc->flags),
test_bit(FLAG_EXPORT, &desc->flags));
status = -EPERM;
- goto fail_unlock;
+ goto err_unlock;
}
if (chip->direction_input && chip->direction_output &&
@@ -556,33 +571,45 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
spin_unlock_irqrestore(&gpio_lock, flags);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ status = -ENOMEM;
+ goto err_unlock;
+ }
+
+ data->desc = desc;
+
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
ioname = chip->names[offset];
dev = device_create_with_groups(&gpio_class, chip->dev,
- MKDEV(0, 0), desc, gpio_groups,
+ MKDEV(0, 0), data, gpio_groups,
ioname ? ioname : "gpio%u",
desc_to_gpio(desc));
if (IS_ERR(dev)) {
status = PTR_ERR(dev);
- goto fail_unlock;
+ goto err_free_data;
}
set_bit(FLAG_EXPORT, &desc->flags);
mutex_unlock(&sysfs_lock);
return 0;
-fail_unlock:
+err_free_data:
+ kfree(data);
+err_unlock:
mutex_unlock(&sysfs_lock);
gpiod_dbg(desc, "%s: status %d\n", __func__, status);
return status;
}
EXPORT_SYMBOL_GPL(gpiod_export);
-static int match_export(struct device *dev, const void *data)
+static int match_export(struct device *dev, const void *desc)
{
- return dev_get_drvdata(dev) == data;
+ struct gpiod_data *data = dev_get_drvdata(dev);
+
+ return data->desc == desc;
}
/**
@@ -638,6 +665,7 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
*/
void gpiod_unexport(struct gpio_desc *desc)
{
+ struct gpiod_data *data;
int status = 0;
struct device *dev = NULL;
@@ -661,6 +689,7 @@ void gpiod_unexport(struct gpio_desc *desc)
mutex_unlock(&sysfs_lock);
if (dev) {
+ data = dev_get_drvdata(dev);
device_unregister(dev);
/*
* Release irq after deregistration to prevent race with
@@ -668,6 +697,7 @@ void gpiod_unexport(struct gpio_desc *desc)
*/
gpio_setup_irq(desc, dev, 0);
put_device(dev);
+ kfree(data);
}
if (status)
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 4cc90f7ce14b..bc4f192c0aa3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -133,9 +133,10 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
return IRQ_HANDLED;
}
-static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
- unsigned long gpio_flags)
+static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
struct kernfs_node *value_sd;
unsigned long irq_flags;
int ret, irq, id;
@@ -242,8 +243,6 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
ssize_t status;
int i;
@@ -255,7 +254,7 @@ static ssize_t edge_store(struct device *dev,
found:
mutex_lock(&sysfs_lock);
- status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ status = gpio_setup_irq(dev, trigger_types[i].flags);
if (!status)
status = size;
@@ -265,9 +264,10 @@ found:
}
static DEVICE_ATTR_RW(edge);
-static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
- int value)
+static int sysfs_set_active_low(struct device *dev, int value)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
int status = 0;
if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
@@ -283,8 +283,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
- gpio_setup_irq(desc, dev, 0);
- status = gpio_setup_irq(desc, dev, trigger_flags);
+ gpio_setup_irq(dev, 0);
+ status = gpio_setup_irq(dev, trigger_flags);
}
return status;
@@ -310,8 +310,6 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
ssize_t status;
long value;
@@ -319,7 +317,7 @@ static ssize_t active_low_store(struct device *dev,
status = kstrtol(buf, 0, &value);
if (status == 0)
- status = sysfs_set_active_low(desc, dev, value != 0);
+ status = sysfs_set_active_low(dev, value != 0);
mutex_unlock(&sysfs_lock);
@@ -695,7 +693,7 @@ void gpiod_unexport(struct gpio_desc *desc)
* Release irq after deregistration to prevent race with
* edge_store.
*/
- gpio_setup_irq(desc, dev, 0);
+ gpio_setup_irq(dev, 0);
put_device(dev);
kfree(data);
}
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index bc4f192c0aa3..8041c1728231 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -12,11 +12,9 @@
struct gpiod_data {
struct gpio_desc *desc;
+ struct kernfs_node *value_kn;
};
-static DEFINE_IDR(dirent_idr);
-
-
/* lock protects against unexport_gpio() being called while
* sysfs files are active.
*/
@@ -127,9 +125,10 @@ static DEVICE_ATTR_RW(value);
static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
{
- struct kernfs_node *value_sd = priv;
+ struct gpiod_data *data = priv;
+
+ sysfs_notify_dirent(data->value_kn);
- sysfs_notify_dirent(value_sd);
return IRQ_HANDLED;
}
@@ -137,9 +136,8 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
- struct kernfs_node *value_sd;
unsigned long irq_flags;
- int ret, irq, id;
+ int ret, irq;
if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
return 0;
@@ -148,16 +146,14 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
if (irq < 0)
return -EIO;
- id = desc->flags >> ID_SHIFT;
- value_sd = idr_find(&dirent_idr, id);
- if (value_sd)
- free_irq(irq, value_sd);
+ if (data->value_kn)
+ free_irq(irq, data);
desc->flags &= ~GPIO_TRIGGER_MASK;
if (!gpio_flags) {
ret = 0;
- goto free_id;
+ goto free_kn;
}
irq_flags = IRQF_SHARED;
@@ -168,41 +164,27 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
- if (!value_sd) {
- value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
- if (!value_sd) {
+ if (!data->value_kn) {
+ data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+ if (!data->value_kn) {
ret = -ENODEV;
goto err_out;
}
-
- ret = idr_alloc(&dirent_idr, value_sd, 1, 0, GFP_KERNEL);
- if (ret < 0)
- goto free_sd;
- id = ret;
-
- desc->flags &= GPIO_FLAGS_MASK;
- desc->flags |= (unsigned long)id << ID_SHIFT;
-
- if (desc->flags >> ID_SHIFT != id) {
- ret = -ERANGE;
- goto free_id;
- }
}
ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
- "gpiolib", value_sd);
+ "gpiolib", data);
if (ret < 0)
- goto free_id;
+ goto free_kn;
desc->flags |= gpio_flags;
return 0;
-free_id:
- idr_remove(&dirent_idr, id);
- desc->flags &= GPIO_FLAGS_MASK;
-free_sd:
- if (value_sd)
- sysfs_put(value_sd);
+free_kn:
+ if (data->value_kn) {
+ sysfs_put(data->value_kn);
+ data->value_kn = NULL;
+ }
err_out:
return ret;
}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 69458a022c90..4deb71d2bd01 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -90,9 +90,6 @@ struct gpio_desc {
#define FLAG_SYSFS_DIR 10 /* show sysfs direction attribute */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
-#define ID_SHIFT 16 /* add new flags before this one */
-
-#define GPIO_FLAGS_MASK ((1 << ID_SHIFT) - 1)
#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
const char *label;
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 8041c1728231..bd22de806182 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -139,9 +139,6 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
unsigned long irq_flags;
int ret, irq;
- if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
- return 0;
-
irq = gpiod_to_irq(desc);
if (irq < 0)
return -EIO;
@@ -225,6 +222,9 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
+ unsigned long flags;
ssize_t status;
int i;
@@ -234,12 +234,20 @@ static ssize_t edge_store(struct device *dev,
return -EINVAL;
found:
+ flags = trigger_types[i].flags;
+
mutex_lock(&sysfs_lock);
- status = gpio_setup_irq(dev, trigger_types[i].flags);
+ if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+ status = size;
+ goto out_unlock;
+ }
+
+ status = gpio_setup_irq(dev, flags);
if (!status)
status = size;
+out_unlock:
mutex_unlock(&sysfs_lock);
return status;
@@ -675,7 +683,8 @@ void gpiod_unexport(struct gpio_desc *desc)
* Release irq after deregistration to prevent race with
* edge_store.
*/
- gpio_setup_irq(dev, 0);
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_setup_irq(dev, 0);
put_device(dev);
kfree(data);
}
--
2.0.5
Add separate helper functions for irq request and free.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 72 ++++++++++++++++++++++----------------------
1 file changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index bd22de806182..323272569292 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -13,6 +13,7 @@
struct gpiod_data {
struct gpio_desc *desc;
struct kernfs_node *value_kn;
+ int irq;
};
/* lock protects against unexport_gpio() being called while
@@ -132,26 +133,20 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
return IRQ_HANDLED;
}
-static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
+static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
unsigned long irq_flags;
- int ret, irq;
+ int ret;
- irq = gpiod_to_irq(desc);
- if (irq < 0)
+ data->irq = gpiod_to_irq(desc);
+ if (data->irq < 0)
return -EIO;
- if (data->value_kn)
- free_irq(irq, data);
-
- desc->flags &= ~GPIO_TRIGGER_MASK;
-
- if (!gpio_flags) {
- ret = 0;
- goto free_kn;
- }
+ data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+ if (!data->value_kn)
+ return -ENODEV;
irq_flags = IRQF_SHARED;
if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
@@ -161,31 +156,31 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
- if (!data->value_kn) {
- data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
- if (!data->value_kn) {
- ret = -ENODEV;
- goto err_out;
- }
- }
-
- ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
+ ret = request_any_context_irq(data->irq, gpio_sysfs_irq, irq_flags,
"gpiolib", data);
if (ret < 0)
- goto free_kn;
+ goto err_put_kn;
desc->flags |= gpio_flags;
+
return 0;
-free_kn:
- if (data->value_kn) {
- sysfs_put(data->value_kn);
- data->value_kn = NULL;
- }
-err_out:
+err_put_kn:
+ sysfs_put(data->value_kn);
+
return ret;
}
+static void gpio_sysfs_free_irq(struct device *dev)
+{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
+
+ desc->flags &= ~GPIO_TRIGGER_MASK;
+ free_irq(data->irq, data);
+ sysfs_put(data->value_kn);
+}
+
static const struct {
const char *name;
unsigned long flags;
@@ -225,7 +220,7 @@ static ssize_t edge_store(struct device *dev,
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
unsigned long flags;
- ssize_t status;
+ ssize_t status = size;
int i;
for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
@@ -243,9 +238,14 @@ found:
goto out_unlock;
}
- status = gpio_setup_irq(dev, flags);
- if (!status)
- status = size;
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_sysfs_free_irq(dev);
+
+ if (flags) {
+ status = gpio_sysfs_request_irq(dev, flags);
+ if (!status)
+ status = size;
+ }
out_unlock:
mutex_unlock(&sysfs_lock);
@@ -273,8 +273,8 @@ static int sysfs_set_active_low(struct device *dev, int value)
!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
- gpio_setup_irq(dev, 0);
- status = gpio_setup_irq(dev, trigger_flags);
+ gpio_sysfs_free_irq(dev);
+ status = gpio_sysfs_request_irq(dev, trigger_flags);
}
return status;
@@ -684,7 +684,7 @@ void gpiod_unexport(struct gpio_desc *desc)
* edge_store.
*/
if (desc->flags & GPIO_TRIGGER_MASK)
- gpio_setup_irq(dev, 0);
+ gpio_sysfs_free_irq(dev);
put_device(dev);
kfree(data);
}
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 323272569292..761e1644cff1 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -221,14 +221,16 @@ static ssize_t edge_store(struct device *dev,
struct gpio_desc *desc = data->desc;
unsigned long flags;
ssize_t status = size;
- int i;
+ int i;
- for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
if (sysfs_streq(trigger_types[i].name, buf))
- goto found;
- return -EINVAL;
+ break;
+ }
+
+ if (i == ARRAY_SIZE(trigger_types))
+ return -EINVAL;
-found:
flags = trigger_types[i].flags;
mutex_lock(&sysfs_lock);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 761e1644cff1..d9265b4609a3 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -616,34 +616,22 @@ static int match_export(struct device *dev, const void *desc)
int gpiod_export_link(struct device *dev, const char *name,
struct gpio_desc *desc)
{
- int status = -EINVAL;
+ struct device *cdev;
+ int ret;
if (!desc) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
- mutex_lock(&sysfs_lock);
-
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
- struct device *tdev;
-
- tdev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (tdev != NULL) {
- status = sysfs_create_link(&dev->kobj, &tdev->kobj,
- name);
- put_device(tdev);
- } else {
- status = -ENODEV;
- }
- }
-
- mutex_unlock(&sysfs_lock);
+ cdev = class_find_device(&gpio_class, NULL, desc, match_export);
+ if (!cdev)
+ return -ENODEV;
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+ ret = sysfs_create_link(&dev->kobj, &cdev->kobj, name);
+ put_device(cdev);
- return status;
+ return ret;
}
EXPORT_SYMBOL_GPL(gpiod_export_link);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index d9265b4609a3..19c4351021d9 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -12,12 +12,15 @@
struct gpiod_data {
struct gpio_desc *desc;
+
+ struct mutex mutex;
struct kernfs_node *value_kn;
int irq;
};
-/* lock protects against unexport_gpio() being called while
- * sysfs files are active.
+/*
+ * Lock to serialise gpiod export and unexport, and prevent re-export of
+ * gpiod whose chip is being unregistered.
*/
static DEFINE_MUTEX(sysfs_lock);
@@ -49,14 +52,15 @@ static ssize_t direction_show(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
gpiod_get_direction(desc);
status = sprintf(buf, "%s\n",
test_bit(FLAG_IS_OUT, &desc->flags)
? "out" : "in");
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -67,7 +71,7 @@ static ssize_t direction_store(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if (sysfs_streq(buf, "high"))
status = gpiod_direction_output_raw(desc, 1);
@@ -78,7 +82,8 @@ static ssize_t direction_store(struct device *dev,
else
status = -EINVAL;
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status ? : size;
}
static DEVICE_ATTR_RW(direction);
@@ -90,11 +95,12 @@ static ssize_t value_show(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = sprintf(buf, "%d\n", gpiod_get_value_cansleep(desc));
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -105,7 +111,7 @@ static ssize_t value_store(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
status = -EPERM;
@@ -119,7 +125,8 @@ static ssize_t value_store(struct device *dev,
}
}
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
static DEVICE_ATTR_RW(value);
@@ -133,6 +140,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
return IRQ_HANDLED;
}
+/* Caller holds gpiod-data mutex. */
static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -171,6 +179,10 @@ err_put_kn:
return ret;
}
+/*
+ * Caller holds gpiod-data mutex (unless called after class-device
+ * deregistration).
+ */
static void gpio_sysfs_free_irq(struct device *dev)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -200,7 +212,7 @@ static ssize_t edge_show(struct device *dev,
ssize_t status = 0;
int i;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
mask = desc->flags & GPIO_TRIGGER_MASK;
@@ -210,7 +222,8 @@ static ssize_t edge_show(struct device *dev,
}
}
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -233,7 +246,7 @@ static ssize_t edge_store(struct device *dev,
flags = trigger_types[i].flags;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
status = size;
@@ -250,12 +263,13 @@ static ssize_t edge_store(struct device *dev,
}
out_unlock:
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
static DEVICE_ATTR_RW(edge);
+/* Caller holds gpiod-data mutex. */
static int sysfs_set_active_low(struct device *dev, int value)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -289,12 +303,12 @@ static ssize_t active_low_show(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = sprintf(buf, "%d\n",
!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
@@ -302,16 +316,17 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
ssize_t status;
long value;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = kstrtol(buf, 0, &value);
if (status == 0)
status = sysfs_set_active_low(dev, value != 0);
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status ? : size;
}
@@ -568,6 +583,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
}
data->desc = desc;
+ mutex_init(&data->mutex);
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 19c4351021d9..5eee3d0b3a81 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -659,9 +659,8 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
*/
void gpiod_unexport(struct gpio_desc *desc)
{
- struct gpiod_data *data;
- int status = 0;
- struct device *dev = NULL;
+ struct gpiod_data *data;
+ struct device *dev;
if (!desc) {
pr_warn("%s: invalid GPIO\n", __func__);
@@ -670,33 +669,35 @@ void gpiod_unexport(struct gpio_desc *desc)
mutex_lock(&sysfs_lock);
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ goto err_unlock;
- dev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (dev) {
- clear_bit(FLAG_SYSFS_DIR, &desc->flags);
- clear_bit(FLAG_EXPORT, &desc->flags);
- } else
- status = -ENODEV;
- }
+ dev = class_find_device(&gpio_class, NULL, desc, match_export);
+ if (!dev)
+ goto err_unlock;
+
+ data = dev_get_drvdata(dev);
+
+ clear_bit(FLAG_SYSFS_DIR, &desc->flags);
+ clear_bit(FLAG_EXPORT, &desc->flags);
+
+ device_unregister(dev);
+
+ /*
+ * Release irq after deregistration to prevent race with edge_store.
+ */
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_sysfs_free_irq(dev);
mutex_unlock(&sysfs_lock);
- if (dev) {
- data = dev_get_drvdata(dev);
- device_unregister(dev);
- /*
- * Release irq after deregistration to prevent race with
- * edge_store.
- */
- if (desc->flags & GPIO_TRIGGER_MASK)
- gpio_sysfs_free_irq(dev);
- put_device(dev);
- kfree(data);
- }
+ put_device(dev);
+ kfree(data);
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+ return;
+
+err_unlock:
+ mutex_unlock(&sysfs_lock);
}
EXPORT_SYMBOL_GPL(gpiod_unexport);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 5eee3d0b3a81..00609e197f50 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -270,7 +270,7 @@ out_unlock:
static DEVICE_ATTR_RW(edge);
/* Caller holds gpiod-data mutex. */
-static int sysfs_set_active_low(struct device *dev, int value)
+static int gpio_sysfs_set_active_low(struct device *dev, int value)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
@@ -324,7 +324,7 @@ static ssize_t active_low_store(struct device *dev,
status = kstrtol(buf, 0, &value);
if (status == 0)
- status = sysfs_set_active_low(dev, value != 0);
+ status = gpio_sysfs_set_active_low(dev, value);
mutex_unlock(&data->mutex);
--
2.0.5
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(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 00609e197f50..27a8c84fc241 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -16,6 +16,8 @@ struct gpiod_data {
struct mutex mutex;
struct kernfs_node *value_kn;
int irq;
+
+ bool direction_can_change;
};
/*
@@ -339,7 +341,7 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
umode_t mode = attr->mode;
- bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
+ bool show_direction = data->direction_can_change;
if (attr == &dev_attr_direction.attr) {
if (!show_direction)
@@ -568,12 +570,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
status = -EPERM;
goto err_unlock;
}
-
- if (chip->direction_input && chip->direction_output &&
- direction_may_change) {
- set_bit(FLAG_SYSFS_DIR, &desc->flags);
- }
-
spin_unlock_irqrestore(&gpio_lock, flags);
data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -584,6 +580,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
data->desc = desc;
mutex_init(&data->mutex);
+ if (chip->direction_input && chip->direction_output)
+ data->direction_can_change = direction_may_change;
+ else
+ data->direction_can_change = false;
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
@@ -678,7 +678,6 @@ void gpiod_unexport(struct gpio_desc *desc)
data = dev_get_drvdata(dev);
- clear_bit(FLAG_SYSFS_DIR, &desc->flags);
clear_bit(FLAG_EXPORT, &desc->flags);
device_unregister(dev);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 4deb71d2bd01..591257a4359a 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -87,7 +87,6 @@ struct gpio_desc {
#define FLAG_OPEN_DRAIN 7 /* Gpio is open drain type */
#define FLAG_OPEN_SOURCE 8 /* Gpio is open source type */
#define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */
-#define FLAG_SYSFS_DIR 10 /* show sysfs direction attribute */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
--
2.0.5
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 | 48 ++++++++++++++++++++++----------------------
drivers/gpio/gpiolib.h | 4 ----
2 files changed, 24 insertions(+), 28 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 27a8c84fc241..8786b8b79ce6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -10,12 +10,18 @@
#include "gpiolib.h"
+#define GPIO_IRQF_TRIGGER_FALLING BIT(0)
+#define GPIO_IRQF_TRIGGER_RISING BIT(1)
+#define GPIO_IRQF_TRIGGER_BOTH (GPIO_IRQF_TRIGGER_FALLING | \
+ GPIO_IRQF_TRIGGER_RISING)
+
struct gpiod_data {
struct gpio_desc *desc;
struct mutex mutex;
struct kernfs_node *value_kn;
int irq;
+ unsigned char irq_flags;
bool direction_can_change;
};
@@ -143,7 +149,7 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
}
/* Caller holds gpiod-data mutex. */
-static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
+static int gpio_sysfs_request_irq(struct device *dev, unsigned char flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
@@ -159,10 +165,10 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
return -ENODEV;
irq_flags = IRQF_SHARED;
- if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
+ if (flags & GPIO_IRQF_TRIGGER_FALLING)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING;
- if (test_bit(FLAG_TRIG_RISE, &gpio_flags))
+ if (flags & GPIO_IRQF_TRIGGER_RISING)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
@@ -171,7 +177,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
if (ret < 0)
goto err_put_kn;
- desc->flags |= gpio_flags;
+ data->irq_flags = flags;
return 0;
@@ -188,37 +194,33 @@ err_put_kn:
static void gpio_sysfs_free_irq(struct device *dev)
{
struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
- desc->flags &= ~GPIO_TRIGGER_MASK;
+ data->irq_flags = 0;
free_irq(data->irq, data);
sysfs_put(data->value_kn);
}
static const struct {
const char *name;
- unsigned long flags;
+ unsigned char flags;
} trigger_types[] = {
{ "none", 0 },
- { "falling", BIT(FLAG_TRIG_FALL) },
- { "rising", BIT(FLAG_TRIG_RISE) },
- { "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
+ { "falling", GPIO_IRQF_TRIGGER_FALLING },
+ { "rising", GPIO_IRQF_TRIGGER_RISING },
+ { "both", GPIO_IRQF_TRIGGER_BOTH },
};
static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
- unsigned long mask;
ssize_t status = 0;
int i;
mutex_lock(&data->mutex);
for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
- mask = desc->flags & GPIO_TRIGGER_MASK;
- if (mask == trigger_types[i].flags) {
+ if (data->irq_flags == trigger_types[i].flags) {
status = sprintf(buf, "%s\n", trigger_types[i].name);
break;
}
@@ -233,8 +235,7 @@ static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
- unsigned long flags;
+ unsigned char flags;
ssize_t status = size;
int i;
@@ -250,12 +251,12 @@ static ssize_t edge_store(struct device *dev,
mutex_lock(&data->mutex);
- if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+ if (flags == data->irq_flags) {
status = size;
goto out_unlock;
}
- if (desc->flags & GPIO_TRIGGER_MASK)
+ if (data->irq_flags)
gpio_sysfs_free_irq(dev);
if (flags) {
@@ -277,6 +278,7 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
int status = 0;
+ unsigned int flags = data->irq_flags;
if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
return 0;
@@ -287,12 +289,10 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
/* reconfigure poll(2) support if enabled on one edge only */
- if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
- !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
- unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
-
+ if (flags == GPIO_IRQF_TRIGGER_FALLING ||
+ flags == GPIO_IRQF_TRIGGER_RISING) {
gpio_sysfs_free_irq(dev);
- status = gpio_sysfs_request_irq(dev, trigger_flags);
+ status = gpio_sysfs_request_irq(dev, flags);
}
return status;
@@ -685,7 +685,7 @@ void gpiod_unexport(struct gpio_desc *desc)
/*
* Release irq after deregistration to prevent race with edge_store.
*/
- if (desc->flags & GPIO_TRIGGER_MASK)
+ if (data->irq_flags)
gpio_sysfs_free_irq(dev);
mutex_unlock(&sysfs_lock);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 591257a4359a..e048be7d1f5e 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -81,16 +81,12 @@ struct gpio_desc {
#define FLAG_IS_OUT 1
#define FLAG_EXPORT 2 /* protected by sysfs_lock */
#define FLAG_SYSFS 3 /* exported via /sys/class/gpio/control */
-#define FLAG_TRIG_FALL 4 /* trigger on falling edge */
-#define FLAG_TRIG_RISE 5 /* trigger on rising edge */
#define FLAG_ACTIVE_LOW 6 /* value has active low */
#define FLAG_OPEN_DRAIN 7 /* Gpio is open drain type */
#define FLAG_OPEN_SOURCE 8 /* Gpio is open source type */
#define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */
#define FLAG_IS_HOGGED 11 /* GPIO is hogged */
-#define GPIO_TRIGGER_MASK (BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE))
-
const char *label;
};
--
2.0.5
On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> 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 <[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]>
> ---
> Documentation/gpio/gpio-legacy.txt | 9 -------
> Documentation/gpio/sysfs.txt | 8 -------
> Documentation/zh_CN/gpio.txt | 8 -------
> drivers/gpio/gpiolib-sysfs.c | 48 ++------------------------------------
> include/asm-generic/gpio.h | 5 ----
> include/linux/gpio.h | 7 ------
> include/linux/gpio/consumer.h | 6 -----
> 7 files changed, 2 insertions(+), 89 deletions(-)
>
> diff --git a/Documentation/gpio/gpio-legacy.txt b/Documentation/gpio/gpio-legacy.txt
> index 6f83fa965b4b..79ab5648d69b 100644
> --- a/Documentation/gpio/gpio-legacy.txt
> +++ b/Documentation/gpio/gpio-legacy.txt
> @@ -751,9 +751,6 @@ requested using gpio_request():
> int gpio_export_link(struct device *dev, const char *name,
> unsigned gpio)
>
> - /* change the polarity of a GPIO node in sysfs */
> - int gpio_sysfs_set_active_low(unsigned gpio, int value);
> -
> After a kernel driver requests a GPIO, it may only be made available in
> the sysfs interface by gpio_export(). The driver can control whether the
> signal direction may change. This helps drivers prevent userspace code
> @@ -767,9 +764,3 @@ After the GPIO has been exported, gpio_export_link() allows creating
> symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
> use this to provide the interface under their own device in sysfs with
> a descriptive name.
> -
> -Drivers can use gpio_sysfs_set_active_low() to hide GPIO line polarity
> -differences between boards from user space. This only affects the
> -sysfs interface. Polarity change can be done both before and after
> -gpio_export(), and previously enabled poll(2) support for either
> -rising or falling edge will be reconfigured to follow this setting.
> diff --git a/Documentation/gpio/sysfs.txt b/Documentation/gpio/sysfs.txt
> index c2c3a97f8ff7..535b6a8a7a7c 100644
> --- a/Documentation/gpio/sysfs.txt
> +++ b/Documentation/gpio/sysfs.txt
> @@ -132,9 +132,6 @@ requested using gpio_request():
> int gpiod_export_link(struct device *dev, const char *name,
> struct gpio_desc *desc);
>
> - /* change the polarity of a GPIO node in sysfs */
> - int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
> -
> After a kernel driver requests a GPIO, it may only be made available in
> the sysfs interface by gpiod_export(). The driver can control whether the
> signal direction may change. This helps drivers prevent userspace code
> @@ -148,8 +145,3 @@ After the GPIO has been exported, gpiod_export_link() allows creating
> symlinks from elsewhere in sysfs to the GPIO sysfs node. Drivers can
> use this to provide the interface under their own device in sysfs with
> a descriptive name.
> -
> -Drivers can use gpiod_sysfs_set_active_low() to hide GPIO line polarity
> -differences between boards from user space. Polarity change can be done both
> -before and after gpiod_export(), and previously enabled poll(2) support for
> -either rising or falling edge will be reconfigured to follow this setting.
> diff --git a/Documentation/zh_CN/gpio.txt b/Documentation/zh_CN/gpio.txt
> index d5b8f01833f4..bce972521065 100644
> --- a/Documentation/zh_CN/gpio.txt
> +++ b/Documentation/zh_CN/gpio.txt
> @@ -638,9 +638,6 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
> int gpio_export_link(struct device *dev, const char *name,
> unsigned gpio)
>
> - /* 改变 sysfs 中的一个 GPIO 节点的极性 */
> - int gpio_sysfs_set_active_low(unsigned gpio, int value);
> -
> 在一个内核驱动申请一个 GPIO 之后，它可以通过 gpio_export()使其在 sysfs
> 接口中可见。该驱动可以控制信号方向是否可修改。这有助于防止用户空间代码无意间
> 破坏重要的系统状态。
> @@ -651,8 +648,3 @@ GPIO 控制器的路径类似 /sys/class/gpio/gpiochip42/ (对于从#42 GPIO
> 在 GPIO 被导出之后，gpio_export_link()允许在 sysfs 文件系统的任何地方
> 创建一个到这个 GPIO sysfs 节点的符号链接。这样驱动就可以通过一个描述性的
> 名字，在 sysfs 中他们所拥有的设备下提供一个(到这个 GPIO sysfs 节点的)接口。
> -
> -驱动可以使用 gpio_sysfs_set_active_low() 来在用户空间隐藏电路板之间
> -GPIO 线的极性差异。这个仅对 sysfs 接口起作用。极性的改变可以在 gpio_export()
> -前后进行,且之前使能的轮询操作(poll(2))支持(上升或下降沿)将会被重新配置来遵循
> -这个设置。
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index 31434c5a90ef..8a95a954f514 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
> clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
>
> /* reconfigure poll(2) support if enabled on one edge only */
> - if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> - !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
> + if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> + !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
This change seems to be unrelated to this patch...
Otherwise, I agree and good riddance!
On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> Rename the gpio-chip export/unexport functions to the more descriptive
> names gpiochip_register and gpiochip_unregister.
Since these functions are related to sysfs, wouldn't
gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
sounds better to me) be even more descriptive?
The renaming should probably also cover the non-static gpiod_*
functions of gpiolib-sysfs.c which are equally ambiguous. Basically
anything non-static from gpiolib-sysfs.c should have that prefix.
On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> Clean gpio-chip class device registration and deregistration.
>
> The class device is registered when a gpio-chip is added (or from
> gpiolib_sysfs_init post-core init call), and deregistered when the chip
> is removed.
>
> Store the class device in struct gpio_chip directly rather than do a
> class-device lookup on deregistration. This also removes the need for
> the exported flag.
>
> Signed-off-by: Johan Hovold <[email protected]>
> ---
> drivers/gpio/gpiolib-sysfs.c | 39 +++++++++++++--------------------------
> include/linux/gpio/driver.h | 4 ++--
> 2 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> index c433f075f349..2f8bdce792f6 100644
> --- a/drivers/gpio/gpiolib-sysfs.c
> +++ b/drivers/gpio/gpiolib-sysfs.c
> @@ -567,7 +567,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
> mutex_lock(&sysfs_lock);
>
> /* check if chip is being removed */
> - if (!chip || !chip->exported) {
> + if (!chip || !chip->cdev) {
> status = -ENODEV;
> goto fail_unlock;
> }
> @@ -752,7 +752,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
>
> int gpiochip_export(struct gpio_chip *chip)
> {
> - int status;
> struct device *dev;
>
> /* Many systems register gpio chips for SOC support very early,
> @@ -768,41 +767,29 @@ int gpiochip_export(struct gpio_chip *chip)
> chip, gpiochip_groups,
> "gpiochip%d", chip->base);
> if (IS_ERR(dev))
> - status = PTR_ERR(dev);
> - else
> - status = 0;
> + return PTR_ERR(dev);
>
> mutex_lock(&sysfs_lock);
> - chip->exported = (status == 0);
> + chip->cdev = dev;
> mutex_unlock(&sysfs_lock);
>
> - if (status)
> - chip_dbg(chip, "%s: status %d\n", __func__, status);
> -
> - return status;
> + return 0;
> }
>
> void gpiochip_unexport(struct gpio_chip *chip)
> {
> - int status;
> - struct device *dev;
> struct gpio_desc *desc;
> unsigned int i;
>
> - dev = class_find_device(&gpio_class, NULL, chip, match_export);
> - if (dev) {
> - put_device(dev);
> - device_unregister(dev);
> - /* prevent further gpiod exports */
> - mutex_lock(&sysfs_lock);
> - chip->exported = false;
> - mutex_unlock(&sysfs_lock);
> - status = 0;
> - } else
> - status = -ENODEV;
> + if (!chip->cdev)
> + return;
>
> - if (status)
> - chip_dbg(chip, "%s: status %d\n", __func__, status);
> + device_unregister(chip->cdev);
> +
> + /* prevent further gpiod exports */
> + mutex_lock(&sysfs_lock);
> + chip->cdev = NULL;
> + mutex_unlock(&sysfs_lock);
>
> /* unregister gpiod class devices owned by sysfs */
> for (i = 0; i < chip->ngpio; i++) {
> @@ -830,7 +817,7 @@ static int __init gpiolib_sysfs_init(void)
> */
> spin_lock_irqsave(&gpio_lock, flags);
> list_for_each_entry(chip, &gpio_chips, list) {
> - if (chip->exported)
> + if (chip->cdev)
> continue;
>
> /*
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index f1b36593ec9f..8c26855fc6ec 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -20,6 +20,7 @@ struct seq_file;
> * struct gpio_chip - abstract a GPIO controller
> * @label: for diagnostics
> * @dev: optional device providing the GPIOs
> + * @cdev: class device (may be NULL)
Maybe a comment explaining that this field is non-NULL when a chip is
exported would be useful to understand how it is used in the code?
On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> These patches fix a number of issues with the gpio sysfs interface,
> including
>
> - fix memory leaks and crashes on device hotplug
> - straighten out the convoluted locking
> - reduce sysfs-interface latencies through more fine-grained locking
> - more clearly separate the sysfs-interface implementation from gpiolib
> core
>
> The first patch is marked for stable and could go into 4.1.
>
> Unfortunately we can't just kill the gpio sysfs interface, but these
> patches will make it more manageable and should allow us to implement a
> new user-space interface while maintaining the old one (for a while at
> least) without losing our sanity.
>
> Note that there is still a race between chip remove and gpiod_request (and
> therefore sysfs export), which needs to be fixed separately (for instance as
> part of a generic solution to chip hotplugging).
Thanks a lot for this great series. To my embarrassment I could not
find anything big to say about it. It simply fixes some of the biggest
issues with the sysfs interface and makes things much more
maintainable.
I made a few minor comments, but globally I like this very much.
Reviewed-by: Alexandre Courbot <[email protected]>
On Mon, Apr 27, 2015 at 12:54:15PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> 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 <[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]>
> > ---
> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
> > index 31434c5a90ef..8a95a954f514 100644
> > --- a/drivers/gpio/gpiolib-sysfs.c
> > +++ b/drivers/gpio/gpiolib-sysfs.c
> > @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
> > clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> >
> > /* reconfigure poll(2) support if enabled on one edge only */
> > - if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> > - !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
> > + if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
> > + !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
>
> This change seems to be unrelated to this patch...
This helper is now only called from the attribute operation and dev
will never be NULL.
On the other hand, it was never called with a NULL argument before this
patch either (the test has always been bogus). Let me know if you prefer
this bit to be a separate patch.
> Otherwise, I agree and good riddance!
Thanks,
Johan
On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> > Rename the gpio-chip export/unexport functions to the more descriptive
> > names gpiochip_register and gpiochip_unregister.
>
> Since these functions are related to sysfs, wouldn't
> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> sounds better to me) be even more descriptive?
I'm trying to get rid of the made up notion of "exporting" things. What
we are doing is to register devices with driver core, and that involves
a representation is sysfs.
Eventually, a gpio chip should always be registered with driver core and
this is not directly related to the (by then hopefully legacy)
sysfs-interface.
> The renaming should probably also cover the non-static gpiod_*
> functions of gpiolib-sysfs.c which are equally ambiguous. Basically
> anything non-static from gpiolib-sysfs.c should have that prefix.
This would be a different change, and some of those functions are also
part of the consumer API.
Johan
On Mon, Apr 27, 2015 at 5:16 PM, Johan Hovold <[email protected]> wrote:
> On Mon, Apr 27, 2015 at 12:54:15PM +0900, Alexandre Courbot wrote:
>> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> 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 <[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]>
>> > ---
>
>> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
>> > index 31434c5a90ef..8a95a954f514 100644
>> > --- a/drivers/gpio/gpiolib-sysfs.c
>> > +++ b/drivers/gpio/gpiolib-sysfs.c
>> > @@ -293,8 +293,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
>> > clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
>> >
>> > /* reconfigure poll(2) support if enabled on one edge only */
>> > - if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
>> > - !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
>> > + if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
>> > + !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
>>
>> This change seems to be unrelated to this patch...
>
> This helper is now only called from the attribute operation and dev
> will never be NULL.
>
> On the other hand, it was never called with a NULL argument before this
> patch either (the test has always been bogus). Let me know if you prefer
> this bit to be a separate patch.
Nope, after reading your explanation I understand why you want to have
this here. Thanks for clarifying!
On Mon, Apr 27, 2015 at 12:54:41PM +0900, Alexandre Courbot wrote:
> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> > Clean gpio-chip class device registration and deregistration.
> >
> > The class device is registered when a gpio-chip is added (or from
> > gpiolib_sysfs_init post-core init call), and deregistered when the chip
> > is removed.
> >
> > Store the class device in struct gpio_chip directly rather than do a
> > class-device lookup on deregistration. This also removes the need for
> > the exported flag.
> >
> > Signed-off-by: Johan Hovold <[email protected]>
> > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> > index f1b36593ec9f..8c26855fc6ec 100644
> > --- a/include/linux/gpio/driver.h
> > +++ b/include/linux/gpio/driver.h
> > @@ -20,6 +20,7 @@ struct seq_file;
> > * struct gpio_chip - abstract a GPIO controller
> > * @label: for diagnostics
> > * @dev: optional device providing the GPIOs
> > + * @cdev: class device (may be NULL)
>
> Maybe a comment explaining that this field is non-NULL when a chip is
> exported would be useful to understand how it is used in the code?
I've added comments where the field is used. I didn't want to get into
explaining sysfs implementation details in the header file, but the "may
be NULL" is there as a hint to actually look at the code.
And a gpio chip will always be registered with driver core (rather than
"exported" ;) ) until it is removed. [ Currently we also allow for
"late" registration, though. ]
This is related to the issue discussed in my last mail, and again the
plan is to let chip registration be used for more than the sysfs
interface.
Johan
On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <[email protected]> wrote:
> On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
>> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
>> > Rename the gpio-chip export/unexport functions to the more descriptive
>> > names gpiochip_register and gpiochip_unregister.
>>
>> Since these functions are related to sysfs, wouldn't
>> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
>> sounds better to me) be even more descriptive?
>
> I'm trying to get rid of the made up notion of "exporting" things. What
> we are doing is to register devices with driver core, and that involves
> a representation is sysfs.
>
> Eventually, a gpio chip should always be registered with driver core and
> this is not directly related to the (by then hopefully legacy)
> sysfs-interface.
I understand and agree, but even after your patch series, registration
of a gpio chip with the driver core is still dependent on the
CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
and either always register GPIO chips (effectively moving the call to
device_create into gpiolib.c) and only keep the legacy bits in
gpiolib-sysfs.c?
We would then only enable the legacy sysfs interface if
CONFIG_GPIO_SYSFS is set, but the gpiochip nodes would still appear as
long as core sysfs support is compiled in.
>> The renaming should probably also cover the non-static gpiod_*
>> functions of gpiolib-sysfs.c which are equally ambiguous. Basically
>> anything non-static from gpiolib-sysfs.c should have that prefix.
>
> This would be a different change, and some of those functions are also
> part of the consumer API.
That could be another patch. I don't mind if an exported function name
changes for consistency as long as all in-kernel users are updated as
well.
On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <[email protected]> wrote:
> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> >> > Rename the gpio-chip export/unexport functions to the more descriptive
> >> > names gpiochip_register and gpiochip_unregister.
> >>
> >> Since these functions are related to sysfs, wouldn't
> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> >> sounds better to me) be even more descriptive?
> >
> > I'm trying to get rid of the made up notion of "exporting" things. What
> > we are doing is to register devices with driver core, and that involves
> > a representation is sysfs.
> >
> > Eventually, a gpio chip should always be registered with driver core and
> > this is not directly related to the (by then hopefully legacy)
> > sysfs-interface.
>
> I understand and agree, but even after your patch series, registration
> of a gpio chip with the driver core is still dependent on the
> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
> and either always register GPIO chips (effectively moving the call to
> device_create into gpiolib.c) and only keep the legacy bits in
> gpiolib-sysfs.c?
That is the plan yes, but there's only so much I can do in one series.
;) The current crazy sysfs API also prevents the decoupling of the sysfs
interface from chip device registration.
Johan
On Mon, Apr 27, 2015 at 6:05 PM, Johan Hovold <[email protected]> wrote:
> On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
>> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <[email protected]> wrote:
>> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
>> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
>> >> > Rename the gpio-chip export/unexport functions to the more descriptive
>> >> > names gpiochip_register and gpiochip_unregister.
>> >>
>> >> Since these functions are related to sysfs, wouldn't
>> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
>> >> sounds better to me) be even more descriptive?
>> >
>> > I'm trying to get rid of the made up notion of "exporting" things. What
>> > we are doing is to register devices with driver core, and that involves
>> > a representation is sysfs.
>> >
>> > Eventually, a gpio chip should always be registered with driver core and
>> > this is not directly related to the (by then hopefully legacy)
>> > sysfs-interface.
>>
>> I understand and agree, but even after your patch series, registration
>> of a gpio chip with the driver core is still dependent on the
>> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
>> and either always register GPIO chips (effectively moving the call to
>> device_create into gpiolib.c) and only keep the legacy bits in
>> gpiolib-sysfs.c?
>
> That is the plan yes, but there's only so much I can do in one series.
> ;) The current crazy sysfs API also prevents the decoupling of the sysfs
> interface from chip device registration.
Sounds good then. This patch series is great anyway, so if Linus has
nothing against it I hope we can merge it quickly.
On Tue, Apr 28, 2015 at 12:27:16PM +0900, Alexandre Courbot wrote:
> On Mon, Apr 27, 2015 at 6:05 PM, Johan Hovold <[email protected]> wrote:
> > On Mon, Apr 27, 2015 at 05:50:54PM +0900, Alexandre Courbot wrote:
> >> On Mon, Apr 27, 2015 at 5:27 PM, Johan Hovold <[email protected]> wrote:
> >> > On Mon, Apr 27, 2015 at 12:54:36PM +0900, Alexandre Courbot wrote:
> >> >> On Wed, Apr 22, 2015 at 12:42 AM, Johan Hovold <[email protected]> wrote:
> >> >> > Rename the gpio-chip export/unexport functions to the more descriptive
> >> >> > names gpiochip_register and gpiochip_unregister.
> >> >>
> >> >> Since these functions are related to sysfs, wouldn't
> >> >> gpiochip_sysfs_export (or gpiochip_sysfs_register, although the former
> >> >> sounds better to me) be even more descriptive?
> >> >
> >> > I'm trying to get rid of the made up notion of "exporting" things. What
> >> > we are doing is to register devices with driver core, and that involves
> >> > a representation is sysfs.
> >> >
> >> > Eventually, a gpio chip should always be registered with driver core and
> >> > this is not directly related to the (by then hopefully legacy)
> >> > sysfs-interface.
> >>
> >> I understand and agree, but even after your patch series, registration
> >> of a gpio chip with the driver core is still dependent on the
> >> CONFIG_GPIO_SYSFS option. So maybe you could push the logic further
> >> and either always register GPIO chips (effectively moving the call to
> >> device_create into gpiolib.c) and only keep the legacy bits in
> >> gpiolib-sysfs.c?
> >
> > That is the plan yes, but there's only so much I can do in one series.
> > ;) The current crazy sysfs API also prevents the decoupling of the sysfs
> > interface from chip device registration.
>
> Sounds good then. This patch series is great anyway, so if Linus has
> nothing against it I hope we can merge it quickly.
Thanks for the review.
Johan
On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <[email protected]> wrote:
> Unregister GPIOs requested through sysfs at chip remove to avoid leaking
> the associated memory and sysfs entries.
>
> The stale sysfs entries prevented the gpio numbers from being exported
> when the gpio range was later reused (e.g. at device reconnect).
>
> This also fixes the related module-reference leak.
>
> Note that kernfs makes sure that any on-going sysfs operations finish
> before the class devices are unregistered and that further accesses
> fail.
>
> The chip exported flag is used to prevent gpiod exports during removal.
> This also makes it harder to trigger, but does not fix, the related race
> between gpiochip_remove and export_store, which is really a race with
> gpiod_request that needs to be addressed separately.
>
> Also note that this would prevent the crashes (e.g. NULL-dereferences)
> at reconnect that affects pre-3.18 kernels, as well as use-after-free on
> operations on open attribute files on pre-3.14 kernels (prior to
> kernfs).
>
> Fixes: d8f388d8dc8d ("gpio: sysfs interface")
> Cc: stable <[email protected]> # v2.6.27: 01cca93a9491
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied for fixes.
I worry a bit about what userspaces do out there, but they
cannot reasonably have behaviours tied to in-flight removal
of GPIO chips, that would be bizarre.
Yours,
Linus Walleij
On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <[email protected]> wrote:
> Drop redundant lock-as-irq in gpio_setup_irq, which has already been
> handled when requesting and releasing the irq (i.e. in the irq chip
> irq_request_resources and irq_release_resources callbacks).
Well we would hope they all do that. And I hope for the vast majority
that is true, but there is a TODO to go over all gpiochip drivers
(some which are elsewhere in the kernel than drivers/gpio) and
make sure they actually do so.
Right now it's a bit arbitrary if so happens, and in not marked by
the driver as IRQ then this kicks in and provides an additional
protection.
But maybe that's overzealous, what do people say?
Yours,
Linus Walleij
On Wed, Apr 29, 2015 at 11:44:18PM +0200, Linus Walleij wrote:
> On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <[email protected]> wrote:
>
> > Unregister GPIOs requested through sysfs at chip remove to avoid leaking
> > the associated memory and sysfs entries.
> >
> > The stale sysfs entries prevented the gpio numbers from being exported
> > when the gpio range was later reused (e.g. at device reconnect).
> >
> > This also fixes the related module-reference leak.
> >
> > Note that kernfs makes sure that any on-going sysfs operations finish
> > before the class devices are unregistered and that further accesses
> > fail.
> >
> > The chip exported flag is used to prevent gpiod exports during removal.
> > This also makes it harder to trigger, but does not fix, the related race
> > between gpiochip_remove and export_store, which is really a race with
> > gpiod_request that needs to be addressed separately.
> >
> > Also note that this would prevent the crashes (e.g. NULL-dereferences)
> > at reconnect that affects pre-3.18 kernels, as well as use-after-free on
> > operations on open attribute files on pre-3.14 kernels (prior to
> > kernfs).
> >
> > Fixes: d8f388d8dc8d ("gpio: sysfs interface")
> > Cc: stable <[email protected]> # v2.6.27: 01cca93a9491
> > Signed-off-by: Johan Hovold <[email protected]>
>
> Patch applied for fixes.
>
> I worry a bit about what userspaces do out there, but they
> cannot reasonably have behaviours tied to in-flight removal
> of GPIO chips, that would be bizarre.
You shouldn't worry too much; even before this patch userspace would see
an -ENODEV when accessing an open sysfs attribute file of a disconnected
device as kernfs would orphan the file -- only now without the associated
leaks and crashes. ;)
Johan
On Wed, Apr 29, 2015 at 11:48:57PM +0200, Linus Walleij wrote:
> On Tue, Apr 21, 2015 at 5:42 PM, Johan Hovold <[email protected]> wrote:
>
> > Drop redundant lock-as-irq in gpio_setup_irq, which has already been
> > handled when requesting and releasing the irq (i.e. in the irq chip
> > irq_request_resources and irq_release_resources callbacks).
>
> Well we would hope they all do that. And I hope for the vast majority
> that is true, but there is a TODO to go over all gpiochip drivers
> (some which are elsewhere in the kernel than drivers/gpio) and
> make sure they actually do so.
>
> Right now it's a bit arbitrary if so happens, and in not marked by
> the driver as IRQ then this kicks in and provides an additional
> protection.
>
> But maybe that's overzealous, what do people say?
No, you're right. The drivers that fail to do this needs to be fixed,
but the "redundant" lock-as-irq in the sysfs interface should not be
removed before that.
I'll respin the series and add it back with a comment explaining why
gpiochip_lock_as_irq is currently called twice.
Thanks,
Johan