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+ker...@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
