[PATCH] net: designware: Don't cache the MAC priv in the MDIO bus device

[PATCH] net: designware: Don't cache the MAC priv in the MDIO bus device

[Yuho Choi]

With DM_MDIO, the MDIO bus of the MAC is a child device. Its probe
method sets its own priv to the priv of the MAC, so that dw_mdio_read()
and dw_mdio_write() can reach the MAC registers through the device in
mii_dev->priv. dw_phy_init() probes this child through
dm_eth_phy_connect() when the PHY is on the MAC's own bus.

If designware_eth_probe() then fails, e.g. because no PHY answers at
the address given in the device tree, device_probe() frees the priv of
the MAC (with DM_DEVICE_REMOVE, the default). The MDIO child stays
probed, its mii_dev stays registered, and its priv still points at the
freed memory. Accesses to the bus then use that memory: the "mdio" and
"mii" commands, and another MAC whose PHY is on this bus, as on the
LicheePi 4A. If the MAC is probed again, dm_eth_phy_connect() finds the
MDIO device already active and does not probe it again, so the copy is
not updated to the new priv either.

Look up the priv of the MAC in the MDIO operations on each access
instead of copying it, and return -ENODEV while the MAC has none. Drop
the probe method that made the copy. The legacy, non-DM_MDIO bus keeps
looking up the priv of the MAC device in mii_dev->priv as before.

Fixes: 5160b4567c71 ("net: designware: add DM_MDIO support")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, on th1520_lpi4a and nanopi-r2s-rk3328 (DM_MDIO=y),
socfpga_arria10 (DM_MDIO=n) and arbel_evb (DM_MDIO=n, BITBANGMII=y),
with no new warnings in designware.c at W=1. Not tested at runtime:
there is no designware MAC in sandbox and I have no affected board.

 drivers/net/designware.c | 52 ++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/net/designware.c b/drivers/net/designware.c
index 9470804ebd1..5d4ae45ddbe 100644
--- a/drivers/net/designware.c
+++ b/drivers/net/designware.c
@@ -37,9 +37,8 @@
 #include <asm/arch/gmac.h>
 #endif
 
-static int dw_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
+static int __dw_mdio_read(struct dw_eth_dev *priv, int addr, int reg)
 {
-	struct dw_eth_dev *priv = dev_get_priv((struct udevice *)bus->priv);
 	struct eth_mac_regs *mac_p = priv->mac_regs_p;
 	ulong start;
 	u16 miiaddr;
@@ -60,10 +59,8 @@ static int dw_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
 	return -ETIMEDOUT;
 }
 
-static int dw_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
-			u16 val)
+static int __dw_mdio_write(struct dw_eth_dev *priv, int addr, int reg, u16 val)
 {
-	struct dw_eth_dev *priv = dev_get_priv((struct udevice *)bus->priv);
 	struct eth_mac_regs *mac_p = priv->mac_regs_p;
 	ulong start;
 	u16 miiaddr;
@@ -87,6 +84,21 @@ static int dw_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
 	return ret;
 }
 
