Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 14:04:43 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Michael Walle <michael@walle.cc>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "David S . Miller" <davem@davemloft.net>,
        Xu Liang <lxu@maxlinear.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag
Message-ID: <YkWm67iM19V5C0Ln@shell.armlinux.org.uk>
References: <20220323183419.2278676-5-michael@walle.cc>
 <Yjt99k57mM5PQ8bT@lunn.ch>
 <8304fb3578ee38525a158af768691e75@walle.cc>
 <Yju+SGuZ9aB52ARi@lunn.ch>
 <30012bd8256be3be9977bd15d1486c84@walle.cc>
 <YjybB/fseibDU4dT@lunn.ch>
 <0d4a2654acd2cc56f7b17981bf14474e@walle.cc>
 <Yjy+oCLdnu3FrNp+@lunn.ch>
 <YkWVku32gbaBAdEH@shell.armlinux.org.uk>
 <YkWZOEGwLcD6xYKu@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YkWZOEGwLcD6xYKu@lunn.ch>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Thu, Mar 31, 2022 at 02:06:16PM +0200, Andrew Lunn wrote:
> On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:
> > On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:
> > > > The
> > > > > only valid case i can think of is for a very oddball PHY which has C45
> > > > > register space, but cannot actually do C45 transfers, and so C45 over
> > > > > C22 is the only option.
> > > > 
> > > > And how would you know that the PHY has the needed registers in c22
> > > > space? Or do we assume that every C45 PHY has these registers?
> > > 
> > > I think it is a reasonable assumption at the moment. We have around
> > > 170 MDIO bus masters in Linux. All but one can do C22.
> > 
> > I don't think that is correct. I'm aware of the Marvell XMDIO driver
> > that is C45 only, and also xgene's non-rgmii "xfi" variant which is
> > also C45 only. Note that the xfi variant doesn't reject C22 and makes
> > no distinction between a C22 and C45 access (so a C22 access to
> > phy_id = 0 reg = 0 hits C45 phy_id = 0 mmd 0 reg 0.
> > 
> > MDIO drivers are IMHO an utter mess and are in dire need of fixing...
> > and I'm coming to the conclusion that the bodge of passing both C22
> > and C45 accesses through the same read/write functions is a huge
> > mistake, one that is crying out for fixing to prevent more prolification
> > of this kind of mess.
> > 
> > Yes, it's a lot of work, but I think it needs to be done. Retrofitting
> > the MDIO drivers with checks etc sounds nice, but if we assume that
> > patches will continue to be applied to net-next with little review,
> > we have a losing battle - it would be better to have interfaces designed
> > to make this kind of mistake impossible.
> 
> Hi Russell
> 
> So what i think you are saying is change the mii_bus structure:
> 
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 36ca2b5c2253..26322ee23867 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -353,10 +353,15 @@ struct mii_bus {
>         const char *name;
>         char id[MII_BUS_ID_SIZE];
>         void *priv;
> -       /** @read: Perform a read transfer on the bus */
> -       int (*read)(struct mii_bus *bus, int addr, int regnum);
> -       /** @write: Perform a write transfer on the bus */
> -       int (*write)(struct mii_bus *bus, int addr, int regnum, u16 val);
> +       /** @read: Perform a C22 read transfer on the bus */
> +       int (*read_c22)(struct mii_bus *bus, int addr, int regnum);
> +       /** @write: Perform a C22 write transfer on the bus */
> +       int (*write_c22)(struct mii_bus *bus, int addr, int regnum, u16 val);
> +       /** @read: Perform a C45 read transfer on the bus */
> +       int (*read_c45)(struct mii_bus *bus, int addr, int devnum, int regnum);
> +       /** @write: Perform a C45 write transfer on the bus */
> +       int (*write_c45)(struct mii_bus *bus, int addr, int devnum,
> +                        int regnum, u16 val);
>         /** @reset: Perform a reset of the bus */
>         int (*reset)(struct mii_bus *bus);
> 
> This way we get a cleaner interface, and the compiler helping us
> finding drivers we miss during conversion?

Yes, and not only does it mean the compiler helps us find unconverted
drivers, it will also stop new drivers that use the old interfaces being
merged and most importantly it puts an end to the ongoing question about
which clause accesses are actually supported by a MDIO driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!