| Message ID | 20200619044759.11387-3-f.fainelli@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: MDIO bus scanning fixes | expand |
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
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? ;)
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.
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;
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(-)