+static int dw_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
+{
+	struct dw_eth_dev *priv = dev_get_priv((struct udevice *)bus->priv);
+
+	return __dw_mdio_read(priv, addr, reg);
+}
+
+static int dw_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
+			 u16 val)
+{
+	struct dw_eth_dev *priv = dev_get_priv((struct udevice *)bus->priv);
+
+	return __dw_mdio_write(priv, addr, reg, val);
+}
+
 #if CONFIG_IS_ENABLED(DM_GPIO)
 static int __dw_mdio_reset(struct udevice *dev)
 {
@@ -140,26 +152,33 @@ static int dw_bb_mdio_reset(struct mii_dev *bus)
 static int designware_eth_mdio_read(struct udevice *mdio_dev, int addr,
 				    int devad, int reg)
 {
-	struct mdio_perdev_priv *pdata = dev_get_uclass_priv(mdio_dev);
+	/* The MAC has no priv if its probe failed */
+	struct dw_eth_dev *priv = dev_get_priv(mdio_dev->parent);
+
+	if (!priv)
+		return -ENODEV;
 
-	return dw_mdio_read(pdata->mii_bus, addr, devad, reg);
+	return __dw_mdio_read(priv, addr, reg);
 }
 
 static int designware_eth_mdio_write(struct udevice *mdio_dev, int addr,
 				     int devad, int reg, u16 val)
 {
-	struct mdio_perdev_priv *pdata = dev_get_uclass_priv(mdio_dev);
+	struct dw_eth_dev *priv = dev_get_priv(mdio_dev->parent);
+
+	if (!priv)
+		return -ENODEV;
 
-	return dw_mdio_write(pdata->mii_bus, addr, devad, reg, val);
+	return __dw_mdio_write(priv, addr, reg, val);
 }
 
 #if CONFIG_IS_ENABLED(DM_GPIO)
 int designware_eth_mdio_reset(struct udevice *mdio_dev)
 {
-	struct mdio_perdev_priv *mdio_pdata = dev_get_uclass_priv(mdio_dev);
-	struct udevice *dev = mdio_pdata->mii_bus->priv;
+	if (!dev_get_priv(mdio_dev->parent))
+		return -ENODEV;
 
-	return __dw_mdio_reset(dev->parent);
+	return __dw_mdio_reset(mdio_dev->parent);
 }
 #endif
 
@@ -171,19 +190,10 @@ static __maybe_unused const struct mdio_ops designware_eth_mdio_ops = {
 #endif
 };
 
-static __maybe_unused int designware_eth_mdio_probe(struct udevice *dev)
-{
-	/* Use the priv data of parent */
-	dev_set_priv(dev, dev_get_priv(dev->parent));
-
-	return 0;
-}
-
 #if CONFIG_IS_ENABLED(DM_MDIO)
 U_BOOT_DRIVER(designware_eth_mdio) = {
 	.name = "eth_designware_mdio",
 	.id = UCLASS_MDIO,
-	.probe = designware_eth_mdio_probe,
 	.ops = &designware_eth_mdio_ops,
 	.plat_auto = sizeof(struct mdio_perdev_priv),
 };
-- 
2.43.0

Re: [PATCH] net: designware: Don't cache the MAC priv in the MDIO bus device

[Jerome Forissier]

> With DM_MDIO, the MDIO bus of the MAC is a child device. Its probe
> method sets its own priv to the priv of the MAC, so that dw_mdio_read()
> and dw_mdio_write() can reach the MAC registers through the device in
> mii_dev->priv. dw_phy_init() probes this child through
> dm_eth_phy_connect() when the PHY is on the MAC's own bus.
>
> If designware_eth_probe() then fails, e.g. because no PHY answers at
> the address given in the device tree, device_probe() frees the priv of
> the MAC (with DM_DEVICE_REMOVE, the default). The MDIO child stays
> probed, its mii_dev stays registered, and its priv still points at the
> freed memory. Accesses to the bus then use that memory: the "mdio" and
> "mii" commands, and another MAC whose PHY is on this bus, as on the
> LicheePi 4A. If the MAC is probed again, dm_eth_phy_connect() finds the
> MDIO device already active and does not probe it again, so the copy is
> not updated to the new priv either.
>
> Look up the priv of the MAC in the MDIO operations on each access
> instead of copying it, and return -ENODEV while the MAC has none. Drop
> the probe method that made the copy. The legacy, non-DM_MDIO bus keeps
> looking up the priv of the MAC device in mii_dev->priv as before.
>
> Fixes: 5160b4567c71 ("net: designware: add DM_MDIO support")
> Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
> ---
> Build-tested only, on th1520_lpi4a and nanopi-r2s-rk3328 (DM_MDIO=y),
> socfpga_arria10 (DM_MDIO=n) and arbel_evb (DM_MDIO=n, BITBANGMII=y),
> with no new warnings in designware.c at W=1. Not tested at runtime:
> there is no designware MAC in sandbox and I have no affected board.
>
>  drivers/net/designware.c | 52 ++++++++++++++++++++++++----------------
>  1 file changed, 31 insertions(+), 21 deletions(-)

Applied to u-boot-net branch for-main [1], thanks!

[1] https://git.u-boot-project.org/u-boot/custodians/u-boot-net/-/commits/for-main

[1/1] net: designware: Don't cache the MAC priv in the MDIO bus device
      commit: 0985149f0d3f35e1f3dd3212ca4decee6b127df5

Best regards,
-- 
Jerome Forissier <jerome.forissier@arm.com>