Re: [PATCH v2] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default

[Marc Zyngier <maz@xxxxxxxxxx>]

Hi Saravana,

Thanks for posting this, much appreciated.

On Sat, 16 Jan 2021 01:14:11 +0000,
Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> 
> There are multiple instances of GPIO devictree 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 lines up with how DT nodes with the "compatible" property are
> generally 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 its child device nodes
> with "compatible" property and instead just loops through its child
> nodes and directly registers the GPIOs with gpiolib without ever
> populating a struct device or binding a driver to it.

That's not quite an accurate description. The gpiolib subsystem does
create a struct device. It doesn't register a driver though, which is
what causes the issue with fr_devlink (more on that below).

> 
> Drivers that follow the 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 between with gpio0 as the supplier and bazz as the
> consumer. After this point, the device link will prevent bazz from
> probing until its supplier (the gpio0 device) has bound to a driver.
> Once the supplier is bound to a driver, the probe of bazz is triggered
> automatically.
> 
> Finding and refactoring all the instances of drivers that follow case 2
> will cause a lot of code churn and it not something that can be done in
> one shot. Examples of such instances are [1] [2].
> 
> This patch works around this problem and avoids all the code churn by
> simply creating a stub driver to bind to the gpio_device. Since the
> gpio_device already points to the GPIO device tree node, this allows all
> the consumers to continue probing when the driver follows case 2.
> 
> If/when all the old drivers are refactored, we can revert this
> patch.

My personal gripe with this approach is that it is an abrupt change in
the way DT and device model are mapped onto each other.

As far as I know (and someone please correct me if I am wrong), there
is zero expectation that a device/subdevice/random IP block described
by a node with a "compatible" property will end-up being managed by a
driver that is bound to that particular node.

The node/subnode division is a good way to express some HW boundaries,
but doesn't say anything about the way this should be handled in the
kernel. Assuming that everything containing a "compatible" string will
eventually be bound to a driver is IMO pretty limiting.

> 
> [1] - https://lore.kernel.org/lkml/20201014191235.7f71fcb4@xhacker.debian/
> [2] - https://lore.kernel.org/lkml/e28e1f38d87c12a3c714a6573beba6e1@xxxxxxxxxx/
> Cc: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Jisheng Zhang <Jisheng.Zhang@xxxxxxxxxxxxx>
> Cc: Kever Yang <kever.yang@xxxxxxxxxxxxxx>
> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default")
> Signed-off-by: Saravana Kannan <saravanak@xxxxxxxxxx>
> ---
> v1 -> v2:
> - Fixed up compilation errors that were introduced accidentally
> - Fixed a missing put_device()
> 
>  drivers/gpio/gpiolib.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index b02cc2abd3b6..12c579a953b0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -574,6 +574,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
>  	unsigned	i;
>  	int		base = gc->base;
>  	struct gpio_device *gdev;
> +	struct device_node *of_node;
> +	struct fwnode_handle *fwnode;
> +	struct device *fwnode_dev;
>  
>  	/*
>  	 * First: allocate and populate the internal stat container, and
> @@ -596,6 +599,22 @@ 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;
> +
> +	of_node = gdev->dev.of_node;
> +	fwnode = of_fwnode_handle(of_node);
> +	fwnode_dev = get_dev_from_fwnode(fwnode);
> +	/*
> +	 * If your driver hits this warning, it's because you are directly
> +	 * parsing a device tree node with "compatible" property and
> +	 * initializing it instead of using the standard DT + device driver
> +	 * model of creating a struct device and then initializing it in the
> +	 * probe function. Please refactor your driver.
> +	 */
> +	if (!fwnode_dev && of_find_property(of_node, "compatible", NULL)) {
> +		chip_warn(gc, "Create a real device for %pOF\n", of_node);

chip_warn() is not a good idea here, as gc->dev hasn't been
initialised yet, and results in the following output:

[    0.113996] gpio (null): (gpio0): Create a real device for /pinctrl/gpio0@ff720000
[    0.114727] gpio (null): (gpio1): Create a real device for /pinctrl/gpio1@ff730000
[    0.115340] gpio (null): (gpio2): Create a real device for /pinctrl/gpio2@ff780000
[    0.115912] gpio (null): (gpio3): Create a real device for /pinctrl/gpio3@ff788000
[    0.116437] gpio (null): (gpio4): Create a real device for /pinctrl/gpio4@ff790000

> +		gdev->dev.fwnode = fwnode;
> +	}
> +	put_device(fwnode_dev);
>  #endif
>  
>  	gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
> @@ -4202,6 +4221,17 @@ void gpiod_put_array(struct gpio_descs *descs)
>  }
>  EXPORT_SYMBOL_GPL(gpiod_put_array);
>  
> +static int gpio_drv_probe(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static struct device_driver gpio_drv = {
> +	.name = "gpio_drv",
> +	.bus = &gpio_bus_type,
> +	.probe = gpio_drv_probe,
> +};
> +
>  static int __init gpiolib_dev_init(void)
>  {
>  	int ret;
> @@ -4213,9 +4243,16 @@ static int __init gpiolib_dev_init(void)
>  		return ret;
>  	}
>  
> +	if (driver_register(&gpio_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_drv);
>  		bus_unregister(&gpio_bus_type);
>  		return ret;
>  	}

On the positive side, this patch brings my RK3399 system back to life.
However, on a system that doesn't suffer from this problem, I end-up
with the following issue:

$ ls -l /sys/bus/platform/drivers/mb86s70-gpio/51000000.gpio/of_node
lrwxrwxrwx 1 root root 0 Jan 17 11:15 /sys/bus/platform/drivers/mb86s70-gpio/51000000.gpio/of_node -> ../../../firmware/devicetree/base/gpio@51000000
$ ls -l /sys/bus/gpio/drivers/gpio_drv/gpiochip0/of_node
lrwxrwxrwx 1 root root 0 Jan 16 18:30 /sys/bus/gpio/drivers/gpio_drv/gpiochip0/of_node -> ../../../../firmware/devicetree/base/gpio@51000000

where two drivers are now handling the same of_node. Somehow, I don't
think this is a good idea, even if I didn't spot anything problematic
yet (maybe because there isn't anything useful hanging off the GPIOs
on this particular machine).

Overall, I'm concerned that this is a change in semantics that affects
the whole device model, and I wonder whether forcing everyone in the
same mould is the right approach.

An alternative I've been thinking about is to flag the device as
"satisfying existing dependencies" at the point where it is
registered. Because at the end of the day, we don't really care for
the *driver* part of it (case in point, the stub driver you introduced
doesn't do anything). We only care about *someone* saying "this device
is ready to be used".

So how about cutting the middleman?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.