The mii_bus API conversion to read_c45() and write_c45() did not cover
the mdio-mux driver before read() and write() were made C22-only.

This broke arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso.
The -EOPNOTSUPP from mdiobus_c45_read() is transformed by
get_phy_c45_devs_in_pkg() into -EIO, is further propagated to
of_mdiobus_register() and this makes the mdio-mux driver fail to probe
the entire child buses, not just the PHYs that cause access errors.

Fix the regression by introducing special c45 read and write accessors
to mdio-mux which forward the operation to the parent MDIO bus.

Fixes: db1a63aed89c ("net: phy: Remove fallback to old C45 method")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/mdio/mdio-mux.c | 45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
index a881e3523328..7d322c08c1e9 100644
--- a/drivers/net/mdio/mdio-mux.c
+++ b/drivers/net/mdio/mdio-mux.c
@@ -55,6 +55,27 @@ static int mdio_mux_read(struct mii_bus *bus, int phy_id, int regnum)
 	return r;
 }
 
+static int mdio_mux_read_c45(struct mii_bus *bus, int phy_id, int dev_addr,
+			     int regnum)
+{
+	struct mdio_mux_child_bus *cb = bus->priv;
+	struct mdio_mux_parent_bus *pb = cb->parent;
+	int r;
+
+	mutex_lock_nested(&pb->mii_bus->mdio_lock, MDIO_MUTEX_MUX);
+	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
+	if (r)
+		goto out;
+
+	pb->current_child = cb->bus_number;
+
+	r = pb->mii_bus->read_c45(pb->mii_bus, phy_id, dev_addr, regnum);
+out:
+	mutex_unlock(&pb->mii_bus->mdio_lock);
+
+	return r;
+}
+
 /*
  * The parent bus' lock is used to order access to the switch_fn.
  */
@@ -80,6 +101,28 @@ static int mdio_mux_write(struct mii_bus *bus, int phy_id,
 	return r;
 }
 
+static int mdio_mux_write_c45(struct mii_bus *bus, int phy_id, int dev_addr,
+			      int regnum, u16 val)
+{
+	struct mdio_mux_child_bus *cb = bus->priv;
+	struct mdio_mux_parent_bus *pb = cb->parent;
+
+	int r;
+
+	mutex_lock_nested(&pb->mii_bus->mdio_lock, MDIO_MUTEX_MUX);
+	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
+	if (r)
+		goto out;
+
+	pb->current_child = cb->bus_number;
+
+	r = pb->mii_bus->write_c45(pb->mii_bus, phy_id, dev_addr, regnum, val);
+out:
+	mutex_unlock(&pb->mii_bus->mdio_lock);
+
+	return r;
+}
+
 static int parent_count;
 
 static void mdio_mux_uninit_children(struct mdio_mux_parent_bus *pb)
@@ -173,6 +216,8 @@ int mdio_mux_init(struct device *dev,
 		cb->mii_bus->parent = dev;
 		cb->mii_bus->read = mdio_mux_read;
 		cb->mii_bus->write = mdio_mux_write;
+		cb->mii_bus->read_c45 = mdio_mux_read_c45;
+		cb->mii_bus->write_c45 = mdio_mux_write_c45;
 		r = of_mdiobus_register(cb->mii_bus, child_bus_node);
 		if (r) {
 			mdiobus_free(cb->mii_bus);
-- 
2.34.1

On Tue, Oct 17, 2023 at 02:32:22PM +0300, Vladimir Oltean wrote:
> The mii_bus API conversion to read_c45() and write_c45() did not cover
> the mdio-mux driver before read() and write() were made C22-only.
> 
> This broke arch/arm64/boot/dts/freescale/fsl-ls1028a-qds-13bb.dtso.
> The -EOPNOTSUPP from mdiobus_c45_read() is transformed by
> get_phy_c45_devs_in_pkg() into -EIO, is further propagated to
> of_mdiobus_register() and this makes the mdio-mux driver fail to probe
> the entire child buses, not just the PHYs that cause access errors.
> 
> Fix the regression by introducing special c45 read and write accessors
> to mdio-mux which forward the operation to the parent MDIO bus.
> 
> Fixes: db1a63aed89c ("net: phy: Remove fallback to old C45 method")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/mdio/mdio-mux.c | 45 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/mdio/mdio-mux.c b/drivers/net/mdio/mdio-mux.c
> index a881e3523328..7d322c08c1e9 100644
> --- a/drivers/net/mdio/mdio-mux.c
> +++ b/drivers/net/mdio/mdio-mux.c
> @@ -55,6 +55,27 @@ static int mdio_mux_read(struct mii_bus *bus, int phy_id, int regnum)
>  	return r;
>  }
>  
> +static int mdio_mux_read_c45(struct mii_bus *bus, int phy_id, int dev_addr,
> +			     int regnum)
> +{
> +	struct mdio_mux_child_bus *cb = bus->priv;
> +	struct mdio_mux_parent_bus *pb = cb->parent;
> +	int r;
> +
> +	mutex_lock_nested(&pb->mii_bus->mdio_lock, MDIO_MUTEX_MUX);
> +	r = pb->switch_fn(pb->current_child, cb->bus_number, pb->switch_data);
> +	if (r)
> +		goto out;
> +
> +	pb->current_child = cb->bus_number;
> +
> +	r = pb->mii_bus->read_c45(pb->mii_bus, phy_id, dev_addr, regnum);

What if the parent bus doesn't have read_c45 or write_c45 ?

> @@ -173,6 +216,8 @@ int mdio_mux_init(struct device *dev,
>  		cb->mii_bus->parent = dev;
>  		cb->mii_bus->read = mdio_mux_read;
>  		cb->mii_bus->write = mdio_mux_write;
> +		cb->mii_bus->read_c45 = mdio_mux_read_c45;
> +		cb->mii_bus->write_c45 = mdio_mux_write_c45;

Maybe make these conditional on the parent bus implementing the c45
read/write ops?

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

On Tue, Oct 17, 2023 at 02:31:30PM +0100, Russell King (Oracle) wrote:
> What if the parent bus doesn't have read_c45 or write_c45 ?

Good question. Predictably, the kernel crashes.

> Maybe make these conditional on the parent bus implementing the c45
> read/write ops?

I can do that, and send a v2 right away. Thanks for the hint.

> Maybe make these conditional on the parent bus implementing the c45
> read/write ops?

And optionally, for net-next, make the c22 read/write ops conditional
on the parent bus having C22 as well. Its a bit of a corner case, but
there are a couple of MDIO bus masters which are C45 only.

Not having C45 is however very common, so we should make this
conditional as part of the fix.

      Andrew

On Tue, Oct 17, 2023 at 03:44:58PM +0200, Andrew Lunn wrote:
> > Maybe make these conditional on the parent bus implementing the c45
> > read/write ops?
> 
> And optionally, for net-next, make the c22 read/write ops conditional
> on the parent bus having C22 as well. Its a bit of a corner case, but
> there are a couple of MDIO bus masters which are C45 only.
> 
> Not having C45 is however very common, so we should make this
> conditional as part of the fix.
> 
>       Andrew

Ok. I don't know if net-next will remain open after this patch gets
accepted and "net" gets merged back into it, so in order to not
completely forget, I'll add a patch with your suggestion to our
Layerscape SDK and that will serve as a reminder for the next
upstreaming session.

In the meantime, for this patch:

pw-bot: cr