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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 15 Oct 2020 09:52:18 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>, f.fainelli@...il.com,
        linux-rpi-kernel@...ts.infradead.org,
        u.kleine-koenig@...gutronix.de,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [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@...e.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@...e.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
>