[PATCH net 0/2] net: phy: MDIO bus scanning fixes

[PATCH net 0/2] net: phy: MDIO bus scanning fixes

[Florian Fainelli]

Hi all,

This patch series fixes two problems with the current MDIO bus scanning
logic which was identified while moving from 4.9 to 5.4 on devices that
do rely on scanning the MDIO bus at runtime because they use pluggable
cards.

Florian Fainelli (2):
  of: of_mdio: Correct loop scanning logic
  net: phy: Check harder for errors in get_phy_id()

 drivers/net/phy/phy_device.c | 6 ++++--
 drivers/of/of_mdio.c         | 5 +++--
 2 files changed, 7 insertions(+), 4 deletions(-)

-- 
2.17.1

[PATCH net 1/2] of: of_mdio: Correct loop scanning logic

[Florian Fainelli]

Commit 209c65b61d94 ("drivers/of/of_mdio.c:fix of_mdiobus_register()")
introduced a break of the loop on the premise that a successful
registration should exit the loop. The premise is correct but not to
code, because rc && rc != -ENODEV is just a special error condition,
that means we would exit the loop even with rc == -ENODEV which is
absolutely not correct since this is the error code to indicate to the
MDIO bus layer that scanning should continue.

Fix this by explicitly checking for rc = 0 as the only valid condition
to break out of the loop.

Fixes: 209c65b61d94 ("drivers/of/of_mdio.c:fix of_mdiobus_register()")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/of/of_mdio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index a04afe79529c..7496dc64d6b5 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -315,9 +315,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 
 			if (of_mdiobus_child_is_phy(child)) {
 				rc = of_mdiobus_register_phy(mdio, child, addr);
-				if (rc && rc != -ENODEV)
+				if (!rc)
+					break;
+				if (rc != -ENODEV)
 					goto unregister;
-				break;
 			}
 		}
 	}
-- 
2.17.1

Re: [PATCH net 1/2] of: of_mdio: Correct loop scanning logic

[Andrew Lunn]

On Thu, Jun 18, 2020 at 09:47:58PM -0700, Florian Fainelli wrote:
> Commit 209c65b61d94 ("drivers/of/of_mdio.c:fix of_mdiobus_register()")
> introduced a break of the loop on the premise that a successful
> registration should exit the loop. The premise is correct but not to
> code, because rc && rc != -ENODEV is just a special error condition,
> that means we would exit the loop even with rc == -ENODEV which is
> absolutely not correct since this is the error code to indicate to the
> MDIO bus layer that scanning should continue.
> 
> Fix this by explicitly checking for rc = 0 as the only valid condition
> to break out of the loop.
> 
> Fixes: 209c65b61d94 ("drivers/of/of_mdio.c:fix of_mdiobus_register()")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/of/of_mdio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index a04afe79529c..7496dc64d6b5 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -315,9 +315,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  
>  			if (of_mdiobus_child_is_phy(child)) {
>  				rc = of_mdiobus_register_phy(mdio, child, addr);
> -				if (rc && rc != -ENODEV)
> +				if (!rc)
> +					break;

Maybe add in a comment here about what ENODEV means in this context?
That might avoid it getting broken again in the future.

> +				if (rc != -ENODEV)
>  					goto unregister;
> -				break;
>  			}
>  		}
>  	}
> -- 

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net 2/2] net: phy: Check harder for errors in get_phy_id()

[Florian Fainelli]

Commit 02a6efcab675 ("net: phy: allow scanning busses with missing
phys") added a special condition to return -ENODEV in case -ENODEV or
-EIO was returned from the first read of the MII_PHYSID1 register.

In case the MDIO bus data line pull-up is not strong enough, the MDIO
bus controller will not flag this as a read error. This can happen when
a pluggable daughter card is not connected and weak internal pull-ups
are used (since that is the only option, otherwise the pins are
floating).

The second read of MII_PHYSID2 will be correctly flagged an error
though, but now we will return -EIO which will be treated as a hard
error, thus preventing MDIO bus scanning loops to continue succesfully.

Apply the same logic to both register reads, thus allowing the scanning
logic to proceed.

Fixes: 02a6efcab675 ("net: phy: allow scanning busses with missing phys")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 04946de74fa0..85ba95b598b5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -794,8 +794,10 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
 
 	/* Grab the bits from PHYIR2, and put them in the lower half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
-	if (phy_reg < 0)
-		return -EIO;
+	if (phy_reg < 0) {
+		/* returning -ENODEV doesn't stop bus scanning */
+		return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
+	}
 
 	*phy_id |= phy_reg;
 
