linux-kernel - RE: [EXT] Re: [PATCH] driver core: improve cycle detection on fwnode graph

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]

Message-ID:
 <DU2PR04MB8822047A07680596415A61358C7A2@DU2PR04MB8822.eurprd04.prod.outlook.com>
Date: Thu, 25 Jan 2024 04:21:24 +0000
From: Xu Yang <xu.yang_2@....com>
To: Saravana Kannan <saravanak@...gle.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"rafael@...nel.org" <rafael@...nel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Android Kernel Team <kernel-team@...roid.com>
Subject: RE: [EXT] Re: [PATCH] driver core: improve cycle detection on fwnode
 graph

Hi Saravana,

> 
> On Wed, Jan 24, 2024 at 12:40 AM Xu Yang <xu.yang_2@....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.

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)

> 
> 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:
---
tcpc@50 {
    compatible = "nxp,ptn5110";
    ...

    port {
        typec_dr_sw: endpoint {
            remote-endpoint = <&usb3_drd_sw>;
        };
    };    
};

usb@...00000 {
    compatible = "snps,dwc3";
    phys = <&usb3_phy0>, <&usb3_phy0>;
    ...

    port {
        usb3_drd_sw: endpoint {
            remote-endpoint = <&typec_dr_sw>;
        };
    };
};

usb3_phy0: usb-phy@...f0040 {
    compatible = "fsl,imx8mp-usb-phy";

    ...
};

And fwnode links are created as below:
---
[    0.059553] /soc@...us@...00000/i2c@...30000/tcpc@50 Linked as a fwnode consumer to /soc@...sb@...10100/usb@...00000/port/endpoint
[    0.066365] /soc@...sb-phy@...f0040 Linked as a fwnode consumer to /soc@...us@...00000/i2c@...30000/tcpc@50
[    0.066624] /soc@...sb@...10100/usb@...00000 Linked as a fwnode consumer to /soc@...sb-phy@...f0040
[    0.066702] /soc@...sb@...10100/usb@...00000 Linked as a fwnode consumer to /soc@...us@...00000/i2c@...30000/tcpc@...port/endpoint

> 
> 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.

Thanks,
Xu Yang