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 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.

-- 
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