-- 
2.17.1

Re: [PATCH net 2/2] net: phy: Check harder for errors in get_phy_id()

[Andrew Lunn]

On Thu, Jun 18, 2020 at 09:47:59PM -0700, Florian Fainelli wrote:
> Commit 02a6efcab675 ("net: phy: allow scanning busses with missing
> phys") added a special condition to return -ENODEV in case -ENODEV or
> -EIO was returned from the first read of the MII_PHYSID1 register.
> 
> In case the MDIO bus data line pull-up is not strong enough, the MDIO
> bus controller will not flag this as a read error. This can happen when
> a pluggable daughter card is not connected and weak internal pull-ups
> are used (since that is the only option, otherwise the pins are
> floating).
> 
> The second read of MII_PHYSID2 will be correctly flagged an error
> though, but now we will return -EIO which will be treated as a hard
> error, thus preventing MDIO bus scanning loops to continue succesfully.
> 
> Apply the same logic to both register reads, thus allowing the scanning
> logic to proceed.

Hi Florian

Maybe extend the kerneldoc for this function to document the return
values and there special meanings?

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

BTW: Did you look at get_phy_c45_ids()? Is it using the correct return
value? Given the current work being done to extend scanning to C45,
maybe it needs reviewing for issues like this.

    Andrew

Re: [PATCH net 2/2] net: phy: Check harder for errors in get_phy_id()

[Russell King - ARM Linux admin]

On Fri, Jun 19, 2020 at 03:26:59PM +0200, Andrew Lunn wrote:
> On Thu, Jun 18, 2020 at 09:47:59PM -0700, Florian Fainelli wrote:
> > Commit 02a6efcab675 ("net: phy: allow scanning busses with missing
> > phys") added a special condition to return -ENODEV in case -ENODEV or
> > -EIO was returned from the first read of the MII_PHYSID1 register.
> > 
> > In case the MDIO bus data line pull-up is not strong enough, the MDIO
> > bus controller will not flag this as a read error. This can happen when
> > a pluggable daughter card is not connected and weak internal pull-ups
> > are used (since that is the only option, otherwise the pins are
> > floating).
> > 
> > The second read of MII_PHYSID2 will be correctly flagged an error
> > though, but now we will return -EIO which will be treated as a hard
> > error, thus preventing MDIO bus scanning loops to continue succesfully.
> > 
> > Apply the same logic to both register reads, thus allowing the scanning
> > logic to proceed.
> 
> Hi Florian
> 
> Maybe extend the kerneldoc for this function to document the return
> values and there special meanings?

You mean like the patch I sent yesterday?

> BTW: Did you look at get_phy_c45_ids()? Is it using the correct return
> value? Given the current work being done to extend scanning to C45,
> maybe it needs reviewing for issues like this.

And the updates I sent for this yesterday? ;)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 2/2] net: phy: Check harder for errors in get_phy_id()

[Florian Fainelli]

On 6/19/2020 6:30 AM, Russell King - ARM Linux admin wrote:
> On Fri, Jun 19, 2020 at 03:26:59PM +0200, Andrew Lunn wrote:
>> On Thu, Jun 18, 2020 at 09:47:59PM -0700, Florian Fainelli wrote:
>>> Commit 02a6efcab675 ("net: phy: allow scanning busses with missing
>>> phys") added a special condition to return -ENODEV in case -ENODEV or
>>> -EIO was returned from the first read of the MII_PHYSID1 register.
>>>
>>> In case the MDIO bus data line pull-up is not strong enough, the MDIO
>>> bus controller will not flag this as a read error. This can happen when
>>> a pluggable daughter card is not connected and weak internal pull-ups
>>> are used (since that is the only option, otherwise the pins are
>>> floating).
>>>
>>> The second read of MII_PHYSID2 will be correctly flagged an error
>>> though, but now we will return -EIO which will be treated as a hard
>>> error, thus preventing MDIO bus scanning loops to continue succesfully.
>>>
>>> Apply the same logic to both register reads, thus allowing the scanning
>>> logic to proceed.
>>
>> Hi Florian
>>
>> Maybe extend the kerneldoc for this function to document the return
>> values and there special meanings?
> 
> You mean like the patch I sent yesterday?
> 
>> BTW: Did you look at get_phy_c45_ids()? Is it using the correct return
>> value? Given the current work being done to extend scanning to C45,
>> maybe it needs reviewing for issues like this.
> 
> And the updates I sent for this yesterday? ;)

When Russell's patches land, they will address this correctly and
because I did not want to introduce any conflicts, this is not addressed
by this two patch series.
-- 
Florian