lkml.org 
[lkml]   [2020]   [Oct]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC] of/platform: Create device link between simple-mfd and its children
On Thu, Oct 15, 2020 at 4:43 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> 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@suse.de>
>
> ---
>
> 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
>

\
 
 \ /
  Last update: 2020-10-15 18:53    [W:0.090 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site