Re: [EXT] Re: [PATCH] driver core: improve cycle detection on fwnode graph

[Saravana Kannan]

On Wed, Jan 24, 2024 at 8:21 PM Xu Yang <xu.yang_2@xxxxxxx> wrote:
>
> Hi Saravana,
>
> >
> > On Wed, Jan 24, 2024 at 12:40 AM Xu Yang <xu.yang_2@xxxxxxx> 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.
>
> Sorry, I represent "-->" as "supplier --> consumer" and it's a fwnode link.
>
> >
> > 2. You say "link" sometimes but it's not clear if you mean fwnode
> > links or device links. So please be explicit about it.
>
> It’s fwnode link by default.
>
> >
> > 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.
>
> The fwnode is not created from the device node with compatible property
> since below commit. The endpoint node is the supplier. No, you can see my
> case later.
>
> 4a032827daa8 (of: property: Simplify of_link_to_phandle(), 2023-02-06)

I think my confusion was because you use ----> in the opposite way to
what I have used for all my fw_devlink and cycle detection patches.

The part I was referring to is related to how driver/of/property.c has
node_not_dev set to true for pasrse_remote_endpoint.

> >
> > 4. Has this actually caused an issue? If so, what is it? And give me
> > an example in an upstream DT.
>
> Yes, there are two cycles (B.EP->A->C->B and B.EP->A/A.EP->B) in above
> example. But only one cycle (B.EP->A->C->B) is recognized.
>
> My real case as below:

I think you still missed some details because usb3_phy0 seems
irrelevant here. Can you just point me to the dts (not dtsi) file for
this platform in the kernel tree?
Also, can you change all the pr_debug and dev_dbg in
drivers/base/core.c to their info equivalent and boot up the system
and give me the logs? That'll be a lot easier for me to understand
your case.

> ---
> tcpc@50 {
>     compatible = "nxp,ptn5110";
>     ...
>
>     port {
>         typec_dr_sw: endpoint {
>             remote-endpoint = <&usb3_drd_sw>;
>         };
>     };
> };
>
> usb@38100000 {
>     compatible = "snps,dwc3";
>     phys = <&usb3_phy0>, <&usb3_phy0>;
>     ...
>
>     port {
>         usb3_drd_sw: endpoint {
>             remote-endpoint = <&typec_dr_sw>;
>         };
>     };
> };
>
> usb3_phy0: usb-phy@381f0040 {
>     compatible = "fsl,imx8mp-usb-phy";
>
>     ...
> };
>
> And fwnode links are created as below:
> ---
> [    0.059553] /soc@0/bus@30800000/i2c@30a30000/tcpc@50 Linked as a fwnode consumer to /soc@0/usb@32f10100/usb@38100000/port/endpoint
> [    0.066365] /soc@0/usb-phy@381f0040 Linked as a fwnode consumer to /soc@0/bus@30800000/i2c@30a30000/tcpc@50
> [    0.066624] /soc@0/usb@32f10100/usb@38100000 Linked as a fwnode consumer to /soc@0/usb-phy@381f0040
> [    0.066702] /soc@0/usb@32f10100/usb@38100000 Linked as a fwnode consumer to /soc@0/bus@30800000/i2c@30a30000/tcpc@50/port/endpoint
>

So let's say I see your logs and what you say is true, but you still
aren't telling me what's the problem you have because of this
incorrect cycle detection. What's breaking? Is something not allowed
to probe? If so, which one? What's supposed to be the right order of
probes?

> >
> > 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.
>
> Sorry for the inconvenience. I saw that you push some patches about fwnode
> link and device link handling, so I think you may understand this issue
> well and give some suggestions.

No worries at all. Thanks for reporting the issue and thanks for
trying to fix it.

-Saravana