Message-ID: <CAJZ5v0iL9-JzTzE7pYTJGuB1BbrE6L12K2FKNpQ8dhX9GureJw@mail.gmail.com>
Date: Fri, 16 May 2025 21:22:20 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Herve Codina <herve.codina@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, Michael Turquette <mturquette@...libre.com>, 
	Stephen Boyd <sboyd@...nel.org>, Andi Shyti <andi.shyti@...nel.org>, 
	Wolfram Sang <wsa+renesas@...g-engineering.com>, Peter Rosin <peda@...ntia.se>, 
	Derek Kiernan <derek.kiernan@....com>, Dragan Cvetic <dragan.cvetic@....com>, 
	Arnd Bergmann <arnd@...db.de>, Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Mark Brown <broonie@...nel.org>, Len Brown <lenb@...nel.org>, 
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, Daniel Scally <djrscally@...il.com>, 
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Wolfram Sang <wsa@...nel.org>, 
	Geert Uytterhoeven <geert+renesas@...der.be>, linux-kernel@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-clk@...r.kernel.org, 
	linux-i2c@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-spi@...r.kernel.org, 
	linux-acpi@...r.kernel.org, Allan Nielsen <allan.nielsen@...rochip.com>, 
	Horatiu Vultur <horatiu.vultur@...rochip.com>, 
	Steen Hegelund <steen.hegelund@...rochip.com>, Luca Ceresoli <luca.ceresoli@...tlin.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 05/26] bus: simple-pm-bus: Populate child nodes at probe

On Wed, May 7, 2025 at 9:13 AM Herve Codina <herve.codina@...tlin.com> wrote:
>
> The simple-pm-bus drivers handles several simple bus. When it is used

s/drivers/driver/ (I think)
s/simple bus/simple busses/

> with busses other than a compatible "simple-pm-bus", it don't populate

s/it don't/it doesn't/

> its child devices during its probe.
>
> This confuses fw_devlink and results in wrong or missing devlinks.

Well, fair enough, but doesn't it do that for a reason?

> Once a driver is bound to a device and the probe() has been called,
> device_links_driver_bound() is called.
>
> This function performs operation based on the following assumption:
>     If a child firmware node of the bound device is not added as a
>     device, it will never be added.
>
> Among operations done on fw_devlinks of those "never be added" devices,
> device_links_driver_bound() changes their supplier.
>
> With devices attached to a simple-bus compatible device, this change
> leads to wrong devlinks where supplier of devices points to the device
> parent (i.e. simple-bus compatible device) instead of the device itself
> (i.e. simple-bus child).
>
> When the device attached to the simple-bus is removed, because devlinks
> are not correct, its consumers are not removed first.
>
> In order to have correct devlinks created, make the simple-pm-bus driver
> compliant with the devlink assumption and create its child devices
> during its probe.
>
> Signed-off-by: Herve Codina <herve.codina@...tlin.com>
> ---
>  drivers/bus/simple-pm-bus.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bus/simple-pm-bus.c b/drivers/bus/simple-pm-bus.c
> index d8e029e7e53f..93c6ba605d7a 100644
> --- a/drivers/bus/simple-pm-bus.c
> +++ b/drivers/bus/simple-pm-bus.c
> @@ -42,14 +42,14 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>         match = of_match_device(dev->driver->of_match_table, dev);
>         /*
>          * These are transparent bus devices (not simple-pm-bus matches) that
> -        * have their child nodes populated automatically.  So, don't need to
> -        * do anything more. We only match with the device if this driver is
> -        * the most specific match because we don't want to incorrectly bind to
> -        * a device that has a more specific driver.
> +        * have their child nodes populated automatically. So, don't need to
> +        * do anything more except populate child nodes.

The above part of the comment has become hard to grasp after the
change.  In particular, why populate child notes if they are populated
automatically?

> + We only match with the
> +        * device if this driver is the most specific match because we don't
> +        * want to incorrectly bind to a device that has a more specific driver.
>          */
>         if (match && match->data) {
>                 if (of_property_match_string(np, "compatible", match->compatible) == 0)
> -                       return 0;
> +                       goto populate;

Doesn't this interfere with anything, like the automatic population of
child nodes mentioned in the comment?

>                 else
>                         return -ENODEV;
>         }
> @@ -64,13 +64,14 @@ static int simple_pm_bus_probe(struct platform_device *pdev)
>
>         dev_set_drvdata(&pdev->dev, bus);
>
> -       dev_dbg(&pdev->dev, "%s\n", __func__);
> -
>         pm_runtime_enable(&pdev->dev);
>
> +populate:
>         if (np)
>                 of_platform_populate(np, NULL, lookup, &pdev->dev);
>
> +       dev_dbg(&pdev->dev, "%s\n", __func__);

So how to distinguish between devices that only have child nodes
populated and the ones that have drvdata set?

> +
>         return 0;
>  }
>
> @@ -78,12 +79,16 @@ static void simple_pm_bus_remove(struct platform_device *pdev)
>  {
>         const void *data = of_device_get_match_data(&pdev->dev);
>
> -       if (pdev->driver_override || data)
> +       if (pdev->driver_override)
>                 return;
>
>         dev_dbg(&pdev->dev, "%s\n", __func__);
>
> -       pm_runtime_disable(&pdev->dev);
> +       if (pdev->dev.of_node)
> +               of_platform_depopulate(&pdev->dev);
> +
> +       if (!data)
> +               pm_runtime_disable(&pdev->dev);
>  }
>
>  static int simple_pm_bus_runtime_suspend(struct device *dev)
> --

Powered by blists - more mailing lists