[PATCH net] net: macb: Probe MDIO bus before registering netdev

[PATCH net] net: macb: Probe MDIO bus before registering netdev

[Florian Fainelli]

The current sequence makes us register for a network device prior to
registering and probing the MDIO bus which could lead to some unwanted
consequences, like a thread of execution calling into ndo_open before
register_netdev() returns, while the MDIO bus is not ready yet.

Rework the sequence to register for the MDIO bus, and therefore attach
to a PHY prior to calling register_netdev(), which implies reworking the
error path a bit.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Tracking down the exact commit which started doing that was a little
difficult, so I can't really provide a proper Fixes tag yet that does
not reference 4-5 commits

 drivers/net/ethernet/cadence/macb.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 48a7d7dee846..e9b470a5ddd0 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -441,7 +441,7 @@ static int macb_mii_init(struct macb *bp)
 	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
 		bp->pdev->name, bp->pdev->id);
 	bp->mii_bus->priv = bp;
-	bp->mii_bus->parent = &bp->dev->dev;
+	bp->mii_bus->parent = &bp->pdev->dev;
 	pdata = dev_get_platdata(&bp->pdev->dev);
 
 	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
@@ -3019,29 +3019,36 @@ static int macb_probe(struct platform_device *pdev)
 	if (err)
 		goto err_out_free_netdev;
 
+	err = macb_mii_init(bp);
+	if (err)
+		goto err_out_free_netdev;
+
+	phydev = bp->phy_dev;
+
+	netif_carrier_off(dev);
+
 	err = register_netdev(dev);
 	if (err) {
 		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
-		goto err_out_unregister_netdev;
+		goto err_out_unregister_mdio;
 	}
 
-	err = macb_mii_init(bp);
-	if (err)
-		goto err_out_unregister_netdev;
-
-	netif_carrier_off(dev);
+	phy_attached_info(phydev);
 
 	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
 		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
 		    dev->base_addr, dev->irq, dev->dev_addr);
 
-	phydev = bp->phy_dev;
-	phy_attached_info(phydev);
-
 	return 0;
 
-err_out_unregister_netdev:
-	unregister_netdev(dev);
+err_out_unregister_mdio:
+	phy_disconnect(bp->phy_dev);
+	mdiobus_unregister(bp->mii_bus);
+	mdiobus_free(bp->mii_bus);
+
+	/* Shutdown the PHY if there is a GPIO reset */
+	if (bp->reset_gpio)
+		gpiod_set_value(bp->reset_gpio, 0);
 
 err_out_free_netdev:
 	free_netdev(dev);
-- 
2.1.0

Re: [PATCH net] net: macb: Probe MDIO bus before registering netdev

[Nicolas Ferre]

Le 03/05/2016 03:38, Florian Fainelli a écrit :
> The current sequence makes us register for a network device prior to
> registering and probing the MDIO bus which could lead to some unwanted
> consequences, like a thread of execution calling into ndo_open before
> register_netdev() returns, while the MDIO bus is not ready yet.
> 
> Rework the sequence to register for the MDIO bus, and therefore attach
> to a PHY prior to calling register_netdev(), which implies reworking the
> error path a bit.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks a lot Florian for this follow-up and the advices you had given to
Alexandre during the debug session when you spotted this problem.

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> ---
> Tracking down the exact commit which started doing that was a little
> difficult, so I can't really provide a proper Fixes tag yet that does
> not reference 4-5 commits

Yes, indeed. Macb is moving quite a bit those days ;-)


Bye,


>  drivers/net/ethernet/cadence/macb.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 48a7d7dee846..e9b470a5ddd0 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -441,7 +441,7 @@ static int macb_mii_init(struct macb *bp)
>  	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
>  		bp->pdev->name, bp->pdev->id);
>  	bp->mii_bus->priv = bp;
> -	bp->mii_bus->parent = &bp->dev->dev;
> +	bp->mii_bus->parent = &bp->pdev->dev;
>  	pdata = dev_get_platdata(&bp->pdev->dev);
>  
>  	dev_set_drvdata(&bp->dev->dev, bp->mii_bus);
> @@ -3019,29 +3019,36 @@ static int macb_probe(struct platform_device *pdev)
>  	if (err)
>  		goto err_out_free_netdev;
>  
> +	err = macb_mii_init(bp);
> +	if (err)
> +		goto err_out_free_netdev;
> +
> +	phydev = bp->phy_dev;
> +
> +	netif_carrier_off(dev);
> +
>  	err = register_netdev(dev);
>  	if (err) {
>  		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
> -		goto err_out_unregister_netdev;
> +		goto err_out_unregister_mdio;
>  	}
>  
> -	err = macb_mii_init(bp);
> -	if (err)
> -		goto err_out_unregister_netdev;
> -
> -	netif_carrier_off(dev);
> +	phy_attached_info(phydev);
>  
>  	netdev_info(dev, "Cadence %s rev 0x%08x at 0x%08lx irq %d (%pM)\n",
>  		    macb_is_gem(bp) ? "GEM" : "MACB", macb_readl(bp, MID),
>  		    dev->base_addr, dev->irq, dev->dev_addr);
>  
> -	phydev = bp->phy_dev;
> -	phy_attached_info(phydev);
> -
>  	return 0;
>  
> -err_out_unregister_netdev:
> -	unregister_netdev(dev);
> +err_out_unregister_mdio:
> +	phy_disconnect(bp->phy_dev);
> +	mdiobus_unregister(bp->mii_bus);
> +	mdiobus_free(bp->mii_bus);
> +
> +	/* Shutdown the PHY if there is a GPIO reset */
> +	if (bp->reset_gpio)
> +		gpiod_set_value(bp->reset_gpio, 0);
>  
>  err_out_free_netdev:
>  	free_netdev(dev);
> 


-- 
Nicolas Ferre

Re: [PATCH net] net: macb: Probe MDIO bus before registering netdev

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Mon,  2 May 2016 18:38:45 -0700

> The current sequence makes us register for a network device prior to
> registering and probing the MDIO bus which could lead to some unwanted
> consequences, like a thread of execution calling into ndo_open before
> register_netdev() returns, while the MDIO bus is not ready yet.
> 
> Rework the sequence to register for the MDIO bus, and therefore attach
> to a PHY prior to calling register_netdev(), which implies reworking the
> error path a bit.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Applied, thanks Florian.