Re: [RFC 04/11] net: phy: Handle c22 regs presence better

[Jeremy Linton]

Hi,

On 5/23/20 1:37 PM, Russell King - ARM Linux admin wrote:
> On Fri, May 22, 2020 at 04:30:52PM -0500, Jeremy Linton wrote:
> > Until this point, we have been sanitizing the c22
> > regs presence bit out of all the MMD device lists.
> > This is incorrect as it causes the 0xFFFFFFFF checks
> > to incorrectly fail. Further, it turns out that we
> > want to utilize this flag to make a determination that
> > there is actually a phy at this location and we should
> > be accessing it using c22.
> >
> > Signed-off-by: Jeremy Linton <jeremy.linton@xxxxxxx>
> > ---
> >   drivers/net/phy/phy_device.c | 16 +++++++++++++---
> >   1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index f0761fa5e40b..2d677490ecab 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -689,9 +689,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
> >   		return -EIO;
> >   	*devices_in_package |= phy_reg;
> >   
> > -	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > -	*devices_in_package &= ~BIT(0);
> > -
> >   	return 0;
> >   }
> >   
> > @@ -742,6 +739,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   	int i;
> >   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> >   	u32 *devs = &c45_ids->devices_in_package;
> > +	bool c22_present = false;
> > +	bool valid_id = false;
> >   
> >   	/* Find first non-zero Devices In package. Device zero is reserved
> >   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
> > @@ -770,6 +769,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   		return 0;
> >   	}
> >   
> > +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > +	c22_present = *devs & BIT(0);
> > +	*devs &= ~BIT(0);
> > +
> >   	/* Now probe Device Identifiers for each device present. */
> >   	for (i = 1; i < num_ids; i++) {
> >   		if (!(c45_ids->devices_in_package & (1 << i)))
> > @@ -778,6 +781,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   		ret = _get_phy_id(bus, addr, i, &c45_ids->device_ids[i], true);
> >   		if (ret < 0)
> >   			return ret;
> > +		if (valid_phy_id(c45_ids->device_ids[i]))
> > +			valid_id = true;
>
> Here you are using your "devices in package" validator to validate the
> PHY ID value.  One of the things it does is mask this value with
> 0x1fffffff.  That means you lose some of the vendor OUI.  To me, this
> looks completely wrong.

I think in this case I was just using it like the comment in 
get_phy_device() "if the phy_id is mostly F's, there is no device here".

My understanding is that the code is trying to avoid the 0xFFFFFFFF 
returns that seem to indicate "bus ok, phy didn't respond".

I just checked the OUI registration, and while there are a couple OUI's 
registered that have a number of FFF's in them, none of those cases 
seems to overlap sufficiently to cause this to throw them out. Plus a 
phy would also have to have model+revision set to 'F's. So while might 
be possible, if unlikely, at the moment I think the OUI registration 
keeps this from being a problem. Particularly, if i'm reading the 
mapping correctly, the OUI mapping guarantees that the field cannot be 
all '1's due to the OUI having X & M bits cleared. It sort of looks like 
the mapping is trying to lose those bits, by tossing bit 1 & 2, but the 
X & M are in the wrong octet (AFAIK, I just read it three times cause it 
didn't make any sense).