| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMuHMdXCAH=5Az9fq33-8izCLRbzxOM6zj8VbPWj0iR=KXPFtw@mail.gmail.com>
Date: Wed, 12 Feb 2025 14:48:12 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Koichiro Den <koichiro.den@...onical.com>
Cc: linux-gpio@...r.kernel.org, brgl@...ev.pl, linus.walleij@...aro.org,
maciej.borzecki@...onical.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/10] gpio: aggregator: introduce basic configfs interface
Hi Den-san,
On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@...onical.com> wrote:
> 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 behaviour is retained for now.
>
> This commit implements minimal functionalities:
>
> /config/gpio-aggregator/<name-of-your-choice>/
> /config/gpio-aggregator/<name-of-your-choice>/live
> /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
>
> Signed-off-by: Koichiro Den <koichiro.den@...onical.com>
Thanks for your patch!
> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -9,10 +9,14 @@
>
> #include <linux/bitmap.h>
> #include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/configfs.h>
Using configfs requires CONFIG_CONFIGFS_FS.
So either GPIO_AGGREGATOR should select CONFIGFS_FS, or
all configfs-related code should be protected by checks for
CONFIG_CONFIGFS_FS. Since we want to encourage people to use the new
API, I think the former is preferred.
> #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>
> @@ -34,11 +38,39 @@
> */
>
> struct gpio_aggregator {
> + struct config_group group;
> struct gpiod_lookup_table *lookups;
> struct platform_device *pdev;
> + struct mutex lock;
> + int id;
> +
> + /* Synchronize with probe */
> + struct notifier_block bus_notifier;
> + struct completion probe_completion;
> + bool driver_bound;
> +
> + /* List of gpio_aggregator_line. Always added in order */
> + struct list_head list_head;
> +
> char args[];
> };
>
> +struct gpio_aggregator_line {
> + struct config_group group;
> + struct gpio_aggregator *parent;
> + struct list_head entry;
> +
> + /* Line index within the aggregator device */
> + int idx;
unsigned int
> +
> + /* GPIO chip label or line name */
> + 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);
> +static bool aggr_is_active(struct gpio_aggregator *aggr)
> +{
> + lockdep_assert_held(&aggr->lock);
> +
> + return !!aggr->pdev && platform_get_drvdata(aggr->pdev);
No need for "!!".
> +}
> +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 (WARN_ON(tmp->idx == line->idx))
Can this really happen?
> + return;
> + if (tmp->idx > line->idx) {
> + list_add_tail(&line->entry, &tmp->entry);
> + return;
> + }
> + }
> + list_add_tail(&line->entry, &aggr->list_head);
> +}
> +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)
> + WARN_ON(configfs_depend_item_unlocked(
> + subsys, &line->group.cg_item));
Can this happen?
I am also worried if this can be triggered by the user, combined
with panic_on_warn.
> + 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 sprintf(page, "%s\n", line->key ?: "");
Please use sysfs_emit() instead (everywhere).
> +}
> +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 (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)
s/Undepend/Lock-up/?
> + * succeeds or if device enablement (live == 1) fails.
> + */
> + if (live == !!ret)
> + aggr_lockup_configfs(aggr, false);
> +
> + return ret ?: count;
> +}
> +static struct config_group *
> +gpio_aggr_make_group(struct config_group *group, const char *name)
> +{
> + /* arg space is unneeded */
> + struct gpio_aggregator *aggr = kzalloc(sizeof(*aggr), GFP_KERNEL);
> + if (!aggr)
> + return ERR_PTR(-ENOMEM);
> +
> + mutex_lock(&gpio_aggregator_lock);
> + aggr->id = idr_alloc(&gpio_aggregator_idr, aggr, 0, 0, GFP_KERNEL);
> + mutex_unlock(&gpio_aggregator_lock);
> +
> + if (aggr->id < 0) {
> + kfree(aggr);
> + return ERR_PTR(aggr->id);
> + }
The above more or less duplicates the existing code in
new_device_store().
> +
> + INIT_LIST_HEAD(&aggr->list_head);
> + mutex_init(&aggr->lock);
> + config_group_init_type_name(&aggr->group, name, &gpio_aggr_device_type);
> + init_completion(&aggr->probe_completion);
> +
> + return &aggr->group;
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists