diff mbox series

[v4] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default

Message ID 20210121223756.1112199-1-saravanak@google.com
State New
Headers show
Series [v4] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default | expand

Commit Message

Saravana Kannan Jan. 21, 2021, 10:37 p.m. UTC
There are multiple instances of GPIO device tree nodes of the form:

foo {
	compatible = "acme,foo";
	...

	gpio0: gpio0@xxxxxxxx {
		compatible = "acme,bar";
		...
		gpio-controller;
	};

	gpio1: gpio1@xxxxxxxx {
		compatible = "acme,bar";
		...
		gpio-controller;
	};

	...
}

bazz {
	my-gpios = <&gpio0 ...>;
}

Case 1: The driver for "foo" populates struct device for these gpio*
nodes and then probes them using a driver that binds with "acme,bar".
This driver for "acme,bar" then registers the gpio* nodes with gpiolib.
This lines up with how DT nodes with the "compatible" property are
typically converted to struct devices and then registered with driver
core to probe them. This also allows the gpio* devices to hook into all
the driver core capabilities like runtime PM, probe deferral,
suspend/resume ordering, device links, etc.

Case 2: The driver for "foo" doesn't populate struct devices for these
gpio* nodes before registering them with gpiolib. Instead it just loops
through its child nodes and directly registers the gpio* nodes with
gpiolib.

Drivers that follow case 2 cause problems with fw_devlink=on. This is
because fw_devlink will prevent bazz from probing until there's a struct
device that has gpio0 as its fwnode (because bazz lists gpio0 as a GPIO
supplier). Once the struct device is available, fw_devlink will create a
device link with gpio0 device as the supplier and bazz device as the
consumer. After this point, since the gpio0 device will never bind to a
driver, the device link will prevent bazz device from ever probing.

Finding and refactoring all the instances of drivers that follow case 2
will cause a lot of code churn and it is not something that can be done
in one shot. In some instances it might not even be possible to refactor
them cleanly. Examples of such instances are [1] [2].

This patch works around this problem and avoids all the code churn by
simply setting the fwnode of the gpio_device and creating a stub driver
to bind to the gpio_device. This allows all the consumers to continue
probing when the driver follows case 2.

[1] - https://lore.kernel.org/lkml/20201014191235.7f71fcb4@xhacker.debian/
[2] - https://lore.kernel.org/lkml/e28e1f38d87c12a3c714a6573beba6e1@kernel.org/
Cc: Marc Zyngier <maz@kernel.org>
Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com>
Cc: Kever Yang <kever.yang@rock-chips.com>
Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
v1 -> v2:
- Fixed up compilation errors that were introduced accidentally
- Fixed a missing put_device()

v2 -> v3:
- Changed chip_warn() to pr_warn()
- Changed some variable names

v3 -> v4:
- Dropped the warning since it's not always valid
- This simplifies the code a lot
- Added comments and fixed up commit text

 drivers/gpio/gpiolib.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

Comments

Andy Shevchenko Jan. 22, 2021, 9:56 a.m. UTC | #1
On Fri, Jan 22, 2021 at 12:40 AM Saravana Kannan <saravanak@google.com> wrote:
>
> There are multiple instances of GPIO device tree nodes of the form:
>
> foo {
>         compatible = "acme,foo";
>         ...
>
>         gpio0: gpio0@xxxxxxxx {
>                 compatible = "acme,bar";
>                 ...
>                 gpio-controller;
>         };
>
>         gpio1: gpio1@xxxxxxxx {
>                 compatible = "acme,bar";
>                 ...
>                 gpio-controller;
>         };
>
>         ...
> }
>
> bazz {
>         my-gpios = <&gpio0 ...>;
> }
>
> Case 1: The driver for "foo" populates struct device for these gpio*
> nodes and then probes them using a driver that binds with "acme,bar".
> This driver for "acme,bar" then registers the gpio* nodes with gpiolib.
> This lines up with how DT nodes with the "compatible" property are
> typically converted to struct devices and then registered with driver
> core to probe them. This also allows the gpio* devices to hook into all
> the driver core capabilities like runtime PM, probe deferral,
> suspend/resume ordering, device links, etc.
>
> Case 2: The driver for "foo" doesn't populate struct devices for these
> gpio* nodes before registering them with gpiolib. Instead it just loops
> through its child nodes and directly registers the gpio* nodes with
> gpiolib.
>
> Drivers that follow case 2 cause problems with fw_devlink=on. This is
> because fw_devlink will prevent bazz from probing until there's a struct
> device that has gpio0 as its fwnode (because bazz lists gpio0 as a GPIO
> supplier). Once the struct device is available, fw_devlink will create a
> device link with gpio0 device as the supplier and bazz device as the
> consumer. After this point, since the gpio0 device will never bind to a
> driver, the device link will prevent bazz device from ever probing.
>
> Finding and refactoring all the instances of drivers that follow case 2
> will cause a lot of code churn and it is not something that can be done
> in one shot. In some instances it might not even be possible to refactor
> them cleanly. Examples of such instances are [1] [2].
>
> This patch works around this problem and avoids all the code churn by
> simply setting the fwnode of the gpio_device and creating a stub driver
> to bind to the gpio_device. This allows all the consumers to continue
> probing when the driver follows case 2.

...

> @@ -596,6 +596,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>                 gdev->dev.of_node = gc->of_node;
>         else
>                 gc->of_node = gdev->dev.of_node;
> +       gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);

This looks like a complete breakage on ACPI enabled systems. Care to
test on ACPI and confirm it works? I could recommend to use Intel
Galileo platform since there is a dwapb GPIO and a lot of nice
(semi-sarcastic, because of many quirks) code over the kernel.

(Yes, you have to have both OF and ACPI in the config being enabled
which is valid combination)

>  #endif

...

> +static int gpio_stub_drv_probe(struct device *dev)
> +{
> +       /*
> +        * The DT node of some GPIO chips have a "compatible" property, but
> +        * never have a struct device added and probed by a driver to register
> +        * the GPIO chip with gpiolib. In such cases, fw_devlink=on will cause
> +        * the consumers of the GPIO chip to get probe deferred forever because

get a probe

> +        * they will be waiting for a device associated with the GPIO chip
> +        * firmware node to get added and bound to a driver.
> +        *
> +        * To allow these consumers to probe, we associate the struct
> +        * gpio_device of the GPIO chip with the firmware node and then simply
> +        * bind it to this stub driver.
> +        */
> +       return 0;
> +}

...

> +       if (driver_register(&gpio_stub_drv) < 0) {
> +               pr_err("gpiolib: could not register GPIO stub driver\n");
> +               bus_unregister(&gpio_bus_type);
> +               return ret;
> +       }
> +
>         ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME);
>         if (ret < 0) {
>                 pr_err("gpiolib: failed to allocate char dev region\n");
> +               driver_unregister(&gpio_stub_drv);
>                 bus_unregister(&gpio_bus_type);
>                 return ret;
>         }

Looks like you missed to fix the __exit call in this module.
Linus Walleij Jan. 22, 2021, 1:08 p.m. UTC | #2
On Fri, Jan 22, 2021 at 10:55 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 22, 2021 at 12:40 AM Saravana Kannan <saravanak@google.com> wrote:

> > @@ -596,6 +596,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> >                 gdev->dev.of_node = gc->of_node;
> >         else
> >                 gc->of_node = gdev->dev.of_node;
> > +       gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);
>
> This looks like a complete breakage on ACPI enabled systems.

This looks broken to me too, if OF functions are compiled out
this happens:

include/linux/of.h:#define of_fwnode_handle(node) NULL

And if there is a valid fwnode in the device it gets overwritten
with NULL.

This is partly why I want the DT code to be in its own file.

Yours,
Linus Walleij
Saravana Kannan Jan. 22, 2021, 6:37 p.m. UTC | #3
On Fri, Jan 22, 2021 at 5:08 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jan 22, 2021 at 10:55 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

Andy, I'll address your exit() comments too.

> > On Fri, Jan 22, 2021 at 12:40 AM Saravana Kannan <saravanak@google.com> wrote:
>
> > > @@ -596,6 +596,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
> > >                 gdev->dev.of_node = gc->of_node;
> > >         else
> > >                 gc->of_node = gdev->dev.of_node;
> > > +       gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);
> >
> > This looks like a complete breakage on ACPI enabled systems.
>
> This looks broken to me too, if OF functions are compiled out
> this happens:
>
> include/linux/of.h:#define of_fwnode_handle(node) NULL
>
> And if there is a valid fwnode in the device it gets overwritten
> with NULL.
>
> This is partly why I want the DT code to be in its own file.

Yeah, both of you are right that this is broken.

So all I really need to do to fix this is:
if (gdev->dev.of_node)
    gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);

I don't mind putting this in gpiolib-of.c. It's just kinda weird to me
to call into gpiolib-of.c just for these 2 lines.

I'll just move the entire block inside #ifdef into gpiolib-of.c in the
next version. Let me know how it looks.

-Saravana
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index b02cc2abd3b6..f42eaca08d9a 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -596,6 +596,7 @@  int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 		gdev->dev.of_node = gc->of_node;
 	else
 		gc->of_node = gdev->dev.of_node;
+	gdev->dev.fwnode = of_fwnode_handle(gdev->dev.of_node);
 #endif
 
 	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
@@ -4202,6 +4203,29 @@  void gpiod_put_array(struct gpio_descs *descs)
 }
 EXPORT_SYMBOL_GPL(gpiod_put_array);
 
+static int gpio_stub_drv_probe(struct device *dev)
+{
+	/*
+	 * The DT node of some GPIO chips have a "compatible" property, but
+	 * never have a struct device added and probed by a driver to register
+	 * the GPIO chip with gpiolib. In such cases, fw_devlink=on will cause
+	 * the consumers of the GPIO chip to get probe deferred forever because
+	 * they will be waiting for a device associated with the GPIO chip
+	 * firmware node to get added and bound to a driver.
+	 *
+	 * To allow these consumers to probe, we associate the struct
+	 * gpio_device of the GPIO chip with the firmware node and then simply
+	 * bind it to this stub driver.
+	 */
+	return 0;
+}
+
+static struct device_driver gpio_stub_drv = {
+	.name = "gpio_stub_drv",
+	.bus = &gpio_bus_type,
+	.probe = gpio_stub_drv_probe,
+};
+
 static int __init gpiolib_dev_init(void)
 {
 	int ret;
@@ -4213,9 +4237,16 @@  static int __init gpiolib_dev_init(void)
 		return ret;
 	}
 
+	if (driver_register(&gpio_stub_drv) < 0) {
+		pr_err("gpiolib: could not register GPIO stub driver\n");
+		bus_unregister(&gpio_bus_type);
+		return ret;
+	}
+
 	ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME);
 	if (ret < 0) {
 		pr_err("gpiolib: failed to allocate char dev region\n");
+		driver_unregister(&gpio_stub_drv);
 		bus_unregister(&gpio_bus_type);
 		return ret;
 	}