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: <20200415150550.28156-1-nsaenzjulienne@suse.de> <20200415150550.28156-5-nsaenzjulienne@suse.de>
In-Reply-To: <20200415150550.28156-5-nsaenzjulienne@suse.de>
From:   Saravana Kannan <saravanak@google.com>
Date:   Wed, 15 Apr 2020 11:52:48 -0700
Message-ID: <CAGETcx9ewwOq3TRWorDf26HQzfQSd0KbtUT9AcoNnKpBwfuu+g@mail.gmail.com>
Subject: Re: [PATCH 4/4] of: property: Avoid linking devices with circular dependencies
To:     Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Apr 15, 2020 at 8:06 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> When creating a consumer/supplier relationship between devices it's
> essential to make sure they aren't supplying each other creating a
> circular dependency.

Kinda correct. But fw_devlink is not just about optimizing probing.
It's also about ensuring sync_state() callbacks work correctly when
drivers are built as modules. And for that to work, circular
"SYNC_STATE_ONLY" device links are allowed. I've explained it in a bit
more detail here [1].

> Introduce a new function to check if such circular dependency exists
> between two device nodes and use it in of_link_to_phandle().
>
> Fixes: a3e1d1a7f5fc ("of: property: Add functional dependency link from DT bindings")
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> ---
>
> NOTE:
>  I feel of_link_is_circular() is a little dense, and could benefit from
>  some abstraction/refactoring. That said, I'd rather get some feedback,
>  before spending time on it.

Good call :)

>  drivers/of/property.c | 50 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 2c7978ef22be1..74a5190408c3b 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1171,6 +1171,44 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         {}
>  };
>
> +/**
> + * of_link_is_circular - Make sure potential link isn't circular
> + *
> + * @sup_np: Supplier device
> + * @con_np: Consumer device
> + *
> + * This function checks if @sup_np's properties contain a reference to @con_np.
> + *
> + * Will return true if there's a circular dependency and false otherwise.
> + */
> +static bool of_link_is_circular(struct device_node *sup_np,
> +                               struct device_node *con_np)
> +{
> +       const struct supplier_bindings *s = of_supplier_bindings;
> +       struct device_node *tmp;
> +       bool matched = false;
> +       struct property *p;
> +       int i = 0;
> +
> +       for_each_property_of_node(sup_np, p) {
> +               while (!matched && s->parse_prop) {
> +                       while ((tmp = s->parse_prop(sup_np, p->name, i))) {
> +                               matched = true;
> +                               i++;
> +
> +                               if (tmp == con_np)
> +                                       return true;
> +                       }
> +                       i = 0;
> +                       s++;
> +               }
> +               s = of_supplier_bindings;
> +               matched = false;
> +       }
> +
> +       return false;
> +}

This only catches circular links made out of 2 devices. If we really
needed such a function that worked correctly to catch bigger
"circles", you'd need to recurse and it'll get super wasteful and
ugly.

Thankfully, device_link_add() already checks for circular dependencies
when we need it and it's much cheaper because the links are at a
device level and not examined at a property level.

Is this a real problem you are hitting with the Raspberry Pi 4's? If
so can you give an example in its DT where you are hitting this?

I'll have to NACK this patch for reasons mentioned above and in [1].
However, I think I have a solution that should work for what I'm
guessing is your real problem. But let me see the description of the
real scenario before I claim to have a solution.

-Saravana

[1] - https://lore.kernel.org/lkml/20191028220027.251605-1-saravanak@google.com/