netdev - Re: [PATCH net] net: phy: ensure PHY is powered up when reading ID registers

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: <152B6387-650F-4E3D-A9A4-2FDADE49C6FB@gmail.com>
Date:   Sat, 05 Jan 2019 16:12:37 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        David Miller <davem@...emloft.net>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: phy: ensure PHY is powered up when reading ID registers



On January 5, 2019 5:21:03 AM PST, Heiner Kallweit <hkallweit1@...il.com> wrote:
>During a bug analysis we came across the fact that there's no guarantee
>that reading from the ID registers returns a valid value if PHY is
>powered down. When reading invalid values we may load no or the wrong
>PHY driver. Therefore let's play safe and power up the PHY before
>reading the ID registers in case PHY is powered down.
>
>Suggested-by: Florian Fainelli <f.fainelli@...il.com>
>Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
>---
> drivers/net/phy/phy_device.c | 23 +++++++++++++----------
> 1 file changed, 13 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/net/phy/phy_device.c
>b/drivers/net/phy/phy_device.c
>index 9560a2b84..d4702313d 100644
>--- a/drivers/net/phy/phy_device.c
>+++ b/drivers/net/phy/phy_device.c
>@@ -754,19 +754,22 @@ static int get_phy_id(struct mii_bus *bus, int
>addr, u32 *phy_id,
> 	if (is_c45)
> 		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
> 

In premise this would also apply to C45 PHYs though not sure whether this would be on a per-package basis or global.


>+	phy_reg = mdiobus_read(bus, addr, MII_BMCR);
>+	if (phy_reg < 0)
>+		/* returning -ENODEV allows to continue bus-scanning */
>+		return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
>+
>+	/* PHY may be powered down and ID registers invalid */
>+	if (phy_reg & BMCR_PDOWN) {
>+		mdiobus_write(bus, addr, MII_BMCR, phy_reg & ~BMCR_PDOWN);
>+		/* give the PHY some time to resume */
>+		msleep(100);
>+	}

We should check the 802.3 spec and see if we can reference an upper bound being defined there, if not, I suppose 100ms is as good as any other delay.

There is one potential concern here which is that if the PHY device is probed but never used subsequently or the connect()/attach() operation is done much much longer, there is a missed opportunity for keeping the PHY in a low power mode. I would take the PHY out of power down, read its ID, then put it back to power down? Then the state machine takes care of taking it out of power down again.

Another thing to possibly watch out for is what the mii_bus::reset callback might have done to the PHY.

>+
> 	/* Grab the bits from PHYIR1, and put them in the upper half */

Typo here: MII_PHYSID1?
-- 
Florian