Re: [RFC] of/platform: Create device link between simple-mfd and its children

[Saravana Kannan]

On Thu, Oct 15, 2020 at 4:43 AM Nicolas Saenz Julienne
<nsaenzjulienne@xxxxxxx> wrote:
>
> 'simple-mfd' usage implies there might be some kind of resource sharing
> between the parent device and its children. By creating a device link
> with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time
> the parent device is unbound while leaving its children unaware that
> some of their resources disappeared.

Doesn't the parent child relationship already ensure that? If not,
maybe that's what needs fixing?

> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@xxxxxxx>
>
> ---
>
> Some questions:
>
> - To what extent do we care about cleanly unbinding platform devices at
>   runtime? My rationale here is: "It's a platform device, for all you
>   know you might be unbinding someting essential to the system. So if
>   you're doing it, you better know what you're doing."
>
> - Would this be an abuse of device links?

Feels like it.

>
> - If applying this to all simple-mfd devices is a bit too much, would
>   this be acceptable for a specific device setup. For example RPi4's
>   firmware interface (simple-mfd user) is passed to consumer drivers
>   trough a custom API (see rpi_firmware_get()). So, when unbound,
>   consumers are left with a firmware handle that points to nothing.

You can always create device link between the real suppliers and consumers.

>
>  drivers/of/platform.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index b557a0fcd4ba..8d5b55b81764 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -390,8 +390,14 @@ static int of_platform_bus_create(struct device_node *bus,
>         }
>
>         dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent);
> -       if (!dev || !of_match_node(matches, bus))
> -               return 0;
> +       if (!dev)
> +              return 0;
> +
> +       if (parent && of_device_is_compatible(parent->of_node, "simple-mfd"))
> +              device_link_add(&dev->dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> +       if (!of_match_node(matches, bus))
> +              return 0;

Even if we think we should add this between parent and child (this
still seems like not a good place to do it). Matching it by compatible
string and doing special stuff doesn't feel right inside here.

-Saravana

>
>         for_each_child_of_node(bus, child) {
>                 pr_debug("   create child: %pOF\n", child);
> --
> 2.28.0
>