[PATCH 1/3] net: xilinx: fix load access fault
From: Greentime Hu greentime.hu@sifive.com
phy_connect() may fail by returning a NULL pointer. Thus, axiemac_phy_init() should handle the case or we may get an access fault when it tries to dereference it.
Signed-off-by: Greentime Hu greentime.hu@sifive.com Reviewed-by: Andy Chiu andy.chiu@sifive.com Signed-off-by: Andy Chiu andy.chiu@sifive.com ---
drivers/net/xilinx_axi_emac.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c index 2ec76d0f52..3117dae05e 100644 --- a/drivers/net/xilinx_axi_emac.c +++ b/drivers/net/xilinx_axi_emac.c @@ -312,6 +312,10 @@ static int axiemac_phy_init(struct udevice *dev)
/* Interface - look at tsec */ phydev = phy_connect(priv->bus, priv->phyaddr, dev, priv->interface); + if (!phydev) { + printf("phy_connect failed\n"); + return -ENODEV; + }
phydev->supported &= supported; phydev->advertising = phydev->supported;
Separate the flow out so that it would be easiler to implement error handling logic.
Signed-off-by: Andy Chiu andy.chiu@sifive.com ---
drivers/net/xilinx_axi_emac.c | 50 +++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 20 deletions(-)
diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c index 3117dae05e..dbda6c70d8 100644 --- a/drivers/net/xilinx_axi_emac.c +++ b/drivers/net/xilinx_axi_emac.c @@ -763,38 +763,48 @@ static int axiemac_miiphy_write(struct mii_dev *bus, int addr, int devad, return phywrite(bus->priv, addr, reg, value); }
-static int axi_emac_probe(struct udevice *dev) +static int axiemac_setup_emac(struct udevice *dev) { struct axidma_plat *plat = dev_get_plat(dev); struct eth_pdata *pdata = &plat->eth_pdata; struct axidma_priv *priv = dev_get_priv(dev); int ret;
+ priv->eth_hasnobuf = plat->eth_hasnobuf; + priv->phyaddr = plat->phyaddr; + priv->phy_of_handle = plat->phy_of_handle; + priv->interface = pdata->phy_interface; + + priv->bus = mdio_alloc(); + priv->bus->read = axiemac_miiphy_read; + priv->bus->write = axiemac_miiphy_write; + priv->bus->priv = priv; + + ret = mdio_register_seq(priv->bus, dev_seq(dev)); + if (ret) + return ret; + + axiemac_phy_init(dev); + + return ret; +} + +static int axi_emac_probe(struct udevice *dev) +{ + struct axidma_plat *plat = dev_get_plat(dev); + struct eth_pdata *pdata = &plat->eth_pdata; + struct axidma_priv *priv = dev_get_priv(dev); + priv->iobase = (struct axi_regs *)pdata->iobase; priv->dmatx = plat->dmatx; /* RX channel offset is 0x30 */ priv->dmarx = (struct axidma_reg *)((phys_addr_t)priv->dmatx + 0x30); priv->mactype = plat->mactype;
- if (priv->mactype == EMAC_1G) { - priv->eth_hasnobuf = plat->eth_hasnobuf; - priv->phyaddr = plat->phyaddr; - priv->phy_of_handle = plat->phy_of_handle; - priv->interface = pdata->phy_interface; - - priv->bus = mdio_alloc(); - priv->bus->read = axiemac_miiphy_read; - priv->bus->write = axiemac_miiphy_write; - priv->bus->priv = priv; - - ret = mdio_register_seq(priv->bus, dev_seq(dev)); - if (ret) - return ret; - - axiemac_phy_init(dev); - } - - return 0; + if (priv->mactype == EMAC_1G) + return axiemac_setup_emac(dev); + else + return 0; }
static int axi_emac_remove(struct udevice *dev)
Or we may get load access faults afterward.
The `phydev` field on axi-ethernet’s private struct is not set on a failed phy_connect():
axi_emac_probe() => axiemac_phy_init() => priv->phydev = phy_connect() <--- may fail
However, all of the following calls on `axi_emac_ops` assume a valid `phydev` pointer. For example:
axiemac_start() => setup_phy() => phy_startup() => if (phydev->drv->startup) <--- deref of phydev return phydev->drv->startup(phydev);
Thus, it would be better to fail at the driver probe and let u-boot handle the rest (e.g. probe the driver again if needed), rather than having access faults.
Signed-off-by: Andy Chiu andy.chiu@sifive.com Reviewed-by: Greentime Hu greentime.hu@sifive.com ---
drivers/net/xilinx_axi_emac.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c index dbda6c70d8..08322aff88 100644 --- a/drivers/net/xilinx_axi_emac.c +++ b/drivers/net/xilinx_axi_emac.c @@ -782,10 +782,16 @@ static int axiemac_setup_emac(struct udevice *dev)
ret = mdio_register_seq(priv->bus, dev_seq(dev)); if (ret) - return ret; + goto fail;
- axiemac_phy_init(dev); + ret = axiemac_phy_init(dev); + if (ret) + goto fail_free_mdio;
+fail_free_mdio: + mdio_unregister(priv->bus); + mdio_free(priv->bus); +fail: return ret; }
Dear Maintainers,
I am sending this email to check if this patch was missed. I would be really appreciated if I get any suggestion from you, thanks!
Best regards Andy
On Thu, Jan 20, 2022 at 3:35 PM Andy Chiu andy.chiu@sifive.com wrote:
Or we may get load access faults afterward.
The `phydev` field on axi-ethernet’s private struct is not set on a failed phy_connect():
axi_emac_probe() => axiemac_phy_init() => priv->phydev = phy_connect() <--- may fail
However, all of the following calls on `axi_emac_ops` assume a valid `phydev` pointer. For example:
axiemac_start() => setup_phy() => phy_startup() => if (phydev->drv->startup) <--- deref of phydev return phydev->drv->startup(phydev);
Thus, it would be better to fail at the driver probe and let u-boot handle the rest (e.g. probe the driver again if needed), rather than having access faults.
Signed-off-by: Andy Chiu andy.chiu@sifive.com Reviewed-by: Greentime Hu greentime.hu@sifive.com
drivers/net/xilinx_axi_emac.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/net/xilinx_axi_emac.c b/drivers/net/xilinx_axi_emac.c index dbda6c70d8..08322aff88 100644 --- a/drivers/net/xilinx_axi_emac.c +++ b/drivers/net/xilinx_axi_emac.c @@ -782,10 +782,16 @@ static int axiemac_setup_emac(struct udevice *dev)
ret = mdio_register_seq(priv->bus, dev_seq(dev)); if (ret)
return ret;
goto fail;
axiemac_phy_init(dev);
ret = axiemac_phy_init(dev);if (ret)goto fail_free_mdio;+fail_free_mdio:
mdio_unregister(priv->bus);mdio_free(priv->bus);+fail: return ret; }
-- 2.34.1
participants (1)
-
Andy Chiu