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

[Tim Menninger <tmenninger@purestorage.com>]

On Mon, Jan 22, 2024 at 3:01 PM Andrew Lunn <andrew@lunn.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?