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

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

[Heiner Kallweit]

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@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);
 
+	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);
+	}
+
 	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
-	if (phy_reg < 0) {
-		/* if there is no device, return without an error so scanning
-		 * the bus works properly
-		 */
-		if (phy_reg == -EIO || phy_reg == -ENODEV) {
-			*phy_id = 0xffffffff;
-			return 0;
-		}
-
+	if (phy_reg < 0)
 		return -EIO;
-	}
 
 	*phy_id = (phy_reg & 0xffff) << 16;
 
-- 
2.20.1

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

[Heiner Kallweit]

On 05.01.2019 14:21, Heiner Kallweit 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.
> 
Fixes: 02a6efcab675 ("net: phy: allow scanning busses with missing phys")

> Suggested-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);
>  
> +	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);
> +	}
> +
>  	/* Grab the bits from PHYIR1, and put them in the upper half */
>  	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
> -	if (phy_reg < 0) {
> -		/* if there is no device, return without an error so scanning
> -		 * the bus works properly
> -		 */
> -		if (phy_reg == -EIO || phy_reg == -ENODEV) {
> -			*phy_id = 0xffffffff;
> -			return 0;
> -		}
> -
> +	if (phy_reg < 0)
>  		return -EIO;
> -	}
>  
>  	*phy_id = (phy_reg & 0xffff) << 16;
>  
> 

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

[Andrew Lunn]

On Sat, Jan 05, 2019 at 02:21:03PM +0100, Heiner Kallweit 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@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);

Hi Heiner

>  
> +	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;

It would be better to copy the code verbatim. I know we have had
issues with scanning depending on the return value. So changing the
return value should be in a separate patch. I would also not make it a
fix, but something for net-next, so it gets more testing.

> +
> +	/* 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);
> +	}
> +

Is this potentially slowing the scan down to 100ms * 32, if the read
of MII_BMCR always returns 0xffff?

Thanks
	Andrew

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

[Heiner Kallweit]

On 05.01.2019 18:33, Andrew Lunn wrote:
> On Sat, Jan 05, 2019 at 02:21:03PM +0100, Heiner Kallweit 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@gmail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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);
> 
> Hi Heiner
> 
>>  
>> +	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;
> 
> It would be better to copy the code verbatim. I know we have had
> issues with scanning depending on the return value. So changing the
> return value should be in a separate patch. I would also not make it a
> fix, but something for net-next, so it gets more testing.
> 
The only caller of get_phy_id() is get_phy_device() which translates
a PHY ID of 0xffffffff back to -ENODEV. So this should be safe and
it avoids some overhead. But right, this is more net-next stuff.

Regarding net vs. net-next:
Quite a few of the latest net commits don't meet the strict criteria
for a fix (as documented). Means: The risk that a problem could
occur isn't sufficient, at least one user has to actually face a
problem. So it seems net vs. net-next criteria is somewhat flexible.
Therefore I wasn't sure in this case.

>> +
>> +	/* 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);
>> +	}
>> +
> 
> Is this potentially slowing the scan down to 100ms * 32, if the read
> of MII_BMCR always returns 0xffff?
> 
Indeed, in a worst case scenario this could happen, provided that:
- there's no mask of MDIO addresses to probe
- the MDIO bus read operation returns 0xffff in case of an error instead
  of a proper errno. To mitigate this we could treat 0xffff as failure
  because normally BMCR can never have this value.

> Thanks
> 	Andrew
> 
Heiner

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 5 Jan 2019 22:07:50 +0100

> Regarding net vs. net-next:
> Quite a few of the latest net commits don't meet the strict criteria
> for a fix (as documented). Means: The risk that a problem could
> occur isn't sufficient, at least one user has to actually face a
> problem. So it seems net vs. net-next criteria is somewhat flexible.
> Therefore I wasn't sure in this case.

Sometimes I can, and often do, make a judgement call.

I try to consider what value there is in deferring a change out to
net-next, and if I honestly don't see one I tend to accept it as a
'net' change.

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

[Andrew Lunn]

> Quite a few of the latest net commits don't meet the strict criteria
> for a fix (as documented). Means: The risk that a problem could
> occur isn't sufficient, at least one user has to actually face a
> problem. So it seems net vs. net-next criteria is somewhat flexible.
> Therefore I wasn't sure in this case.

Hi Heiner

I tend to be more paranoid after listening to recent discussions about
this. At LPC and online, there have been comments that patches to
stable are more likely to break something than patches going via the
normal merge window. Normal patches get a lot more testing, are in -rc
kernels for 6 or more weeks, etc, where as stable patches go live
after a week or less of testing.

> > Is this potentially slowing the scan down to 100ms * 32, if the read
> > of MII_BMCR always returns 0xffff?
> > 
> Indeed, in a worst case scenario this could happen, provided that:
> - there's no mask of MDIO addresses to probe

Which is pretty normal

> - the MDIO bus read operation returns 0xffff in case of an error instead
>   of a proper errno.

The bus driver cannot easily return an proper errno. The MDIO protocol
mostly has no way to indicate if there is a device on the bus. There
is the TA bit, but many PHYs are broken, so most bus drivers just
ignore it. And the bus is wired with a pull-up. So a read on an
address with no device will return 0xffff.

	Andrew

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

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Sat, 5 Jan 2019 23:04:12 +0100

> I tend to be more paranoid after listening to recent discussions about
> this. At LPC and online, there have been comments that patches to
> stable are more likely to break something than patches going via the
> normal merge window. Normal patches get a lot more testing, are in -rc
> kernels for 6 or more weeks, etc, where as stable patches go live
> after a week or less of testing.

Well, with networking it's a little bit different.

I do let patches "cook" in my stable queue before sending them off.
Sometimes for a couple weeks.

And yes, very often, follow-on fixups for a change show up during that
time.

This is primarily why I handle -stable submission in this way, and
therefore I completely agree with your concerns.

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

[Florian Fainelli]

On January 5, 2019 5:21:03 AM PST, Heiner Kallweit <hkallweit1@gmail.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@gmail.com>
>Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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

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

[Heiner Kallweit]

On 06.01.2019 01:12, Florian Fainelli wrote:
> 
> 
> On January 5, 2019 5:21:03 AM PST, Heiner Kallweit <hkallweit1@gmail.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@gmail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.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.

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

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Sat, 5 Jan 2019 14:21:03 +0100

> 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@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

There are several concerns still being discussed with this patch.

I personally don't like the idea that this can leave the PHY powered up
fully on a misprobe, and the unexpected power draw that might create.

Anyways, I'm marking this as changes requested.  If anything, some more
discussion should be added to the commit message even if the patch itself
remains the same.

There has been a lot of enlightening information in that thread, and at
least in a condensed state that needs to end up in the commit message.

Thanks.