diff mbox series

[RFC/RFT] gpiolib: reverse-assign the fwnode to struct gpio_chip

Message ID 20231006115147.18559-1-brgl@bgdev.pl
State New
Headers show
Series [RFC/RFT] gpiolib: reverse-assign the fwnode to struct gpio_chip | expand

Commit Message

Bartosz Golaszewski Oct. 6, 2023, 11:51 a.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

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@linaro.org>
---
This is something that Dipen reported with one of the tegra drivers where
a GPIO lookup by fwnode does not work because the fwnode pointer in struct
gpio_chip is NULL. This patch addresses this use-case.

 drivers/gpio/gpiolib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko Oct. 6, 2023, 1:24 p.m. UTC | #1 
On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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.

...

> +		gc->fwnode = parent_fwnode;

Ah, this is basically reverts my commit, the whole idea of which was to go
towards constant struct gpio_chip object that is supplied by a provider.
Bartosz Golaszewski Oct. 6, 2023, 7:07 p.m. UTC | #2 
On Fri, Oct 6, 2023 at 3:24 PM Andy Shevchenko <andy@kernel.org> wrote:
>
> On Fri, Oct 06, 2023 at 01:51:47PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > 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.
>
> ...
>
> > +             gc->fwnode = parent_fwnode;
>
> Ah, this is basically reverts my commit, the whole idea of which was to go
> towards constant struct gpio_chip object that is supplied by a provider.
>

Then this idea was wrong in the first place and that goal will never
be achieved. Whether that's a correct approach is questionable but
struct gpio_chip has become so much more than a simple config
structure and - given how ubiquitous GPIO providers are throughout the
different subsystems of the kernel - it'll stay that way unless we're
ready to rebuild every GPIO provider in linux.

The best we can do now is at least make its usage safe. Meaning: it's
a structure with which providers will interact using GPIOLIB callbacks
which will in turn assure that during the execution of any function
taking struct gpio_chip as argument, it will remain alive and
protected from concurrent access.

The providers however will continue to use gpio_chip for many
purposes. One of such purposes is matching the GPIO device BY its
backing gpio_chip structure. It not having the same fwnode in this
particular case is an inconsistency rather than design IMO.

I don't see any good reason for it not having the fwnode assigned.
User calling gpio_device_find() will have to jump through hoops in
order to match the device by fwnode (include gpiolib.h and dereference
gpiodev?) but it could be very easily facilitated by just assigning it
at registration-time - just like we assign a whole bunch of other
pointers and data structures.

Bart

> --
> With Best Regards,
> Andy Shevchenko
>
>
Linus Walleij Oct. 6, 2023, 10:14 p.m. UTC | #3 
On Fri, Oct 6, 2023 at 1:51 PM Bartosz Golaszewski <brgl@bgdev.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@linaro.org>

Acked-by: Linus Walleij <linus.walleij@linaro.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
Andy Shevchenko Oct. 7, 2023, 7:03 a.m. UTC | #4 
On Sat, Oct 7, 2023 at 1:14 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Oct 6, 2023 at 1:51 PM Bartosz Golaszewski <brgl@bgdev.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@linaro.org>
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> because we want the code to work (rough consensus and running code)
>

> 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.

No, we don't. We have plenty of drivers that have gc->fwnode == NULL,
which means that it is shared with the parent device.


...

> 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?

That would work I think. But I'm definitely against this change. It is
the way to nowhere. We should really be quite strict about fwnode and
do NOT assign the gc one behind the provider's back. If something is
not working in this scenario, that should be fixed and not with a hack
like this.
Andy Shevchenko Oct. 7, 2023, 7:39 a.m. UTC | #5 
On Sat, Oct 07, 2023 at 12:22:01AM +0200, Linus Walleij wrote:
> On Fri, Oct 6, 2023 at 9:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > I don't see any good reason for it not having the fwnode assigned.

> > User calling gpio_device_find() will have to jump through hoops in
> > order to match the device by fwnode
> 
> Yeah I would add
> 
> struct fwnode_handle *gpiochip_get_fwnode(struct gpio_chip *gc)
> {
>    return dev_fwnode(&gc->gpiodev->dev);
> }
> 
> so it's easy for external users to get the fwnode if they really need it.
> This and a few more changes and we can drop gc->fwnode altogether
> can't we?

This would work, but the problem here is to understand which fwnode
(semantically) the caller wants to use.

One is the GPIO device's, and the other is what provider explicitly assigned.
Currently the latter case is transparent in a sense that GPIO device will get
the same fwnode as GPIO chip submitted by the provider.

Internally GPIOLIB must use GPIO device fwnode and rely only on it.
Externally it depends. Basically it's provider's business to know if it
is safe to use gc->fwnode or not and when.
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 191f9c87b4d0..a0e3d255fb73 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -782,6 +782,7 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			       struct lock_class_key *lock_key,
 			       struct lock_class_key *request_key)
 {
+	struct fwnode_handle *parent_fwnode;
 	struct gpio_device *gdev;
 	unsigned long flags;
 	unsigned int i;
@@ -806,10 +807,13 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 	 * If the calling driver did not initialize firmware node,
 	 * do it here using the parent device, if any.
 	 */
-	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;
+	}
 
 	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
 	if (gdev->id < 0) {