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
>
