* [PATCH v5 1/9] gpio: aggregator: protect driver attr handlers against module unload
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-27 9:51 ` Bartosz Golaszewski
2025-02-24 14:31 ` [PATCH v5 2/9] gpio: aggregator: reorder functions to prepare for configfs introduction Koichiro Den
` (9 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Both new_device_store and delete_device_store touch module global
resources (e.g. gpio_aggregator_lock). To prevent race conditions with
module unload, a reference needs to be held.
Add try_module_get() in these handlers.
For new_device_store, this eliminates what appears to be the most dangerous
scenario: if an id is allocated from gpio_aggregator_idr but
platform_device_register has not yet been called or completed, a concurrent
module unload could fail to unregister/delete the device, leaving behind a
dangling platform device/GPIO forwarder. This can result in various issues.
The following simple reproducer demonstrates these problems:
#!/bin/bash
while :; do
# note: whether 'gpiochip0 0' exists or not does not matter.
echo 'gpiochip0 0' > /sys/bus/platform/drivers/gpio-aggregator/new_device
done &
while :; do
modprobe gpio-aggregator
modprobe -r gpio-aggregator
done &
wait
Starting with the following warning, several kinds of warnings will appear
and the system may become unstable:
------------[ cut here ]------------
list_del corruption, ffff888103e2e980->next is LIST_POISON1 (dead000000000100)
WARNING: CPU: 1 PID: 1327 at lib/list_debug.c:56 __list_del_entry_valid_or_report+0xa3/0x120
[...]
RIP: 0010:__list_del_entry_valid_or_report+0xa3/0x120
[...]
Call Trace:
<TASK>
? __list_del_entry_valid_or_report+0xa3/0x120
? __warn.cold+0x93/0xf2
? __list_del_entry_valid_or_report+0xa3/0x120
? report_bug+0xe6/0x170
? __irq_work_queue_local+0x39/0xe0
? handle_bug+0x58/0x90
? exc_invalid_op+0x13/0x60
? asm_exc_invalid_op+0x16/0x20
? __list_del_entry_valid_or_report+0xa3/0x120
gpiod_remove_lookup_table+0x22/0x60
new_device_store+0x315/0x350 [gpio_aggregator]
kernfs_fop_write_iter+0x137/0x1f0
vfs_write+0x262/0x430
ksys_write+0x60/0xd0
do_syscall_64+0x6c/0x180
entry_SYSCALL_64_after_hwframe+0x76/0x7e
[...]
</TASK>
---[ end trace 0000000000000000 ]---
Fixes: 828546e24280 ("gpio: Add GPIO Aggregator")
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 65f41cc3eafc..d668ddb2e81d 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -119,10 +119,15 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
struct platform_device *pdev;
int res, id;
+ if (!try_module_get(THIS_MODULE))
+ return -ENOENT;
+
/* kernfs guarantees string termination, so count + 1 is safe */
aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
- if (!aggr)
- return -ENOMEM;
+ if (!aggr) {
+ res = -ENOMEM;
+ goto put_module;
+ }
memcpy(aggr->args, buf, count + 1);
@@ -161,6 +166,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
}
aggr->pdev = pdev;
+ module_put(THIS_MODULE);
return count;
remove_table:
@@ -175,6 +181,8 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
kfree(aggr->lookups);
free_ga:
kfree(aggr);
+put_module:
+ module_put(THIS_MODULE);
return res;
}
@@ -203,13 +211,19 @@ static ssize_t delete_device_store(struct device_driver *driver,
if (error)
return error;
+ if (!try_module_get(THIS_MODULE))
+ return -ENOENT;
+
mutex_lock(&gpio_aggregator_lock);
aggr = idr_remove(&gpio_aggregator_idr, id);
mutex_unlock(&gpio_aggregator_lock);
- if (!aggr)
+ if (!aggr) {
+ module_put(THIS_MODULE);
return -ENOENT;
+ }
gpio_aggregator_free(aggr);
+ module_put(THIS_MODULE);
return count;
}
static DRIVER_ATTR_WO(delete_device);
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 1/9] gpio: aggregator: protect driver attr handlers against module unload
2025-02-24 14:31 ` [PATCH v5 1/9] gpio: aggregator: protect driver attr handlers against module unload Koichiro Den
@ 2025-02-27 9:51 ` Bartosz Golaszewski
0 siblings, 0 replies; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-02-27 9:51 UTC (permalink / raw)
To: Koichiro Den, geert+renesas
Cc: linux-gpio, linus.walleij, maciej.borzecki, linux-kernel
On Mon, Feb 24, 2025 at 3:31 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> Both new_device_store and delete_device_store touch module global
> resources (e.g. gpio_aggregator_lock). To prevent race conditions with
> module unload, a reference needs to be held.
>
> Add try_module_get() in these handlers.
>
> For new_device_store, this eliminates what appears to be the most dangerous
> scenario: if an id is allocated from gpio_aggregator_idr but
> platform_device_register has not yet been called or completed, a concurrent
> module unload could fail to unregister/delete the device, leaving behind a
> dangling platform device/GPIO forwarder. This can result in various issues.
> The following simple reproducer demonstrates these problems:
>
> #!/bin/bash
> while :; do
> # note: whether 'gpiochip0 0' exists or not does not matter.
> echo 'gpiochip0 0' > /sys/bus/platform/drivers/gpio-aggregator/new_device
> done &
> while :; do
> modprobe gpio-aggregator
> modprobe -r gpio-aggregator
> done &
> wait
>
> Starting with the following warning, several kinds of warnings will appear
> and the system may become unstable:
>
> ------------[ cut here ]------------
> list_del corruption, ffff888103e2e980->next is LIST_POISON1 (dead000000000100)
> WARNING: CPU: 1 PID: 1327 at lib/list_debug.c:56 __list_del_entry_valid_or_report+0xa3/0x120
> [...]
> RIP: 0010:__list_del_entry_valid_or_report+0xa3/0x120
> [...]
> Call Trace:
> <TASK>
> ? __list_del_entry_valid_or_report+0xa3/0x120
> ? __warn.cold+0x93/0xf2
> ? __list_del_entry_valid_or_report+0xa3/0x120
> ? report_bug+0xe6/0x170
> ? __irq_work_queue_local+0x39/0xe0
> ? handle_bug+0x58/0x90
> ? exc_invalid_op+0x13/0x60
> ? asm_exc_invalid_op+0x16/0x20
> ? __list_del_entry_valid_or_report+0xa3/0x120
> gpiod_remove_lookup_table+0x22/0x60
> new_device_store+0x315/0x350 [gpio_aggregator]
> kernfs_fop_write_iter+0x137/0x1f0
> vfs_write+0x262/0x430
> ksys_write+0x60/0xd0
> do_syscall_64+0x6c/0x180
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [...]
> </TASK>
> ---[ end trace 0000000000000000 ]---
>
> Fixes: 828546e24280 ("gpio: Add GPIO Aggregator")
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
Geert, does this look good to you? I'd like to send this fix upstream
first and backport it to stable.
Bartosz
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 2/9] gpio: aggregator: reorder functions to prepare for configfs introduction
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
2025-02-24 14:31 ` [PATCH v5 1/9] gpio: aggregator: protect driver attr handlers against module unload Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 3/9] gpio: aggregator: add aggr_alloc()/aggr_free() Koichiro Den
` (8 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Reorder functions in drivers/gpio/gpio-aggregator.c to prepare for the
configfs-based interface additions in subsequent commits. Arrange the
code so that the configfs implementations will appear above the existing
sysfs-specific code, since the latter will partly depend on the configfs
interface implementations when it starts to expose the settings to
configfs.
The order in drivers/gpio/gpio-aggregator.c will be as follows:
* Basic gpio_aggregator/gpio_aggregator_line representations
* Common utility functions
* GPIO Forwarder implementations
* Configfs interface implementations
* Sysfs interface implementations
* Platform device implementations
* Module init/exit implementations
This separate commit ensures a clean diff for the subsequent commits.
No functional change.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 380 +++++++++++++++++----------------
1 file changed, 192 insertions(+), 188 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index d668ddb2e81d..893cd56de867 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -61,194 +61,6 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
return 0;
}
-static int aggr_parse(struct gpio_aggregator *aggr)
-{
- char *args = skip_spaces(aggr->args);
- char *name, *offsets, *p;
- unsigned int i, n = 0;
- int error = 0;
-
- unsigned long *bitmap __free(bitmap) =
- bitmap_alloc(AGGREGATOR_MAX_GPIOS, GFP_KERNEL);
- if (!bitmap)
- return -ENOMEM;
-
- args = next_arg(args, &name, &p);
- while (*args) {
- args = next_arg(args, &offsets, &p);
-
- p = get_options(offsets, 0, &error);
- if (error == 0 || *p) {
- /* Named GPIO line */
- error = aggr_add_gpio(aggr, name, U16_MAX, &n);
- if (error)
- return error;
-
- name = offsets;
- continue;
- }
-
- /* GPIO chip + offset(s) */
- error = bitmap_parselist(offsets, bitmap, AGGREGATOR_MAX_GPIOS);
- if (error) {
- pr_err("Cannot parse %s: %d\n", offsets, error);
- return error;
- }
-
- for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
- error = aggr_add_gpio(aggr, name, i, &n);
- if (error)
- return error;
- }
-
- args = next_arg(args, &name, &p);
- }
-
- if (!n) {
- pr_err("No GPIOs specified\n");
- return -EINVAL;
- }
-
- return 0;
-}
-
-static ssize_t new_device_store(struct device_driver *driver, const char *buf,
- size_t count)
-{
- struct gpio_aggregator *aggr;
- struct platform_device *pdev;
- int res, id;
-
- if (!try_module_get(THIS_MODULE))
- return -ENOENT;
-
- /* kernfs guarantees string termination, so count + 1 is safe */
- aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
- if (!aggr) {
- res = -ENOMEM;
- goto put_module;
- }
-
- memcpy(aggr->args, buf, count + 1);
-
- aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
- GFP_KERNEL);
- if (!aggr->lookups) {
- res = -ENOMEM;
- goto free_ga;
- }
-
- mutex_lock(&gpio_aggregator_lock);
- id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
- mutex_unlock(&gpio_aggregator_lock);
-
- if (id < 0) {
- res = id;
- goto free_table;
- }
-
- aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
- if (!aggr->lookups->dev_id) {
- res = -ENOMEM;
- goto remove_idr;
- }
-
- res = aggr_parse(aggr);
- if (res)
- goto free_dev_id;
-
- gpiod_add_lookup_table(aggr->lookups);
-
- pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
- if (IS_ERR(pdev)) {
- res = PTR_ERR(pdev);
- goto remove_table;
- }
-
- aggr->pdev = pdev;
- module_put(THIS_MODULE);
- return count;
-
-remove_table:
- gpiod_remove_lookup_table(aggr->lookups);
-free_dev_id:
- kfree(aggr->lookups->dev_id);
-remove_idr:
- mutex_lock(&gpio_aggregator_lock);
- idr_remove(&gpio_aggregator_idr, id);
- mutex_unlock(&gpio_aggregator_lock);
-free_table:
- kfree(aggr->lookups);
-free_ga:
- kfree(aggr);
-put_module:
- module_put(THIS_MODULE);
- return res;
-}
-
-static DRIVER_ATTR_WO(new_device);
-
-static void gpio_aggregator_free(struct gpio_aggregator *aggr)
-{
- platform_device_unregister(aggr->pdev);
- gpiod_remove_lookup_table(aggr->lookups);
- kfree(aggr->lookups->dev_id);
- kfree(aggr->lookups);
- kfree(aggr);
-}
-
-static ssize_t delete_device_store(struct device_driver *driver,
- const char *buf, size_t count)
-{
- struct gpio_aggregator *aggr;
- unsigned int id;
- int error;
-
- if (!str_has_prefix(buf, DRV_NAME "."))
- return -EINVAL;
-
- error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
- if (error)
- return error;
-
- if (!try_module_get(THIS_MODULE))
- return -ENOENT;
-
- mutex_lock(&gpio_aggregator_lock);
- aggr = idr_remove(&gpio_aggregator_idr, id);
- mutex_unlock(&gpio_aggregator_lock);
- if (!aggr) {
- module_put(THIS_MODULE);
- return -ENOENT;
- }
-
- gpio_aggregator_free(aggr);
- module_put(THIS_MODULE);
- return count;
-}
-static DRIVER_ATTR_WO(delete_device);
-
-static struct attribute *gpio_aggregator_attrs[] = {
- &driver_attr_new_device.attr,
- &driver_attr_delete_device.attr,
- NULL
-};
-ATTRIBUTE_GROUPS(gpio_aggregator);
-
-static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
-{
- gpio_aggregator_free(p);
- return 0;
-}
-
-static void __exit gpio_aggregator_remove_all(void)
-{
- mutex_lock(&gpio_aggregator_lock);
- idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
- idr_destroy(&gpio_aggregator_idr);
- mutex_unlock(&gpio_aggregator_lock);
-}
-
/*
* GPIO Forwarder
@@ -573,6 +385,184 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
}
+/*
+ * Sysfs interface
+ */
+static int aggr_parse(struct gpio_aggregator *aggr)
+{
+ char *args = skip_spaces(aggr->args);
+ char *name, *offsets, *p;
+ unsigned int i, n = 0;
+ int error = 0;
+
+ unsigned long *bitmap __free(bitmap) =
+ bitmap_alloc(AGGREGATOR_MAX_GPIOS, GFP_KERNEL);
+ if (!bitmap)
+ return -ENOMEM;
+
+ args = next_arg(args, &name, &p);
+ while (*args) {
+ args = next_arg(args, &offsets, &p);
+
+ p = get_options(offsets, 0, &error);
+ if (error == 0 || *p) {
+ /* Named GPIO line */
+ error = aggr_add_gpio(aggr, name, U16_MAX, &n);
+ if (error)
+ return error;
+
+ name = offsets;
+ continue;
+ }
+
+ /* GPIO chip + offset(s) */
+ error = bitmap_parselist(offsets, bitmap, AGGREGATOR_MAX_GPIOS);
+ if (error) {
+ pr_err("Cannot parse %s: %d\n", offsets, error);
+ return error;
+ }
+
+ for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
+ error = aggr_add_gpio(aggr, name, i, &n);
+ if (error)
+ return error;
+ }
+
+ args = next_arg(args, &name, &p);
+ }
+
+ if (!n) {
+ pr_err("No GPIOs specified\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static ssize_t new_device_store(struct device_driver *driver, const char *buf,
+ size_t count)
+{
+ struct gpio_aggregator *aggr;
+ struct platform_device *pdev;
+ int res, id;
+
+ if (!try_module_get(THIS_MODULE))
+ return -ENOENT;
+
+ /* kernfs guarantees string termination, so count + 1 is safe */
+ aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
+ if (!aggr) {
+ res = -ENOMEM;
+ goto put_module;
+ }
+
+ memcpy(aggr->args, buf, count + 1);
+
+ aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
+ GFP_KERNEL);
+ if (!aggr->lookups) {
+ res = -ENOMEM;
+ goto free_ga;
+ }
+
+ mutex_lock(&gpio_aggregator_lock);
+ id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
+ mutex_unlock(&gpio_aggregator_lock);
+
+ if (id < 0) {
+ res = id;
+ goto free_table;
+ }
+
+ aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
+ if (!aggr->lookups->dev_id) {
+ res = -ENOMEM;
+ goto remove_idr;
+ }
+
+ res = aggr_parse(aggr);
+ if (res)
+ goto free_dev_id;
+
+ gpiod_add_lookup_table(aggr->lookups);
+
+ pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
+ if (IS_ERR(pdev)) {
+ res = PTR_ERR(pdev);
+ goto remove_table;
+ }
+
+ aggr->pdev = pdev;
+ module_put(THIS_MODULE);
+ return count;
+
+remove_table:
+ gpiod_remove_lookup_table(aggr->lookups);
+free_dev_id:
+ kfree(aggr->lookups->dev_id);
+remove_idr:
+ mutex_lock(&gpio_aggregator_lock);
+ idr_remove(&gpio_aggregator_idr, id);
+ mutex_unlock(&gpio_aggregator_lock);
+free_table:
+ kfree(aggr->lookups);
+free_ga:
+ kfree(aggr);
+put_module:
+ module_put(THIS_MODULE);
+ return res;
+}
+
+static DRIVER_ATTR_WO(new_device);
+
+static void gpio_aggregator_free(struct gpio_aggregator *aggr)
+{
+ platform_device_unregister(aggr->pdev);
+ gpiod_remove_lookup_table(aggr->lookups);
+ kfree(aggr->lookups->dev_id);
+ kfree(aggr->lookups);
+ kfree(aggr);
+}
+
+static ssize_t delete_device_store(struct device_driver *driver,
+ const char *buf, size_t count)
+{
+ struct gpio_aggregator *aggr;
+ unsigned int id;
+ int error;
+
+ if (!str_has_prefix(buf, DRV_NAME "."))
+ return -EINVAL;
+
+ error = kstrtouint(buf + strlen(DRV_NAME "."), 10, &id);
+ if (error)
+ return error;
+
+ if (!try_module_get(THIS_MODULE))
+ return -ENOENT;
+
+ mutex_lock(&gpio_aggregator_lock);
+ aggr = idr_remove(&gpio_aggregator_idr, id);
+ mutex_unlock(&gpio_aggregator_lock);
+ if (!aggr) {
+ module_put(THIS_MODULE);
+ return -ENOENT;
+ }
+
+ gpio_aggregator_free(aggr);
+ module_put(THIS_MODULE);
+ return count;
+}
+static DRIVER_ATTR_WO(delete_device);
+
+static struct attribute *gpio_aggregator_attrs[] = {
+ &driver_attr_new_device.attr,
+ &driver_attr_delete_device.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(gpio_aggregator);
+
+
/*
* GPIO Aggregator platform device
*/
@@ -630,6 +620,20 @@ static struct platform_driver gpio_aggregator_driver = {
},
};
+static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
+{
+ gpio_aggregator_free(p);
+ return 0;
+}
+
+static void __exit gpio_aggregator_remove_all(void)
+{
+ mutex_lock(&gpio_aggregator_lock);
+ idr_for_each(&gpio_aggregator_idr, gpio_aggregator_idr_remove, NULL);
+ idr_destroy(&gpio_aggregator_idr);
+ mutex_unlock(&gpio_aggregator_lock);
+}
+
static int __init gpio_aggregator_init(void)
{
return platform_driver_register(&gpio_aggregator_driver);
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 3/9] gpio: aggregator: add aggr_alloc()/aggr_free()
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
2025-02-24 14:31 ` [PATCH v5 1/9] gpio: aggregator: protect driver attr handlers against module unload Koichiro Den
2025-02-24 14:31 ` [PATCH v5 2/9] gpio: aggregator: reorder functions to prepare for configfs introduction Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface Koichiro Den
` (7 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Prepare for the upcoming configfs interface. These functions will be
used by both the existing sysfs interface and the new configfs
interface, reducing code duplication.
No functional change.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 58 +++++++++++++++++++++-------------
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 893cd56de867..a93b7d3de929 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -36,12 +36,41 @@
struct gpio_aggregator {
struct gpiod_lookup_table *lookups;
struct platform_device *pdev;
+ int id;
char args[];
};
static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */
static DEFINE_IDR(gpio_aggregator_idr);
+static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size)
+{
+ struct gpio_aggregator *new __free(kfree) = NULL;
+ int ret;
+
+ new = kzalloc(sizeof(*new) + arg_size, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ mutex_lock(&gpio_aggregator_lock);
+ ret = idr_alloc(&gpio_aggregator_idr, new, 0, 0, GFP_KERNEL);
+ mutex_unlock(&gpio_aggregator_lock);
+ if (ret < 0)
+ return ret;
+
+ new->id = ret;
+ *aggr = no_free_ptr(new);
+ return 0;
+}
+
+static void aggr_free(struct gpio_aggregator *aggr)
+{
+ mutex_lock(&gpio_aggregator_lock);
+ idr_remove(&gpio_aggregator_idr, aggr->id);
+ mutex_unlock(&gpio_aggregator_lock);
+ kfree(aggr);
+}
+
static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
int hwnum, unsigned int *n)
{
@@ -444,17 +473,15 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
{
struct gpio_aggregator *aggr;
struct platform_device *pdev;
- int res, id;
+ int res;
if (!try_module_get(THIS_MODULE))
return -ENOENT;
/* kernfs guarantees string termination, so count + 1 is safe */
- aggr = kzalloc(sizeof(*aggr) + count + 1, GFP_KERNEL);
- if (!aggr) {
- res = -ENOMEM;
+ res = aggr_alloc(&aggr, count + 1);
+ if (res)
goto put_module;
- }
memcpy(aggr->args, buf, count + 1);
@@ -465,19 +492,10 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
goto free_ga;
}
- mutex_lock(&gpio_aggregator_lock);
- id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
- mutex_unlock(&gpio_aggregator_lock);
-
- if (id < 0) {
- res = id;
- goto free_table;
- }
-
- aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, id);
+ aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
if (!aggr->lookups->dev_id) {
res = -ENOMEM;
- goto remove_idr;
+ goto free_table;
}
res = aggr_parse(aggr);
@@ -486,7 +504,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
gpiod_add_lookup_table(aggr->lookups);
- pdev = platform_device_register_simple(DRV_NAME, id, NULL, 0);
+ pdev = platform_device_register_simple(DRV_NAME, aggr->id, NULL, 0);
if (IS_ERR(pdev)) {
res = PTR_ERR(pdev);
goto remove_table;
@@ -500,14 +518,10 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
gpiod_remove_lookup_table(aggr->lookups);
free_dev_id:
kfree(aggr->lookups->dev_id);
-remove_idr:
- mutex_lock(&gpio_aggregator_lock);
- idr_remove(&gpio_aggregator_idr, id);
- mutex_unlock(&gpio_aggregator_lock);
free_table:
kfree(aggr->lookups);
free_ga:
- kfree(aggr);
+ aggr_free(aggr);
put_module:
module_put(THIS_MODULE);
return res;
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (2 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 3/9] gpio: aggregator: add aggr_alloc()/aggr_free() Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names Koichiro Den
` (6 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
The existing sysfs 'new_device' interface has several limitations:
* No way to determine when GPIO aggregator creation is complete.
* No way to retrieve errors when creating a GPIO aggregator.
* No way to trace a GPIO line of an aggregator back to its
corresponding physical device.
* The 'new_device' echo does not indicate which virtual gpiochip<N>
was created.
* No way to assign names to GPIO lines exported through an aggregator.
Introduce the new configfs interface for gpio-aggregator to address
these limitations. It provides a more streamlined, modern, and
extensible configuration method. For backward compatibility, the
'new_device' interface and its behavior is retained for now.
This commit implements basic functionalities:
/config/gpio-aggregator/<name-of-your-choice>/
/config/gpio-aggregator/<name-of-your-choice>/live
/config/gpio-aggregator/<name-of-your-choice>/dev_name
/config/gpio-aggregator/<name-of-your-choice>/<lineY>/
/config/gpio-aggregator/<name-of-your-choice>/<lineY>/key
/config/gpio-aggregator/<name-of-your-choice>/<lineY>/offset
Basic setup flow is:
1. Create a directory for a GPIO aggregator.
2. Create subdirectories for each line you want to instantiate.
3. In each line directory, configure the key and offset.
The key/offset semantics are as follows:
* If offset is >= 0:
- key specifies the name of the chip this GPIO belongs to
- offset specifies the line offset within that chip.
* If offset is <0:
- key needs to specify the GPIO line name.
4. Return to the aggregator's root directory and write '1' to the live
attribute.
For example, the command in the existing kernel doc:
echo 'e6052000.gpio 19 e6050000.gpio 20-21' > new_device
is equivalent to:
mkdir /sys/kernel/config/gpio-aggregator/<custom-name>
# Change <custom-name> to name of your choice (e.g. "aggr0")
cd /sys/kernel/config/gpio-aggregator/<custom-name>
mkdir line0 line1 line2 # Only "line<Y>" naming allowed.
echo e6052000.gpio > line0/key
echo 19 > line0/offset
echo e6050000.gpio > line1/key
echo 20 > line1/offset
echo e6050000.gpio > line2/key
echo 21 > line2/offset
echo 1 > live
The corresponding gpio_device id can be identified as follows:
cd /sys/kernel/config/gpio-aggregator/<custom-name>
ls -d /sys/devices/platform/`cat dev_name`/gpiochip*
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/Kconfig | 2 +
drivers/gpio/gpio-aggregator.c | 546 ++++++++++++++++++++++++++++++++-
2 files changed, 538 insertions(+), 10 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 06cc9b826483..4c157266b428 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1868,6 +1868,8 @@ menu "Virtual GPIO drivers"
config GPIO_AGGREGATOR
tristate "GPIO Aggregator"
+ select CONFIGFS_FS
+ select DEV_SYNC_PROBE
help
Say yes here to enable the GPIO Aggregator, which provides a way to
aggregate existing GPIO lines into a new virtual GPIO chip.
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index a93b7d3de929..0edb177bd816 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -9,10 +9,13 @@
#include <linux/bitmap.h>
#include <linux/bitops.h>
+#include <linux/configfs.h>
#include <linux/ctype.h>
#include <linux/delay.h>
#include <linux/idr.h>
#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/lockdep.h>
#include <linux/mod_devicetable.h>
#include <linux/module.h>
#include <linux/mutex.h>
@@ -27,6 +30,8 @@
#include <linux/gpio/driver.h>
#include <linux/gpio/machine.h>
+#include "dev-sync-probe.h"
+
#define AGGREGATOR_MAX_GPIOS 512
/*
@@ -34,12 +39,36 @@
*/
struct gpio_aggregator {
+ struct dev_sync_probe_data probe_data;
+ struct config_group group;
struct gpiod_lookup_table *lookups;
- struct platform_device *pdev;
+ struct mutex lock;
int id;
+
+ /* List of gpio_aggregator_line. Always added in order */
+ struct list_head list_head;
+
+ /* used by legacy sysfs interface only */
+ bool init_via_sysfs;
char args[];
};
+struct gpio_aggregator_line {
+ struct config_group group;
+ struct gpio_aggregator *parent;
+ struct list_head entry;
+
+ /* Line index within the aggregator device */
+ unsigned int idx;
+
+ /* GPIO chip label or line name */
+ const char *key;
+ /* Can be negative to indicate lookup by line name */
+ int offset;
+
+ enum gpio_lookup_flags flags;
+};
+
static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */
static DEFINE_IDR(gpio_aggregator_idr);
@@ -59,6 +88,8 @@ static int aggr_alloc(struct gpio_aggregator **aggr, size_t arg_size)
return ret;
new->id = ret;
+ INIT_LIST_HEAD(&new->list_head);
+ mutex_init(&new->lock);
*aggr = no_free_ptr(new);
return 0;
}
@@ -68,6 +99,7 @@ static void aggr_free(struct gpio_aggregator *aggr)
mutex_lock(&gpio_aggregator_lock);
idr_remove(&gpio_aggregator_idr, aggr->id);
mutex_unlock(&gpio_aggregator_lock);
+ mutex_destroy(&aggr->lock);
kfree(aggr);
}
@@ -90,6 +122,70 @@ static int aggr_add_gpio(struct gpio_aggregator *aggr, const char *key,
return 0;
}
+static bool aggr_is_active(struct gpio_aggregator *aggr)
+{
+ lockdep_assert_held(&aggr->lock);
+
+ return aggr->probe_data.pdev && platform_get_drvdata(aggr->probe_data.pdev);
+}
+
+static size_t aggr_count_lines(struct gpio_aggregator *aggr)
+{
+ lockdep_assert_held(&aggr->lock);
+
+ return list_count_nodes(&aggr->list_head);
+}
+
+static struct gpio_aggregator_line *aggr_line_alloc(
+ struct gpio_aggregator *parent, unsigned int idx, char *key, int offset)
+{
+ struct gpio_aggregator_line *line;
+
+ line = kzalloc(sizeof(*line), GFP_KERNEL);
+ if (!line)
+ return ERR_PTR(-ENOMEM);
+
+ if (key) {
+ line->key = kstrdup(key, GFP_KERNEL);
+ if (!line->key) {
+ kfree(line);
+ return ERR_PTR(-ENOMEM);
+ }
+ }
+
+ line->flags = GPIO_LOOKUP_FLAGS_DEFAULT;
+ line->parent = parent;
+ line->idx = idx;
+ line->offset = offset;
+ INIT_LIST_HEAD(&line->entry);
+
+ return line;
+}
+
+static void aggr_line_add(struct gpio_aggregator *aggr,
+ struct gpio_aggregator_line *line)
+{
+ struct gpio_aggregator_line *tmp;
+
+ lockdep_assert_held(&aggr->lock);
+
+ list_for_each_entry(tmp, &aggr->list_head, entry) {
+ if (tmp->idx > line->idx) {
+ list_add_tail(&line->entry, &tmp->entry);
+ return;
+ }
+ }
+ list_add_tail(&line->entry, &aggr->list_head);
+}
+
+static void aggr_line_del(struct gpio_aggregator *aggr,
+ struct gpio_aggregator_line *line)
+{
+ lockdep_assert_held(&aggr->lock);
+
+ list_del(&line->entry);
+}
+
/*
* GPIO Forwarder
@@ -414,6 +510,400 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
}
+/*
+ * Configfs interface
+ */
+
+static struct gpio_aggregator *
+to_gpio_aggregator(struct config_item *item)
+{
+ struct config_group *group = to_config_group(item);
+
+ return container_of(group, struct gpio_aggregator, group);
+}
+
+static struct gpio_aggregator_line *
+to_gpio_aggregator_line(struct config_item *item)
+{
+ struct config_group *group = to_config_group(item);
+
+ return container_of(group, struct gpio_aggregator_line, group);
+}
+
+static int aggr_activate(struct gpio_aggregator *aggr)
+{
+ struct platform_device_info pdevinfo;
+ struct gpio_aggregator_line *line;
+ unsigned int n = 0;
+ int ret = 0;
+
+ if (aggr_count_lines(aggr) == 0)
+ return -EINVAL;
+
+ aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
+ GFP_KERNEL);
+ if (!aggr->lookups)
+ return -ENOMEM;
+
+ memset(&pdevinfo, 0, sizeof(pdevinfo));
+ pdevinfo.name = DRV_NAME;
+ pdevinfo.id = aggr->id;
+
+ /* The list is always sorted as new elements are inserted in order. */
+ list_for_each_entry(line, &aggr->list_head, entry) {
+ /*
+ * - Either GPIO chip label or line name must be configured
+ * (i.e. line->key must be non-NULL)
+ * - Line directories must be named with sequential numeric
+ * suffixes starting from 0. (i.e. ./line0, ./line1, ...)
+ */
+ if (!line->key || line->idx != n) {
+ ret = -EINVAL;
+ goto err_remove_lookups;
+ }
+
+ if (line->offset < 0)
+ ret = aggr_add_gpio(aggr, line->key, U16_MAX, &n);
+ else
+ ret = aggr_add_gpio(aggr, line->key, line->offset, &n);
+ if (ret)
+ goto err_remove_lookups;
+ }
+
+ aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
+ if (!aggr->lookups->dev_id) {
+ ret = -ENOMEM;
+ goto err_remove_lookups;
+ }
+
+ gpiod_add_lookup_table(aggr->lookups);
+
+ ret = dev_sync_probe_register(&aggr->probe_data, &pdevinfo);
+ if (ret)
+ goto err_remove_lookup_table;
+
+ return 0;
+
+err_remove_lookup_table:
+ kfree(aggr->lookups->dev_id);
+ gpiod_remove_lookup_table(aggr->lookups);
+err_remove_lookups:
+ kfree(aggr->lookups);
+
+ return ret;
+}
+
+static void aggr_deactivate(struct gpio_aggregator *aggr)
+{
+ dev_sync_probe_unregister(&aggr->probe_data);
+ gpiod_remove_lookup_table(aggr->lookups);
+ kfree(aggr->lookups->dev_id);
+ kfree(aggr->lookups);
+}
+
+static void aggr_lockup_configfs(struct gpio_aggregator *aggr, bool lock)
+{
+ struct configfs_subsystem *subsys = aggr->group.cg_subsys;
+ struct gpio_aggregator_line *line;
+
+ /*
+ * The device only needs to depend on leaf lines. This is
+ * sufficient to lock up all the configfs entries that the
+ * instantiated, alive device depends on.
+ */
+ list_for_each_entry(line, &aggr->list_head, entry) {
+ if (lock)
+ configfs_depend_item_unlocked(
+ subsys, &line->group.cg_item);
+ else
+ configfs_undepend_item_unlocked(
+ &line->group.cg_item);
+ }
+}
+
+static ssize_t
+gpio_aggr_line_key_show(struct config_item *item, char *page)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+
+ guard(mutex)(&aggr->lock);
+
+ return sysfs_emit(page, "%s\n", line->key ?: "");
+}
+
+static ssize_t
+gpio_aggr_line_key_store(struct config_item *item, const char *page,
+ size_t count)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+
+ char *key __free(kfree) = kstrndup(skip_spaces(page), count,
+ GFP_KERNEL);
+ if (!key)
+ return -ENOMEM;
+
+ strim(key);
+
+ guard(mutex)(&aggr->lock);
+
+ if (aggr_is_active(aggr))
+ return -EBUSY;
+
+ kfree(line->key);
+ line->key = no_free_ptr(key);
+
+ return count;
+}
+CONFIGFS_ATTR(gpio_aggr_line_, key);
+
+static ssize_t
+gpio_aggr_line_offset_show(struct config_item *item, char *page)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+ unsigned int offset;
+
+ scoped_guard(mutex, &aggr->lock)
+ offset = line->offset;
+
+ return sysfs_emit(page, "%d\n", offset);
+}
+
+static ssize_t
+gpio_aggr_line_offset_store(struct config_item *item, const char *page,
+ size_t count)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+ int offset, ret;
+
+ ret = kstrtoint(page, 0, &offset);
+ if (ret)
+ return ret;
+
+ /*
+ * When offset == -1, 'key' represents a line name to lookup.
+ * When 0 <= offset < 65535, 'key' represents the label of the chip with
+ * the 'offset' value representing the line within that chip.
+ *
+ * GPIOLIB uses the U16_MAX value to indicate lookup by line name so
+ * the greatest offset we can accept is (U16_MAX - 1).
+ */
+ if (offset > (U16_MAX - 1) || offset < -1)
+ return -EINVAL;
+
+ guard(mutex)(&aggr->lock);
+
+ if (aggr_is_active(aggr))
+ return -EBUSY;
+
+ line->offset = offset;
+
+ return count;
+}
+CONFIGFS_ATTR(gpio_aggr_line_, offset);
+
+static struct configfs_attribute *gpio_aggr_line_attrs[] = {
+ &gpio_aggr_line_attr_key,
+ &gpio_aggr_line_attr_offset,
+ NULL
+};
+
+static ssize_t
+gpio_aggr_device_dev_name_show(struct config_item *item, char *page)
+{
+ struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+ struct platform_device *pdev;
+
+ guard(mutex)(&aggr->lock);
+
+ pdev = aggr->probe_data.pdev;
+ if (pdev)
+ return sysfs_emit(page, "%s\n", dev_name(&pdev->dev));
+
+ return sysfs_emit(page, "%s.%d\n", DRV_NAME, aggr->id);
+}
+CONFIGFS_ATTR_RO(gpio_aggr_device_, dev_name);
+
+static ssize_t
+gpio_aggr_device_live_show(struct config_item *item, char *page)
+{
+ struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+ bool active;
+
+ scoped_guard(mutex, &aggr->lock)
+ active = aggr_is_active(aggr);
+
+ return sysfs_emit(page, "%c\n", active ? '1' : '0');
+}
+
+static ssize_t
+gpio_aggr_device_live_store(struct config_item *item, const char *page,
+ size_t count)
+{
+ struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+ int ret = 0;
+ bool live;
+
+ ret = kstrtobool(page, &live);
+ if (ret)
+ return ret;
+
+ if (!try_module_get(THIS_MODULE))
+ return -ENOENT;
+
+ if (live)
+ aggr_lockup_configfs(aggr, true);
+
+ scoped_guard(mutex, &aggr->lock) {
+ if (live == aggr_is_active(aggr))
+ ret = -EPERM;
+ else if (live)
+ ret = aggr_activate(aggr);
+ else
+ aggr_deactivate(aggr);
+ }
+
+ /*
+ * Undepend is required only if device disablement (live == 0)
+ * succeeds or if device enablement (live == 1) fails.
+ */
+ if (live == !!ret)
+ aggr_lockup_configfs(aggr, false);
+
+ module_put(THIS_MODULE);
+
+ return ret ?: count;
+}
+CONFIGFS_ATTR(gpio_aggr_device_, live);
+
+static struct configfs_attribute *gpio_aggr_device_attrs[] = {
+ &gpio_aggr_device_attr_dev_name,
+ &gpio_aggr_device_attr_live,
+ NULL
+};
+
+static void
+gpio_aggr_line_release(struct config_item *item)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+
+ guard(mutex)(&aggr->lock);
+
+ aggr_line_del(aggr, line);
+ kfree(line->key);
+ kfree(line);
+}
+
+static struct configfs_item_operations gpio_aggr_line_item_ops = {
+ .release = gpio_aggr_line_release,
+};
+
+static const struct config_item_type gpio_aggr_line_type = {
+ .ct_item_ops = &gpio_aggr_line_item_ops,
+ .ct_attrs = gpio_aggr_line_attrs,
+ .ct_owner = THIS_MODULE,
+};
+
+static void gpio_aggr_device_release(struct config_item *item)
+{
+ struct gpio_aggregator *aggr = to_gpio_aggregator(item);
+
+ guard(mutex)(&aggr->lock);
+
+ /*
+ * If the aggregator is active, this code wouldn't be reached,
+ * so calling aggr_deactivate() is always unnecessary.
+ */
+ aggr_free(aggr);
+}
+
+static struct configfs_item_operations gpio_aggr_device_item_ops = {
+ .release = gpio_aggr_device_release,
+};
+
+static struct config_group *
+gpio_aggr_device_make_group(struct config_group *group, const char *name)
+{
+ struct gpio_aggregator *aggr = to_gpio_aggregator(&group->cg_item);
+ struct gpio_aggregator_line *line;
+ unsigned int idx;
+ int ret, nchar;
+
+ ret = sscanf(name, "line%u%n", &idx, &nchar);
+ if (ret != 1 || nchar != strlen(name))
+ return ERR_PTR(-EINVAL);
+
+ guard(mutex)(&aggr->lock);
+
+ if (aggr_is_active(aggr))
+ return ERR_PTR(-EBUSY);
+
+ list_for_each_entry(line, &aggr->list_head, entry)
+ if (line->idx == idx)
+ return ERR_PTR(-EINVAL);
+
+ line = aggr_line_alloc(aggr, idx, NULL, -1);
+ if (!line)
+ return ERR_PTR(-ENOMEM);
+
+ config_group_init_type_name(&line->group, name, &gpio_aggr_line_type);
+
+ aggr_line_add(aggr, line);
+
+ return &line->group;
+}
+
+static struct configfs_group_operations gpio_aggr_device_group_ops = {
+ .make_group = gpio_aggr_device_make_group,
+};
+
+static const struct config_item_type gpio_aggr_device_type = {
+ .ct_group_ops = &gpio_aggr_device_group_ops,
+ .ct_item_ops = &gpio_aggr_device_item_ops,
+ .ct_attrs = gpio_aggr_device_attrs,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct config_group *
+gpio_aggr_make_group(struct config_group *group, const char *name)
+{
+ struct gpio_aggregator *aggr;
+ int ret;
+
+ /* arg space is unneeded */
+ ret = aggr_alloc(&aggr, 0);
+ if (ret)
+ return ERR_PTR(ret);
+
+ config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
+ dev_sync_probe_init(&aggr->probe_data);
+
+ return &aggr->group;
+}
+
+static struct configfs_group_operations gpio_aggr_group_ops = {
+ .make_group = gpio_aggr_make_group,
+};
+
+static const struct config_item_type gpio_aggr_type = {
+ .ct_group_ops = &gpio_aggr_group_ops,
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem gpio_aggr_subsys = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = DRV_NAME,
+ .ci_type = &gpio_aggr_type,
+ },
+ },
+};
+
+
/*
* Sysfs interface
*/
@@ -485,6 +975,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
memcpy(aggr->args, buf, count + 1);
+ aggr->init_via_sysfs = true;
aggr->lookups = kzalloc(struct_size(aggr->lookups, table, 1),
GFP_KERNEL);
if (!aggr->lookups) {
@@ -510,7 +1001,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
goto remove_table;
}
- aggr->pdev = pdev;
+ aggr->probe_data.pdev = pdev;
module_put(THIS_MODULE);
return count;
@@ -531,10 +1022,7 @@ static DRIVER_ATTR_WO(new_device);
static void gpio_aggregator_free(struct gpio_aggregator *aggr)
{
- platform_device_unregister(aggr->pdev);
- gpiod_remove_lookup_table(aggr->lookups);
- kfree(aggr->lookups->dev_id);
- kfree(aggr->lookups);
+ aggr_deactivate(aggr);
kfree(aggr);
}
@@ -556,12 +1044,19 @@ static ssize_t delete_device_store(struct device_driver *driver,
return -ENOENT;
mutex_lock(&gpio_aggregator_lock);
- aggr = idr_remove(&gpio_aggregator_idr, id);
- mutex_unlock(&gpio_aggregator_lock);
- if (!aggr) {
+ aggr = idr_find(&gpio_aggregator_idr, id);
+ /*
+ * For simplicity, devices created via configfs cannot be deleted
+ * via sysfs.
+ */
+ if (aggr && aggr->init_via_sysfs)
+ idr_remove(&gpio_aggregator_idr, id);
+ else {
+ mutex_unlock(&gpio_aggregator_lock);
module_put(THIS_MODULE);
return -ENOENT;
}
+ mutex_unlock(&gpio_aggregator_lock);
gpio_aggregator_free(aggr);
module_put(THIS_MODULE);
@@ -636,6 +1131,10 @@ static struct platform_driver gpio_aggregator_driver = {
static int __exit gpio_aggregator_idr_remove(int id, void *p, void *data)
{
+ /*
+ * There should be no aggregator created via configfs, as their
+ * presence would prevent module unloading.
+ */
gpio_aggregator_free(p);
return 0;
}
@@ -650,7 +1149,33 @@ static void __exit gpio_aggregator_remove_all(void)
static int __init gpio_aggregator_init(void)
{
- return platform_driver_register(&gpio_aggregator_driver);
+ int ret = 0;
+
+ config_group_init(&gpio_aggr_subsys.su_group);
+ mutex_init(&gpio_aggr_subsys.su_mutex);
+ ret = configfs_register_subsystem(&gpio_aggr_subsys);
+ if (ret) {
+ pr_err("Failed to register the '%s' configfs subsystem: %d\n",
+ gpio_aggr_subsys.su_group.cg_item.ci_namebuf, ret);
+ mutex_destroy(&gpio_aggr_subsys.su_mutex);
+ return ret;
+ }
+
+ /*
+ * CAVEAT: This must occur after configfs registration. Otherwise,
+ * a race condition could arise: driver attribute groups might be
+ * exposed and accessed by users before configfs registration
+ * completes. new_device_store() does not expect a partially
+ * initialized configfs state.
+ */
+ ret = platform_driver_register(&gpio_aggregator_driver);
+ if (ret) {
+ pr_err("Failed to register the platform driver: %d\n", ret);
+ mutex_destroy(&gpio_aggr_subsys.su_mutex);
+ configfs_unregister_subsystem(&gpio_aggr_subsys);
+ }
+
+ return ret;
}
module_init(gpio_aggregator_init);
@@ -658,6 +1183,7 @@ static void __exit gpio_aggregator_exit(void)
{
gpio_aggregator_remove_all();
platform_driver_unregister(&gpio_aggregator_driver);
+ configfs_unregister_subsystem(&gpio_aggr_subsys);
}
module_exit(gpio_aggregator_exit);
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (3 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-27 9:50 ` Bartosz Golaszewski
2025-02-24 14:31 ` [PATCH v5 6/9] gpio: aggregator: rename 'name' to 'key' in aggr_parse() Koichiro Den
` (5 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Previously, there was no way to assign names to GPIO lines exported
through the aggregator when the device was created via sysfs.
Allow users to set custom line names via a 'name' attribute and
expose them using swnode.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 84 ++++++++++++++++++++++++++++++++--
1 file changed, 81 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 0edb177bd816..e6d455089a27 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -61,6 +61,8 @@ struct gpio_aggregator_line {
/* Line index within the aggregator device */
unsigned int idx;
+ /* Custom name for the virtual line */
+ const char *name;
/* GPIO chip label or line name */
const char *key;
/* Can be negative to indicate lookup by line name */
@@ -530,10 +532,40 @@ to_gpio_aggregator_line(struct config_item *item)
return container_of(group, struct gpio_aggregator_line, group);
}
+static struct fwnode_handle *aggr_make_device_swnode(struct gpio_aggregator *aggr)
+{
+ const char **line_names __free(kfree) = NULL;
+ struct property_entry properties[2];
+ struct gpio_aggregator_line *line;
+ size_t num_lines;
+ int n = 0;
+
+ memset(properties, 0, sizeof(properties));
+
+ num_lines = aggr_count_lines(aggr);
+ if (num_lines == 0)
+ return NULL;
+
+ line_names = kcalloc(num_lines, sizeof(*line_names), GFP_KERNEL);
+ if (!line_names)
+ return ERR_PTR(-ENOMEM);
+
+ /* The list is always sorted as new elements are inserted in order. */
+ list_for_each_entry(line, &aggr->list_head, entry)
+ line_names[n++] = line->name ?: "";
+
+ properties[0] = PROPERTY_ENTRY_STRING_ARRAY_LEN(
+ "gpio-line-names",
+ line_names, num_lines);
+
+ return fwnode_create_software_node(properties, NULL);
+}
+
static int aggr_activate(struct gpio_aggregator *aggr)
{
struct platform_device_info pdevinfo;
struct gpio_aggregator_line *line;
+ struct fwnode_handle *swnode;
unsigned int n = 0;
int ret = 0;
@@ -545,9 +577,14 @@ static int aggr_activate(struct gpio_aggregator *aggr)
if (!aggr->lookups)
return -ENOMEM;
+ swnode = aggr_make_device_swnode(aggr);
+ if (IS_ERR(swnode))
+ goto err_remove_lookups;
+
memset(&pdevinfo, 0, sizeof(pdevinfo));
pdevinfo.name = DRV_NAME;
pdevinfo.id = aggr->id;
+ pdevinfo.fwnode = swnode;
/* The list is always sorted as new elements are inserted in order. */
list_for_each_entry(line, &aggr->list_head, entry) {
@@ -559,7 +596,7 @@ static int aggr_activate(struct gpio_aggregator *aggr)
*/
if (!line->key || line->idx != n) {
ret = -EINVAL;
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
if (line->offset < 0)
@@ -567,13 +604,13 @@ static int aggr_activate(struct gpio_aggregator *aggr)
else
ret = aggr_add_gpio(aggr, line->key, line->offset, &n);
if (ret)
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
aggr->lookups->dev_id = kasprintf(GFP_KERNEL, "%s.%d", DRV_NAME, aggr->id);
if (!aggr->lookups->dev_id) {
ret = -ENOMEM;
- goto err_remove_lookups;
+ goto err_remove_swnode;
}
gpiod_add_lookup_table(aggr->lookups);
@@ -587,6 +624,8 @@ static int aggr_activate(struct gpio_aggregator *aggr)
err_remove_lookup_table:
kfree(aggr->lookups->dev_id);
gpiod_remove_lookup_table(aggr->lookups);
+err_remove_swnode:
+ fwnode_remove_software_node(swnode);
err_remove_lookups:
kfree(aggr->lookups);
@@ -658,6 +697,43 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
}
CONFIGFS_ATTR(gpio_aggr_line_, key);
+static ssize_t
+gpio_aggr_line_name_show(struct config_item *item, char *page)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+
+ guard(mutex)(&aggr->lock);
+
+ return sysfs_emit(page, "%s\n", line->name ?: "");
+}
+
+static ssize_t
+gpio_aggr_line_name_store(struct config_item *item, const char *page,
+ size_t count)
+{
+ struct gpio_aggregator_line *line = to_gpio_aggregator_line(item);
+ struct gpio_aggregator *aggr = line->parent;
+
+ char *name __free(kfree) = kstrndup(skip_spaces(page), count,
+ GFP_KERNEL);
+ if (!name)
+ return -ENOMEM;
+
+ strim(name);
+
+ guard(mutex)(&aggr->lock);
+
+ if (aggr_is_active(aggr))
+ return -EBUSY;
+
+ kfree(line->name);
+ line->name = no_free_ptr(name);
+
+ return count;
+}
+CONFIGFS_ATTR(gpio_aggr_line_, name);
+
static ssize_t
gpio_aggr_line_offset_show(struct config_item *item, char *page)
{
@@ -707,6 +783,7 @@ CONFIGFS_ATTR(gpio_aggr_line_, offset);
static struct configfs_attribute *gpio_aggr_line_attrs[] = {
&gpio_aggr_line_attr_key,
+ &gpio_aggr_line_attr_name,
&gpio_aggr_line_attr_offset,
NULL
};
@@ -795,6 +872,7 @@ gpio_aggr_line_release(struct config_item *item)
aggr_line_del(aggr, line);
kfree(line->key);
+ kfree(line->name);
kfree(line);
}
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names
2025-02-24 14:31 ` [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names Koichiro Den
@ 2025-02-27 9:50 ` Bartosz Golaszewski
2025-03-05 12:44 ` Koichiro Den
0 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-02-27 9:50 UTC (permalink / raw)
To: Koichiro Den
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Feb 24, 2025 at 3:32 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> Previously, there was no way to assign names to GPIO lines exported
> through the aggregator when the device was created via sysfs.
>
> Allow users to set custom line names via a 'name' attribute and
> expose them using swnode.
>
> Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> ---
IMO this should be part of the previous commit, there's no reason to
split the configfs implementation. But only change that if there'll be
a v6, otherwise I'll take it as is.
Bart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names
2025-02-27 9:50 ` Bartosz Golaszewski
@ 2025-03-05 12:44 ` Koichiro Den
0 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-03-05 12:44 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Thu, Feb 27, 2025 at 10:50:34AM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 24, 2025 at 3:32 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > Previously, there was no way to assign names to GPIO lines exported
> > through the aggregator when the device was created via sysfs.
> >
> > Allow users to set custom line names via a 'name' attribute and
> > expose them using swnode.
> >
> > Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
> > ---
>
> IMO this should be part of the previous commit, there's no reason to
> split the configfs implementation. But only change that if there'll be
> a v6, otherwise I'll take it as is.
Honestly, I held off replying here, hoping to get some input on:
https://lore.kernel.org/all/CAMRc=MegKxwX-RjQQcWMGe_JQyRCv82WRRbD0naYkeXshTGXGQ@mail.gmail.com/
since I thought the decision on sending v6 might depend on the feedback.
Anyhow, thank you for reviewing.
Koichiro
>
> Bart
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v5 6/9] gpio: aggregator: rename 'name' to 'key' in aggr_parse()
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (4 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 5/9] gpio: aggregator: add 'name' attribute for custom GPIO line names Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 7/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs Koichiro Den
` (4 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Rename the local variable 'name' in aggr_parse() to 'key' because struct
gpio_aggregator_line now uses the 'name' field for the custom line name
and the local variable actually represents a 'key'. This change prepares
for the next but one commit.
No functional change.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index e6d455089a27..0c30153ce9ab 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -988,7 +988,7 @@ static struct configfs_subsystem gpio_aggr_subsys = {
static int aggr_parse(struct gpio_aggregator *aggr)
{
char *args = skip_spaces(aggr->args);
- char *name, *offsets, *p;
+ char *key, *offsets, *p;
unsigned int i, n = 0;
int error = 0;
@@ -997,18 +997,18 @@ static int aggr_parse(struct gpio_aggregator *aggr)
if (!bitmap)
return -ENOMEM;
- args = next_arg(args, &name, &p);
+ args = next_arg(args, &key, &p);
while (*args) {
args = next_arg(args, &offsets, &p);
p = get_options(offsets, 0, &error);
if (error == 0 || *p) {
/* Named GPIO line */
- error = aggr_add_gpio(aggr, name, U16_MAX, &n);
+ error = aggr_add_gpio(aggr, key, U16_MAX, &n);
if (error)
return error;
- name = offsets;
+ key = offsets;
continue;
}
@@ -1020,12 +1020,12 @@ static int aggr_parse(struct gpio_aggregator *aggr)
}
for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
- error = aggr_add_gpio(aggr, name, i, &n);
+ error = aggr_add_gpio(aggr, key, i, &n);
if (error)
return error;
}
- args = next_arg(args, &name, &p);
+ args = next_arg(args, &key, &p);
}
if (!n) {
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 7/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (5 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 6/9] gpio: aggregator: rename 'name' to 'key' in aggr_parse() Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 8/9] gpio: aggregator: cancel deferred probe for devices created via configfs Koichiro Den
` (3 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Expose settings for aggregators created using the sysfs 'new_device'
interface to configfs. Once written to 'new_device', an "_sysfs.<N>" path
appears in the configfs regardless of whether the probe succeeds.
Consequently, users can no longer use that prefix for custom GPIO
aggregator names. The 'live' attribute changes to 1 when the probe
succeeds and the GPIO forwarder is instantiated.
Note that the aggregator device created via sysfs is asynchronous, i.e.
writing into 'new_device' returns without waiting for probe completion,
and the probe may succeed, fail, or eventually succeed via deferred
probe. Thus, the 'live' attribute may change from 0 to 1 asynchronously
without notice. So, editing key/offset/name while it's waiting for
deferred probe is prohibited.
The configfs auto-generation relies on create_default_group(), which
inherently prohibits rmdir(2). To align with the limitation, this commit
also prohibits mkdir(2) for them. When users want to change the number
of lines for an aggregator initialized via 'new_device', they need to
tear down the device using 'delete_device' and reconfigure it from
scratch. This does not break previous behavior; users of legacy sysfs
interface simply gain additional almost read-only configfs exposure.
Still, users can write to the 'live' attribute to toggle the device
unless it's waiting for deferred probe. So once probe succeeds, they can
deactivate it in the same manner as the devices initialized via
configfs.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 117 +++++++++++++++++++++++++++++----
1 file changed, 105 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 0c30153ce9ab..6b0e42774b86 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -33,6 +33,7 @@
#include "dev-sync-probe.h"
#define AGGREGATOR_MAX_GPIOS 512
+#define AGGREGATOR_LEGACY_PREFIX "_sysfs"
/*
* GPIO Aggregator sysfs interface
@@ -131,6 +132,14 @@ static bool aggr_is_active(struct gpio_aggregator *aggr)
return aggr->probe_data.pdev && platform_get_drvdata(aggr->probe_data.pdev);
}
+/* Only aggregators created via legacy sysfs can be "activating". */
+static bool aggr_is_activating(struct gpio_aggregator *aggr)
+{
+ lockdep_assert_held(&aggr->lock);
+
+ return aggr->probe_data.pdev && !platform_get_drvdata(aggr->probe_data.pdev);
+}
+
static size_t aggr_count_lines(struct gpio_aggregator *aggr)
{
lockdep_assert_held(&aggr->lock);
@@ -188,6 +197,18 @@ static void aggr_line_del(struct gpio_aggregator *aggr,
list_del(&line->entry);
}
+static void aggr_free_lines(struct gpio_aggregator *aggr)
+{
+ struct gpio_aggregator_line *line, *tmp;
+
+ list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
+ configfs_unregister_group(&line->group);
+ aggr_line_del(aggr, line);
+ kfree(line->key);
+ kfree(line);
+ }
+}
+
/*
* GPIO Forwarder
@@ -687,7 +708,7 @@ gpio_aggr_line_key_store(struct config_item *item, const char *page,
guard(mutex)(&aggr->lock);
- if (aggr_is_active(aggr))
+ if (aggr_is_activating(aggr) || aggr_is_active(aggr))
return -EBUSY;
kfree(line->key);
@@ -724,7 +745,7 @@ gpio_aggr_line_name_store(struct config_item *item, const char *page,
guard(mutex)(&aggr->lock);
- if (aggr_is_active(aggr))
+ if (aggr_is_activating(aggr) || aggr_is_active(aggr))
return -EBUSY;
kfree(line->name);
@@ -772,7 +793,7 @@ gpio_aggr_line_offset_store(struct config_item *item, const char *page,
guard(mutex)(&aggr->lock);
- if (aggr_is_active(aggr))
+ if (aggr_is_activating(aggr) || aggr_is_active(aggr))
return -EBUSY;
line->offset = offset;
@@ -831,11 +852,12 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
if (!try_module_get(THIS_MODULE))
return -ENOENT;
- if (live)
+ if (live && !aggr->init_via_sysfs)
aggr_lockup_configfs(aggr, true);
scoped_guard(mutex, &aggr->lock) {
- if (live == aggr_is_active(aggr))
+ if (aggr_is_activating(aggr) ||
+ (live == aggr_is_active(aggr)))
ret = -EPERM;
else if (live)
ret = aggr_activate(aggr);
@@ -847,7 +869,7 @@ gpio_aggr_device_live_store(struct config_item *item, const char *page,
* Undepend is required only if device disablement (live == 0)
* succeeds or if device enablement (live == 1) fails.
*/
- if (live == !!ret)
+ if (live == !!ret && !aggr->init_via_sysfs)
aggr_lockup_configfs(aggr, false);
module_put(THIS_MODULE);
@@ -893,7 +915,7 @@ static void gpio_aggr_device_release(struct config_item *item)
guard(mutex)(&aggr->lock);
/*
- * If the aggregator is active, this code wouldn't be reached,
+ * At this point, aggr is neither active nor activating,
* so calling aggr_deactivate() is always unnecessary.
*/
aggr_free(aggr);
@@ -915,6 +937,15 @@ gpio_aggr_device_make_group(struct config_group *group, const char *name)
if (ret != 1 || nchar != strlen(name))
return ERR_PTR(-EINVAL);
+ if (aggr->init_via_sysfs)
+ /*
+ * Aggregators created via legacy sysfs interface are exposed as
+ * default groups, which means rmdir(2) is prohibited for them.
+ * For simplicity, and to avoid confusion, we also prohibit
+ * mkdir(2).
+ */
+ return ERR_PTR(-EPERM);
+
guard(mutex)(&aggr->lock);
if (aggr_is_active(aggr))
@@ -952,6 +983,14 @@ gpio_aggr_make_group(struct config_group *group, const char *name)
struct gpio_aggregator *aggr;
int ret;
+ /*
+ * "_sysfs" prefix is reserved for auto-generated config group
+ * for devices create via legacy sysfs interface.
+ */
+ if (strncmp(name, AGGREGATOR_LEGACY_PREFIX,
+ sizeof(AGGREGATOR_LEGACY_PREFIX)) == 0)
+ return ERR_PTR(-EINVAL);
+
/* arg space is unneeded */
ret = aggr_alloc(&aggr, 0);
if (ret)
@@ -988,6 +1027,8 @@ static struct configfs_subsystem gpio_aggr_subsys = {
static int aggr_parse(struct gpio_aggregator *aggr)
{
char *args = skip_spaces(aggr->args);
+ struct gpio_aggregator_line *line;
+ char name[CONFIGFS_ITEM_NAME_LEN];
char *key, *offsets, *p;
unsigned int i, n = 0;
int error = 0;
@@ -999,14 +1040,29 @@ static int aggr_parse(struct gpio_aggregator *aggr)
args = next_arg(args, &key, &p);
while (*args) {
+ scnprintf(name, sizeof(name), "line%u", n);
+
args = next_arg(args, &offsets, &p);
p = get_options(offsets, 0, &error);
if (error == 0 || *p) {
/* Named GPIO line */
+ line = aggr_line_alloc(aggr, n, key, -1);
+ if (!line) {
+ error = -ENOMEM;
+ goto err;
+ }
+ config_group_init_type_name(&line->group, name,
+ &gpio_aggr_line_type);
+ error = configfs_register_group(&aggr->group,
+ &line->group);
+ if (error)
+ goto err;
+ aggr_line_add(aggr, line);
+
error = aggr_add_gpio(aggr, key, U16_MAX, &n);
if (error)
- return error;
+ goto err;
key = offsets;
continue;
@@ -1020,9 +1076,22 @@ static int aggr_parse(struct gpio_aggregator *aggr)
}
for_each_set_bit(i, bitmap, AGGREGATOR_MAX_GPIOS) {
+ line = aggr_line_alloc(aggr, n, key, i);
+ if (!line) {
+ error = -ENOMEM;
+ goto err;
+ }
+ config_group_init_type_name(&line->group, name,
+ &gpio_aggr_line_type);
+ error = configfs_register_group(&aggr->group,
+ &line->group);
+ if (error)
+ goto err;
+ aggr_line_add(aggr, line);
+
error = aggr_add_gpio(aggr, key, i, &n);
if (error)
- return error;
+ goto err;
}
args = next_arg(args, &key, &p);
@@ -1030,15 +1099,20 @@ static int aggr_parse(struct gpio_aggregator *aggr)
if (!n) {
pr_err("No GPIOs specified\n");
- return -EINVAL;
+ goto err;
}
return 0;
+
+err:
+ aggr_free_lines(aggr);
+ return error;
}
static ssize_t new_device_store(struct device_driver *driver, const char *buf,
size_t count)
{
+ char name[CONFIGFS_ITEM_NAME_LEN];
struct gpio_aggregator *aggr;
struct platform_device *pdev;
int res;
@@ -1067,10 +1141,24 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
goto free_table;
}
- res = aggr_parse(aggr);
+ scnprintf(name, sizeof(name), "%s.%d", AGGREGATOR_LEGACY_PREFIX, aggr->id);
+ config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
+
+ /*
+ * Since the device created by sysfs might be toggled via configfs
+ * 'live' attribute later, this initialization is needed.
+ */
+ dev_sync_probe_init(&aggr->probe_data);
+
+ /* Expose to configfs */
+ res = configfs_register_group(&gpio_aggr_subsys.su_group, &aggr->group);
if (res)
goto free_dev_id;
+ res = aggr_parse(aggr);
+ if (res)
+ goto unregister_group;
+
gpiod_add_lookup_table(aggr->lookups);
pdev = platform_device_register_simple(DRV_NAME, aggr->id, NULL, 0);
@@ -1085,6 +1173,8 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
remove_table:
gpiod_remove_lookup_table(aggr->lookups);
+unregister_group:
+ configfs_unregister_group(&aggr->group);
free_dev_id:
kfree(aggr->lookups->dev_id);
free_table:
@@ -1100,7 +1190,10 @@ static DRIVER_ATTR_WO(new_device);
static void gpio_aggregator_free(struct gpio_aggregator *aggr)
{
- aggr_deactivate(aggr);
+ if (aggr_is_activating(aggr) || aggr_is_active(aggr))
+ aggr_deactivate(aggr);
+ aggr_free_lines(aggr);
+ configfs_unregister_group(&aggr->group);
kfree(aggr);
}
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 8/9] gpio: aggregator: cancel deferred probe for devices created via configfs
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (6 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 7/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-02-24 14:31 ` [PATCH v5 9/9] Documentation: gpio: document configfs interface for gpio-aggregator Koichiro Den
` (2 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
For aggregators initialized via configfs, write 1 to 'live' waits for
probe completion and returns an error if the probe fails, unlike the
legacy sysfs interface, which is asynchronous.
Since users control the liveness of the aggregator device and might be
editing configurations while 'live' is 0, deferred probing is both
unnatural and unsafe.
Cancel deferred probe for purely configfs-based aggregators when probe
fails.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
drivers/gpio/gpio-aggregator.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index 6b0e42774b86..137b5d032427 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -72,6 +72,10 @@ struct gpio_aggregator_line {
enum gpio_lookup_flags flags;
};
+struct gpio_aggregator_pdev_meta {
+ bool init_via_sysfs;
+};
+
static DEFINE_MUTEX(gpio_aggregator_lock); /* protects idr */
static DEFINE_IDR(gpio_aggregator_idr);
@@ -1112,6 +1116,7 @@ static int aggr_parse(struct gpio_aggregator *aggr)
static ssize_t new_device_store(struct device_driver *driver, const char *buf,
size_t count)
{
+ struct gpio_aggregator_pdev_meta meta = { .init_via_sysfs = true };
char name[CONFIGFS_ITEM_NAME_LEN];
struct gpio_aggregator *aggr;
struct platform_device *pdev;
@@ -1161,7 +1166,7 @@ static ssize_t new_device_store(struct device_driver *driver, const char *buf,
gpiod_add_lookup_table(aggr->lookups);
- pdev = platform_device_register_simple(DRV_NAME, aggr->id, NULL, 0);
+ pdev = platform_device_register_data(NULL, DRV_NAME, aggr->id, &meta, sizeof(meta));
if (IS_ERR(pdev)) {
res = PTR_ERR(pdev);
goto remove_table;
@@ -1242,14 +1247,15 @@ static struct attribute *gpio_aggregator_attrs[] = {
};
ATTRIBUTE_GROUPS(gpio_aggregator);
-
/*
* GPIO Aggregator platform device
*/
static int gpio_aggregator_probe(struct platform_device *pdev)
{
+ struct gpio_aggregator_pdev_meta *meta;
struct device *dev = &pdev->dev;
+ bool init_via_sysfs = false;
struct gpio_desc **descs;
struct gpiochip_fwd *fwd;
unsigned long features;
@@ -1263,10 +1269,28 @@ static int gpio_aggregator_probe(struct platform_device *pdev)
if (!descs)
return -ENOMEM;
+ meta = dev_get_platdata(&pdev->dev);
+ if (meta && meta->init_via_sysfs)
+ init_via_sysfs = true;
+
for (i = 0; i < n; i++) {
descs[i] = devm_gpiod_get_index(dev, NULL, i, GPIOD_ASIS);
- if (IS_ERR(descs[i]))
+ if (IS_ERR(descs[i])) {
+ /*
+ * Deferred probing is not suitable when the aggregator
+ * is created via configfs. They should just retry later
+ * whenever they like. For device creation via sysfs,
+ * error is propagated without overriding for backward
+ * compatibility. .prevent_deferred_probe is kept unset
+ * for other cases.
+ */
+ if (!init_via_sysfs && !dev_of_node(dev) &&
+ descs[i] == ERR_PTR(-EPROBE_DEFER)) {
+ pr_warn("Deferred probe canceled for creation via configfs.\n");
+ return -ENODEV;
+ }
return PTR_ERR(descs[i]);
+ }
}
features = (uintptr_t)device_get_match_data(dev);
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v5 9/9] Documentation: gpio: document configfs interface for gpio-aggregator
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (7 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 8/9] gpio: aggregator: cancel deferred probe for devices created via configfs Koichiro Den
@ 2025-02-24 14:31 ` Koichiro Den
2025-03-05 12:15 ` (subset) [PATCH v5 0/9] Introduce configfs-based " Bartosz Golaszewski
2025-03-10 10:19 ` Bartosz Golaszewski
10 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-02-24 14:31 UTC (permalink / raw)
To: linux-gpio
Cc: brgl, geert+renesas, linus.walleij, maciej.borzecki, linux-kernel
Add documentation for the newly added configfs-based interface for GPIO
aggregator.
Signed-off-by: Koichiro Den <koichiro.den@canonical.com>
---
.../admin-guide/gpio/gpio-aggregator.rst | 107 ++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/Documentation/admin-guide/gpio/gpio-aggregator.rst b/Documentation/admin-guide/gpio/gpio-aggregator.rst
index 5cd1e7221756..8374a9df9105 100644
--- a/Documentation/admin-guide/gpio/gpio-aggregator.rst
+++ b/Documentation/admin-guide/gpio/gpio-aggregator.rst
@@ -69,6 +69,113 @@ write-only attribute files in sysfs.
$ echo gpio-aggregator.0 > delete_device
+Aggregating GPIOs using Configfs
+--------------------------------
+
+**Group:** ``/config/gpio-aggregator``
+
+ This is the root directory of the gpio-aggregator configfs tree.
+
+**Group:** ``/config/gpio-aggregator/<example-name>``
+
+ This directory represents a GPIO aggregator device. You can assign any
+ name to ``<example-name>`` (e.g. ``agg0``), except names starting with
+ ``_sysfs`` prefix, which are reserved for auto-generated configfs
+ entries corresponding to devices created via Sysfs.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/live``
+
+ The ``live`` attribute allows to trigger the actual creation of the device
+ once it's fully configured. Accepted values are:
+
+ * ``1``, ``yes``, ``true`` : enable the virtual device
+ * ``0``, ``no``, ``false`` : disable the virtual device
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/dev_name``
+
+ The read-only ``dev_name`` attribute exposes the name of the device as it
+ will appear in the system on the platform bus (e.g. ``gpio-aggregator.0``).
+ This is useful for identifying a character device for the newly created
+ aggregator. If it's ``gpio-aggregator.0``,
+ ``/sys/devices/platform/gpio-aggregator.0/gpiochipX`` path tells you that the
+ GPIO device id is ``X``.
+
+You must create subdirectories for each virtual line you want to
+instantiate, named exactly as ``line0``, ``line1``, ..., ``lineY``, when
+you want to instantiate ``Y+1`` (Y >= 0) lines. Configure all lines before
+activating the device by setting ``live`` to 1.
+
+**Group:** ``/config/gpio-aggregator/<example-name>/<lineY>/``
+
+ This directory represents a GPIO line to include in the aggregator.
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/key``
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/offset``
+
+ The default values after creating the ``<lineY>`` directory are:
+
+ * ``key`` : <empty>
+ * ``offset`` : -1
+
+ ``key`` must always be explicitly configured, while ``offset`` depends.
+ Two configuration patterns exist for each ``<lineY>``:
+
+ (a). For lookup by GPIO line name:
+
+ * Set ``key`` to the line name.
+ * Ensure ``offset`` remains -1 (the default).
+
+ (b). For lookup by GPIO chip name and the line offset within the chip:
+
+ * Set ``key`` to the chip name.
+ * Set ``offset`` to the line offset (0 <= ``offset`` < 65535).
+
+**Attribute:** ``/config/gpio-aggregator/<example-name>/<lineY>/name``
+
+ The ``name`` attribute sets a custom name for lineY. If left unset, the
+ line will remain unnamed.
+
+Once the configuration is done, the ``'live'`` attribute must be set to 1
+in order to instantiate the aggregator device. It can be set back to 0 to
+destroy the virtual device. The module will synchronously wait for the new
+aggregator device to be successfully probed and if this doesn't happen, writing
+to ``'live'`` will result in an error. This is a different behaviour from the
+case when you create it using sysfs ``new_device`` interface.
+
+.. note::
+
+ For aggregators created via Sysfs, the configfs entries are
+ auto-generated and appear as ``/config/gpio-aggregator/_sysfs.<N>/``. You
+ cannot add or remove line directories with mkdir(2)/rmdir(2). To modify
+ lines, you must use the "delete_device" interface to tear down the
+ existing device and reconfigure it from scratch. However, you can still
+ toggle the aggregator with the ``live`` attribute and adjust the
+ ``key``, ``offset``, and ``name`` attributes for each line when ``live``
+ is set to 0 by hand (i.e. it's not waiting for deferred probe).
+
+Sample configuration commands
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+.. code-block:: sh
+
+ # Create a directory for an aggregator device
+ $ mkdir /sys/kernel/config/gpio-aggregator/agg0
+
+ # Configure each line
+ $ mkdir /sys/kernel/config/gpio-aggregator/agg0/line0
+ $ echo gpiochip0 > /sys/kernel/config/gpio-aggregator/agg0/line0/key
+ $ echo 6 > /sys/kernel/config/gpio-aggregator/agg0/line0/offset
+ $ echo test0 > /sys/kernel/config/gpio-aggregator/agg0/line0/name
+ $ mkdir /sys/kernel/config/gpio-aggregator/agg0/line1
+ $ echo gpiochip0 > /sys/kernel/config/gpio-aggregator/agg0/line1/key
+ $ echo 7 > /sys/kernel/config/gpio-aggregator/agg0/line1/offset
+ $ echo test1 > /sys/kernel/config/gpio-aggregator/agg0/line1/name
+
+ # Activate the aggregator device
+ $ echo 1 > /sys/kernel/config/gpio-aggregator/agg0/live
+
+
Generic GPIO Driver
-------------------
--
2.45.2
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: (subset) [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (8 preceding siblings ...)
2025-02-24 14:31 ` [PATCH v5 9/9] Documentation: gpio: document configfs interface for gpio-aggregator Koichiro Den
@ 2025-03-05 12:15 ` Bartosz Golaszewski
2025-03-10 10:19 ` Bartosz Golaszewski
10 siblings, 0 replies; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-03-05 12:15 UTC (permalink / raw)
To: linux-gpio, Koichiro Den
Cc: Bartosz Golaszewski, brgl, geert+renesas, linus.walleij,
maciej.borzecki, linux-kernel
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
On Mon, 24 Feb 2025 23:31:25 +0900, Koichiro Den wrote:
> This patch series introduces a configfs-based interface to gpio-aggregator
> to address limitations in the existing 'new_device' interface.
>
> The existing 'new_device' interface has several limitations:
>
> Issue#1. No way to determine when GPIO aggregator creation is complete.
> Issue#2. No way to retrieve errors when creating a GPIO aggregator.
> Issue#3. No way to trace a GPIO line of an aggregator back to its
> corresponding physical device.
> Issue#4. The 'new_device' echo does not indicate which virtual
> gpiochip<N> was created.
> Issue#5. No way to assign names to GPIO lines exported through an
> aggregator.
>
> [...]
I don't see anything wrong with this patch so I queued it for fixes. I
will pull it back into my gpio/for-next branch once it's upstream and
apply the rest of the aggregator changes for v6.15.
[1/9] gpio: aggregator: protect driver attr handlers against module unload
commit: 12f65d1203507f7db3ba59930fe29a3b8eee9945
Best regards,
--
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-02-24 14:31 [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator Koichiro Den
` (9 preceding siblings ...)
2025-03-05 12:15 ` (subset) [PATCH v5 0/9] Introduce configfs-based " Bartosz Golaszewski
@ 2025-03-10 10:19 ` Bartosz Golaszewski
2025-03-10 13:32 ` Koichiro Den
10 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-03-10 10:19 UTC (permalink / raw)
To: Koichiro Den
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Feb 24, 2025 at 3:31 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
> This patch series introduces a configfs-based interface to gpio-aggregator
> to address limitations in the existing 'new_device' interface.
>
> The existing 'new_device' interface has several limitations:
>
> Issue#1. No way to determine when GPIO aggregator creation is complete.
> Issue#2. No way to retrieve errors when creating a GPIO aggregator.
> Issue#3. No way to trace a GPIO line of an aggregator back to its
> corresponding physical device.
> Issue#4. The 'new_device' echo does not indicate which virtual
> gpiochip<N> was created.
> Issue#5. No way to assign names to GPIO lines exported through an
> aggregator.
>
> Although Issue#1 to #3 could technically be resolved easily without
> configfs, using configfs offers a streamlined, modern, and extensible
> approach, especially since gpio-sim and gpio-virtuser already utilize
> configfs.
>
> This v5 patch series includes 9 patches:
>
> Patch#1: Fix an issue that was spotted during v3 preparation.
> Patch#2: Reorder functions to prepare for configfs introduction.
> Patch#3: Add aggr_alloc() to reduce code duplication.
> Patch#4: Introduce basic configfs interface. Address Issue#1 to #4.
> Patch#5: Address Issue#5.
> Patch#6: Prepare for Patch#7.
> Patch#7: Expose devices created with sysfs to configfs.
> Patch#8: Suppress deferred probe for purely configfs-based aggregators.
> Patch#9: Documentation for the new configfs interface.
>
> N.B. This v5 is based on the latest gpio/for-next commit as of writing this:
> * 45af02f06f69 ("gpio: virtuser: convert to use dev-sync-probe utilities")
>
>
> v4->v5 changes:
> - Rebased off of the latest gpio/for-next, that includes the patch series:
> "Add synchronous fake device creation utility for GPIO drivers"
> (https://lore.kernel.org/all/20250221133501.2203897-1-koichiro.den@canonical.com/)
>
> v3->v4 changes:
> - Splitted off the introduction of gpio-pseudo.[ch] and conversions.
> - Reordered commits to place a fix commit first.
> - Squashed the trivial update for gpio-aggregator's conversion to gpio-pseudo
> into the primary commit "gpio: aggregator: introduce basic configfs interface"
> as it is only meaningful when combined.
>
> v2->v3 changes:
> - Addressed feedback from Bartosz:
> * Factored out the common mechanism for synchronizing platform device
> probe by adding gpio-pseudo.[ch].
> * Renamed "_auto." prefix to "_sysfs." for auto-generated
> configfs entries corresponding to sysfs-created devices.
> * Squashed v2 Patch#3 into its predecessor.
> - Addressed feedback from Geert:
> * Factored out duplicate code in struct gpio_aggregator initialization
> by adding gpio_alloc()/gpio_free() functions. Note that v2 Patch#7
> was dropped for other reasons as mentioned below, so aggr_free() in
> v3 is unrelated to the same-named function in v2.
> * Removed redundant parsing of gpio-line-names and unnecessary
> chip->names assignments; squashed v2 Patch#4 + v2 Patch#5 into v3
> Patch#9.
> * Updated to use sysfs_emit().
> * Updated Kconfig (select CONFIGFS_FS).
> * Fixed typos, coding style issues, missing const qualifiers, and other
> minor issues.
> - Resolved an issue that was spotted during v3 preparation. See Patch#2.
> - Reordered resource initialization order in gpio_aggregator_init() to
> both eliminate a potential race condition (as noted in the source code
> comment) and simplify the code. See Patch#8. This enabled:
> * Removal of v2 Patch#7.
> * Merging of aggr_unregister_lines() and aggr_free_lines() into a
> unified function.
> - Disabled 'delete_device' functionality for devices created via configfs
> for simplicity. It was mistakenly allowed in v2 and proved buggy. See
> Patch #8.
>
> RFC->v2 changes:
> - Addressed feedback from Bartosz:
> * Expose devices created with sysfs to configfs.
> * Drop 'num_lines' attribute.
> * Fix bugs and crashes.
> * Organize internal symbol prefixes more cleanly.
> - Split diffs for improved reviewability.
> - Update kernel doc to reflect the changes.
>
> v4: https://lore.kernel.org/all/20250217143531.541185-1-koichiro.den@canonical.com/
> v3: https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/
> v2: https://lore.kernel.org/all/20250203031213.399914-1-koichiro.den@canonical.com/
> RFC (v1): https://lore.kernel.org/linux-gpio/20250129155525.663780-1-koichiro.den@canonical.com/T/#u
>
>
> *** BLURB HERE ***
>
> Koichiro Den (9):
> gpio: aggregator: protect driver attr handlers against module unload
> gpio: aggregator: reorder functions to prepare for configfs
> introduction
> gpio: aggregator: add aggr_alloc()/aggr_free()
> gpio: aggregator: introduce basic configfs interface
> gpio: aggregator: add 'name' attribute for custom GPIO line names
> gpio: aggregator: rename 'name' to 'key' in aggr_parse()
> gpio: aggregator: expose aggregator created via legacy sysfs to
> configfs
> gpio: aggregator: cancel deferred probe for devices created via
> configfs
> Documentation: gpio: document configfs interface for gpio-aggregator
>
> .../admin-guide/gpio/gpio-aggregator.rst | 107 ++
> drivers/gpio/Kconfig | 2 +
> drivers/gpio/gpio-aggregator.c | 1129 ++++++++++++++---
> 3 files changed, 1050 insertions(+), 188 deletions(-)
>
> --
> 2.45.2
>
I did some more testing as I want to pick it up for v6.15 but I now
noticed that we're hitting the lockdep_assert_held(&aggr->lock) splat
in aggr_line_add(). Could you please look into it?
Bartosz
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-03-10 10:19 ` Bartosz Golaszewski
@ 2025-03-10 13:32 ` Koichiro Den
2025-03-10 14:17 ` Bartosz Golaszewski
0 siblings, 1 reply; 20+ messages in thread
From: Koichiro Den @ 2025-03-10 13:32 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Mar 10, 2025 at 11:19:40AM GMT, Bartosz Golaszewski wrote:
> On Mon, Feb 24, 2025 at 3:31 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
> > This patch series introduces a configfs-based interface to gpio-aggregator
> > to address limitations in the existing 'new_device' interface.
> >
> > The existing 'new_device' interface has several limitations:
> >
> > Issue#1. No way to determine when GPIO aggregator creation is complete.
> > Issue#2. No way to retrieve errors when creating a GPIO aggregator.
> > Issue#3. No way to trace a GPIO line of an aggregator back to its
> > corresponding physical device.
> > Issue#4. The 'new_device' echo does not indicate which virtual
> > gpiochip<N> was created.
> > Issue#5. No way to assign names to GPIO lines exported through an
> > aggregator.
> >
> > Although Issue#1 to #3 could technically be resolved easily without
> > configfs, using configfs offers a streamlined, modern, and extensible
> > approach, especially since gpio-sim and gpio-virtuser already utilize
> > configfs.
> >
> > This v5 patch series includes 9 patches:
> >
> > Patch#1: Fix an issue that was spotted during v3 preparation.
> > Patch#2: Reorder functions to prepare for configfs introduction.
> > Patch#3: Add aggr_alloc() to reduce code duplication.
> > Patch#4: Introduce basic configfs interface. Address Issue#1 to #4.
> > Patch#5: Address Issue#5.
> > Patch#6: Prepare for Patch#7.
> > Patch#7: Expose devices created with sysfs to configfs.
> > Patch#8: Suppress deferred probe for purely configfs-based aggregators.
> > Patch#9: Documentation for the new configfs interface.
> >
> > N.B. This v5 is based on the latest gpio/for-next commit as of writing this:
> > * 45af02f06f69 ("gpio: virtuser: convert to use dev-sync-probe utilities")
> >
> >
> > v4->v5 changes:
> > - Rebased off of the latest gpio/for-next, that includes the patch series:
> > "Add synchronous fake device creation utility for GPIO drivers"
> > (https://lore.kernel.org/all/20250221133501.2203897-1-koichiro.den@canonical.com/)
> >
> > v3->v4 changes:
> > - Splitted off the introduction of gpio-pseudo.[ch] and conversions.
> > - Reordered commits to place a fix commit first.
> > - Squashed the trivial update for gpio-aggregator's conversion to gpio-pseudo
> > into the primary commit "gpio: aggregator: introduce basic configfs interface"
> > as it is only meaningful when combined.
> >
> > v2->v3 changes:
> > - Addressed feedback from Bartosz:
> > * Factored out the common mechanism for synchronizing platform device
> > probe by adding gpio-pseudo.[ch].
> > * Renamed "_auto." prefix to "_sysfs." for auto-generated
> > configfs entries corresponding to sysfs-created devices.
> > * Squashed v2 Patch#3 into its predecessor.
> > - Addressed feedback from Geert:
> > * Factored out duplicate code in struct gpio_aggregator initialization
> > by adding gpio_alloc()/gpio_free() functions. Note that v2 Patch#7
> > was dropped for other reasons as mentioned below, so aggr_free() in
> > v3 is unrelated to the same-named function in v2.
> > * Removed redundant parsing of gpio-line-names and unnecessary
> > chip->names assignments; squashed v2 Patch#4 + v2 Patch#5 into v3
> > Patch#9.
> > * Updated to use sysfs_emit().
> > * Updated Kconfig (select CONFIGFS_FS).
> > * Fixed typos, coding style issues, missing const qualifiers, and other
> > minor issues.
> > - Resolved an issue that was spotted during v3 preparation. See Patch#2.
> > - Reordered resource initialization order in gpio_aggregator_init() to
> > both eliminate a potential race condition (as noted in the source code
> > comment) and simplify the code. See Patch#8. This enabled:
> > * Removal of v2 Patch#7.
> > * Merging of aggr_unregister_lines() and aggr_free_lines() into a
> > unified function.
> > - Disabled 'delete_device' functionality for devices created via configfs
> > for simplicity. It was mistakenly allowed in v2 and proved buggy. See
> > Patch #8.
> >
> > RFC->v2 changes:
> > - Addressed feedback from Bartosz:
> > * Expose devices created with sysfs to configfs.
> > * Drop 'num_lines' attribute.
> > * Fix bugs and crashes.
> > * Organize internal symbol prefixes more cleanly.
> > - Split diffs for improved reviewability.
> > - Update kernel doc to reflect the changes.
> >
> > v4: https://lore.kernel.org/all/20250217143531.541185-1-koichiro.den@canonical.com/
> > v3: https://lore.kernel.org/all/20250216125816.14430-1-koichiro.den@canonical.com/
> > v2: https://lore.kernel.org/all/20250203031213.399914-1-koichiro.den@canonical.com/
> > RFC (v1): https://lore.kernel.org/linux-gpio/20250129155525.663780-1-koichiro.den@canonical.com/T/#u
> >
> >
> > *** BLURB HERE ***
> >
> > Koichiro Den (9):
> > gpio: aggregator: protect driver attr handlers against module unload
> > gpio: aggregator: reorder functions to prepare for configfs
> > introduction
> > gpio: aggregator: add aggr_alloc()/aggr_free()
> > gpio: aggregator: introduce basic configfs interface
> > gpio: aggregator: add 'name' attribute for custom GPIO line names
> > gpio: aggregator: rename 'name' to 'key' in aggr_parse()
> > gpio: aggregator: expose aggregator created via legacy sysfs to
> > configfs
> > gpio: aggregator: cancel deferred probe for devices created via
> > configfs
> > Documentation: gpio: document configfs interface for gpio-aggregator
> >
> > .../admin-guide/gpio/gpio-aggregator.rst | 107 ++
> > drivers/gpio/Kconfig | 2 +
> > drivers/gpio/gpio-aggregator.c | 1129 ++++++++++++++---
> > 3 files changed, 1050 insertions(+), 188 deletions(-)
> >
> > --
> > 2.45.2
> >
>
> I did some more testing as I want to pick it up for v6.15 but I now
> noticed that we're hitting the lockdep_assert_held(&aggr->lock) splat
> in aggr_line_add(). Could you please look into it?
Thanks. Could you share with me a sample splat?
Koichiro
>
> Bartosz
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-03-10 13:32 ` Koichiro Den
@ 2025-03-10 14:17 ` Bartosz Golaszewski
2025-03-10 16:28 ` Koichiro Den
0 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-03-10 14:17 UTC (permalink / raw)
To: Koichiro Den
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel, Bartosz Golaszewski
On Mon, 10 Mar 2025 14:32:12 +0100, Koichiro Den
<koichiro.den@canonical.com> said:
>>
>> I did some more testing as I want to pick it up for v6.15 but I now
>> noticed that we're hitting the lockdep_assert_held(&aggr->lock) splat
>> in aggr_line_add(). Could you please look into it?
>
> Thanks. Could you share with me a sample splat?
>
Just a simple aggregator instantiation using the new_device attribute like:
echo "gpio-sim.0:node0 3 gpio-sim.1:node0 5" > new_device
is giving me this:
[ 11.125700] ------------[ cut here ]------------
[ 11.125754] WARNING: CPU: 11 PID: 349 at
drivers/gpio/gpio-aggregator.c:185 aggr_line_add+0x2a5/0x370
[gpio_aggregator]
[ 11.125799] Modules linked in: gpio_sim gpio_aggregator
dev_sync_probe cfg80211 parport_pc parport nfsd sch_fq_codel fuse
configfs
[ 11.125933] CPU: 11 UID: 0 PID: 349 Comm: sh Not tainted
6.14.0-rc6-yocto-standard+ #80
[ 11.125959] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[ 11.125977] RIP: 0010:aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.126005] Code: c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc
cc 49 8d be 80 01 00 00 be ff ff ff ff e8 43 f8 eb d7 85 c0 0f 85 b3
fd ff ff <0f> 0b e9 ac fd ff ff 48 c7 c7 fc aa f1 99 e8 78 c1 50 d5 e9
8d fd
[ 11.126027] RSP: 0018:ffffc90002bd79e0 EFLAGS: 00010246
[ 11.126057] RAX: 0000000000000000 RBX: ffff888108bb1000 RCX: 0000000000000001
[ 11.126076] RDX: 0000000000000001 RSI: ffff888108bb1180 RDI: ffff88810cbf2400
[ 11.126093] RBP: ffffc90002bd7a18 R08: 0000000000000001 R09: ffffed102208fc00
[ 11.126110] R10: ffff88811047e003 R11: 0000000000000000 R12: 0000000000000003
[ 11.126125] R13: ffff88811047e100 R14: ffff888108bb1000 R15: ffffc90002bd7ac8
[ 11.126143] FS: 00007f9b3b293740(0000) GS:ffff888155180000(0000)
knlGS:0000000000000000
[ 11.126171] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 11.126261] CR2: 0000561597676ec0 CR3: 000000010f110000 CR4: 00000000001506f0
[ 11.126282] Call Trace:
[ 11.126294] <TASK>
[ 11.126308] ? show_regs.cold+0x19/0x1e
[ 11.126356] ? __warn.cold+0x60/0x245
[ 11.126375] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.126402] ? report_bug+0x20b/0x2d0
[ 11.126439] ? handle_bug+0x5b/0x90
[ 11.126462] ? exc_invalid_op+0x1c/0x50
[ 11.126485] ? asm_exc_invalid_op+0x1f/0x30
[ 11.126528] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.126553] ? configfs_register_group+0x239/0x390 [configfs]
[ 11.126606] new_device_store+0x88a/0xae0 [gpio_aggregator]
[ 11.126649] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
[ 11.126694] ? kernfs_fop_write_iter+0x258/0x5a0
[ 11.126742] ? __pfx_lock_acquire+0x10/0x10
[ 11.126774] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
[ 11.126795] ? __pfx_drv_attr_store+0x10/0x10
[ 11.126819] drv_attr_store+0x71/0xb0
[ 11.126838] ? sysfs_file_ops+0x129/0x180
[ 11.126863] sysfs_kf_write+0x10d/0x160
[ 11.126888] ? __pfx_sysfs_kf_write+0x10/0x10
[ 11.126908] kernfs_fop_write_iter+0x398/0x5a0
[ 11.126939] vfs_write+0x612/0x10a0
[ 11.126963] ? vfs_getattr_nosec+0x22a/0x310
[ 11.126989] ? __pfx_vfs_write+0x10/0x10
[ 11.127020] ? __do_sys_newfstat+0xed/0x100
[ 11.127070] ksys_write+0x112/0x200
[ 11.127096] ? __pfx_ksys_write+0x10/0x10
[ 11.127119] ? do_raw_spin_unlock+0x5d/0x220
[ 11.127154] __x64_sys_write+0x76/0xb0
[ 11.127175] ? trace_hardirqs_on+0x26/0x140
[ 11.127232] x64_sys_call+0x293/0x1d70
[ 11.127252] do_syscall_64+0x71/0x140
[ 11.127277] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 11.127294] RIP: 0033:0x7f9b3b392424
[ 11.127312] Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f
84 00 00 00 00 00 f3 0f 1e fa 80 3d 45 7c 0e 00 00 74 13 b8 01 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 48 89 54 24
18 48
[ 11.127327] RSP: 002b:00007fff16e6fb78 EFLAGS: 00000202 ORIG_RAX:
0000000000000001
[ 11.127351] RAX: ffffffffffffffda RBX: 0000000000000026 RCX: 00007f9b3b392424
[ 11.127365] RDX: 0000000000000026 RSI: 00005615976c3590 RDI: 0000000000000001
[ 11.127378] RBP: 00007f9b3b4735c0 R08: 0000000000000000 R09: 0000000000000001
[ 11.127390] R10: 0000000000000004 R11: 0000000000000202 R12: 0000000000000026
[ 11.127403] R13: 00005615976c3590 R14: 00007f9b3b470ec0 R15: 0000000000000000
[ 11.127457] </TASK>
[ 11.127469] irq event stamp: 54943
[ 11.127481] hardirqs last enabled at (54949): [<ffffffff958284fc>]
__up_console_sem+0x5c/0x70
[ 11.127503] hardirqs last disabled at (54954): [<ffffffff958284e1>]
__up_console_sem+0x41/0x70
[ 11.127522] softirqs last enabled at (54242): [<ffffffff956b645a>]
irq_exit_rcu+0x11a/0x1b0
[ 11.127541] softirqs last disabled at (54237): [<ffffffff956b645a>]
irq_exit_rcu+0x11a/0x1b0
[ 11.127576] ---[ end trace 0000000000000000 ]---
[ 11.127886] ------------[ cut here ]------------
[ 11.127902] WARNING: CPU: 11 PID: 349 at
drivers/gpio/gpio-aggregator.c:185 aggr_line_add+0x2a5/0x370
[gpio_aggregator]
[ 11.127932] Modules linked in: gpio_sim gpio_aggregator
dev_sync_probe cfg80211 parport_pc parport nfsd sch_fq_codel fuse
configfs
[ 11.128059] CPU: 11 UID: 0 PID: 349 Comm: sh Tainted: G W
6.14.0-rc6-yocto-standard+ #80
[ 11.128086] Tainted: [W]=WARN
[ 11.128102] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[ 11.128118] RIP: 0010:aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.128143] Code: c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc
cc 49 8d be 80 01 00 00 be ff ff ff ff e8 43 f8 eb d7 85 c0 0f 85 b3
fd ff ff <0f> 0b e9 ac fd ff ff 48 c7 c7 fc aa f1 99 e8 78 c1 50 d5 e9
8d fd
[ 11.128162] RSP: 0018:ffffc90002bd79e0 EFLAGS: 00010246
[ 11.128188] RAX: 0000000000000000 RBX: ffff888108bb1000 RCX: 0000000000000001
[ 11.128254] RDX: 0000000000000001 RSI: ffff888108bb1180 RDI: ffff88810cbf2400
[ 11.128270] RBP: ffffc90002bd7a18 R08: 0000000000000001 R09: ffffed1020a1cfa0
[ 11.128287] R10: ffff8881050e7d03 R11: 0000000000000000 R12: 0000000000000005
[ 11.128302] R13: ffff8881050e7a00 R14: ffff888108bb1000 R15: ffffc90002bd7ac8
[ 11.128319] FS: 00007f9b3b293740(0000) GS:ffff888155180000(0000)
knlGS:0000000000000000
[ 11.128344] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 11.128362] CR2: 0000561597676ec0 CR3: 000000010f110000 CR4: 00000000001506f0
[ 11.128379] Call Trace:
[ 11.128393] <TASK>
[ 11.128410] ? show_regs.cold+0x19/0x1e
[ 11.128435] ? __warn.cold+0x60/0x245
[ 11.128454] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.128482] ? report_bug+0x20b/0x2d0
[ 11.128517] ? handle_bug+0x5b/0x90
[ 11.128538] ? exc_invalid_op+0x1c/0x50
[ 11.128574] ? asm_exc_invalid_op+0x1f/0x30
[ 11.128617] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
[ 11.128643] ? configfs_register_group+0x239/0x390 [configfs]
[ 11.128678] new_device_store+0x88a/0xae0 [gpio_aggregator]
[ 11.128721] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
[ 11.128766] ? kernfs_fop_write_iter+0x258/0x5a0
[ 11.128814] ? __pfx_lock_acquire+0x10/0x10
[ 11.128845] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
[ 11.128865] ? __pfx_drv_attr_store+0x10/0x10
[ 11.128888] drv_attr_store+0x71/0xb0
[ 11.128908] ? sysfs_file_ops+0x129/0x180
[ 11.128933] sysfs_kf_write+0x10d/0x160
[ 11.128957] ? __pfx_sysfs_kf_write+0x10/0x10
[ 11.128977] kernfs_fop_write_iter+0x398/0x5a0
[ 11.129009] vfs_write+0x612/0x10a0
[ 11.129031] ? vfs_getattr_nosec+0x22a/0x310
[ 11.129056] ? __pfx_vfs_write+0x10/0x10
[ 11.129088] ? __do_sys_newfstat+0xed/0x100
[ 11.129138] ksys_write+0x112/0x200
[ 11.129163] ? __pfx_ksys_write+0x10/0x10
[ 11.129187] ? do_raw_spin_unlock+0x5d/0x220
[ 11.129250] __x64_sys_write+0x76/0xb0
[ 11.129271] ? trace_hardirqs_on+0x26/0x140
[ 11.129292] x64_sys_call+0x293/0x1d70
[ 11.129312] do_syscall_64+0x71/0x140
[ 11.129337] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 11.129354] RIP: 0033:0x7f9b3b392424
[ 11.129369] Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f
84 00 00 00 00 00 f3 0f 1e fa 80 3d 45 7c 0e 00 00 74 13 b8 01 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 48 89 54 24
18 48
[ 11.129384] RSP: 002b:00007fff16e6fb78 EFLAGS: 00000202 ORIG_RAX:
0000000000000001
[ 11.129405] RAX: ffffffffffffffda RBX: 0000000000000026 RCX: 00007f9b3b392424
[ 11.129418] RDX: 0000000000000026 RSI: 00005615976c3590 RDI: 0000000000000001
[ 11.129432] RBP: 00007f9b3b4735c0 R08: 0000000000000000 R09: 0000000000000001
[ 11.129444] R10: 0000000000000004 R11: 0000000000000202 R12: 0000000000000026
[ 11.129457] R13: 00005615976c3590 R14: 00007f9b3b470ec0 R15: 0000000000000000
[ 11.129502] </TASK>
[ 11.129513] irq event stamp: 55455
[ 11.129524] hardirqs last enabled at (55461): [<ffffffff958284fc>]
__up_console_sem+0x5c/0x70
[ 11.129544] hardirqs last disabled at (55466): [<ffffffff958284e1>]
__up_console_sem+0x41/0x70
[ 11.129574] softirqs last enabled at (55142): [<ffffffff956b645a>]
irq_exit_rcu+0x11a/0x1b0
[ 11.129592] softirqs last disabled at (55137): [<ffffffff956b645a>]
irq_exit_rcu+0x11a/0x1b0
[ 11.129609] ---[ end trace 0000000000000000 ]---
While at it I now noticed that removing a top-level attribute from
/sys/kernel/config/gpio-aggregator results in the following lockdep AND KASAN
splats:
[ 18.245874] ------------[ cut here ]------------
[ 18.246196] DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock))
[ 18.246205] WARNING: CPU: 11 PID: 394 at
kernel/locking/mutex-debug.c:114 mutex_destroy+0xd1/0x110
[ 18.247143] Modules linked in: gpio_aggregator dev_sync_probe
cfg80211 parport_pc parport nfsd sch_fq_codel fuse configfs
[ 18.247888] CPU: 11 UID: 0 PID: 394 Comm: rmdir Not tainted
6.14.0-rc6-yocto-standard+ #80
[ 18.248413] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[ 18.249124] RIP: 0010:mutex_destroy+0xd1/0x110
[ 18.249414] Code: 03 0f b6 14 11 38 d0 7c 04 84 d2 75 3f 8b 05 9e
b3 71 04 85 c0 75 86 48 c7 c6 c0 f8 e7 b0 48 c7 c7 00 f9 e7 b0 e8 4f
b6 e9 ff <0f> 0b e9 6c ff ff ff 48 c7 c7 80 7d 4d b3 e8 0c 5f 6d 00 e9
51 ff
[ 18.250594] RSP: 0018:ffffc900028b7c60 EFLAGS: 00010286
[ 18.250962] RAX: 0000000000000000 RBX: ffff88810fc71118 RCX: 0000000000000027
[ 18.251415] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88815b1a8a48
[ 18.251920] RBP: ffffc900028b7c68 R08: 0000000000000001 R09: ffffed102b635149
[ 18.252401] R10: ffff88815b1a8a4b R11: 0000000000000001 R12: ffff88810fc71118
[ 18.252887] R13: ffff88810fc71000 R14: ffffffffc0a96660 R15: 0000000000000000
[ 18.253367] FS: 00007faabbf07740(0000) GS:ffff88815b180000(0000)
knlGS:0000000000000000
[ 18.253920] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 18.254283] CR2: 00007faabc0e8000 CR3: 000000010331c000 CR4: 00000000001506f0
[ 18.254765] Call Trace:
[ 18.254942] <TASK>
[ 18.255093] ? show_regs.cold+0x19/0x1e
[ 18.255359] ? __warn.cold+0x60/0x245
[ 18.255610] ? mutex_destroy+0xd1/0x110
[ 18.255882] ? report_bug+0x20b/0x2d0
[ 18.256140] ? handle_bug+0x5b/0x90
[ 18.256382] ? exc_invalid_op+0x1c/0x50
[ 18.256644] ? asm_exc_invalid_op+0x1f/0x30
[ 18.256943] ? mutex_destroy+0xd1/0x110
[ 18.257206] gpio_aggr_device_release+0x86/0xb0 [gpio_aggregator]
[ 18.257617] config_item_cleanup+0x122/0x220 [configfs]
[ 18.257981] config_item_put+0x52/0x6a [configfs]
[ 18.258302] configfs_rmdir+0x6f9/0xe20 [configfs]
[ 18.258629] ? __pfx_configfs_rmdir+0x10/0x10 [configfs]
[ 18.258999] ? __kasan_check_read+0x15/0x20
[ 18.259284] ? do_raw_spin_unlock+0x5d/0x220
[ 18.259577] vfs_rmdir+0x1b0/0x5d0
[ 18.259821] ? hook_path_rmdir+0x17/0x20
[ 18.260092] do_rmdir+0x31c/0x380
[ 18.260322] ? __pfx_do_rmdir+0x10/0x10
[ 18.260585] ? __kasan_check_write+0x18/0x20
[ 18.260885] ? __kasan_check_write+0x18/0x20
[ 18.261179] ? getname_flags+0xb5/0x400
[ 18.261442] __x64_sys_rmdir+0x44/0x60
[ 18.261698] x64_sys_call+0x114f/0x1d70
[ 18.261973] do_syscall_64+0x71/0x140
[ 18.262225] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 18.262565] RIP: 0033:0x7faabc005d7b
[ 18.262821] Code: f0 ff ff 73 01 c3 48 8b 0d a2 00 0e 00 f7 d8 64
89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 54 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 71 00 0e 00
f7 d8
[ 18.264054] RSP: 002b:00007ffea3b3d9b8 EFLAGS: 00000246 ORIG_RAX:
0000000000000054
[ 18.264559] RAX: ffffffffffffffda RBX: 00007ffea3b3dba8 RCX: 00007faabc005d7b
[ 18.265047] RDX: 0000000000000000 RSI: 00007ffea3b3dee2 RDI: 00007ffea3b3dee2
[ 18.265520] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
[ 18.266004] R10: fffffffffffff199 R11: 0000000000000246 R12: 00007ffea3b3dee2
[ 18.266478] R13: 0000000000000001 R14: 00007faabc135000 R15: 0000559416916a78
[ 18.266965] </TASK>
[ 18.267122] irq event stamp: 3193
[ 18.267351] hardirqs last enabled at (3193): [<ffffffffadca3bf7>]
__call_rcu_common.constprop.0+0x597/0xdf0
[ 18.268009] hardirqs last disabled at (3192): [<ffffffffadca3c1a>]
__call_rcu_common.constprop.0+0x5ba/0xdf0
[ 18.268656] softirqs last enabled at (2320): [<ffffffffb0095be7>]
release_sock+0x147/0x190
[ 18.269216] softirqs last disabled at (2318): [<ffffffffb0095ac4>]
release_sock+0x24/0x190
[ 18.269760] ---[ end trace 0000000000000000 ]---
[ 18.270091] ==================================================================
[ 18.270573] BUG: KASAN: slab-use-after-free in
__mutex_unlock_slowpath+0xb1/0x6d0
[ 18.271078] Read of size 8 at addr ffff88810fc71118 by task rmdir/394
[ 18.271508]
[ 18.271623] CPU: 11 UID: 0 PID: 394 Comm: rmdir Tainted: G W
6.14.0-rc6-yocto-standard+ #80
[ 18.271627] Tainted: [W]=WARN
[ 18.271629] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
[ 18.271630] Call Trace:
[ 18.271632] <TASK>
[ 18.271633] dump_stack_lvl+0x63/0x90
[ 18.271637] print_report+0x159/0x53a
[ 18.271640] ? __virt_addr_valid+0x201/0x420
[ 18.271644] ? kasan_complete_mode_report_info+0x64/0x200
[ 18.271648] kasan_report+0xec/0x130
[ 18.271652] ? __mutex_unlock_slowpath+0xb1/0x6d0
[ 18.271656] ? __mutex_unlock_slowpath+0xb1/0x6d0
[ 18.271661] kasan_check_range+0x122/0x1f0
[ 18.271664] __kasan_check_read+0x15/0x20
[ 18.271667] __mutex_unlock_slowpath+0xb1/0x6d0
[ 18.271670] ? __kasan_slab_free+0x61/0x70
[ 18.271674] ? __kasan_slab_free+0x61/0x70
[ 18.271677] ? trace_hardirqs_on+0x26/0x140
[ 18.271681] ? __pfx___mutex_unlock_slowpath+0x10/0x10
[ 18.271685] ? __kasan_slab_free+0x61/0x70
[ 18.271689] ? kfree+0x154/0x420
[ 18.271693] mutex_unlock+0x16/0x20
[ 18.271696] gpio_aggr_device_release+0x9b/0xb0 [gpio_aggregator]
[ 18.271701] config_item_cleanup+0x122/0x220 [configfs]
[ 18.271707] config_item_put+0x52/0x6a [configfs]
[ 18.271713] configfs_rmdir+0x6f9/0xe20 [configfs]
[ 18.271721] ? __pfx_configfs_rmdir+0x10/0x10 [configfs]
[ 18.271727] ? __kasan_check_read+0x15/0x20
[ 18.271730] ? do_raw_spin_unlock+0x5d/0x220
[ 18.271735] vfs_rmdir+0x1b0/0x5d0
[ 18.271738] ? hook_path_rmdir+0x17/0x20
[ 18.271742] do_rmdir+0x31c/0x380
[ 18.271746] ? __pfx_do_rmdir+0x10/0x10
[ 18.271750] ? __kasan_check_write+0x18/0x20
[ 18.271754] ? __kasan_check_write+0x18/0x20
[ 18.271757] ? getname_flags+0xb5/0x400
[ 18.271761] __x64_sys_rmdir+0x44/0x60
[ 18.271765] x64_sys_call+0x114f/0x1d70
[ 18.271768] do_syscall_64+0x71/0x140
[ 18.271772] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 18.271775] RIP: 0033:0x7faabc005d7b
[ 18.271777] Code: f0 ff ff 73 01 c3 48 8b 0d a2 00 0e 00 f7 d8 64
89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 54 00 00
00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 71 00 0e 00
f7 d8
[ 18.271779] RSP: 002b:00007ffea3b3d9b8 EFLAGS: 00000246 ORIG_RAX:
0000000000000054
[ 18.271782] RAX: ffffffffffffffda RBX: 00007ffea3b3dba8 RCX: 00007faabc005d7b
[ 18.271784] RDX: 0000000000000000 RSI: 00007ffea3b3dee2 RDI: 00007ffea3b3dee2
[ 18.271786] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
[ 18.271787] R10: fffffffffffff199 R11: 0000000000000246 R12: 00007ffea3b3dee2
[ 18.271789] R13: 0000000000000001 R14: 00007faabc135000 R15: 0000559416916a78
[ 18.271795] </TASK>
[ 18.271796]
[ 18.288331] Allocated by task 392:
[ 18.288566] kasan_save_stack+0x3d/0x60
[ 18.288830] kasan_save_track+0x18/0x40
[ 18.289118] kasan_save_alloc_info+0x3b/0x50
[ 18.289410] __kasan_kmalloc+0xb7/0xc0
[ 18.289667] __kmalloc_noprof+0x1c8/0x4e0
[ 18.289945] aggr_alloc+0x22/0x190 [gpio_aggregator]
[ 18.290281] gpio_aggr_make_group+0xc1/0x100 [gpio_aggregator]
[ 18.290673] configfs_mkdir+0x3f9/0xd40 [configfs]
[ 18.291003] vfs_mkdir+0x3ca/0x610
[ 18.291239] do_mkdirat+0x27b/0x320
[ 18.291481] __x64_sys_mkdir+0x69/0x90
[ 18.291739] x64_sys_call+0x15d6/0x1d70
[ 18.292004] do_syscall_64+0x71/0x140
[ 18.292259] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 18.292602]
[ 18.292717] Freed by task 394:
[ 18.292933] kasan_save_stack+0x3d/0x60
[ 18.293198] kasan_save_track+0x18/0x40
[ 18.293462] kasan_save_free_info+0x3f/0x70
[ 18.293748] __kasan_slab_free+0x56/0x70
[ 18.294022] kfree+0x154/0x420
[ 18.294237] gpio_aggr_device_release+0x8e/0xb0 [gpio_aggregator]
[ 18.294649] config_item_cleanup+0x122/0x220 [configfs]
[ 18.295008] config_item_put+0x52/0x6a [configfs]
[ 18.295331] configfs_rmdir+0x6f9/0xe20 [configfs]
[ 18.295659] vfs_rmdir+0x1b0/0x5d0
[ 18.295896] do_rmdir+0x31c/0x380
[ 18.296131] __x64_sys_rmdir+0x44/0x60
[ 18.296390] x64_sys_call+0x114f/0x1d70
[ 18.296654] do_syscall_64+0x71/0x140
[ 18.296909] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 18.297251]
[ 18.297365] The buggy address belongs to the object at ffff88810fc71000
[ 18.297365] which belongs to the cache kmalloc-512 of size 512
[ 18.298186] The buggy address is located 280 bytes inside of
[ 18.298186] freed 512-byte region [ffff88810fc71000, ffff88810fc71200)
[ 18.298992]
[ 18.299106] The buggy address belongs to the physical page:
[ 18.299481] page: refcount:0 mapcount:0 mapping:0000000000000000
index:0x0 pfn:0x10fc70
[ 18.300018] head: order:3 mapcount:0 entire_mapcount:0
nr_pages_mapped:0 pincount:0
[ 18.300526] flags: 0x8000000000000040(head|zone=2)
[ 18.300852] page_type: f5(slab)
[ 18.301076] raw: 8000000000000040 ffff888100042c80 dead000000000100
dead000000000122
[ 18.301591] raw: 0000000000000000 0000000080200020 00000000f5000000
0000000000000000
[ 18.302109] head: 8000000000000040 ffff888100042c80
dead000000000100 dead000000000122
[ 18.302629] head: 0000000000000000 0000000080200020
00000000f5000000 0000000000000000
[ 18.303152] head: 8000000000000003 ffffea00043f1c01
ffffffffffffffff 0000000000000000
[ 18.303671] head: 0000000000000008 0000000000000000
00000000ffffffff 0000000000000000
[ 18.304194] page dumped because: kasan: bad access detected
[ 18.304568]
[ 18.304682] Memory state around the buggy address:
[ 18.305008] ffff88810fc71000: fa fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[ 18.305491] ffff88810fc71080: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[ 18.305975] >ffff88810fc71100: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[ 18.306456] ^
[ 18.306731] ffff88810fc71180: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[ 18.307215] ffff88810fc71200: fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc fc
[ 18.307696] ==================================================================
Bart
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-03-10 14:17 ` Bartosz Golaszewski
@ 2025-03-10 16:28 ` Koichiro Den
2025-03-10 17:46 ` Bartosz Golaszewski
0 siblings, 1 reply; 20+ messages in thread
From: Koichiro Den @ 2025-03-10 16:28 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Mar 10, 2025 at 10:17:44AM GMT, Bartosz Golaszewski wrote:
> On Mon, 10 Mar 2025 14:32:12 +0100, Koichiro Den
> <koichiro.den@canonical.com> said:
> >>
> >> I did some more testing as I want to pick it up for v6.15 but I now
> >> noticed that we're hitting the lockdep_assert_held(&aggr->lock) splat
> >> in aggr_line_add(). Could you please look into it?
> >
> > Thanks. Could you share with me a sample splat?
> >
>
> Just a simple aggregator instantiation using the new_device attribute like:
>
> echo "gpio-sim.0:node0 3 gpio-sim.1:node0 5" > new_device
>
> is giving me this:
>
> [ 11.125700] ------------[ cut here ]------------
> [ 11.125754] WARNING: CPU: 11 PID: 349 at
> drivers/gpio/gpio-aggregator.c:185 aggr_line_add+0x2a5/0x370
> [gpio_aggregator]
> [ 11.125799] Modules linked in: gpio_sim gpio_aggregator
> dev_sync_probe cfg80211 parport_pc parport nfsd sch_fq_codel fuse
> configfs
> [ 11.125933] CPU: 11 UID: 0 PID: 349 Comm: sh Not tainted
> 6.14.0-rc6-yocto-standard+ #80
> [ 11.125959] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [ 11.125977] RIP: 0010:aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.126005] Code: c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc
> cc 49 8d be 80 01 00 00 be ff ff ff ff e8 43 f8 eb d7 85 c0 0f 85 b3
> fd ff ff <0f> 0b e9 ac fd ff ff 48 c7 c7 fc aa f1 99 e8 78 c1 50 d5 e9
> 8d fd
> [ 11.126027] RSP: 0018:ffffc90002bd79e0 EFLAGS: 00010246
> [ 11.126057] RAX: 0000000000000000 RBX: ffff888108bb1000 RCX: 0000000000000001
> [ 11.126076] RDX: 0000000000000001 RSI: ffff888108bb1180 RDI: ffff88810cbf2400
> [ 11.126093] RBP: ffffc90002bd7a18 R08: 0000000000000001 R09: ffffed102208fc00
> [ 11.126110] R10: ffff88811047e003 R11: 0000000000000000 R12: 0000000000000003
> [ 11.126125] R13: ffff88811047e100 R14: ffff888108bb1000 R15: ffffc90002bd7ac8
> [ 11.126143] FS: 00007f9b3b293740(0000) GS:ffff888155180000(0000)
> knlGS:0000000000000000
> [ 11.126171] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 11.126261] CR2: 0000561597676ec0 CR3: 000000010f110000 CR4: 00000000001506f0
> [ 11.126282] Call Trace:
> [ 11.126294] <TASK>
> [ 11.126308] ? show_regs.cold+0x19/0x1e
> [ 11.126356] ? __warn.cold+0x60/0x245
> [ 11.126375] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.126402] ? report_bug+0x20b/0x2d0
> [ 11.126439] ? handle_bug+0x5b/0x90
> [ 11.126462] ? exc_invalid_op+0x1c/0x50
> [ 11.126485] ? asm_exc_invalid_op+0x1f/0x30
> [ 11.126528] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.126553] ? configfs_register_group+0x239/0x390 [configfs]
> [ 11.126606] new_device_store+0x88a/0xae0 [gpio_aggregator]
> [ 11.126649] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
> [ 11.126694] ? kernfs_fop_write_iter+0x258/0x5a0
> [ 11.126742] ? __pfx_lock_acquire+0x10/0x10
> [ 11.126774] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
> [ 11.126795] ? __pfx_drv_attr_store+0x10/0x10
> [ 11.126819] drv_attr_store+0x71/0xb0
> [ 11.126838] ? sysfs_file_ops+0x129/0x180
> [ 11.126863] sysfs_kf_write+0x10d/0x160
> [ 11.126888] ? __pfx_sysfs_kf_write+0x10/0x10
> [ 11.126908] kernfs_fop_write_iter+0x398/0x5a0
> [ 11.126939] vfs_write+0x612/0x10a0
> [ 11.126963] ? vfs_getattr_nosec+0x22a/0x310
> [ 11.126989] ? __pfx_vfs_write+0x10/0x10
> [ 11.127020] ? __do_sys_newfstat+0xed/0x100
> [ 11.127070] ksys_write+0x112/0x200
> [ 11.127096] ? __pfx_ksys_write+0x10/0x10
> [ 11.127119] ? do_raw_spin_unlock+0x5d/0x220
> [ 11.127154] __x64_sys_write+0x76/0xb0
> [ 11.127175] ? trace_hardirqs_on+0x26/0x140
> [ 11.127232] x64_sys_call+0x293/0x1d70
> [ 11.127252] do_syscall_64+0x71/0x140
> [ 11.127277] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 11.127294] RIP: 0033:0x7f9b3b392424
> [ 11.127312] Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f
> 84 00 00 00 00 00 f3 0f 1e fa 80 3d 45 7c 0e 00 00 74 13 b8 01 00 00
> 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 48 89 54 24
> 18 48
> [ 11.127327] RSP: 002b:00007fff16e6fb78 EFLAGS: 00000202 ORIG_RAX:
> 0000000000000001
> [ 11.127351] RAX: ffffffffffffffda RBX: 0000000000000026 RCX: 00007f9b3b392424
> [ 11.127365] RDX: 0000000000000026 RSI: 00005615976c3590 RDI: 0000000000000001
> [ 11.127378] RBP: 00007f9b3b4735c0 R08: 0000000000000000 R09: 0000000000000001
> [ 11.127390] R10: 0000000000000004 R11: 0000000000000202 R12: 0000000000000026
> [ 11.127403] R13: 00005615976c3590 R14: 00007f9b3b470ec0 R15: 0000000000000000
> [ 11.127457] </TASK>
> [ 11.127469] irq event stamp: 54943
> [ 11.127481] hardirqs last enabled at (54949): [<ffffffff958284fc>]
> __up_console_sem+0x5c/0x70
> [ 11.127503] hardirqs last disabled at (54954): [<ffffffff958284e1>]
> __up_console_sem+0x41/0x70
> [ 11.127522] softirqs last enabled at (54242): [<ffffffff956b645a>]
> irq_exit_rcu+0x11a/0x1b0
> [ 11.127541] softirqs last disabled at (54237): [<ffffffff956b645a>]
> irq_exit_rcu+0x11a/0x1b0
> [ 11.127576] ---[ end trace 0000000000000000 ]---
> [ 11.127886] ------------[ cut here ]------------
> [ 11.127902] WARNING: CPU: 11 PID: 349 at
> drivers/gpio/gpio-aggregator.c:185 aggr_line_add+0x2a5/0x370
> [gpio_aggregator]
> [ 11.127932] Modules linked in: gpio_sim gpio_aggregator
> dev_sync_probe cfg80211 parport_pc parport nfsd sch_fq_codel fuse
> configfs
> [ 11.128059] CPU: 11 UID: 0 PID: 349 Comm: sh Tainted: G W
> 6.14.0-rc6-yocto-standard+ #80
> [ 11.128086] Tainted: [W]=WARN
> [ 11.128102] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [ 11.128118] RIP: 0010:aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.128143] Code: c4 10 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc
> cc 49 8d be 80 01 00 00 be ff ff ff ff e8 43 f8 eb d7 85 c0 0f 85 b3
> fd ff ff <0f> 0b e9 ac fd ff ff 48 c7 c7 fc aa f1 99 e8 78 c1 50 d5 e9
> 8d fd
> [ 11.128162] RSP: 0018:ffffc90002bd79e0 EFLAGS: 00010246
> [ 11.128188] RAX: 0000000000000000 RBX: ffff888108bb1000 RCX: 0000000000000001
> [ 11.128254] RDX: 0000000000000001 RSI: ffff888108bb1180 RDI: ffff88810cbf2400
> [ 11.128270] RBP: ffffc90002bd7a18 R08: 0000000000000001 R09: ffffed1020a1cfa0
> [ 11.128287] R10: ffff8881050e7d03 R11: 0000000000000000 R12: 0000000000000005
> [ 11.128302] R13: ffff8881050e7a00 R14: ffff888108bb1000 R15: ffffc90002bd7ac8
> [ 11.128319] FS: 00007f9b3b293740(0000) GS:ffff888155180000(0000)
> knlGS:0000000000000000
> [ 11.128344] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 11.128362] CR2: 0000561597676ec0 CR3: 000000010f110000 CR4: 00000000001506f0
> [ 11.128379] Call Trace:
> [ 11.128393] <TASK>
> [ 11.128410] ? show_regs.cold+0x19/0x1e
> [ 11.128435] ? __warn.cold+0x60/0x245
> [ 11.128454] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.128482] ? report_bug+0x20b/0x2d0
> [ 11.128517] ? handle_bug+0x5b/0x90
> [ 11.128538] ? exc_invalid_op+0x1c/0x50
> [ 11.128574] ? asm_exc_invalid_op+0x1f/0x30
> [ 11.128617] ? aggr_line_add+0x2a5/0x370 [gpio_aggregator]
> [ 11.128643] ? configfs_register_group+0x239/0x390 [configfs]
> [ 11.128678] new_device_store+0x88a/0xae0 [gpio_aggregator]
> [ 11.128721] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
> [ 11.128766] ? kernfs_fop_write_iter+0x258/0x5a0
> [ 11.128814] ? __pfx_lock_acquire+0x10/0x10
> [ 11.128845] ? __pfx_new_device_store+0x10/0x10 [gpio_aggregator]
> [ 11.128865] ? __pfx_drv_attr_store+0x10/0x10
> [ 11.128888] drv_attr_store+0x71/0xb0
> [ 11.128908] ? sysfs_file_ops+0x129/0x180
> [ 11.128933] sysfs_kf_write+0x10d/0x160
> [ 11.128957] ? __pfx_sysfs_kf_write+0x10/0x10
> [ 11.128977] kernfs_fop_write_iter+0x398/0x5a0
> [ 11.129009] vfs_write+0x612/0x10a0
> [ 11.129031] ? vfs_getattr_nosec+0x22a/0x310
> [ 11.129056] ? __pfx_vfs_write+0x10/0x10
> [ 11.129088] ? __do_sys_newfstat+0xed/0x100
> [ 11.129138] ksys_write+0x112/0x200
> [ 11.129163] ? __pfx_ksys_write+0x10/0x10
> [ 11.129187] ? do_raw_spin_unlock+0x5d/0x220
> [ 11.129250] __x64_sys_write+0x76/0xb0
> [ 11.129271] ? trace_hardirqs_on+0x26/0x140
> [ 11.129292] x64_sys_call+0x293/0x1d70
> [ 11.129312] do_syscall_64+0x71/0x140
> [ 11.129337] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 11.129354] RIP: 0033:0x7f9b3b392424
> [ 11.129369] Code: c7 00 16 00 00 00 b8 ff ff ff ff c3 66 2e 0f 1f
> 84 00 00 00 00 00 f3 0f 1e fa 80 3d 45 7c 0e 00 00 74 13 b8 01 00 00
> 00 0f 05 <48> 3d 00 f0 ff ff 77 54 c3 0f 1f 00 48 83 ec 28 48 89 54 24
> 18 48
> [ 11.129384] RSP: 002b:00007fff16e6fb78 EFLAGS: 00000202 ORIG_RAX:
> 0000000000000001
> [ 11.129405] RAX: ffffffffffffffda RBX: 0000000000000026 RCX: 00007f9b3b392424
> [ 11.129418] RDX: 0000000000000026 RSI: 00005615976c3590 RDI: 0000000000000001
> [ 11.129432] RBP: 00007f9b3b4735c0 R08: 0000000000000000 R09: 0000000000000001
> [ 11.129444] R10: 0000000000000004 R11: 0000000000000202 R12: 0000000000000026
> [ 11.129457] R13: 00005615976c3590 R14: 00007f9b3b470ec0 R15: 0000000000000000
> [ 11.129502] </TASK>
> [ 11.129513] irq event stamp: 55455
> [ 11.129524] hardirqs last enabled at (55461): [<ffffffff958284fc>]
> __up_console_sem+0x5c/0x70
> [ 11.129544] hardirqs last disabled at (55466): [<ffffffff958284e1>]
> __up_console_sem+0x41/0x70
> [ 11.129574] softirqs last enabled at (55142): [<ffffffff956b645a>]
> irq_exit_rcu+0x11a/0x1b0
> [ 11.129592] softirqs last disabled at (55137): [<ffffffff956b645a>]
> irq_exit_rcu+0x11a/0x1b0
> [ 11.129609] ---[ end trace 0000000000000000 ]---
>
> While at it I now noticed that removing a top-level attribute from
> /sys/kernel/config/gpio-aggregator results in the following lockdep AND KASAN
> splats:
>
> [ 18.245874] ------------[ cut here ]------------
> [ 18.246196] DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock))
> [ 18.246205] WARNING: CPU: 11 PID: 394 at
> kernel/locking/mutex-debug.c:114 mutex_destroy+0xd1/0x110
> [ 18.247143] Modules linked in: gpio_aggregator dev_sync_probe
> cfg80211 parport_pc parport nfsd sch_fq_codel fuse configfs
> [ 18.247888] CPU: 11 UID: 0 PID: 394 Comm: rmdir Not tainted
> 6.14.0-rc6-yocto-standard+ #80
> [ 18.248413] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [ 18.249124] RIP: 0010:mutex_destroy+0xd1/0x110
> [ 18.249414] Code: 03 0f b6 14 11 38 d0 7c 04 84 d2 75 3f 8b 05 9e
> b3 71 04 85 c0 75 86 48 c7 c6 c0 f8 e7 b0 48 c7 c7 00 f9 e7 b0 e8 4f
> b6 e9 ff <0f> 0b e9 6c ff ff ff 48 c7 c7 80 7d 4d b3 e8 0c 5f 6d 00 e9
> 51 ff
> [ 18.250594] RSP: 0018:ffffc900028b7c60 EFLAGS: 00010286
> [ 18.250962] RAX: 0000000000000000 RBX: ffff88810fc71118 RCX: 0000000000000027
> [ 18.251415] RDX: 0000000000000000 RSI: 0000000000000004 RDI: ffff88815b1a8a48
> [ 18.251920] RBP: ffffc900028b7c68 R08: 0000000000000001 R09: ffffed102b635149
> [ 18.252401] R10: ffff88815b1a8a4b R11: 0000000000000001 R12: ffff88810fc71118
> [ 18.252887] R13: ffff88810fc71000 R14: ffffffffc0a96660 R15: 0000000000000000
> [ 18.253367] FS: 00007faabbf07740(0000) GS:ffff88815b180000(0000)
> knlGS:0000000000000000
> [ 18.253920] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 18.254283] CR2: 00007faabc0e8000 CR3: 000000010331c000 CR4: 00000000001506f0
> [ 18.254765] Call Trace:
> [ 18.254942] <TASK>
> [ 18.255093] ? show_regs.cold+0x19/0x1e
> [ 18.255359] ? __warn.cold+0x60/0x245
> [ 18.255610] ? mutex_destroy+0xd1/0x110
> [ 18.255882] ? report_bug+0x20b/0x2d0
> [ 18.256140] ? handle_bug+0x5b/0x90
> [ 18.256382] ? exc_invalid_op+0x1c/0x50
> [ 18.256644] ? asm_exc_invalid_op+0x1f/0x30
> [ 18.256943] ? mutex_destroy+0xd1/0x110
> [ 18.257206] gpio_aggr_device_release+0x86/0xb0 [gpio_aggregator]
> [ 18.257617] config_item_cleanup+0x122/0x220 [configfs]
> [ 18.257981] config_item_put+0x52/0x6a [configfs]
> [ 18.258302] configfs_rmdir+0x6f9/0xe20 [configfs]
> [ 18.258629] ? __pfx_configfs_rmdir+0x10/0x10 [configfs]
> [ 18.258999] ? __kasan_check_read+0x15/0x20
> [ 18.259284] ? do_raw_spin_unlock+0x5d/0x220
> [ 18.259577] vfs_rmdir+0x1b0/0x5d0
> [ 18.259821] ? hook_path_rmdir+0x17/0x20
> [ 18.260092] do_rmdir+0x31c/0x380
> [ 18.260322] ? __pfx_do_rmdir+0x10/0x10
> [ 18.260585] ? __kasan_check_write+0x18/0x20
> [ 18.260885] ? __kasan_check_write+0x18/0x20
> [ 18.261179] ? getname_flags+0xb5/0x400
> [ 18.261442] __x64_sys_rmdir+0x44/0x60
> [ 18.261698] x64_sys_call+0x114f/0x1d70
> [ 18.261973] do_syscall_64+0x71/0x140
> [ 18.262225] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 18.262565] RIP: 0033:0x7faabc005d7b
> [ 18.262821] Code: f0 ff ff 73 01 c3 48 8b 0d a2 00 0e 00 f7 d8 64
> 89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 54 00 00
> 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 71 00 0e 00
> f7 d8
> [ 18.264054] RSP: 002b:00007ffea3b3d9b8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000054
> [ 18.264559] RAX: ffffffffffffffda RBX: 00007ffea3b3dba8 RCX: 00007faabc005d7b
> [ 18.265047] RDX: 0000000000000000 RSI: 00007ffea3b3dee2 RDI: 00007ffea3b3dee2
> [ 18.265520] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
> [ 18.266004] R10: fffffffffffff199 R11: 0000000000000246 R12: 00007ffea3b3dee2
> [ 18.266478] R13: 0000000000000001 R14: 00007faabc135000 R15: 0000559416916a78
> [ 18.266965] </TASK>
> [ 18.267122] irq event stamp: 3193
> [ 18.267351] hardirqs last enabled at (3193): [<ffffffffadca3bf7>]
> __call_rcu_common.constprop.0+0x597/0xdf0
> [ 18.268009] hardirqs last disabled at (3192): [<ffffffffadca3c1a>]
> __call_rcu_common.constprop.0+0x5ba/0xdf0
> [ 18.268656] softirqs last enabled at (2320): [<ffffffffb0095be7>]
> release_sock+0x147/0x190
> [ 18.269216] softirqs last disabled at (2318): [<ffffffffb0095ac4>]
> release_sock+0x24/0x190
> [ 18.269760] ---[ end trace 0000000000000000 ]---
> [ 18.270091] ==================================================================
> [ 18.270573] BUG: KASAN: slab-use-after-free in
> __mutex_unlock_slowpath+0xb1/0x6d0
> [ 18.271078] Read of size 8 at addr ffff88810fc71118 by task rmdir/394
> [ 18.271508]
> [ 18.271623] CPU: 11 UID: 0 PID: 394 Comm: rmdir Tainted: G W
> 6.14.0-rc6-yocto-standard+ #80
> [ 18.271627] Tainted: [W]=WARN
> [ 18.271629] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009),
> BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
> [ 18.271630] Call Trace:
> [ 18.271632] <TASK>
> [ 18.271633] dump_stack_lvl+0x63/0x90
> [ 18.271637] print_report+0x159/0x53a
> [ 18.271640] ? __virt_addr_valid+0x201/0x420
> [ 18.271644] ? kasan_complete_mode_report_info+0x64/0x200
> [ 18.271648] kasan_report+0xec/0x130
> [ 18.271652] ? __mutex_unlock_slowpath+0xb1/0x6d0
> [ 18.271656] ? __mutex_unlock_slowpath+0xb1/0x6d0
> [ 18.271661] kasan_check_range+0x122/0x1f0
> [ 18.271664] __kasan_check_read+0x15/0x20
> [ 18.271667] __mutex_unlock_slowpath+0xb1/0x6d0
> [ 18.271670] ? __kasan_slab_free+0x61/0x70
> [ 18.271674] ? __kasan_slab_free+0x61/0x70
> [ 18.271677] ? trace_hardirqs_on+0x26/0x140
> [ 18.271681] ? __pfx___mutex_unlock_slowpath+0x10/0x10
> [ 18.271685] ? __kasan_slab_free+0x61/0x70
> [ 18.271689] ? kfree+0x154/0x420
> [ 18.271693] mutex_unlock+0x16/0x20
> [ 18.271696] gpio_aggr_device_release+0x9b/0xb0 [gpio_aggregator]
> [ 18.271701] config_item_cleanup+0x122/0x220 [configfs]
> [ 18.271707] config_item_put+0x52/0x6a [configfs]
> [ 18.271713] configfs_rmdir+0x6f9/0xe20 [configfs]
> [ 18.271721] ? __pfx_configfs_rmdir+0x10/0x10 [configfs]
> [ 18.271727] ? __kasan_check_read+0x15/0x20
> [ 18.271730] ? do_raw_spin_unlock+0x5d/0x220
> [ 18.271735] vfs_rmdir+0x1b0/0x5d0
> [ 18.271738] ? hook_path_rmdir+0x17/0x20
> [ 18.271742] do_rmdir+0x31c/0x380
> [ 18.271746] ? __pfx_do_rmdir+0x10/0x10
> [ 18.271750] ? __kasan_check_write+0x18/0x20
> [ 18.271754] ? __kasan_check_write+0x18/0x20
> [ 18.271757] ? getname_flags+0xb5/0x400
> [ 18.271761] __x64_sys_rmdir+0x44/0x60
> [ 18.271765] x64_sys_call+0x114f/0x1d70
> [ 18.271768] do_syscall_64+0x71/0x140
> [ 18.271772] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 18.271775] RIP: 0033:0x7faabc005d7b
> [ 18.271777] Code: f0 ff ff 73 01 c3 48 8b 0d a2 00 0e 00 f7 d8 64
> 89 01 48 83 c8 ff c3 0f 1f 84 00 00 00 00 00 f3 0f 1e fa b8 54 00 00
> 00 0f 05 <48> 3d 00 f0 ff ff 77 05 c3 0f 1f 40 00 48 8b 15 71 00 0e 00
> f7 d8
> [ 18.271779] RSP: 002b:00007ffea3b3d9b8 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000054
> [ 18.271782] RAX: ffffffffffffffda RBX: 00007ffea3b3dba8 RCX: 00007faabc005d7b
> [ 18.271784] RDX: 0000000000000000 RSI: 00007ffea3b3dee2 RDI: 00007ffea3b3dee2
> [ 18.271786] RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
> [ 18.271787] R10: fffffffffffff199 R11: 0000000000000246 R12: 00007ffea3b3dee2
> [ 18.271789] R13: 0000000000000001 R14: 00007faabc135000 R15: 0000559416916a78
> [ 18.271795] </TASK>
> [ 18.271796]
> [ 18.288331] Allocated by task 392:
> [ 18.288566] kasan_save_stack+0x3d/0x60
> [ 18.288830] kasan_save_track+0x18/0x40
> [ 18.289118] kasan_save_alloc_info+0x3b/0x50
> [ 18.289410] __kasan_kmalloc+0xb7/0xc0
> [ 18.289667] __kmalloc_noprof+0x1c8/0x4e0
> [ 18.289945] aggr_alloc+0x22/0x190 [gpio_aggregator]
> [ 18.290281] gpio_aggr_make_group+0xc1/0x100 [gpio_aggregator]
> [ 18.290673] configfs_mkdir+0x3f9/0xd40 [configfs]
> [ 18.291003] vfs_mkdir+0x3ca/0x610
> [ 18.291239] do_mkdirat+0x27b/0x320
> [ 18.291481] __x64_sys_mkdir+0x69/0x90
> [ 18.291739] x64_sys_call+0x15d6/0x1d70
> [ 18.292004] do_syscall_64+0x71/0x140
> [ 18.292259] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 18.292602]
> [ 18.292717] Freed by task 394:
> [ 18.292933] kasan_save_stack+0x3d/0x60
> [ 18.293198] kasan_save_track+0x18/0x40
> [ 18.293462] kasan_save_free_info+0x3f/0x70
> [ 18.293748] __kasan_slab_free+0x56/0x70
> [ 18.294022] kfree+0x154/0x420
> [ 18.294237] gpio_aggr_device_release+0x8e/0xb0 [gpio_aggregator]
> [ 18.294649] config_item_cleanup+0x122/0x220 [configfs]
> [ 18.295008] config_item_put+0x52/0x6a [configfs]
> [ 18.295331] configfs_rmdir+0x6f9/0xe20 [configfs]
> [ 18.295659] vfs_rmdir+0x1b0/0x5d0
> [ 18.295896] do_rmdir+0x31c/0x380
> [ 18.296131] __x64_sys_rmdir+0x44/0x60
> [ 18.296390] x64_sys_call+0x114f/0x1d70
> [ 18.296654] do_syscall_64+0x71/0x140
> [ 18.296909] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [ 18.297251]
> [ 18.297365] The buggy address belongs to the object at ffff88810fc71000
> [ 18.297365] which belongs to the cache kmalloc-512 of size 512
> [ 18.298186] The buggy address is located 280 bytes inside of
> [ 18.298186] freed 512-byte region [ffff88810fc71000, ffff88810fc71200)
> [ 18.298992]
> [ 18.299106] The buggy address belongs to the physical page:
> [ 18.299481] page: refcount:0 mapcount:0 mapping:0000000000000000
> index:0x0 pfn:0x10fc70
> [ 18.300018] head: order:3 mapcount:0 entire_mapcount:0
> nr_pages_mapped:0 pincount:0
> [ 18.300526] flags: 0x8000000000000040(head|zone=2)
> [ 18.300852] page_type: f5(slab)
> [ 18.301076] raw: 8000000000000040 ffff888100042c80 dead000000000100
> dead000000000122
> [ 18.301591] raw: 0000000000000000 0000000080200020 00000000f5000000
> 0000000000000000
> [ 18.302109] head: 8000000000000040 ffff888100042c80
> dead000000000100 dead000000000122
> [ 18.302629] head: 0000000000000000 0000000080200020
> 00000000f5000000 0000000000000000
> [ 18.303152] head: 8000000000000003 ffffea00043f1c01
> ffffffffffffffff 0000000000000000
> [ 18.303671] head: 0000000000000008 0000000000000000
> 00000000ffffffff 0000000000000000
> [ 18.304194] page dumped because: kasan: bad access detected
> [ 18.304568]
> [ 18.304682] Memory state around the buggy address:
> [ 18.305008] ffff88810fc71000: fa fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [ 18.305491] ffff88810fc71080: fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [ 18.305975] >ffff88810fc71100: fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [ 18.306456] ^
> [ 18.306731] ffff88810fc71180: fb fb fb fb fb fb fb fb fb fb fb fb
> fb fb fb fb
> [ 18.307215] ffff88810fc71200: fc fc fc fc fc fc fc fc fc fc fc fc
> fc fc fc fc
> [ 18.307696] ==================================================================
Thanks, I've confirmed it. It seems I overlooked it because somehow
lockdep and kasan were not enabled for a while.
Assuming the v5 patch series rebased onto the latest gpio/for-next
21c853ad9309 ("gpio: adnp: use new line value setter callbacks"),
the following follow-up patch should suffice.
------------8<--------------8<---------------
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
index df34d8fcb79a..56f0fde8c843 100644
--- a/drivers/gpio/gpio-aggregator.c
+++ b/drivers/gpio/gpio-aggregator.c
@@ -207,7 +207,18 @@ static void aggr_free_lines(struct gpio_aggregator *aggr)
list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
configfs_unregister_group(&line->group);
- aggr_line_del(aggr, line);
+ /*
+ * Normally, we acquire aggr->lock within the configfs
+ * callback. However, in the legacy sysfs interface case,
+ * calling configfs_(un)register_group while holding
+ * aggr->lock could cause a deadlock. Fortunately, this is
+ * unnecessary because the new_device/delete_device path
+ * and the module unload path are mutually exclusive,
+ * thanks to an explicit try_module_get. That's why this
+ * minimal scoped_guard suffices here.
+ */
+ scoped_guard(mutex, &aggr->lock)
+ aggr_line_del(aggr, line);
kfree(line->key);
kfree(line);
}
@@ -926,8 +937,6 @@ static void gpio_aggr_device_release(struct config_item *item)
{
struct gpio_aggregator *aggr = to_gpio_aggregator(item);
- guard(mutex)(&aggr->lock);
-
/*
* At this point, aggr is neither active nor activating,
* so calling aggr_deactivate() is always unnecessary.
@@ -1072,7 +1081,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
&line->group);
if (error)
goto err;
- aggr_line_add(aggr, line);
+ scoped_guard(mutex, &aggr->lock)
+ aggr_line_add(aggr, line);
error = aggr_add_gpio(aggr, key, U16_MAX, &n);
if (error)
@@ -1101,7 +1111,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
&line->group);
if (error)
goto err;
- aggr_line_add(aggr, line);
+ scoped_guard(mutex, &aggr->lock)
+ aggr_line_add(aggr, line);
error = aggr_add_gpio(aggr, key, i, &n);
if (error)
@@ -1205,8 +1216,10 @@ static DRIVER_ATTR_WO(new_device);
static void gpio_aggregator_free(struct gpio_aggregator *aggr)
{
- if (aggr_is_activating(aggr) || aggr_is_active(aggr))
- aggr_deactivate(aggr);
+ scoped_guard(mutex, &aggr->lock) {
+ if (aggr_is_activating(aggr) || aggr_is_active(aggr))
+ aggr_deactivate(aggr);
+ }
aggr_free_lines(aggr);
configfs_unregister_group(&aggr->group);
kfree(aggr);
------------8<--------------8<---------------
* The second hunk should be squashed into
[PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface
* The rest of the hunks should be squashed into
[PATCH v5 8/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs
If you agree with the above approach, I'll send out v6,
while also addressing your feedback here:
https://lore.kernel.org/all/CAMRc=MdoMKdqyzGMFDa3aMz3h=vfZ0OtwARxY7FdsPKcBu9HQA@mail.gmail.com/
Koichiro
>
> Bart
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-03-10 16:28 ` Koichiro Den
@ 2025-03-10 17:46 ` Bartosz Golaszewski
2025-03-13 17:00 ` Koichiro Den
0 siblings, 1 reply; 20+ messages in thread
From: Bartosz Golaszewski @ 2025-03-10 17:46 UTC (permalink / raw)
To: Koichiro Den
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Mar 10, 2025 at 5:28 PM Koichiro Den <koichiro.den@canonical.com> wrote:
>
[snip!]
Please remove unnecessary context from responses. You attached
hundreds of lines of stack traces here. :(
>
> Thanks, I've confirmed it. It seems I overlooked it because somehow
> lockdep and kasan were not enabled for a while.
>
> Assuming the v5 patch series rebased onto the latest gpio/for-next
> 21c853ad9309 ("gpio: adnp: use new line value setter callbacks"),
> the following follow-up patch should suffice.
>
> ------------8<--------------8<---------------
> diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> index df34d8fcb79a..56f0fde8c843 100644
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -207,7 +207,18 @@ static void aggr_free_lines(struct gpio_aggregator *aggr)
>
> list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
> configfs_unregister_group(&line->group);
> - aggr_line_del(aggr, line);
> + /*
> + * Normally, we acquire aggr->lock within the configfs
> + * callback. However, in the legacy sysfs interface case,
> + * calling configfs_(un)register_group while holding
> + * aggr->lock could cause a deadlock. Fortunately, this is
> + * unnecessary because the new_device/delete_device path
> + * and the module unload path are mutually exclusive,
> + * thanks to an explicit try_module_get. That's why this
> + * minimal scoped_guard suffices here.
> + */
> + scoped_guard(mutex, &aggr->lock)
> + aggr_line_del(aggr, line);
> kfree(line->key);
> kfree(line);
> }
> @@ -926,8 +937,6 @@ static void gpio_aggr_device_release(struct config_item *item)
> {
> struct gpio_aggregator *aggr = to_gpio_aggregator(item);
>
> - guard(mutex)(&aggr->lock);
> -
> /*
> * At this point, aggr is neither active nor activating,
> * so calling aggr_deactivate() is always unnecessary.
> @@ -1072,7 +1081,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> &line->group);
> if (error)
> goto err;
> - aggr_line_add(aggr, line);
> + scoped_guard(mutex, &aggr->lock)
> + aggr_line_add(aggr, line);
>
> error = aggr_add_gpio(aggr, key, U16_MAX, &n);
> if (error)
> @@ -1101,7 +1111,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> &line->group);
> if (error)
> goto err;
> - aggr_line_add(aggr, line);
> + scoped_guard(mutex, &aggr->lock)
> + aggr_line_add(aggr, line);
>
> error = aggr_add_gpio(aggr, key, i, &n);
> if (error)
> @@ -1205,8 +1216,10 @@ static DRIVER_ATTR_WO(new_device);
>
> static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> {
> - if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> - aggr_deactivate(aggr);
> + scoped_guard(mutex, &aggr->lock) {
> + if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> + aggr_deactivate(aggr);
> + }
> aggr_free_lines(aggr);
> configfs_unregister_group(&aggr->group);
> kfree(aggr);
> ------------8<--------------8<---------------
>
>
> * The second hunk should be squashed into
> [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface
>
> * The rest of the hunks should be squashed into
> [PATCH v5 8/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs
>
> If you agree with the above approach, I'll send out v6,
> while also addressing your feedback here:
> https://lore.kernel.org/all/CAMRc=MdoMKdqyzGMFDa3aMz3h=vfZ0OtwARxY7FdsPKcBu9HQA@mail.gmail.com/
>
> Koichiro
>
I won't be testing in-line diff chunks. Please, just fix these issues
and send a v6. Also: please do write some sort of a script to automate
the testing of this driver if possible. Ideally: add test script to
selftests.
Bart
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v5 0/9] Introduce configfs-based interface for gpio-aggregator
2025-03-10 17:46 ` Bartosz Golaszewski
@ 2025-03-13 17:00 ` Koichiro Den
0 siblings, 0 replies; 20+ messages in thread
From: Koichiro Den @ 2025-03-13 17:00 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: linux-gpio, geert+renesas, linus.walleij, maciej.borzecki,
linux-kernel
On Mon, Mar 10, 2025 at 06:46:25PM GMT, Bartosz Golaszewski wrote:
> On Mon, Mar 10, 2025 at 5:28 PM Koichiro Den <koichiro.den@canonical.com> wrote:
> >
>
> [snip!]
>
> Please remove unnecessary context from responses. You attached
> hundreds of lines of stack traces here. :(
Right, this will never happen. Sorry for inconvenience.
>
> >
> > Thanks, I've confirmed it. It seems I overlooked it because somehow
> > lockdep and kasan were not enabled for a while.
> >
> > Assuming the v5 patch series rebased onto the latest gpio/for-next
> > 21c853ad9309 ("gpio: adnp: use new line value setter callbacks"),
> > the following follow-up patch should suffice.
> >
> > ------------8<--------------8<---------------
> > diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c
> > index df34d8fcb79a..56f0fde8c843 100644
> > --- a/drivers/gpio/gpio-aggregator.c
> > +++ b/drivers/gpio/gpio-aggregator.c
> > @@ -207,7 +207,18 @@ static void aggr_free_lines(struct gpio_aggregator *aggr)
> >
> > list_for_each_entry_safe(line, tmp, &aggr->list_head, entry) {
> > configfs_unregister_group(&line->group);
> > - aggr_line_del(aggr, line);
> > + /*
> > + * Normally, we acquire aggr->lock within the configfs
> > + * callback. However, in the legacy sysfs interface case,
> > + * calling configfs_(un)register_group while holding
> > + * aggr->lock could cause a deadlock. Fortunately, this is
> > + * unnecessary because the new_device/delete_device path
> > + * and the module unload path are mutually exclusive,
> > + * thanks to an explicit try_module_get. That's why this
> > + * minimal scoped_guard suffices here.
> > + */
> > + scoped_guard(mutex, &aggr->lock)
> > + aggr_line_del(aggr, line);
> > kfree(line->key);
> > kfree(line);
> > }
> > @@ -926,8 +937,6 @@ static void gpio_aggr_device_release(struct config_item *item)
> > {
> > struct gpio_aggregator *aggr = to_gpio_aggregator(item);
> >
> > - guard(mutex)(&aggr->lock);
> > -
> > /*
> > * At this point, aggr is neither active nor activating,
> > * so calling aggr_deactivate() is always unnecessary.
> > @@ -1072,7 +1081,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> > &line->group);
> > if (error)
> > goto err;
> > - aggr_line_add(aggr, line);
> > + scoped_guard(mutex, &aggr->lock)
> > + aggr_line_add(aggr, line);
> >
> > error = aggr_add_gpio(aggr, key, U16_MAX, &n);
> > if (error)
> > @@ -1101,7 +1111,8 @@ static int aggr_parse(struct gpio_aggregator *aggr)
> > &line->group);
> > if (error)
> > goto err;
> > - aggr_line_add(aggr, line);
> > + scoped_guard(mutex, &aggr->lock)
> > + aggr_line_add(aggr, line);
> >
> > error = aggr_add_gpio(aggr, key, i, &n);
> > if (error)
> > @@ -1205,8 +1216,10 @@ static DRIVER_ATTR_WO(new_device);
> >
> > static void gpio_aggregator_free(struct gpio_aggregator *aggr)
> > {
> > - if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> > - aggr_deactivate(aggr);
> > + scoped_guard(mutex, &aggr->lock) {
> > + if (aggr_is_activating(aggr) || aggr_is_active(aggr))
> > + aggr_deactivate(aggr);
> > + }
> > aggr_free_lines(aggr);
> > configfs_unregister_group(&aggr->group);
> > kfree(aggr);
> > ------------8<--------------8<---------------
> >
> >
> > * The second hunk should be squashed into
> > [PATCH v5 4/9] gpio: aggregator: introduce basic configfs interface
> >
> > * The rest of the hunks should be squashed into
> > [PATCH v5 8/9] gpio: aggregator: expose aggregator created via legacy sysfs to configfs
> >
> > If you agree with the above approach, I'll send out v6,
> > while also addressing your feedback here:
> > https://lore.kernel.org/all/CAMRc=MdoMKdqyzGMFDa3aMz3h=vfZ0OtwARxY7FdsPKcBu9HQA@mail.gmail.com/
> >
> > Koichiro
> >
>
> I won't be testing in-line diff chunks. Please, just fix these issues
> and send a v6. Also: please do write some sort of a script to automate
> the testing of this driver if possible. Ideally: add test script to
> selftests.
Sorry for the delayed response, I've been so tied up with other tasks this
week. Ok, I'll introduce a kselftest for gpio-aggregator. Actually I've
wanted that from the beginning.. I believe it should rely on gpio-sim for
convenience, but please let me know if you don't think so.
Thanks.
>
> Bart
^ permalink raw reply [flat|nested] 20+ messages in thread