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_46Ltq0Ju_BO+rfvAbe7F=T6m0hZZKu9gzv7=bMV5n6naw@mail.gmail.com>
Date: Tue, 23 Jan 2024 07:27:27 -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 3:01 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> > 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;
>
> As i said to Vladimir, what i posted so far is just a minimal fix for
> stable. After that, i have two patches for net-next, which are the
> full, clean fix. And the first patch is similar to what you suggest:
>
> +++ b/drivers/net/phy/phy_device.c
> @@ -780,7 +780,7 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
>   * and identifiers in @c45_ids.
>   *
>   * Returns zero on success, %-EIO on bus access error, or %-ENODEV if
> - * the "devices in package" is invalid.
> + * the "devices in package" is invalid or no device responds.
>   */
>  static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>                            struct phy_c45_device_ids *c45_ids)
> @@ -803,7 +803,10 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>                          */
>                         ret = phy_c45_probe_present(bus, addr, i);
>                         if (ret < 0)
> -                               return -EIO;
> +                               /* returning -ENODEV doesn't stop bus
> +                                * scanning */
> +                               return (phy_reg == -EIO ||
> +                                       phy_reg == -ENODEV) ? -ENODEV : -EIO;
>
>                         if (!ret)
>                                 continue;
>
> This makes C22 and C45 handling of -ENODEV the same.
>
> I then have another patch which changed mv88e6xxx to return -ENODEV.
> I cannot post the net-next patches for merging until the net patch is
> accepted and then merged into net-next.
>
>   Andrew

Does that mean if there's a device there but it doesn't support C45 (no
phy_read_c45), it will now return ENODEV?

I suppose that's my only nit but at the end of the day I'm not unhappy with it.

Thank you for taking the time to look at this with me. Is there anything
else you need from me?