Re: [PATCH net v1] net: dsa: mv88e6xxx: Make unsupported C45 reads return 0xffff

[Tim Menninger <tmenninger@purestorage.com>]

On Mon, Jan 22, 2024 at 5:39 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Jan 22, 2024 at 02:24:57PM +0200, Vladimir Oltean wrote:
> > Hi Andrew,
> >
> > On Sat, Jan 20, 2024 at 08:21:25PM +0100, Andrew Lunn wrote:
> > > When there is no device on the bus for a given address, the pull up
> > > resistor on the data line results in the read returning 0xffff. The
> > > phylib core code understands this when scanning for devices on the
> > > bus, and a number of MDIO bus masters make use of this as a way to
> > > indicate they cannot perform the read.
> > >
> > > Make us of this as a minimal fix for stable where the mv88e6xxx
> >
> > s/us/use/
> >
> > Also, what is the "proper" fix if this is the minimal one for stable?
>
> Hi Vladimir
>
> I have a patchset for net-next, once it opens. I looked at how C22 and
> C45 differ in handling error codes. C22 allows the MDIO bus driver to
> return -ENODEV to indicate its impossible for a device to be at a
> given address. The scan code then skips that address and continues to
> the next address. Current C45 code would turn that -ENODEV into an
> -EIO and consider it fatal. So i change the C45 code to allow for
> -ENODEV in the same way, and change the mv88e6xxx driver to return
> -ENODEV if there are is no C45 read op.
>
> Since making the handling of the error codes uniform is more than a
> simple fix, i decided on a minimal fix for net.
>
> Thanks for the comments on the commit message, i will address them
> soon.
>
>         Andrew

I'm not sure I fully agree with returning 0xffff here, and especially not
for just one of the four functions (reads and writes, c22 and c45). If the
end goal is to unify error handling, what if we keep the return values as
they are, i.e. continue to return -EOPNOTSUPP, and then in get_phy_c22_id
and get_phy_c45_ids on error we do something like:

    return (phy_reg == -EIO || phy_reg == -ENODEV || phy_reg == -EOPNOTSUPP)
        ? -ENODEV : -EIO;

So the diff looks something like (just getting a point across, haven't
tried or style checked this)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3611ea64875e..f21f07f33f06 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -758,12 +758,14 @@ static int get_phy_c45_devs_in_pkg(struct
mii_bus *bus, int addr, int dev_addr,

        phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
        if (phy_reg < 0)
-               return -EIO;
+               return (phy_reg == -EIO || phy_reg == -ENODEV ||
phy_reg == -EOPNOTSUPP)
+                       ? -ENODEV : -EIO;
        *devices_in_package = phy_reg << 16;

        phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1);
        if (phy_reg < 0)
-               return -EIO;
+               return (phy_reg == -EIO || phy_reg == -ENODEV ||
phy_reg == -EOPNOTSUPP)
+                       ? -ENODEV : -EIO;
        *devices_in_package |= phy_reg;

        return 0;
@@ -882,7 +884,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int
addr, u32 *phy_id)
        phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
        if (phy_reg < 0) {
                /* returning -ENODEV doesn't stop bus scanning */
-               return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
+               return (phy_reg == -EIO || phy_reg == -ENODEV ||
phy_reg == -EOPNOTSUPP)
+                       ? -ENODEV : -EIO;
        }

        *phy_id = phy_reg << 16;
@@ -891,7 +894,8 @@ static int get_phy_c22_id(struct mii_bus *bus, int
addr, u32 *phy_id)
        phy_reg = mdiobus_read(bus, addr, MII_PHYSID2);
        if (phy_reg < 0) {
                /* returning -ENODEV doesn't stop bus scanning */
-               return (phy_reg == -EIO || phy_reg == -ENODEV) ? -ENODEV : -EIO;
+               return (phy_reg == -EIO || phy_reg == -ENODEV ||
phy_reg == -EOPNOTSUPP)
+                       ? -ENODEV : -EIO;
        }

        *phy_id |= phy_reg;

This might even resemble what you had in mind in your initial feedback...

Tim