[PATCH 1/3] net: xilinx: fix load access fault

[PATCH 1/3] net: xilinx: fix load access fault

[Andy Chiu]

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;
-- 
2.34.1

[PATCH 2/3] net: xilinx: Move setup of 1G MAC to a function

[Andy Chiu]

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)
-- 
2.34.1

[PATCH 3/3] net: xilinx: Force a probe failure if it cannot setup phy

[Andy Chiu]

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

Re: [PATCH 3/3] net: xilinx: Force a probe failure if it cannot setup phy

[Andy Chiu]

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
>