linux-kernel - Re: [RFC net-next 0/2] Introduce MDIO probe order C45 over C22

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: <20210602235155.GA31624@linux.intel.com>
Date:   Thu, 3 Jun 2021 07:51:55 +0800
From:   Wong Vee Khee <vee.khee.wong@...ux.intel.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next 0/2] Introduce MDIO probe order C45 over C22

On Wed, Jun 02, 2021 at 05:03:52PM +0200, Andrew Lunn wrote:
> > I took a look at how most ethernet drivers implement their "bus->read"
> > function. Most of them either return -EIO or -ENODEV.
> > 
> > I think it safe to drop the return error type when we try with C45 access:
> > 
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 1539ea021ac0..282d16fdf6e1 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -870,6 +870,18 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> >         if (r)
> >                 return ERR_PTR(r);
> > 
> > +       /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
> > +        * of 0 when probed using get_phy_c22_id() with no error. Proceed to
> > +        * probe with C45 to see if we're able to get a valid PHY ID in the C45
> > +        * space, if successful, create the C45 PHY device.
> > +        */
> > +       if ((!is_c45) && (phy_id == 0)) {
> > +               r = get_phy_c45_ids(bus, addr, &c45_ids);
> > +               if (!r)
> > +                       return phy_device_create(bus, addr, phy_id,
> > +                                                true, &c45_ids);
> > +       }
> 
> This is getting better. But look at for example
> drivers/net/mdio/mdio-bcm-unimac.c. What will happen when you ask it
> to do get_phy_c45_ids()?
>

I will add an additional check for bus->probe_capabilities. This will ensure
that only a MDIO bus that is capable for C45 access will go for the 'try getting
PHY ID from C45 space' approach. Currently, only Freescale's QorIQ 10G MDIO
Controller driver and STMMAC driver has a bus->probe_capabilities of > MDIOBUS_C45.
So, I would say with this additional checking, it would not break most of the drivers:-


diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1539ea021ac0..460c0866ac84 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -870,6 +870,19 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
        if (r)
                return ERR_PTR(r);

+       /* PHY device such as the Marvell Alaska 88E2110 will return a PHY ID
+        * of 0 when probed using get_phy_c22_id() with no error. Proceed to
+        * probe with C45 to see if we're able to get a valid PHY ID in the C45
+        * space, if successful, create the C45 PHY device.
+        */
+       if ((!is_c45) && (phy_id == 0) &&
+            (bus->probe_capabilities >= MDIOBUS_C45)) {
+               r = get_phy_c45_ids(bus, addr, &c45_ids);
+               if (!r)
+                       return phy_device_create(bus, addr, phy_id,
+                                                true, &c45_ids);
+       }
+
        return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);

  VK