Re: [PATCH RFC net-next 4/5] net: phy: introduce is_c45_over_c22 flag

On Thu, Mar 31, 2022 at 12:50:42PM +0100, Russell King (Oracle) wrote:

quoted

On Thu, Mar 24, 2022 at 07:55:28PM +0100, Andrew Lunn wrote:

quoted

quoted

The

quoted

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?