| Message ID | 20200514170025.1379981-1-jeremy.linton@arm.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: Fix c45 no phy detected logic | expand |
On 14.05.2020 19:00, Jeremy Linton wrote: > The commit "disregard Clause 22 registers present bit..." clears > the low bit of the devices_in_package data which is being used > in get_phy_c45_ids() to determine if a phy/register is responding > correctly. That check is against 0x1FFFFFFF, but since the low > bit is always cleared, the check can never be true. This leads to > detecting c45 phy devices where none exist. > > Lets fix this by also clearing the low bit in the mask to 0x1FFFFFFE. > This allows us to continue to autoprobe standards compliant devices > without also gaining a large number of bogus ones. > > Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg") > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/phy/phy_device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index ac2784192472..b93d984d35cc 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > if (phy_reg < 0) > return -EIO; > > - if ((*devs & 0x1fffffff) == 0x1fffffff) { > + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { > /* If mostly Fs, there is no device there, Looks good to me, it would just be good to extend the comment and explain why we exclude bit 0 here. > * then let's continue to probe more, as some > * 10G PHYs have zero Devices In package, > @@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > if (phy_reg < 0) > return -EIO; > /* no device there, let's get out of here */ > - if ((*devs & 0x1fffffff) == 0x1fffffff) { > + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { > *phy_id = 0xffffffff; > return 0; > } else { >
Hi, On 5/14/20 2:59 PM, Heiner Kallweit wrote: > On 14.05.2020 19:00, Jeremy Linton wrote: >> The commit "disregard Clause 22 registers present bit..." clears >> the low bit of the devices_in_package data which is being used >> in get_phy_c45_ids() to determine if a phy/register is responding >> correctly. That check is against 0x1FFFFFFF, but since the low >> bit is always cleared, the check can never be true. This leads to >> detecting c45 phy devices where none exist. >> >> Lets fix this by also clearing the low bit in the mask to 0x1FFFFFFE. >> This allows us to continue to autoprobe standards compliant devices >> without also gaining a large number of bogus ones. >> >> Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg") >> Cc: Heiner Kallweit <hkallweit1@gmail.com> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> drivers/net/phy/phy_device.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c >> index ac2784192472..b93d984d35cc 100644 >> --- a/drivers/net/phy/phy_device.c >> +++ b/drivers/net/phy/phy_device.c >> @@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >> if (phy_reg < 0) >> return -EIO; >> >> - if ((*devs & 0x1fffffff) == 0x1fffffff) { >> + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { >> /* If mostly Fs, there is no device there, > > Looks good to me, it would just be good to extend the comment and explain > why we exclude bit 0 here. Sure, sounds good. > > >> * then let's continue to probe more, as some >> * 10G PHYs have zero Devices In package, >> @@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, >> if (phy_reg < 0) >> return -EIO; >> /* no device there, let's get out of here */ >> - if ((*devs & 0x1fffffff) == 0x1fffffff) { >> + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { >> *phy_id = 0xffffffff; >> return 0; >> } else { >> >
Hi, On 5/14/20 12:00 PM, Jeremy Linton wrote: > The commit "disregard Clause 22 registers present bit..." clears > the low bit of the devices_in_package data which is being used > in get_phy_c45_ids() to determine if a phy/register is responding > correctly. That check is against 0x1FFFFFFF, but since the low > bit is always cleared, the check can never be true. This leads to > detecting c45 phy devices where none exist. > > Lets fix this by also clearing the low bit in the mask to 0x1FFFFFFE. > This allows us to continue to autoprobe standards compliant devices > without also gaining a large number of bogus ones. So, I've been reworking the c45 ID detection logic, with an aim to hinting to the scanner that it should fallback to c22 for a given phy address (as well as giving it some additional standardized areas to probe for phy ids). It turns out that the c22 registers present bit is a pretty useful signal that this needs to happen. So, I think this patch really should move the BIT(0) sanitation after the MMD detection loop in get_phy_c45_ids(). But having dug into this code for a while now, I'm hard pressed to understand the case that the original 3b5e74e0afe3 commit fixed. The only thing I can see is that the "bug" i'm fixing here was intentionally creating bogus phy nodes when the MMDs weren't responding. Thanks, > > Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg") > Cc: Heiner Kallweit <hkallweit1@gmail.com> > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > drivers/net/phy/phy_device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index ac2784192472..b93d984d35cc 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > if (phy_reg < 0) > return -EIO; > > - if ((*devs & 0x1fffffff) == 0x1fffffff) { > + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { > /* If mostly Fs, there is no device there, > * then let's continue to probe more, as some > * 10G PHYs have zero Devices In package, > @@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, > if (phy_reg < 0) > return -EIO; > /* no device there, let's get out of here */ > - if ((*devs & 0x1fffffff) == 0x1fffffff) { > + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { > *phy_id = 0xffffffff; > return 0; > } else { >
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index ac2784192472..b93d984d35cc 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -723,7 +723,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, if (phy_reg < 0) return -EIO; - if ((*devs & 0x1fffffff) == 0x1fffffff) { + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { /* If mostly Fs, there is no device there, * then let's continue to probe more, as some * 10G PHYs have zero Devices In package, @@ -733,7 +733,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id, if (phy_reg < 0) return -EIO; /* no device there, let's get out of here */ - if ((*devs & 0x1fffffff) == 0x1fffffff) { + if ((*devs & 0x1ffffffe) == 0x1ffffffe) { *phy_id = 0xffffffff; return 0; } else {
The commit "disregard Clause 22 registers present bit..." clears the low bit of the devices_in_package data which is being used in get_phy_c45_ids() to determine if a phy/register is responding correctly. That check is against 0x1FFFFFFF, but since the low bit is always cleared, the check can never be true. This leads to detecting c45 phy devices where none exist. Lets fix this by also clearing the low bit in the mask to 0x1FFFFFFE. This allows us to continue to autoprobe standards compliant devices without also gaining a large number of bogus ones. Fixes: 3b5e74e0afe3 ("net: phy: disregard "Clause 22 registers present" bit in get_phy_c45_devs_in_pkg") Cc: Heiner Kallweit <hkallweit1@gmail.com> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- drivers/net/phy/phy_device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)