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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO-L_45iCb+TFMSqZJex-mZKfopBXxR=KH5aV4Wfx5eF5_N_8Q@mail.gmail.com>
Date: Mon, 22 Jan 2024 12:44:42 -0800
From: Tim Menninger <tmenninger@...estorage.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Vladimir Oltean <vladimir.oltean@....com>, netdev-maintainers <edumazet@...gle.com>, kuba@...nel.org, 
	pabeni@...hat.com, davem@...emloft.net, netdev <netdev@...r.kernel.org>, 
	stable@...r.kernel.org
Subject: Re: [PATCH net v1] net: dsa: mv88e6xxx: Make unsupported C45 reads
 return 0xffff

On Mon, Jan 22, 2024 at 5:39 AM Andrew Lunn <andrew@...n.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