These patches fix a number of issues with the gpio sysfs interface,
including
- fix memory leaks and crashes on device hotplug
- straighten out the convoluted locking
- reduce sysfs-interface latencies through more fine-grained locking
- more clearly separate the sysfs-interface implementation from gpiolib
core
The first patch is marked for stable and could go into 4.1. [ May
already have been applied but not pushed by Linus, but included in v2
for completeness. ]
Unfortunately we can't just kill the gpio sysfs interface, but these
patches will make it more manageable and should allow us to implement a
new user-space interface while maintaining the old one (for a while at
least) without losing our sanity.
Note that there is still a race between chip remove and gpiod_request (and
therefore sysfs export), which needs to be fixed separately (for instance as
part of a generic solution to chip hotplugging).
Johan
Changes since v1:
- Keep explicit lock-as-irq call in sysfs and add comment that it
should be removed once the broken drivers have been fixed (patch
3/23).
- Add comment that the class-device field of struct gpiochip is used by
the sysfs interface (patch 6/23).
- Add "sysfs"-infix to gpiochip sysfs registration functions as suggested
by Alexandre (patch 7/23).
Johan Hovold (23):
gpio: sysfs: fix memory leaks and device hotplug
gpio: clean up gpiochip_remove
gpio: sysfs: fix redundant lock-as-irq handling
gpio: sysfs: preparatory clean ups
gpio: sysfs: reduce gpiochip-export locking scope
gpio: sysfs: clean up chip class-device handling
gpio: sysfs: rename gpiochip registration functions
gpio: remove gpiod_sysfs_set_active_low
gpio: sysfs: use DEVICE_ATTR macros
gpio: sysfs: release irq after class-device deregistration
gpio: sysfs: remove redundant export tests
gpio: sysfs: add gpiod class-device data
gpio: sysfs: remove redundant gpio-descriptor parameters
gpio: sysfs: clean up interrupt-interface implementation
gpio: sysfs: only call irq helper if needed
gpio: sysfs: split irq allocation and deallocation
gpio: sysfs: clean up edge_store
gpio: sysfs: clean up gpiod_export_link locking
gpio: sysfs: use per-gpio locking
gpio: sysfs: fix race between gpiod export and unexport
gpio: sysfs: rename active-low helper
gpio: sysfs: remove FLAG_SYSFS_DIR
gpio: sysfs: move irq trigger flags to class-device data
Documentation/gpio/gpio-legacy.txt | 9 -
Documentation/gpio/sysfs.txt | 8 -
Documentation/zh_CN/gpio.txt | 8 -
drivers/gpio/gpiolib-sysfs.c | 578 ++++++++++++++++++-------------------
drivers/gpio/gpiolib.c | 18 +-
drivers/gpio/gpiolib.h | 16 +-
include/asm-generic/gpio.h | 5 -
include/linux/gpio.h | 7 -
include/linux/gpio/consumer.h | 6 -
include/linux/gpio/driver.h | 4 +-
10 files changed, 290 insertions(+), 369 deletions(-)
--
2.0.5
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
Drivers should call gpiochip_lock_as_irq (which prevents the pin
direction from being changed) in their irq_request_resources callbacks
but some drivers currently fail to do so.
Instead a second, explicit and often redundant call to lock-as-irq is
made by the sysfs-interface implementation after an irq has been
requested.
Move the explicit call before the irq-request to match the unlock done
after the irq is later released. Note that this also fixes an irq leak,
should the explicit call ever have failed.
Also add a comment about removing the redundant call once the broken
drivers have been fixed.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index af3bc7a8033b..b2b62cc6f9e1 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -195,20 +195,28 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
}
}
- ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
- "gpiolib", value_sd);
+ /*
+ * FIXME: This should be done in the irq_request_resources callback
+ * when the irq is requested, but a few drivers currently fail
+ * to do so.
+ *
+ * Remove this redundant call (along with the corresponding
+ * unlock) when those drivers have been fixed.
+ */
+ ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
if (ret < 0)
goto free_id;
- ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
- if (ret < 0) {
- gpiod_warn(desc, "failed to flag the GPIO for IRQ\n");
- goto free_id;
- }
+ ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
+ "gpiolib", value_sd);
+ if (ret < 0)
+ goto err_unlock;
desc->flags |= gpio_flags;
return 0;
+err_unlock:
+ gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
free_id:
idr_remove(&dirent_idr, id);
desc->flags &= GPIO_FLAGS_MASK;
--
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 b2b62cc6f9e1..658ed28e6d7d 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -599,7 +599,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
goto fail_unlock;
}
- if (desc->chip->direction_input && desc->chip->direction_output &&
+ if (chip->direction_input && chip->direction_output &&
direction_may_change) {
set_bit(FLAG_SYSFS_DIR, &desc->flags);
}
@@ -607,10 +607,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
spin_unlock_irqrestore(&gpio_lock, flags);
offset = gpio_chip_hwgpio(desc);
- if (desc->chip->names && desc->chip->names[offset])
- ioname = desc->chip->names[offset];
+ if (chip->names && chip->names[offset])
+ ioname = chip->names[offset];
- dev = device_create_with_groups(&gpio_class, desc->chip->dev,
+ dev = device_create_with_groups(&gpio_class, chip->dev,
MKDEV(0, 0), desc, gpio_groups,
ioname ? ioname : "gpio%u",
desc_to_gpio(desc));
--
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 658ed28e6d7d..d19bf234e878 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -779,7 +779,6 @@ int gpiochip_export(struct gpio_chip *chip)
return 0;
/* use chip->base for the ID; it's already known to be unique */
- mutex_lock(&sysfs_lock);
dev = device_create_with_groups(&gpio_class, chip->dev, MKDEV(0, 0),
chip, gpiochip_groups,
"gpiochip%d", chip->base);
@@ -787,6 +786,8 @@ int gpiochip_export(struct gpio_chip *chip)
status = PTR_ERR(dev);
else
status = 0;
+
+ mutex_lock(&sysfs_lock);
chip->exported = (status == 0);
mutex_unlock(&sysfs_lock);
@@ -803,17 +804,17 @@ void gpiochip_unexport(struct gpio_chip *chip)
struct gpio_desc *desc;
unsigned int i;
- mutex_lock(&sysfs_lock);
dev = class_find_device(&gpio_class, NULL, chip, match_export);
if (dev) {
put_device(dev);
device_unregister(dev);
/* prevent further gpiod exports */
+ mutex_lock(&sysfs_lock);
chip->exported = false;
+ mutex_unlock(&sysfs_lock);
status = 0;
} else
status = -ENODEV;
- mutex_unlock(&sysfs_lock);
if (status)
chip_dbg(chip, "%s: status %d\n", __func__, status);
--
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 d19bf234e878..767b79adb9a4 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -582,7 +582,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
mutex_lock(&sysfs_lock);
/* check if chip is being removed */
- if (!chip || !chip->exported) {
+ if (!chip || !chip->cdev) {
status = -ENODEV;
goto fail_unlock;
}
@@ -767,7 +767,6 @@ EXPORT_SYMBOL_GPL(gpiod_unexport);
int gpiochip_export(struct gpio_chip *chip)
{
- int status;
struct device *dev;
/* Many systems register gpio chips for SOC support very early,
@@ -783,41 +782,29 @@ int gpiochip_export(struct gpio_chip *chip)
chip, gpiochip_groups,
"gpiochip%d", chip->base);
if (IS_ERR(dev))
- status = PTR_ERR(dev);
- else
- status = 0;
+ return PTR_ERR(dev);
mutex_lock(&sysfs_lock);
- chip->exported = (status == 0);
+ chip->cdev = dev;
mutex_unlock(&sysfs_lock);
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
-
- return status;
+ return 0;
}
void gpiochip_unexport(struct gpio_chip *chip)
{
- int status;
- struct device *dev;
struct gpio_desc *desc;
unsigned int i;
- dev = class_find_device(&gpio_class, NULL, chip, match_export);
- if (dev) {
- put_device(dev);
- device_unregister(dev);
- /* prevent further gpiod exports */
- mutex_lock(&sysfs_lock);
- chip->exported = false;
- mutex_unlock(&sysfs_lock);
- status = 0;
- } else
- status = -ENODEV;
+ if (!chip->cdev)
+ return;
- if (status)
- chip_dbg(chip, "%s: status %d\n", __func__, status);
+ device_unregister(chip->cdev);
+
+ /* prevent further gpiod exports */
+ mutex_lock(&sysfs_lock);
+ chip->cdev = NULL;
+ mutex_unlock(&sysfs_lock);
/* unregister gpiod class devices owned by sysfs */
for (i = 0; i < chip->ngpio; i++) {
@@ -845,7 +832,7 @@ static int __init gpiolib_sysfs_init(void)
*/
spin_lock_irqsave(&gpio_lock, flags);
list_for_each_entry(chip, &gpio_chips, list) {
- if (chip->exported)
+ if (chip->cdev)
continue;
/*
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index f1b36593ec9f..2c1e639f66bd 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 used by sysfs interface (may be NULL)
* @owner: helps prevent removal of modules exporting active GPIOs
* @list: links gpio_chips together for traversal
* @request: optional hook for chip-specific activation, such as
@@ -57,7 +58,6 @@ struct seq_file;
* implies that if the chip supports IRQs, these IRQs need to be threaded
* as the chip access may sleep when e.g. reading out the IRQ status
* registers.
- * @exported: flags if the gpiochip is exported for use from sysfs. Private.
* @irq_not_threaded: flag must be set if @can_sleep is set but the
* IRQs don't need to be threaded
*
@@ -74,6 +74,7 @@ struct seq_file;
struct gpio_chip {
const char *label;
struct device *dev;
+ struct device *cdev;
struct module *owner;
struct list_head list;
@@ -109,7 +110,6 @@ struct gpio_chip {
const char *const *names;
bool can_sleep;
bool irq_not_threaded;
- bool exported;
#ifdef CONFIG_GPIOLIB_IRQCHIP
/*
--
2.0.5
Rename the gpio-chip export/unexport functions to the more descriptive
names gpiochip_sysfs_register and gpiochip_sysfs_unregister.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 16 +++++++++-------
drivers/gpio/gpiolib.c | 4 ++--
drivers/gpio/gpiolib.h | 8 ++++----
3 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 767b79adb9a4..aeb73ef2955e 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -765,13 +765,14 @@ void gpiod_unexport(struct gpio_desc *desc)
}
EXPORT_SYMBOL_GPL(gpiod_unexport);
-int gpiochip_export(struct gpio_chip *chip)
+int gpiochip_sysfs_register(struct gpio_chip *chip)
{
struct device *dev;
- /* Many systems register gpio chips for SOC support very early,
+ /*
+ * Many systems add gpio chips for SOC support very early,
* before driver model support is available. In those cases we
- * export this later, in gpiolib_sysfs_init() ... here we just
+ * register later, in gpiolib_sysfs_init() ... here we just
* verify that _some_ field of gpio_class got initialized.
*/
if (!gpio_class.p)
@@ -791,7 +792,7 @@ int gpiochip_export(struct gpio_chip *chip)
return 0;
}
-void gpiochip_unexport(struct gpio_chip *chip)
+void gpiochip_sysfs_unregister(struct gpio_chip *chip)
{
struct gpio_desc *desc;
unsigned int i;
@@ -836,15 +837,16 @@ static int __init gpiolib_sysfs_init(void)
continue;
/*
- * TODO we yield gpio_lock here because gpiochip_export()
- * acquires a mutex. This is unsafe and needs to be fixed.
+ * TODO we yield gpio_lock here because
+ * gpiochip_sysfs_register() acquires a mutex. This is unsafe
+ * and needs to be fixed.
*
* Also it would be nice to use gpiochip_find() here so we
* can keep gpio_chips local to gpiolib.c, but the yield of
* gpio_lock prevents us from doing this.
*/
spin_unlock_irqrestore(&gpio_lock, flags);
- status = gpiochip_export(chip);
+ status = gpiochip_sysfs_register(chip);
spin_lock_irqsave(&gpio_lock, flags);
}
spin_unlock_irqrestore(&gpio_lock, flags);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5a5c208d31c7..2ce3df3504e6 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_sysfs_register(chip);
if (status)
goto err_remove_chip;
@@ -330,7 +330,7 @@ void gpiochip_remove(struct gpio_chip *chip)
unsigned id;
bool requested = false;
- gpiochip_unexport(chip);
+ gpiochip_sysfs_unregister(chip);
gpiochip_irqchip_remove(chip);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 54bc5ec91398..efdd5ded4965 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_sysfs_register(struct gpio_chip *chip);
+void gpiochip_sysfs_unregister(struct gpio_chip *chip);
#else
-static inline int gpiochip_export(struct gpio_chip *chip)
+static inline int gpiochip_sysfs_register(struct gpio_chip *chip)
{
return 0;
}
-static inline void gpiochip_unexport(struct gpio_chip *chip)
+static inline void gpiochip_sysfs_unregister(struct gpio_chip *chip)
{
}
--
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 aeb73ef2955e..9dcd346a20fb 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -308,8 +308,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
/* reconfigure poll(2) support if enabled on one edge only */
- if (dev != NULL && (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
- !!test_bit(FLAG_TRIG_FALL, &desc->flags))) {
+ if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
+ !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
gpio_setup_irq(desc, dev, 0);
@@ -681,50 +681,6 @@ int gpiod_export_link(struct device *dev, const char *name,
EXPORT_SYMBOL_GPL(gpiod_export_link);
/**
- * gpiod_sysfs_set_active_low - set the polarity of gpio sysfs value
- * @gpio: gpio to change
- * @value: non-zero to use active low, i.e. inverted values
- *
- * Set the polarity of /sys/class/gpio/gpioN/value sysfs attribute.
- * The GPIO does not have to be exported yet. If poll(2) support has
- * been enabled for either rising or falling edge, it will be
- * reconfigured to follow the new polarity.
- *
- * Returns zero on success, else an error.
- */
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
-{
- struct device *dev = NULL;
- int status = -EINVAL;
-
- if (!desc) {
- pr_warn("%s: invalid GPIO\n", __func__);
- return -EINVAL;
- }
-
- mutex_lock(&sysfs_lock);
-
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
- dev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (dev == NULL) {
- status = -ENODEV;
- goto unlock;
- }
- }
-
- status = sysfs_set_active_low(desc, dev, value);
- put_device(dev);
-unlock:
- mutex_unlock(&sysfs_lock);
-
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
-
- return status;
-}
-EXPORT_SYMBOL_GPL(gpiod_sysfs_set_active_low);
-
-/**
* gpiod_unexport - reverse effect of gpio_export()
* @gpio: gpio to make unavailable
*
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 9bb0d11729c9..40ec1433f05d 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -128,11 +128,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
return gpiod_export_link(dev, name, gpio_to_desc(gpio));
}
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
- return gpiod_sysfs_set_active_low(gpio_to_desc(gpio), value);
-}
-
static inline void gpio_unexport(unsigned gpio)
{
gpiod_unexport(gpio_to_desc(gpio));
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index ab81339a8590..d12b5d566e4b 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -196,13 +196,6 @@ static inline int gpio_export_link(struct device *dev, const char *name,
return -EINVAL;
}
-static inline int gpio_sysfs_set_active_low(unsigned gpio, int value)
-{
- /* GPIO can never have been requested */
- WARN_ON(1);
- return -EINVAL;
-}
-
static inline void gpio_unexport(unsigned gpio)
{
/* GPIO can never have been exported */
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 3a7c9ffd5ab9..09a7fb0062a6 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -449,7 +449,6 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
int gpiod_export(struct gpio_desc *desc, bool direction_may_change);
int gpiod_export_link(struct device *dev, const char *name,
struct gpio_desc *desc);
-int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value);
void gpiod_unexport(struct gpio_desc *desc);
#else /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
@@ -466,11 +465,6 @@ static inline int gpiod_export_link(struct device *dev, const char *name,
return -ENOSYS;
}
-static inline int gpiod_sysfs_set_active_low(struct gpio_desc *desc, int value)
-{
- return -ENOSYS;
-}
-
static inline void gpiod_unexport(struct gpio_desc *desc)
{
}
--
2.0.5
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 9dcd346a20fb..a78dabd4035b 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)
{
@@ -237,7 +233,7 @@ static const struct {
{ "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
};
-static ssize_t gpio_edge_show(struct device *dev,
+static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -264,7 +260,7 @@ static ssize_t gpio_edge_show(struct device *dev,
return status;
}
-static ssize_t gpio_edge_store(struct device *dev,
+static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -291,8 +287,7 @@ found:
return status;
}
-
-static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store);
+static DEVICE_ATTR_RW(edge);
static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
int value)
@@ -319,7 +314,7 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
return status;
}
-static ssize_t gpio_active_low_show(struct device *dev,
+static ssize_t active_low_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -338,7 +333,7 @@ static ssize_t gpio_active_low_show(struct device *dev,
return status;
}
-static ssize_t gpio_active_low_store(struct device *dev,
+static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpio_desc *desc = dev_get_drvdata(dev);
@@ -360,9 +355,7 @@ static ssize_t gpio_active_low_store(struct device *dev,
return status ? : size;
}
-
-static DEVICE_ATTR(active_low, 0644,
- gpio_active_low_show, gpio_active_low_store);
+static DEVICE_ATTR_RW(active_low);
static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
@@ -410,32 +403,32 @@ static const struct attribute_group *gpio_groups[] = {
* /ngpio ... matching gpio_chip.ngpio
*/
-static ssize_t chip_base_show(struct device *dev,
+static ssize_t base_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%d\n", chip->base);
}
-static DEVICE_ATTR(base, 0444, chip_base_show, NULL);
+static DEVICE_ATTR_RO(base);
-static ssize_t chip_label_show(struct device *dev,
+static ssize_t label_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%s\n", chip->label ? : "");
}
-static DEVICE_ATTR(label, 0444, chip_label_show, NULL);
+static DEVICE_ATTR_RO(label);
-static ssize_t chip_ngpio_show(struct device *dev,
+static ssize_t ngpio_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_chip *chip = dev_get_drvdata(dev);
return sprintf(buf, "%u\n", chip->ngpio);
}
-static DEVICE_ATTR(ngpio, 0444, chip_ngpio_show, NULL);
+static DEVICE_ATTR_RO(ngpio);
static struct attribute *gpiochip_attrs[] = {
&dev_attr_base.attr,
--
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 a78dabd4035b..e4b079eec9bc 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -695,7 +695,6 @@ void gpiod_unexport(struct gpio_desc *desc)
dev = class_find_device(&gpio_class, NULL, desc, match_export);
if (dev) {
- gpio_setup_irq(desc, dev, 0);
clear_bit(FLAG_SYSFS_DIR, &desc->flags);
clear_bit(FLAG_EXPORT, &desc->flags);
} else
@@ -706,6 +705,11 @@ void gpiod_unexport(struct gpio_desc *desc)
if (dev) {
device_unregister(dev);
+ /*
+ * Release irq after deregistration to prevent race with
+ * edge_store.
+ */
+ gpio_setup_irq(desc, dev, 0);
put_device(dev);
}
--
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 e4b079eec9bc..10baf31efe74 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);
@@ -237,23 +226,18 @@ static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
const struct gpio_desc *desc = dev_get_drvdata(dev);
- ssize_t status;
+ unsigned long mask;
+ ssize_t status = 0;
+ int i;
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else {
- int i;
-
- status = 0;
- for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
- if ((desc->flags & GPIO_TRIGGER_MASK)
- == trigger_types[i].flags) {
- status = sprintf(buf, "%s\n",
- trigger_types[i].name);
- break;
- }
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
+ mask = desc->flags & GPIO_TRIGGER_MASK;
+ if (mask == trigger_types[i].flags) {
+ status = sprintf(buf, "%s\n", trigger_types[i].name);
+ break;
+ }
}
mutex_unlock(&sysfs_lock);
@@ -275,13 +259,9 @@ static ssize_t edge_store(struct device *dev,
found:
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else {
- status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
- if (!status)
- status = size;
- }
+ status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ if (!status)
+ status = size;
mutex_unlock(&sysfs_lock);
@@ -322,10 +302,7 @@ static ssize_t active_low_show(struct device *dev,
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags))
- status = -EIO;
- else
- status = sprintf(buf, "%d\n",
+ status = sprintf(buf, "%d\n",
!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
mutex_unlock(&sysfs_lock);
@@ -338,18 +315,13 @@ static ssize_t active_low_store(struct device *dev,
{
struct gpio_desc *desc = dev_get_drvdata(dev);
ssize_t status;
+ long value;
mutex_lock(&sysfs_lock);
- if (!test_bit(FLAG_EXPORT, &desc->flags)) {
- status = -EIO;
- } else {
- long value;
-
- status = kstrtol(buf, 0, &value);
- if (status == 0)
- status = sysfs_set_active_low(desc, dev, value != 0);
- }
+ status = kstrtol(buf, 0, &value);
+ if (status == 0)
+ status = sysfs_set_active_low(desc, dev, value != 0);
mutex_unlock(&sysfs_lock);
--
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 10baf31efe74..097e7e539c9d 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);
@@ -225,7 +234,8 @@ static const struct {
static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- const struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
unsigned long mask;
ssize_t status = 0;
int i;
@@ -247,7 +257,8 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
int i;
@@ -297,7 +308,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
static ssize_t active_low_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- const struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
mutex_lock(&sysfs_lock);
@@ -313,7 +325,8 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
ssize_t status;
long value;
@@ -333,7 +346,8 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
int n)
{
struct device *dev = container_of(kobj, struct device, kobj);
- struct gpio_desc *desc = dev_get_drvdata(dev);
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
umode_t mode = attr->mode;
bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
@@ -525,6 +539,7 @@ static struct class gpio_class = {
int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
{
struct gpio_chip *chip;
+ struct gpiod_data *data;
unsigned long flags;
int status;
const char *ioname = NULL;
@@ -549,7 +564,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
/* check if chip is being removed */
if (!chip || !chip->cdev) {
status = -ENODEV;
- goto fail_unlock;
+ goto err_unlock;
}
spin_lock_irqsave(&gpio_lock, flags);
@@ -561,7 +576,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
test_bit(FLAG_REQUESTED, &desc->flags),
test_bit(FLAG_EXPORT, &desc->flags));
status = -EPERM;
- goto fail_unlock;
+ goto err_unlock;
}
if (chip->direction_input && chip->direction_output &&
@@ -571,33 +586,45 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
spin_unlock_irqrestore(&gpio_lock, flags);
+ data = kzalloc(sizeof(*data), GFP_KERNEL);
+ if (!data) {
+ status = -ENOMEM;
+ goto err_unlock;
+ }
+
+ data->desc = desc;
+
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
ioname = chip->names[offset];
dev = device_create_with_groups(&gpio_class, chip->dev,
- MKDEV(0, 0), desc, gpio_groups,
+ MKDEV(0, 0), data, gpio_groups,
ioname ? ioname : "gpio%u",
desc_to_gpio(desc));
if (IS_ERR(dev)) {
status = PTR_ERR(dev);
- goto fail_unlock;
+ goto err_free_data;
}
set_bit(FLAG_EXPORT, &desc->flags);
mutex_unlock(&sysfs_lock);
return 0;
-fail_unlock:
+err_free_data:
+ kfree(data);
+err_unlock:
mutex_unlock(&sysfs_lock);
gpiod_dbg(desc, "%s: status %d\n", __func__, status);
return status;
}
EXPORT_SYMBOL_GPL(gpiod_export);
-static int match_export(struct device *dev, const void *data)
+static int match_export(struct device *dev, const void *desc)
{
- return dev_get_drvdata(dev) == data;
+ struct gpiod_data *data = dev_get_drvdata(dev);
+
+ return data->desc == desc;
}
/**
@@ -653,6 +680,7 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
*/
void gpiod_unexport(struct gpio_desc *desc)
{
+ struct gpiod_data *data;
int status = 0;
struct device *dev = NULL;
@@ -676,6 +704,7 @@ void gpiod_unexport(struct gpio_desc *desc)
mutex_unlock(&sysfs_lock);
if (dev) {
+ data = dev_get_drvdata(dev);
device_unregister(dev);
/*
* Release irq after deregistration to prevent race with
@@ -683,6 +712,7 @@ void gpiod_unexport(struct gpio_desc *desc)
*/
gpio_setup_irq(desc, dev, 0);
put_device(dev);
+ kfree(data);
}
if (status)
--
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 097e7e539c9d..0bc959fbcf23 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;
@@ -257,8 +258,6 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
ssize_t status;
int i;
@@ -270,7 +269,7 @@ static ssize_t edge_store(struct device *dev,
found:
mutex_lock(&sysfs_lock);
- status = gpio_setup_irq(desc, dev, trigger_types[i].flags);
+ status = gpio_setup_irq(dev, trigger_types[i].flags);
if (!status)
status = size;
@@ -280,9 +279,10 @@ found:
}
static DEVICE_ATTR_RW(edge);
-static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
- int value)
+static int sysfs_set_active_low(struct device *dev, int value)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
int status = 0;
if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
@@ -298,8 +298,8 @@ static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev,
!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
- gpio_setup_irq(desc, dev, 0);
- status = gpio_setup_irq(desc, dev, trigger_flags);
+ gpio_setup_irq(dev, 0);
+ status = gpio_setup_irq(dev, trigger_flags);
}
return status;
@@ -325,8 +325,6 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
- struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
ssize_t status;
long value;
@@ -334,7 +332,7 @@ static ssize_t active_low_store(struct device *dev,
status = kstrtol(buf, 0, &value);
if (status == 0)
- status = sysfs_set_active_low(desc, dev, value != 0);
+ status = sysfs_set_active_low(dev, value != 0);
mutex_unlock(&sysfs_lock);
@@ -710,7 +708,7 @@ void gpiod_unexport(struct gpio_desc *desc)
* Release irq after deregistration to prevent race with
* edge_store.
*/
- gpio_setup_irq(desc, dev, 0);
+ gpio_setup_irq(dev, 0);
put_device(dev);
kfree(data);
}
--
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 0bc959fbcf23..bccba406fc22 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,17 +146,15 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
if (irq < 0)
return -EIO;
- id = desc->flags >> ID_SHIFT;
- value_sd = idr_find(&dirent_idr, id);
- if (value_sd)
- free_irq(irq, value_sd);
+ if (data->value_kn)
+ free_irq(irq, data);
desc->flags &= ~GPIO_TRIGGER_MASK;
if (!gpio_flags) {
gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
ret = 0;
- goto free_id;
+ goto free_kn;
}
irq_flags = IRQF_SHARED;
@@ -169,25 +165,12 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
- if (!value_sd) {
- value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
- if (!value_sd) {
+ if (!data->value_kn) {
+ data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+ if (!data->value_kn) {
ret = -ENODEV;
goto err_out;
}
-
- ret = idr_alloc(&dirent_idr, value_sd, 1, 0, GFP_KERNEL);
- if (ret < 0)
- goto free_sd;
- id = ret;
-
- desc->flags &= GPIO_FLAGS_MASK;
- desc->flags |= (unsigned long)id << ID_SHIFT;
-
- if (desc->flags >> ID_SHIFT != id) {
- ret = -ERANGE;
- goto free_id;
- }
}
/*
@@ -200,10 +183,10 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
*/
ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
if (ret < 0)
- goto free_id;
+ goto free_kn;
ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
- "gpiolib", value_sd);
+ "gpiolib", data);
if (ret < 0)
goto err_unlock;
@@ -212,12 +195,11 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
err_unlock:
gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-free_id:
- idr_remove(&dirent_idr, id);
- desc->flags &= GPIO_FLAGS_MASK;
-free_sd:
- if (value_sd)
- sysfs_put(value_sd);
+free_kn:
+ if (data->value_kn) {
+ sysfs_put(data->value_kn);
+ data->value_kn = NULL;
+ }
err_out:
return ret;
}
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index efdd5ded4965..4a857da698b2 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 bccba406fc22..201b757ad0db 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;
@@ -240,6 +237,9 @@ static ssize_t edge_show(struct device *dev,
static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
+ unsigned long flags;
ssize_t status;
int i;
@@ -249,12 +249,20 @@ static ssize_t edge_store(struct device *dev,
return -EINVAL;
found:
+ flags = trigger_types[i].flags;
+
mutex_lock(&sysfs_lock);
- status = gpio_setup_irq(dev, trigger_types[i].flags);
+ if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+ status = size;
+ goto out_unlock;
+ }
+
+ status = gpio_setup_irq(dev, flags);
if (!status)
status = size;
+out_unlock:
mutex_unlock(&sysfs_lock);
return status;
@@ -690,7 +698,8 @@ void gpiod_unexport(struct gpio_desc *desc)
* Release irq after deregistration to prevent race with
* edge_store.
*/
- gpio_setup_irq(dev, 0);
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_setup_irq(dev, 0);
put_device(dev);
kfree(data);
}
--
2.0.5
Add separate helper functions for irq request and free.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/gpio/gpiolib-sysfs.c | 74 ++++++++++++++++++++++----------------------
1 file changed, 37 insertions(+), 37 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 201b757ad0db..d9b3faa01fee 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,27 +133,20 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
return IRQ_HANDLED;
}
-static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
+static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
unsigned long irq_flags;
- int ret, irq;
+ int ret;
- irq = gpiod_to_irq(desc);
- if (irq < 0)
+ data->irq = gpiod_to_irq(desc);
+ if (data->irq < 0)
return -EIO;
- if (data->value_kn)
- free_irq(irq, data);
-
- desc->flags &= ~GPIO_TRIGGER_MASK;
-
- if (!gpio_flags) {
- gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
- ret = 0;
- goto free_kn;
- }
+ data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
+ if (!data->value_kn)
+ return -ENODEV;
irq_flags = IRQF_SHARED;
if (test_bit(FLAG_TRIG_FALL, &gpio_flags))
@@ -162,14 +156,6 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
- if (!data->value_kn) {
- data->value_kn = sysfs_get_dirent(dev->kobj.sd, "value");
- if (!data->value_kn) {
- ret = -ENODEV;
- goto err_out;
- }
- }
-
/*
* FIXME: This should be done in the irq_request_resources callback
* when the irq is requested, but a few drivers currently fail
@@ -180,27 +166,36 @@ static int gpio_setup_irq(struct device *dev, unsigned long gpio_flags)
*/
ret = gpiochip_lock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
if (ret < 0)
- goto free_kn;
+ goto err_put_kn;
- ret = request_any_context_irq(irq, gpio_sysfs_irq, irq_flags,
+ ret = request_any_context_irq(data->irq, gpio_sysfs_irq, irq_flags,
"gpiolib", data);
if (ret < 0)
goto err_unlock;
desc->flags |= gpio_flags;
+
return 0;
err_unlock:
gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
-free_kn:
- if (data->value_kn) {
- sysfs_put(data->value_kn);
- data->value_kn = NULL;
- }
-err_out:
+err_put_kn:
+ sysfs_put(data->value_kn);
+
return ret;
}
+static void gpio_sysfs_free_irq(struct device *dev)
+{
+ struct gpiod_data *data = dev_get_drvdata(dev);
+ struct gpio_desc *desc = data->desc;
+
+ desc->flags &= ~GPIO_TRIGGER_MASK;
+ free_irq(data->irq, data);
+ gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
+ sysfs_put(data->value_kn);
+}
+
static const struct {
const char *name;
unsigned long flags;
@@ -240,7 +235,7 @@ static ssize_t edge_store(struct device *dev,
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
unsigned long flags;
- ssize_t status;
+ ssize_t status = size;
int i;
for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
@@ -258,9 +253,14 @@ found:
goto out_unlock;
}
- status = gpio_setup_irq(dev, flags);
- if (!status)
- status = size;
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_sysfs_free_irq(dev);
+
+ if (flags) {
+ status = gpio_sysfs_request_irq(dev, flags);
+ if (!status)
+ status = size;
+ }
out_unlock:
mutex_unlock(&sysfs_lock);
@@ -288,8 +288,8 @@ static int sysfs_set_active_low(struct device *dev, int value)
!!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
- gpio_setup_irq(dev, 0);
- status = gpio_setup_irq(dev, trigger_flags);
+ gpio_sysfs_free_irq(dev);
+ status = gpio_sysfs_request_irq(dev, trigger_flags);
}
return status;
@@ -699,7 +699,7 @@ void gpiod_unexport(struct gpio_desc *desc)
* edge_store.
*/
if (desc->flags & GPIO_TRIGGER_MASK)
- gpio_setup_irq(dev, 0);
+ gpio_sysfs_free_irq(dev);
put_device(dev);
kfree(data);
}
--
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 d9b3faa01fee..1161a46618dd 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -236,14 +236,16 @@ static ssize_t edge_store(struct device *dev,
struct gpio_desc *desc = data->desc;
unsigned long flags;
ssize_t status = size;
- int i;
+ int i;
- for (i = 0; i < ARRAY_SIZE(trigger_types); i++)
+ for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
if (sysfs_streq(trigger_types[i].name, buf))
- goto found;
- return -EINVAL;
+ break;
+ }
+
+ if (i == ARRAY_SIZE(trigger_types))
+ return -EINVAL;
-found:
flags = trigger_types[i].flags;
mutex_lock(&sysfs_lock);
--
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 1161a46618dd..682e4d34999c 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -631,34 +631,22 @@ static int match_export(struct device *dev, const void *desc)
int gpiod_export_link(struct device *dev, const char *name,
struct gpio_desc *desc)
{
- int status = -EINVAL;
+ struct device *cdev;
+ int ret;
if (!desc) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
- mutex_lock(&sysfs_lock);
-
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
- struct device *tdev;
-
- tdev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (tdev != NULL) {
- status = sysfs_create_link(&dev->kobj, &tdev->kobj,
- name);
- put_device(tdev);
- } else {
- status = -ENODEV;
- }
- }
-
- mutex_unlock(&sysfs_lock);
+ cdev = class_find_device(&gpio_class, NULL, desc, match_export);
+ if (!cdev)
+ return -ENODEV;
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+ ret = sysfs_create_link(&dev->kobj, &cdev->kobj, name);
+ put_device(cdev);
- return status;
+ return ret;
}
EXPORT_SYMBOL_GPL(gpiod_export_link);
--
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 682e4d34999c..1bb05aa33a84 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);
@@ -185,6 +193,10 @@ err_put_kn:
return ret;
}
+/*
+ * Caller holds gpiod-data mutex (unless called after class-device
+ * deregistration).
+ */
static void gpio_sysfs_free_irq(struct device *dev)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -215,7 +227,7 @@ static ssize_t edge_show(struct device *dev,
ssize_t status = 0;
int i;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
mask = desc->flags & GPIO_TRIGGER_MASK;
@@ -225,7 +237,8 @@ static ssize_t edge_show(struct device *dev,
}
}
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
+
return status;
}
@@ -248,7 +261,7 @@ static ssize_t edge_store(struct device *dev,
flags = trigger_types[i].flags;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
status = size;
@@ -265,12 +278,13 @@ static ssize_t edge_store(struct device *dev,
}
out_unlock:
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
static DEVICE_ATTR_RW(edge);
+/* Caller holds gpiod-data mutex. */
static int sysfs_set_active_low(struct device *dev, int value)
{
struct gpiod_data *data = dev_get_drvdata(dev);
@@ -304,12 +318,12 @@ static ssize_t active_low_show(struct device *dev,
struct gpio_desc *desc = data->desc;
ssize_t status;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = sprintf(buf, "%d\n",
!!test_bit(FLAG_ACTIVE_LOW, &desc->flags));
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status;
}
@@ -317,16 +331,17 @@ static ssize_t active_low_show(struct device *dev,
static ssize_t active_low_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
+ struct gpiod_data *data = dev_get_drvdata(dev);
ssize_t status;
long value;
- mutex_lock(&sysfs_lock);
+ mutex_lock(&data->mutex);
status = kstrtol(buf, 0, &value);
if (status == 0)
status = sysfs_set_active_low(dev, value != 0);
- mutex_unlock(&sysfs_lock);
+ mutex_unlock(&data->mutex);
return status ? : size;
}
@@ -583,6 +598,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
}
data->desc = desc;
+ mutex_init(&data->mutex);
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
--
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 1bb05aa33a84..1540f7d60f06 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -674,9 +674,8 @@ EXPORT_SYMBOL_GPL(gpiod_export_link);
*/
void gpiod_unexport(struct gpio_desc *desc)
{
- struct gpiod_data *data;
- int status = 0;
- struct device *dev = NULL;
+ struct gpiod_data *data;
+ struct device *dev;
if (!desc) {
pr_warn("%s: invalid GPIO\n", __func__);
@@ -685,33 +684,35 @@ void gpiod_unexport(struct gpio_desc *desc)
mutex_lock(&sysfs_lock);
- if (test_bit(FLAG_EXPORT, &desc->flags)) {
+ if (!test_bit(FLAG_EXPORT, &desc->flags))
+ goto err_unlock;
- dev = class_find_device(&gpio_class, NULL, desc, match_export);
- if (dev) {
- clear_bit(FLAG_SYSFS_DIR, &desc->flags);
- clear_bit(FLAG_EXPORT, &desc->flags);
- } else
- status = -ENODEV;
- }
+ dev = class_find_device(&gpio_class, NULL, desc, match_export);
+ if (!dev)
+ goto err_unlock;
+
+ data = dev_get_drvdata(dev);
+
+ clear_bit(FLAG_SYSFS_DIR, &desc->flags);
+ clear_bit(FLAG_EXPORT, &desc->flags);
+
+ device_unregister(dev);
+
+ /*
+ * Release irq after deregistration to prevent race with edge_store.
+ */
+ if (desc->flags & GPIO_TRIGGER_MASK)
+ gpio_sysfs_free_irq(dev);
mutex_unlock(&sysfs_lock);
- if (dev) {
- data = dev_get_drvdata(dev);
- device_unregister(dev);
- /*
- * Release irq after deregistration to prevent race with
- * edge_store.
- */
- if (desc->flags & GPIO_TRIGGER_MASK)
- gpio_sysfs_free_irq(dev);
- put_device(dev);
- kfree(data);
- }
+ put_device(dev);
+ kfree(data);
- if (status)
- gpiod_dbg(desc, "%s: status %d\n", __func__, status);
+ return;
+
+err_unlock:
+ mutex_unlock(&sysfs_lock);
}
EXPORT_SYMBOL_GPL(gpiod_unexport);
--
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 1540f7d60f06..06372c7c822c 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -285,7 +285,7 @@ out_unlock:
static DEVICE_ATTR_RW(edge);
/* Caller holds gpiod-data mutex. */
-static int sysfs_set_active_low(struct device *dev, int value)
+static int gpio_sysfs_set_active_low(struct device *dev, int value)
{
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
@@ -339,7 +339,7 @@ static ssize_t active_low_store(struct device *dev,
status = kstrtol(buf, 0, &value);
if (status == 0)
- status = sysfs_set_active_low(dev, value != 0);
+ status = gpio_sysfs_set_active_low(dev, value);
mutex_unlock(&data->mutex);
--
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 06372c7c822c..9047b2d556a0 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;
};
/*
@@ -354,7 +356,7 @@ static umode_t gpio_is_visible(struct kobject *kobj, struct attribute *attr,
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
umode_t mode = attr->mode;
- bool show_direction = test_bit(FLAG_SYSFS_DIR, &desc->flags);
+ bool show_direction = data->direction_can_change;
if (attr == &dev_attr_direction.attr) {
if (!show_direction)
@@ -583,12 +585,6 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
status = -EPERM;
goto err_unlock;
}
-
- if (chip->direction_input && chip->direction_output &&
- direction_may_change) {
- set_bit(FLAG_SYSFS_DIR, &desc->flags);
- }
-
spin_unlock_irqrestore(&gpio_lock, flags);
data = kzalloc(sizeof(*data), GFP_KERNEL);
@@ -599,6 +595,10 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
data->desc = desc;
mutex_init(&data->mutex);
+ if (chip->direction_input && chip->direction_output)
+ data->direction_can_change = direction_may_change;
+ else
+ data->direction_can_change = false;
offset = gpio_chip_hwgpio(desc);
if (chip->names && chip->names[offset])
@@ -693,7 +693,6 @@ void gpiod_unexport(struct gpio_desc *desc)
data = dev_get_drvdata(dev);
- clear_bit(FLAG_SYSFS_DIR, &desc->flags);
clear_bit(FLAG_EXPORT, &desc->flags);
device_unregister(dev);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 4a857da698b2..34eea33e482c 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 | 47 ++++++++++++++++++++++----------------------
drivers/gpio/gpiolib.h | 4 ----
2 files changed, 24 insertions(+), 27 deletions(-)
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index 9047b2d556a0..b57ed8e55ab5 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;
@@ -183,7 +189,7 @@ static int gpio_sysfs_request_irq(struct device *dev, unsigned long gpio_flags)
if (ret < 0)
goto err_unlock;
- desc->flags |= gpio_flags;
+ data->irq_flags = flags;
return 0;
@@ -204,7 +210,7 @@ static void gpio_sysfs_free_irq(struct device *dev)
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
- desc->flags &= ~GPIO_TRIGGER_MASK;
+ data->irq_flags = 0;
free_irq(data->irq, data);
gpiochip_unlock_as_irq(desc->chip, gpio_chip_hwgpio(desc));
sysfs_put(data->value_kn);
@@ -212,28 +218,25 @@ static void gpio_sysfs_free_irq(struct device *dev)
static const struct {
const char *name;
- unsigned long flags;
+ unsigned char flags;
} trigger_types[] = {
{ "none", 0 },
- { "falling", BIT(FLAG_TRIG_FALL) },
- { "rising", BIT(FLAG_TRIG_RISE) },
- { "both", BIT(FLAG_TRIG_FALL) | BIT(FLAG_TRIG_RISE) },
+ { "falling", GPIO_IRQF_TRIGGER_FALLING },
+ { "rising", GPIO_IRQF_TRIGGER_RISING },
+ { "both", GPIO_IRQF_TRIGGER_BOTH },
};
static ssize_t edge_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
- unsigned long mask;
ssize_t status = 0;
int i;
mutex_lock(&data->mutex);
for (i = 0; i < ARRAY_SIZE(trigger_types); i++) {
- mask = desc->flags & GPIO_TRIGGER_MASK;
- if (mask == trigger_types[i].flags) {
+ if (data->irq_flags == trigger_types[i].flags) {
status = sprintf(buf, "%s\n", trigger_types[i].name);
break;
}
@@ -248,8 +251,7 @@ static ssize_t edge_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t size)
{
struct gpiod_data *data = dev_get_drvdata(dev);
- struct gpio_desc *desc = data->desc;
- unsigned long flags;
+ unsigned char flags;
ssize_t status = size;
int i;
@@ -265,12 +267,12 @@ static ssize_t edge_store(struct device *dev,
mutex_lock(&data->mutex);
- if ((desc->flags & GPIO_TRIGGER_MASK) == flags) {
+ if (flags == data->irq_flags) {
status = size;
goto out_unlock;
}
- if (desc->flags & GPIO_TRIGGER_MASK)
+ if (data->irq_flags)
gpio_sysfs_free_irq(dev);
if (flags) {
@@ -292,6 +294,7 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
struct gpiod_data *data = dev_get_drvdata(dev);
struct gpio_desc *desc = data->desc;
int status = 0;
+ unsigned int flags = data->irq_flags;
if (!!test_bit(FLAG_ACTIVE_LOW, &desc->flags) == !!value)
return 0;
@@ -302,12 +305,10 @@ static int gpio_sysfs_set_active_low(struct device *dev, int value)
clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
/* reconfigure poll(2) support if enabled on one edge only */
- if (!!test_bit(FLAG_TRIG_RISE, &desc->flags) ^
- !!test_bit(FLAG_TRIG_FALL, &desc->flags)) {
- unsigned long trigger_flags = desc->flags & GPIO_TRIGGER_MASK;
-
+ if (flags == GPIO_IRQF_TRIGGER_FALLING ||
+ flags == GPIO_IRQF_TRIGGER_RISING) {
gpio_sysfs_free_irq(dev);
- status = gpio_sysfs_request_irq(dev, trigger_flags);
+ status = gpio_sysfs_request_irq(dev, flags);
}
return status;
@@ -700,7 +701,7 @@ void gpiod_unexport(struct gpio_desc *desc)
/*
* Release irq after deregistration to prevent race with edge_store.
*/
- if (desc->flags & GPIO_TRIGGER_MASK)
+ if (data->irq_flags)
gpio_sysfs_free_irq(dev);
mutex_unlock(&sysfs_lock);
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 34eea33e482c..42c2ede96545 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 Tue, May 5, 2015 at 12:10 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. [ May
> already have been applied but not pushed by Linus, but included in v2
> for completeness. ]
>
> Unfortunately we can't just kill the gpio sysfs interface, but these
> patches will make it more manageable and should allow us to implement a
> new user-space interface while maintaining the old one (for a while at
> least) without losing our sanity.
>
> Note that there is still a race between chip remove and gpiod_request (and
> therefore sysfs export), which needs to be fixed separately (for instance as
> part of a generic solution to chip hotplugging).
Reiterating my
Reviewed-by: Alexandre Courbot <[email protected]>
on this very nice series.
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Clean up gpiochip_remove somewhat and only output warning about removing
> chip with GPIOs requested once.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Drivers should call gpiochip_lock_as_irq (which prevents the pin
> direction from being changed) in their irq_request_resources callbacks
> but some drivers currently fail to do so.
>
> Instead a second, explicit and often redundant call to lock-as-irq is
> made by the sysfs-interface implementation after an irq has been
> requested.
>
> Move the explicit call before the irq-request to match the unlock done
> after the irq is later released. Note that this also fixes an irq leak,
> should the explicit call ever have failed.
>
> Also add a comment about removing the redundant call once the broken
> drivers have been fixed.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Put the recently introduced gpio-chip pointer to some more use in
> gpiod_export.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Reduce scope of sysfs_lock protection during chip export and unexport,
> which is only needed to prevent gpiod (re-)exports during chip removal.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied with some manual interventions.
Pls check result!
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, 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]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Rename the gpio-chip export/unexport functions to the more descriptive
> names gpiochip_sysfs_register and gpiochip_sysfs_unregister.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, 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]>
GOOD RIDDANCE.
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Use DEVICE_ATTR_RO and DEVICE_ATTR_RW rather than specifying masks and
> callbacks directly.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> 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]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> The attribute operations will never be called for an unregistered device
> so remove redundant checks for FLAG_EXPORT.
>
> Note that kernfs will also guarantee that any active sysfs operation has
> finished before the attribute is removed during deregistration.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Add gpiod class-device data.
>
> This is a first step in getting rid of the insane gpio-descriptor flag
> overloading, backward irq-interface implementation, and course grained
> sysfs-interface locking (a single static mutex for every operation on
> all exported gpios in a system).
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Remove redundant gpio-descriptor parameters from sysfs_set_active_low and
> gpio_setup_irq.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Store the value sysfs entry in the gpiod data rather than in a global
> table accessed through an index stored in the overloaded gpio-descriptor
> flag field.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Only call irq helper if actually reconfiguring interrupt state.
>
> This is a preparatory step in introducing separate gpio-irq request and
> free functions.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Add separate helper functions for irq request and free.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Remove goto from success path.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Drop unnecessary locking from gpiod_export_link. If the class device has
> not already been unregistered, class_find_device returns the ref-counted
> class device so there's no need for locking.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> 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]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Make sure to deregister the class device (and release the irq) while
> holding the sysfs lock in gpio_unexport to prevent racing with
> gpio_export.
>
> Note that this requires the recently introduced per-gpio locking to
> avoid a deadlock with the kernfs active protection when waiting for the
> attribute operations to drain during deregistration.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Rename active-low helper using common prefix.
>
> Also remove unnecessary manipulation of value argument.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Remove FLAG_SYSFS_DIR, which is sysfs-interface specific, and store it
> in the class-device data instead.
>
> Note that the flag is only used during export.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij
On Mon, May 4, 2015 at 5:10 PM, Johan Hovold <[email protected]> wrote:
> Move irq trigger flags, which as sysfs-interface specific, to the class
> device data.
>
> This avoids accessing the gpio-descriptor flags field using non-atomic
> operations without any locking, and allows for a more clear separation
> of the sysfs interface from gpiolib core.
>
> Signed-off-by: Johan Hovold <[email protected]>
Patch applied.
Yours,
Linus Walleij