netdev - Re: [PATCH] net: mdio: force deassert MDIO reset signal

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]

Date:   Mon, 16 Jan 2023 00:55:27 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Lars-Peter Clausen <lars@...afoo.de>
Cc:     Pierluigi Passaro <pierluigi.passaro@...il.com>,
        hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        eran.m@...iscite.com, nate.d@...iscite.com,
        francesco.f@...iscite.com, pierluigi.p@...iscite.com
Subject: Re: [PATCH] net: mdio: force deassert MDIO reset signal

> Specifying the ID as part of the compatible string works for clause 22 PHYs,
> but for clause 45 PHYs it does not work. The code always wants to read the
> ID from the PHY itself. But I do not understand things well enough to tell
> whether that's a fundamental restriction of C45 or just our implementation
> and the implementation can be changed to fix it.
> 
> Do you have some thoughts on this?

Do you have more details about what goes wrong? Which PHY driver is
it? What compatibles do you put into DT for the PHY?

To some extent, the ID is only used to find the driver. A C45 device
has a lot of ID register, and all of them are used by phy_bus_match()
to see if a driver matches. So you need to be careful which ID you
pick, it needs to match the driver.

It is the driver which decides to use C22 or C45 to talk to the PHY.
However, we do have:

static int phy_probe(struct device *dev)
{
...
        else if (phydev->is_c45)
                err = genphy_c45_pma_read_abilities(phydev);
        else
                err = genphy_read_abilities(phydev);

so it could be a C45 PHY probed using an ID does not have
phydev->is_c45 set, and so it looks in the wrong place for the
capabilities. Make sure you also have the compatible
"ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
set.

There is no fundamental restriction that i know of here, it probably
just needs somebody to debug it and find where it goes wrong.

Ah!

int fwnode_mdiobus_register_phy(struct mii_bus *bus,
                                struct fwnode_handle *child, u32 addr)
{
...
        rc = fwnode_property_match_string(child, "compatible",
                                          "ethernet-phy-ieee802.3-c45");
        if (rc >= 0)
                is_c45 = true;

        if (is_c45 || fwnode_get_phy_id(child, &phy_id))
                phy = get_phy_device(bus, addr, is_c45);
        else
                phy = phy_device_create(bus, addr, phy_id, 0, NULL);

So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
true. The if (is_c45 || is then true, so it does not need to call
fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
asks the PHY.

Try this, totally untested:

diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index b782c35c4ac1..13be23f8ac97 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
        if (rc >= 0)
                is_c45 = true;

-       if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+       if (fwnode_get_phy_id (child, &phy_id))
                phy = get_phy_device(bus, addr, is_c45);
        else
-               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+               phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
        if (IS_ERR(phy)) {
                rc = PTR_ERR(phy);
                goto clean_mii_ts;

     Andrew