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: <5be909fd-59cb-c16a-41dc-e23ed0f0d345@gmail.com>
Date:   Sun, 6 Jan 2019 13:03:52 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Florian Fainelli <f.fainelli@...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 06.01.2019 01:12, Florian Fainelli wrote:
> 
> 
> 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.
> 
That's what the spec says:

BMCR Power-Down (22.2.4.1.5) 
"While in the power-down state, the PHY shall respond to management transactions."
There's no mentioning of any delay.

"Management transaction" most likely refers to everything under 22.2.4 (Management functions).
Because the ID registers are described in 22.2.4.3.1 I would interpret this in a way
that the PHY shall provide the correct ID values also in power-down.

So the question is: Do we really need to power-up the PHY before reading the ID registers?
IOW: Are you aware of any PHY not properly responding to ID register reads in power-down?


> 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.
> 
Yes, I think this should be added (in case we come to the conclusion we need this functionality at all).

> 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?
> 
I would say: yes. But PHYIR1/PHYIR2 is what is currently used in the comment.