From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
In order to enable moving away from the global GPIO numberspace-based
exporting of lines over sysfs: add a parallel, per-chip entry under
/sys/class/gpio/ for every registered GPIO chip, denoted by device ID
in the file name and not its base GPIO number.
Compared to the existing chip group: it does not contain the "base"
attribute as the goal of this change is to not refer to GPIOs by their
global number from user-space anymore. It also contains its own,
per-chip export/unexport attribute pair which allow to export lines by
their hardware offset within the chip.
Caveat #1: the new device cannot be a link to (or be linked to by) the
existing "gpiochip<BASE>" entry as we cannot create links in
/sys/class/xyz/.
Caveat #2: the new entry cannot be named "gpiochipX" as it could
conflict with devices whose base is statically defined to a low number.
Let's go with "chipX" instead.
While at it: the chip label is unique so update the untrue statement
when extending the docs.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
Documentation/ABI/obsolete/sysfs-gpio | 7 +-
drivers/gpio/gpiolib-sysfs.c | 191 +++++++++++++++++++++++++---------
2 files changed, 149 insertions(+), 49 deletions(-)
diff --git a/Documentation/ABI/obsolete/sysfs-gpio b/Documentation/ABI/obsolete/sysfs-gpio
index 480316fee6d80fb7a0ed61706559838591ec0932..ff694708a3bef787afa42dedf94faf209c44dbf0 100644
--- a/Documentation/ABI/obsolete/sysfs-gpio
+++ b/Documentation/ABI/obsolete/sysfs-gpio
@@ -25,8 +25,13 @@ Description:
/active_low ... r/w as: 0, 1
/gpiochipN ... for each gpiochip; #N is its first GPIO
/base ... (r/o) same as N
- /label ... (r/o) descriptive, not necessarily unique
+ /label ... (r/o) descriptive chip name
/ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
+ /chipX ... for each gpiochip; #X is the gpio device ID
+ /export ... asks the kernel to export a GPIO at HW offset X to userspace
+ /unexport ... to return a GPIO at HW offset X to the kernel
+ /label ... (r/o) descriptive chip name
+ /ngpio ... (r/o) number of GPIOs exposed by the chip
This ABI is obsoleted by Documentation/ABI/testing/gpio-cdev and will be
removed after 2020.
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c
index c4c21e25c682b939e4a0517393308343feb6585a..fbe93cda4ca16038a1cffe766f7e5ead55ace5e6 100644
--- a/drivers/gpio/gpiolib-sysfs.c
+++ b/drivers/gpio/gpiolib-sysfs.c
@@ -46,6 +46,7 @@ struct gpiod_data {
struct gpiodev_data {
struct gpio_device *gdev;
struct device *cdev_base; /* Class device by GPIO base */
+ struct device *cdev_id; /* Class device by GPIO device ID */
};
/*
@@ -399,6 +400,14 @@ static const struct attribute_group *gpio_groups[] = {
* /base ... matching gpio_chip.base (N)
* /label ... matching gpio_chip.label
* /ngpio ... matching gpio_chip.ngpio
+ *
+ * AND
+ *
+ * /sys/class/gpio/chipX/
+ * /export ... export GPIO at given offset
+ * /unexport ... unexport GPIO at given offset
+ * /label ... matching gpio_chip.label
+ * /ngpio ... matching gpio_chip.ngpio
*/
static ssize_t base_show(struct device *dev, struct device_attribute *attr,
@@ -428,6 +437,111 @@ static ssize_t ngpio_show(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RO(ngpio);
+static int export_gpio_desc(struct gpio_desc *desc)
+{
+ int offset, ret;
+
+ CLASS(gpio_chip_guard, guard)(desc);
+ if (!guard.gc)
+ return -ENODEV;
+
+ offset = gpio_chip_hwgpio(desc);
+ if (!gpiochip_line_is_valid(guard.gc, offset)) {
+ pr_debug_ratelimited("%s: GPIO %d masked\n", __func__,
+ gpio_chip_hwgpio(desc));
+ return -EINVAL;
+ }
+
+ /*
+ * No extra locking here; FLAG_SYSFS just signifies that the
+ * request and export were done by on behalf of userspace, so
+ * they may be undone on its behalf too.
+ */
+
+ ret = gpiod_request_user(desc, "sysfs");
+ if (ret)
+ return ret;
+
+ ret = gpiod_set_transitory(desc, false);
+ if (ret) {
+ gpiod_free(desc);
+ return ret;
+ }
+
+ ret = gpiod_export(desc, true);
+ if (ret < 0) {
+ gpiod_free(desc);
+ } else {
+ set_bit(FLAG_SYSFS, &desc->flags);
+ gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
+ }
+
+ return ret;
+}
+
+static int unexport_gpio_desc(struct gpio_desc *desc)
+{
+ /*
+ * No extra locking here; FLAG_SYSFS just signifies that the
+ * request and export were done by on behalf of userspace, so
+ * they may be undone on its behalf too.
+ */
+ if (!test_and_clear_bit(FLAG_SYSFS, &desc->flags))
+ return -EINVAL;
+
+ gpiod_unexport(desc);
+ gpiod_free(desc);
+
+ return 0;
+}
+
+static ssize_t do_chip_export_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, ssize_t size,
+ int (*handler)(struct gpio_desc *desc))
+{
+ struct gpiodev_data *data = dev_get_drvdata(dev);
+ struct gpio_device *gdev = data->gdev;
+ struct gpio_desc *desc;
+ unsigned int gpio;
+ int ret;
+
+ ret = kstrtouint(buf, 0, &gpio);
+ if (ret)
+ return ret;
+
+ desc = gpio_device_get_desc(gdev, gpio);
+ if (IS_ERR(desc))
+ return PTR_ERR(desc);
+
+ ret = handler(desc);
+ if (ret)
+ return ret;
+
+ return size;
+}
+
+static ssize_t chip_export_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ return do_chip_export_store(dev, attr, buf, size, export_gpio_desc);
+}
+
+static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
+ chip_export_store);
+
+static ssize_t chip_unexport_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t size)
+{
+ return do_chip_export_store(dev, attr, buf, size, unexport_gpio_desc);
+}
+
+static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
+ NULL,
+ chip_unexport_store);
+
static struct attribute *gpiochip_attrs[] = {
&dev_attr_base.attr,
&dev_attr_label.attr,
@@ -436,6 +550,15 @@ static struct attribute *gpiochip_attrs[] = {
};
ATTRIBUTE_GROUPS(gpiochip);
+static struct attribute *gpiochip_ext_attrs[] = {
+ &dev_attr_label.attr,
+ &dev_attr_ngpio.attr,
+ &dev_attr_export.attr,
+ &dev_attr_unexport.attr,
+ NULL,
+};
+ATTRIBUTE_GROUPS(gpiochip_ext);
+
/*
* /sys/class/gpio/export ... write-only
* integer N ... number of GPIO to export (full access)
@@ -447,7 +570,7 @@ static ssize_t export_store(const struct class *class,
const char *buf, size_t len)
{
struct gpio_desc *desc;
- int status, offset;
+ int status;
long gpio;
status = kstrtol(buf, 0, &gpio);
@@ -461,40 +584,7 @@ static ssize_t export_store(const struct class *class,
return -EINVAL;
}
- CLASS(gpio_chip_guard, guard)(desc);
- if (!guard.gc)
- return -ENODEV;
-
- offset = gpio_chip_hwgpio(desc);
- if (!gpiochip_line_is_valid(guard.gc, offset)) {
- pr_debug_ratelimited("%s: GPIO %ld masked\n", __func__, gpio);
- return -EINVAL;
- }
-
- /* No extra locking here; FLAG_SYSFS just signifies that the
- * request and export were done by on behalf of userspace, so
- * they may be undone on its behalf too.
- */
-
- status = gpiod_request_user(desc, "sysfs");
- if (status)
- goto done;
-
- status = gpiod_set_transitory(desc, false);
- if (status) {
- gpiod_free(desc);
- goto done;
- }
-
- status = gpiod_export(desc, true);
- if (status < 0) {
- gpiod_free(desc);
- } else {
- set_bit(FLAG_SYSFS, &desc->flags);
- gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
- }
-
-done:
+ status = export_gpio_desc(desc);
if (status)
pr_debug("%s: status %d\n", __func__, status);
return status ? : len;
@@ -511,7 +601,7 @@ static ssize_t unexport_store(const struct class *class,
status = kstrtol(buf, 0, &gpio);
if (status < 0)
- goto done;
+ return status;
desc = gpio_to_desc(gpio);
/* reject bogus commands (gpiod_unexport() ignores them) */
@@ -520,18 +610,7 @@ static ssize_t unexport_store(const struct class *class,
return -EINVAL;
}
- status = -EINVAL;
-
- /* No extra locking here; FLAG_SYSFS just signifies that the
- * request and export were done by on behalf of userspace, so
- * they may be undone on its behalf too.
- */
- if (test_and_clear_bit(FLAG_SYSFS, &desc->flags)) {
- gpiod_unexport(desc);
- gpiod_free(desc);
- status = 0;
- }
-done:
+ status = unexport_gpio_desc(desc);
if (status)
pr_debug("%s: status %d\n", __func__, status);
return status ? : len;
@@ -561,6 +640,11 @@ static int match_gdev(struct device *dev, const void *desc)
static struct gpiodev_data *
gdev_get_data(struct gpio_device *gdev) __must_hold(&sysfs_lock)
{
+ /*
+ * Find the first device in GPIO class that matches. Whether that's
+ * the one indexed by GPIO base or device ID doesn't matter, it has
+ * the same address set as driver data.
+ */
struct device *cdev __free(put_device) = class_find_device(&gpio_class,
NULL, gdev,
match_gdev);
@@ -787,6 +871,16 @@ int gpiochip_sysfs_register(struct gpio_device *gdev)
return err;
}
+ data->cdev_id = device_create_with_groups(&gpio_class, parent,
+ MKDEV(0, 0), data,
+ gpiochip_ext_groups,
+ "chip%d", gdev->id);
+ if (IS_ERR(data->cdev_id)) {
+ device_unregister(data->cdev_base);
+ kfree(data);
+ return PTR_ERR(data->cdev_id);
+ }
+
return 0;
}
@@ -802,6 +896,7 @@ void gpiochip_sysfs_unregister(struct gpio_device *gdev)
return;
device_unregister(data->cdev_base);
+ device_unregister(data->cdev_id);
kfree(data);
}
--
2.48.1On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:
>
> In order to enable moving away from the global GPIO numberspace-based
> exporting of lines over sysfs: add a parallel, per-chip entry under
> /sys/class/gpio/ for every registered GPIO chip, denoted by device ID
> in the file name and not its base GPIO number.
>
> Compared to the existing chip group: it does not contain the "base"
> attribute as the goal of this change is to not refer to GPIOs by their
> global number from user-space anymore. It also contains its own,
> per-chip export/unexport attribute pair which allow to export lines by
> their hardware offset within the chip.
>
> Caveat #1: the new device cannot be a link to (or be linked to by) the
> existing "gpiochip<BASE>" entry as we cannot create links in
> /sys/class/xyz/.
>
> Caveat #2: the new entry cannot be named "gpiochipX" as it could
> conflict with devices whose base is statically defined to a low number.
> Let's go with "chipX" instead.
>
> While at it: the chip label is unique so update the untrue statement
> when extending the docs.
...
> struct gpiodev_data {
> struct gpio_device *gdev;
> struct device *cdev_base; /* Class device by GPIO base */
> + struct device *cdev_id; /* Class device by GPIO device ID */
I would add it in the middle in a way of the possible drop or conditional
compiling of the legacy access in the future.
> };
...
> +static int export_gpio_desc(struct gpio_desc *desc)
> +{
> + int offset, ret;
Why offset is signed?
> + CLASS(gpio_chip_guard, guard)(desc);
> + if (!guard.gc)
> + return -ENODEV;
> +
> + offset = gpio_chip_hwgpio(desc);
> + if (!gpiochip_line_is_valid(guard.gc, offset)) {
> + pr_debug_ratelimited("%s: GPIO %d masked\n", __func__,
> + gpio_chip_hwgpio(desc));
Can we use gdev here? (IIRC we can't due to some legacy corner cases)
> + return -EINVAL;
> + }
> +
> + /*
> + * No extra locking here; FLAG_SYSFS just signifies that the
> + * request and export were done by on behalf of userspace, so
> + * they may be undone on its behalf too.
> + */
> +
> + ret = gpiod_request_user(desc, "sysfs");
> + if (ret)
> + return ret;
> +
> + ret = gpiod_set_transitory(desc, false);
> + if (ret) {
> + gpiod_free(desc);
> + return ret;
> + }
> +
> + ret = gpiod_export(desc, true);
> + if (ret < 0) {
> + gpiod_free(desc);
> + } else {
> + set_bit(FLAG_SYSFS, &desc->flags);
> + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
> + }
> +
> + return ret;
> +}
...
> +static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> + chip_export_store);
__ATTR_WO()
...
> +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> + NULL,
> + chip_unexport_store);
Ditto.
...
> +static struct attribute *gpiochip_ext_attrs[] = {
> + &dev_attr_label.attr,
> + &dev_attr_ngpio.attr,
> + &dev_attr_export.attr,
> + &dev_attr_unexport.attr,
> + NULL,
No comma for the terminator, please.
> +};
...
> + data->cdev_id = device_create_with_groups(&gpio_class, parent,
> + MKDEV(0, 0), data,
> + gpiochip_ext_groups,
> + "chip%d", gdev->id);
> + if (IS_ERR(data->cdev_id)) {
> + device_unregister(data->cdev_base);
> + kfree(data);
UAF
> + return PTR_ERR(data->cdev_id);
> + }
--
With Best Regards,
Andy Shevchenko
On Fri, Jun 27, 2025 at 5:21 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:
> >
> > In order to enable moving away from the global GPIO numberspace-based
> > exporting of lines over sysfs: add a parallel, per-chip entry under
> > /sys/class/gpio/ for every registered GPIO chip, denoted by device ID
> > in the file name and not its base GPIO number.
> >
> > Compared to the existing chip group: it does not contain the "base"
> > attribute as the goal of this change is to not refer to GPIOs by their
> > global number from user-space anymore. It also contains its own,
> > per-chip export/unexport attribute pair which allow to export lines by
> > their hardware offset within the chip.
> >
> > Caveat #1: the new device cannot be a link to (or be linked to by) the
> > existing "gpiochip<BASE>" entry as we cannot create links in
> > /sys/class/xyz/.
> >
> > Caveat #2: the new entry cannot be named "gpiochipX" as it could
> > conflict with devices whose base is statically defined to a low number.
> > Let's go with "chipX" instead.
> >
> > While at it: the chip label is unique so update the untrue statement
> > when extending the docs.
>
> ...
>
> > struct gpiodev_data {
> > struct gpio_device *gdev;
> > struct device *cdev_base; /* Class device by GPIO base */
> > + struct device *cdev_id; /* Class device by GPIO device ID */
>
> I would add it in the middle in a way of the possible drop or conditional
> compiling of the legacy access in the future.
>
I'm not sure what difference it makes?
> > };
>
> ...
>
> > +static int export_gpio_desc(struct gpio_desc *desc)
> > +{
> > + int offset, ret;
>
> Why offset is signed?
>
Because gpio_chip_hwgpio() returns a signed int.
> > + CLASS(gpio_chip_guard, guard)(desc);
> > + if (!guard.gc)
> > + return -ENODEV;
> > +
> > + offset = gpio_chip_hwgpio(desc);
> > + if (!gpiochip_line_is_valid(guard.gc, offset)) {
> > + pr_debug_ratelimited("%s: GPIO %d masked\n", __func__,
> > + gpio_chip_hwgpio(desc));
>
> Can we use gdev here? (IIRC we can't due to some legacy corner cases)
>
Yeah, I think there was some revert here? In any case: it's material
for a different series, I'm just moving the code here.
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * No extra locking here; FLAG_SYSFS just signifies that the
> > + * request and export were done by on behalf of userspace, so
> > + * they may be undone on its behalf too.
> > + */
> > +
> > + ret = gpiod_request_user(desc, "sysfs");
> > + if (ret)
> > + return ret;
> > +
> > + ret = gpiod_set_transitory(desc, false);
> > + if (ret) {
> > + gpiod_free(desc);
> > + return ret;
> > + }
> > +
> > + ret = gpiod_export(desc, true);
> > + if (ret < 0) {
> > + gpiod_free(desc);
> > + } else {
> > + set_bit(FLAG_SYSFS, &desc->flags);
> > + gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_REQUESTED);
> > + }
> > +
> > + return ret;
> > +}
>
> ...
>
> > +static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> > + chip_export_store);
>
> __ATTR_WO()
>
No can do, the attribute would have to be called "chip_export". A
function called export_store() already exists in this file.
> ...
>
> > +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> > + NULL,
> > + chip_unexport_store);
>
> Ditto.
>
> ...
>
> > +static struct attribute *gpiochip_ext_attrs[] = {
> > + &dev_attr_label.attr,
> > + &dev_attr_ngpio.attr,
> > + &dev_attr_export.attr,
> > + &dev_attr_unexport.attr,
> > + NULL,
>
> No comma for the terminator, please.
>
Ok.
> > +};
>
> ...
>
> > + data->cdev_id = device_create_with_groups(&gpio_class, parent,
> > + MKDEV(0, 0), data,
> > + gpiochip_ext_groups,
> > + "chip%d", gdev->id);
> > + if (IS_ERR(data->cdev_id)) {
> > + device_unregister(data->cdev_base);
> > + kfree(data);
>
> UAF
>
Ok.
> > + return PTR_ERR(data->cdev_id);
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks,
Bart
On Mon, Jun 30, 2025 at 10:34:52AM +0200, Bartosz Golaszewski wrote:
> On Fri, Jun 27, 2025 at 5:21 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Mon, Jun 23, 2025 at 10:59:49AM +0200, Bartosz Golaszewski wrote:
...
> > > struct gpiodev_data {
> > > struct gpio_device *gdev;
> > > struct device *cdev_base; /* Class device by GPIO base */
> > > + struct device *cdev_id; /* Class device by GPIO device ID */
> >
> > I would add it in the middle in a way of the possible drop or conditional
> > compiling of the legacy access in the future.
>
> I'm not sure what difference it makes?
It collects optional (which you do with ifdeffery later on) at the end of the
structure. Maybe there is no effect now, but it might be in the future.
> > > };
...
> > > +static struct device_attribute dev_attr_export = __ATTR(export, 0200, NULL,
> > > + chip_export_store);
> >
> > __ATTR_WO()
> >
>
> No can do, the attribute would have to be called "chip_export". A
> function called export_store() already exists in this file.
I didn't get, we have two "export" nodes implemented in the same file?
...
> > > +static struct device_attribute dev_attr_unexport = __ATTR(unexport, 0200,
> > > + NULL,
> > > + chip_unexport_store);
> >
> > Ditto.
Ditto.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.