Re: [PATCH RFC 5/7] net: phy: set devices_in_package only after validation

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Tue, May 26, 2020 at 04:50:28PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 26, 2020 at 10:39:49AM -0500, Jeremy Linton wrote:
> > Hi,
> > 
> > On 5/26/20 9:31 AM, Russell King wrote:
> > > Only set the devices_in_package to a non-zero value if we find a valid
> > > value for this field, so we avoid leaving it set to e.g. 0x1fffffff.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >   drivers/net/phy/phy_device.c | 17 ++++++++++-------
> > >   1 file changed, 10 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index fa9164ac0f3d..a483d79cfc87 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			   struct phy_c45_device_ids *c45_ids)
> > >   {
> > >   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> > > -	u32 *devs = &c45_ids->devices_in_package;
> > > +	u32 devs_in_pkg = 0;
> > >   	int i, ret, phy_reg;
> > >   	/* Find first non-zero Devices In package. Device zero is reserved
> > >   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
> > >   	 */
> > > -	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> > > +	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
> > >   		if (i >= 8) {
> > >   			/* Only probe for the devices-in-package if there
> > >   			 * is a PHY reporting as present here; this avoids
> > > @@ -750,22 +750,22 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			if (!ret)
> > >   				continue;
> > >   		}
> > > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
> > > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
> > >   		if (phy_reg < 0)
> > >   			return -EIO;
> > >   	}
> > > -	if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > > +	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> > >   		/* If mostly Fs, there is no device there, then let's probe
> > >   		 * MMD 0, as some 10G PHYs have zero Devices In package,
> > >   		 * e.g. Cortina CS4315/CS4340 PHY.
> > >   		 */
> > > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
> > > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
> > >   		if (phy_reg < 0)
> > >   			return -EIO;
> > >   		/* no device there, let's get out of here */
> > > -		if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > > +		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> > >   			*phy_id = 0xffffffff;
> > >   			return 0;
> > >   		}
> > > @@ -773,7 +773,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   	/* Now probe Device Identifiers for each device present. */
> > >   	for (i = 1; i < num_ids; i++) {
> > > -		if (!(c45_ids->devices_in_package & (1 << i)))
> > > +		if (!(devs_in_pkg & (1 << i)))
> > >   			continue;
> > >   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
> > > @@ -786,6 +786,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			return -EIO;
> > >   		c45_ids->device_ids[i] |= phy_reg;
> > >   	}
> > > +
> > > +	c45_ids->devices_in_package = devs_in_pkg;
> > > +
> > >   	*phy_id = 0;
> > >   	return 0;
> > >   }
> > > 
> > 
> > You have solved the case of 0xFFFFFFFFF devices in package, but It looks
> > like the case of devs in package = 0  still gets a successful return from
> > this path. Which can still create phys with phy_id=0 and 0 MMDs AFAIK.
> 
> Correct - I'm not looking to change the behaviour in this patch
> beyond the intended change of ensuring that c45_ids->devices_in_package
> remains untouched until we intend to return successfully.
> 
> The zero MMDs issue is an entirely orthogonal problem not addressed
> in this series.

Please note that this problem has existed back to the original addition
of clause 45 support in this commit:

commit ac28b9f8cd66d6bc54f8063df59e99abd62173a4
Author: David Daney <david.daney@cavium.com>
Date:   Wed Jun 27 07:33:35 2012 +0000

I think it only becomes a problem when we start autoprobing clause 45
PHYs.

The scanning in __mdiobus_register() does not support locating a
clause 45 PHY - it only issues clause 22 cycles.  If you are lucky
enough to have a clause 45 PHY that responds to clause 22 cycles,
then the PHY device will be created using the information in the
clause 22 registers.  get_phy_device() is called with is_c45 false.
Hence, the path above will not be used.

The only way for a clause 45 PHY to be created is explicitly via
some method - either through DT declaring a C45 PHY, or via a driver
calling get_phy_device() with is_c45 true.  There are two ethernet
drivers which do this in drivers/net, using explicit addresses
rather than autoprobing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up