Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201015114346.15743-1-nsaenzjulienne@suse.de>
In-Reply-To: <20201015114346.15743-1-nsaenzjulienne@suse.de>
From:   Saravana Kannan <saravanak@google.com>
Date:   Thu, 15 Oct 2020 09:52:18 -0700
Message-ID: <CAGETcx9OxQKtYwYtHS7brKpUxfbcbd+VWju3tB1MSPR4dUYxQg@mail.gmail.com>
Subject: Re: [RFC] of/platform: Create device link between simple-mfd and its children
To:     Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>, f.fainelli@gmail.com,
        linux-rpi-kernel@lists.infradead.org,
        u.kleine-koenig@pengutronix.de,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>