Received-SPF: pass (google.com: domain of linux-kernel+bounces-37902-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240124084636.1415652-1-xu.yang_2@nxp.com>
In-Reply-To: <20240124084636.1415652-1-xu.yang_2@nxp.com>
From: Saravana Kannan <saravanak@google.com>
Date: Wed, 24 Jan 2024 18:55:38 -0800
Message-ID: <CAGETcx9h8gA8EenyR0B0OPa23uw_8dk-Kft8c8+F3StfpyMtaw@mail.gmail.com>
Subject: Re: [PATCH] driver core: improve cycle detection on fwnode graph
To: Xu Yang <xu.yang_2@nxp.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org, 
	linux-kernel@vger.kernel.org, Android Kernel Team <kernel-team@android.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 24, 2024 at 12:40=E2=80=AFAM Xu Yang <xu.yang_2@nxp.com> wrote:
>
> Currently, cycle detection on fwnode graph is still defective.
> Such as fwnode link A.EP->B is not marked as cycle in below case:
>
>                  +-----+
>                  |     |
>  +-----+         |  +--|
>  |     |<-----------|EP|
>  |--+  |         |  +--|
>  |EP|----------->|     |
>  |--+  |         |  B  |
>  |     |         +-----+
>  |  A  |            ^
>  +-----+   +-----+  |
>     |      |     |  |
>     +----->|  C  |--+
>            |     |
>            +-----+
>
> 1. Node C is populated as device C. But nodes A and B are still not
>    populated. When do cycle detection with device C, no cycle is found.
> 2. Node B is populated as device B. When do cycle detection with device
>    B, it found a link cycle B.EP->A->C->B. Then, fwnode link B.EP->A,
>    A->C and C->B are marked as cycle. The fwnode link C->B is converted
>    to device link too.
> 3. Node A is populated as device A. When do cycle detection with device
>    A, it find A->C is marked as cycle and convert it to device link. It
>    also find B.EP->A is marked as cycle but will not convert it to device
>    link since node B.EP is not a device.

Your example doesn't sound correct (I'l explain further down) and it
is vague. Need a couple of clarifications first.

1. What is the ---> representing? Is it references in DT or fwnode
links? Which end of the arrow is the consumer? The tail or the pointy
end? I typically use the format consumer --> supplier.

2. You say "link" sometimes but it's not clear if you mean fwnode
links or device links. So please be explicit about it.

3. Your statement "Such as fwnode link A.EP->B is not marked as cycle"
doesn't sound correct. When remote-endpoint properties are parsed, the
fwnode is created from the device node with compatible property to the
destination. So A.EP ----> B can't exist if I assume the consumer -->
supplier format.

4. Has this actually caused an issue? If so, what is it? And give me
an example in an upstream DT.

Btw, I definitely don't anticipate ACKing this patch because the cycle
detection code shouldn't be having property specific logic. It's not
even DT specific in this place. If there is an issue and it needs
fixing, it should be where the fwnode links are created. But then
again I'm not sure what the actual symptom we are trying to solve is.


-Saravana

>
> Finally, fwnode link C->B and A->C is removed, B.EP->A is only marked as
> cycle and A.EP->B is neither been marked as cycle nor removed.
>
> For fwnode graph, the endpoint node can only be a supplier of other node
> and the endpoint node will never be populated as device. Therefore, when
> creating device link to supplier for fwnode graph, we need to relax cycle
> with the real node rather endpoint node.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/base/core.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 14d46af40f9a..278ded6cd3ce 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2217,6 +2217,9 @@ static void __fw_devlink_link_to_suppliers(struct d=
evice *dev,
>                 int ret;
>                 struct fwnode_handle *sup =3D link->supplier;
>
> +               if (fwnode_graph_is_endpoint(sup))
> +                       sup =3D fwnode_graph_get_port_parent(sup);
> +
>                 ret =3D fw_devlink_create_devlink(dev, sup, link);
>                 if (!own_link || ret =3D=3D -EAGAIN)
>                         continue;
> --
> 2.34.1
>