linux-kernel - Re: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct 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]

Date:   Sat, 7 Oct 2023 00:14:14 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Bartosz Golaszewski <brgl@...ev.pl>
Cc:     Andy Shevchenko <andy@...nel.org>, linux-gpio@...r.kernel.org,
        linux-kernel@...r.kernel.org, Dipen Patel <dipenp@...dia.com>,
        Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
Subject: Re: [RFC/RFT PATCH] gpiolib: reverse-assign the fwnode to struct gpio_chip

On Fri, Oct 6, 2023 at 1:51 PM Bartosz Golaszewski <brgl@...ev.pl> wrote:

> struct gpio_chip is not only used to carry the information needed to
> set-up a GPIO device but is also used in all GPIOLIB callbacks and is
> passed to the matching functions of lookup helpers.
>
> In that last case, it is currently impossible to match a GPIO device by
> fwnode unless it was explicitly assigned to the chip in the provider
> code. If the fwnode is taken from the parent device, the pointer in
> struct gpio_chip will remain NULL.
>
> If we have a parent device but gc->fwnode was not assigned by the
> provider, let's assign it ourselves so that lookup by fwnode can work in
> all cases.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>

Acked-by: Linus Walleij <linus.walleij@...aro.org>

because we want the code to work (rough consensus and running code)

> -       if (gc->fwnode)
> +       if (gc->fwnode) {
>                 device_set_node(&gdev->dev, gc->fwnode);
> -       else if (gc->parent)
> -               device_set_node(&gdev->dev, dev_fwnode(gc->parent));
> +       } else if (gc->parent) {
> +               parent_fwnode = dev_fwnode(gc->parent);
> +               device_set_node(&gdev->dev, parent_fwnode);
> +               gc->fwnode = parent_fwnode;

The core of the crux is that we have
information duplication with a reference to the fwnode in two
places. One in gdev->dev and one in gc->fwnode.

gc->of_node was the same duplicated before.

A gdev is created for each gpio_chip so in my naive brain we could
get rid of gc->fwnode and only have the one inside gdev->dev?
+/- some helpful getters/setters if need be.

Or what am I thinking wrong here?

Yours,
Linus Walleij