linux-kernel - Re: [PATCH v2 05/10] gpio: aggregator: expose custom line names to forwarder gpio

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdVAiuvxD4ydtsWfWXBhHLDJz66Ay8w15WHr278OnB=ZKg@mail.gmail.com>
Date: Wed, 12 Feb 2025 15:44:46 +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 05/10] gpio: aggregator: expose custom line names to
 forwarder gpio_chip

Hi Den-san,

Thanks for your patch!

On Mon, 3 Feb 2025 at 04:12, Koichiro Den <koichiro.den@...onical.com> wrote:
> Previously, GPIO lines in the aggregator had empty names. Now that the

That is only true for aggregators created through the sysfs interface,
right?  When created from DT, gpio-line-names is already supported:
https://elixir.bootlin.com/linux/v6.13.2/source/Documentation/admin-guide/gpio/gpio-aggregator.rst#L72

> configfs interface supports custom names, update the GPIO forwarder to
> use them.
>
> Signed-off-by: Koichiro Den <koichiro.den@...onical.com>

> --- a/drivers/gpio/gpio-aggregator.c
> +++ b/drivers/gpio/gpio-aggregator.c
> @@ -425,6 +425,20 @@ static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *c
>  }
>  #endif /* !CONFIG_OF_GPIO */
>
> +static int gpiochip_fwd_line_names(struct device *dev, const char **names, int len)
> +{
> +       int num = device_property_string_array_count(dev, "gpio-line-names");
> +       if (!num)
> +               return 0;
> +       if (num > len) {
> +               pr_warn("gpio-line-names contains %d lines while %d expected",
> +                       num, len);
> +               num = len;
> +       }
> +       return device_property_read_string_array(dev, "gpio-line-names",
> +                                                names, num);
> +}
> +
>  /**
>   * gpiochip_fwd_create() - Create a new GPIO forwarder
>   * @dev: Parent device pointer
> @@ -447,6 +461,7 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>  {
>         const char *label = dev_name(dev);
>         struct gpiochip_fwd *fwd;
> +       const char **line_names;
>         struct gpio_chip *chip;
>         unsigned int i;
>         int error;
> @@ -458,6 +473,16 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>
>         chip = &fwd->chip;
>
> +       if (!dev_of_node(dev)) {
> +               line_names = devm_kcalloc(dev, sizeof(*line_names), ngpios, GFP_KERNEL);

So this is always allocated, even when no names are specified?

> +               if (!line_names)
> +                       return ERR_PTR(-ENOMEM);
> +
> +               error = gpiochip_fwd_line_names(dev, line_names, ngpios);
> +               if (error < 0)
> +                       return ERR_PTR(-ENOMEM);
> +       }
> +
>         /*
>          * If any of the GPIO lines are sleeping, then the entire forwarder
>          * will be sleeping.
> @@ -491,6 +516,9 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev,
>         chip->ngpio = ngpios;
>         fwd->descs = descs;
>
> +       if (!dev_of_node(dev))
> +               chip->names = line_names;
> +

Do you actually need to collect the names yourself?
Below, the driver does:

     error = devm_gpiochip_add_data(dev, chip, fwd);

and gpiochip_add_data_with_key() already calls gpiochip_set_names()
to retrieve the names from the gpio-line-names property.
What is missing to make that work?

>         if (chip->can_sleep)
>                 mutex_init(&fwd->mlock);
>         else
> @@ -530,10 +558,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)
> +{
> +       char **line_names __free(kfree) = NULL;

const (needed when gpio_aggregator_line.name becomes const)

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