netdev - [PATCH RFT] of: property: fw_devlink: Add support for the "phy-handle" binding

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-next>] [day] [month] [year] [list]

Message-Id: <20240930-phy-handle-fw-devlink-v1-1-4ea46acfcc12@redhat.com>
Date: Mon, 30 Sep 2024 16:28:38 -0500
From: Andrew Halaney <ahalaney@...hat.com>
To: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>
Cc: "Russell King (Oracle)" <linux@...linux.org.uk>, 
 Andrew Lunn <andrew@...n.ch>, Abhishek Chauhan <quic_abchauha@...cinc.com>, 
 Serge Semin <fancer.lancer@...il.com>, devicetree@...r.kernel.org, 
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
 Andrew Halaney <ahalaney@...hat.com>
Subject: [PATCH RFT] of: property: fw_devlink: Add support for the
 "phy-handle" binding

Add support for parsing the phy-handle binding so that fw_devlink can
enforce the dependency. This prevents MACs (that use this binding to
claim they're using the corresponding phy) from probing prior to the
phy, unless the phy is a child of the MAC (which results in a
dependency cycle) or similar.

For some motivation, imagine a device topology like so:

    &ethernet0 {
            phy-mode = "sgmii";
            phy-handle = <&sgmii_phy0>;

            mdio {
                    compatible = "snps,dwmac-mdio";
                    sgmii_phy0: phy@8 {
                            compatible = "ethernet-phy-id0141.0dd4";
                            reg = <0x8>;
                            device_type = "ethernet-phy";
                    };

                    sgmii_phy1: phy@a {
                            compatible = "ethernet-phy-id0141.0dd4";
                            reg = <0xa>;
                            device_type = "ethernet-phy";
                    };
            };
    };

    &ethernet1 {
            phy-mode = "sgmii";
            phy-handle = <&sgmii_phy1>;
    };

Here ethernet1 depends on sgmii_phy1 to function properly. In the below
link an issue is reported where ethernet1 is probed and used prior to
sgmii_phy1, resulting in a failure to get things working for ethernet1.
With this change in place ethernet1 doesn't probe until sgmii_phy1 is
ready, resulting in ethernet1 functioning properly.

ethernet0 consumes sgmii_phy0, but this dependency isn't enforced
via the device_links backing fw_devlink since ethernet0 is the parent of
sgmii_phy0. Here's a log showing that in action:

    [    7.000432] qcom-ethqos 23040000.ethernet: Fixed dependency cycle(s) with /soc@...thernet@...40000/mdio/phy@8

With this change in place ethernet1's dependency is properly described,
and it doesn't probe prior to sgmii_phy1 being available.

Link: https://lore.kernel.org/netdev/7723d4l2kqgrez3yfauvp2ueu6awbizkrq4otqpsqpytzp45q2@rju2nxmqu4ew/
Suggested-by: Serge Semin <fancer.lancer@...il.com>
Signed-off-by: Andrew Halaney <ahalaney@...hat.com>
---
I've marked this as an RFT because when looking through old mailing
list discusssions and kernel tech talks on this subject, I was unable
to really understand why in the past phy-handle had been left out. There
were some loose references to circular dependencies (which seem more or
less handled by fw_devlink to me), and the fact that a lot of behavior
happens in ndo_open() (but I couldn't quite grok the concern there).

I'd appreciate more testing by others and some feedback from those who
know this a bit better to indicate whether fw_devlink is ready to handle
this or not.

At least in my narrow point of view, it's working well for me.

Thanks,
Andrew
---
 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 11b922fde7af..4a2fca75e1c6 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1220,6 +1220,7 @@ DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
 DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
 DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", "#nvmem-cell-cells")
 DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
+DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
 DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
 DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
 DEFINE_SIMPLE_PROP(pinctrl1, "pinctrl-1", NULL)
@@ -1366,6 +1367,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_extcon, },
 	{ .parse_prop = parse_nvmem_cells, },
 	{ .parse_prop = parse_phys, },
+	{ .parse_prop = parse_phy_handle, },
 	{ .parse_prop = parse_wakeup_parent, },
 	{ .parse_prop = parse_pinctrl0, },
 	{ .parse_prop = parse_pinctrl1, },

---
base-commit: cea5425829f77e476b03702426f6b3701299b925
change-id: 20240829-phy-handle-fw-devlink-b829622200ae

Best regards,
-- 
Andrew Halaney <ahalaney@...hat.com>