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