| Message ID | E1jdabx-0005sh-T7@rmk-PC.armlinux.org.uk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Clause 45 PHY probing cleanups | expand |
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.
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.
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.
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; }
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(-)