Patchwork netdev/fec: fix ifconfig eth0 down hang issue

login
register
mail settings
Submitter Bryan Wu
Date April 29, 2010, 7:18 a.m.
Message ID <1272525529-18100-1-git-send-email-bryan.wu@canonical.com>
Download mbox | patch
Permalink /patch/51269/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Bryan Wu - April 29, 2010, 7:18 a.m.
BugLink: http://bugs.launchpad.net/bugs/559065

In fec open/close function, we need to use phy_connect and phy_disconnect
operation before we start/stop phy. Otherwise it will cause system hang.

Only call fec_enet_mii_probe() in open function, because the first open
action will cause NULL pointer error.

Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/net/fec.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)
Bryan Wu - May 5, 2010, 8:39 a.m.
Folks,

Can I get some ACK for this patch? Tim, I believe this time you will ack it, right?

Thanks,
-Bryan

On 04/29/2010 03:18 PM, Bryan Wu wrote:
> BugLink: http://bugs.launchpad.net/bugs/559065
>
> In fec open/close function, we need to use phy_connect and phy_disconnect
> operation before we start/stop phy. Otherwise it will cause system hang.
>
> Only call fec_enet_mii_probe() in open function, because the first open
> action will cause NULL pointer error.
>
> Signed-off-by: Bryan Wu<bryan.wu@canonical.com>
> ---
>   drivers/net/fec.c |   28 ++++++++++++++++------------
>   1 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 2280373..692d3c4 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -691,6 +691,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
>   	struct phy_device *phy_dev = NULL;
>   	int phy_addr;
>
> +	fep->phy_dev = NULL;
> +
>   	/* find the first phy */
>   	for (phy_addr = 0; phy_addr<  PHY_MAX_ADDR; phy_addr++) {
>   		if (fep->mii_bus->phy_map[phy_addr]) {
> @@ -721,6 +723,11 @@ static int fec_enet_mii_probe(struct net_device *dev)
>   	fep->link = 0;
>   	fep->full_duplex = 0;
>
> +	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> +		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
> +		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> +		fep->phy_dev->irq);
> +
>   	return 0;
>   }
>
> @@ -768,13 +775,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>   	if (mdiobus_register(fep->mii_bus))
>   		goto err_out_free_mdio_irq;
>
> -	if (fec_enet_mii_probe(dev) != 0)
> -		goto err_out_unregister_bus;
> -
>   	return 0;
>
> -err_out_unregister_bus:
> -	mdiobus_unregister(fep->mii_bus);
>   err_out_free_mdio_irq:
>   	kfree(fep->mii_bus->irq);
>   err_out_free_mdiobus:
> @@ -928,7 +930,12 @@ fec_enet_open(struct net_device *dev)
>   	if (ret)
>   		return ret;
>
> -	/* schedule a link state check */
> +	/* Probe and connect to PHY when open the interface */
> +	ret = fec_enet_mii_probe(dev);
> +	if (ret) {
> +		fec_enet_free_buffers(dev);
> +		return ret;
> +	}
>   	phy_start(fep->phy_dev);
>   	netif_start_queue(dev);
>   	fep->opened = 1;
> @@ -942,10 +949,12 @@ fec_enet_close(struct net_device *dev)
>
>   	/* Don't know what to do yet. */
>   	fep->opened = 0;
> -	phy_stop(fep->phy_dev);
>   	netif_stop_queue(dev);
>   	fec_stop(dev);
>
> +	if (fep->phy_dev)
> +		phy_disconnect(fep->phy_dev);
> +
>           fec_enet_free_buffers(dev);
>   	clk_disable(fep->clk);
>
> @@ -1337,11 +1346,6 @@ fec_probe(struct platform_device *pdev)
>
>   	clk_disable(fep->clk);
>
> -	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> -		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
> -		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> -		fep->phy_dev->irq);
> -
>   	return 0;
>
>   failed_register:
Tim Gardner - May 5, 2010, 7:48 p.m.
On 05/05/2010 10:39 AM, Bryan Wu wrote:
> Folks,
>
> Can I get some ACK for this patch? Tim, I believe this time you will ack it, right?
>
> Thanks,
> -Bryan
>
> On 04/29/2010 03:18 PM, Bryan Wu wrote:
>> BugLink: http://bugs.launchpad.net/bugs/559065
>>
>> In fec open/close function, we need to use phy_connect and phy_disconnect
>> operation before we start/stop phy. Otherwise it will cause system hang.
>>
>> Only call fec_enet_mii_probe() in open function, because the first open
>> action will cause NULL pointer error.
>>
>> Signed-off-by: Bryan Wu<bryan.wu@canonical.com>
>> ---
>>    drivers/net/fec.c |   28 ++++++++++++++++------------
>>    1 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>> index 2280373..692d3c4 100644
>> --- a/drivers/net/fec.c
>> +++ b/drivers/net/fec.c
>> @@ -691,6 +691,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
>>    	struct phy_device *phy_dev = NULL;
>>    	int phy_addr;
>>
>> +	fep->phy_dev = NULL;
>> +
>>    	/* find the first phy */
>>    	for (phy_addr = 0; phy_addr<   PHY_MAX_ADDR; phy_addr++) {
>>    		if (fep->mii_bus->phy_map[phy_addr]) {
>> @@ -721,6 +723,11 @@ static int fec_enet_mii_probe(struct net_device *dev)
>>    	fep->link = 0;
>>    	fep->full_duplex = 0;
>>
>> +	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
>> +		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
>> +		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
>> +		fep->phy_dev->irq);
>> +
>>    	return 0;
>>    }
>>
>> @@ -768,13 +775,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>>    	if (mdiobus_register(fep->mii_bus))
>>    		goto err_out_free_mdio_irq;
>>
>> -	if (fec_enet_mii_probe(dev) != 0)
>> -		goto err_out_unregister_bus;
>> -
>>    	return 0;
>>
>> -err_out_unregister_bus:
>> -	mdiobus_unregister(fep->mii_bus);
>>    err_out_free_mdio_irq:
>>    	kfree(fep->mii_bus->irq);
>>    err_out_free_mdiobus:
>> @@ -928,7 +930,12 @@ fec_enet_open(struct net_device *dev)
>>    	if (ret)
>>    		return ret;
>>
>> -	/* schedule a link state check */
>> +	/* Probe and connect to PHY when open the interface */
>> +	ret = fec_enet_mii_probe(dev);
>> +	if (ret) {
>> +		fec_enet_free_buffers(dev);
>> +		return ret;
>> +	}
>>    	phy_start(fep->phy_dev);
>>    	netif_start_queue(dev);
>>    	fep->opened = 1;
>> @@ -942,10 +949,12 @@ fec_enet_close(struct net_device *dev)
>>
>>    	/* Don't know what to do yet. */
>>    	fep->opened = 0;
>> -	phy_stop(fep->phy_dev);
>>    	netif_stop_queue(dev);
>>    	fec_stop(dev);
>>
>> +	if (fep->phy_dev)
>> +		phy_disconnect(fep->phy_dev);
>> +
>>            fec_enet_free_buffers(dev);
>>    	clk_disable(fep->clk);
>>
>> @@ -1337,11 +1346,6 @@ fec_probe(struct platform_device *pdev)
>>
>>    	clk_disable(fep->clk);
>>
>> -	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
>> -		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
>> -		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
>> -		fep->phy_dev->irq);
>> -
>>    	return 0;
>>
>>    failed_register:
>
>

I thought I _did_ ack it once. Just apply the dang thing. It'll either 
work or it won't. There aren't any corner cases with this.

Acked-by: Tim Gardner <tim.gardner@canonical.com>
Colin King - May 6, 2010, 8:22 a.m.
On Wed, 2010-05-05 at 16:39 +0800, Bryan Wu wrote:
> Folks,
> 
> Can I get some ACK for this patch? Tim, I believe this time you will ack it, right?
> 
> Thanks,
> -Bryan
> 
> On 04/29/2010 03:18 PM, Bryan Wu wrote:
> > BugLink: http://bugs.launchpad.net/bugs/559065
> >
> > In fec open/close function, we need to use phy_connect and phy_disconnect
> > operation before we start/stop phy. Otherwise it will cause system hang.
> >
> > Only call fec_enet_mii_probe() in open function, because the first open
> > action will cause NULL pointer error.
> >
> > Signed-off-by: Bryan Wu<bryan.wu@canonical.com>
> > ---
> >   drivers/net/fec.c |   28 ++++++++++++++++------------
> >   1 files changed, 16 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> > index 2280373..692d3c4 100644
> > --- a/drivers/net/fec.c
> > +++ b/drivers/net/fec.c
> > @@ -691,6 +691,8 @@ static int fec_enet_mii_probe(struct net_device *dev)
> >   	struct phy_device *phy_dev = NULL;
> >   	int phy_addr;
> >
> > +	fep->phy_dev = NULL;
> > +
> >   	/* find the first phy */
> >   	for (phy_addr = 0; phy_addr<  PHY_MAX_ADDR; phy_addr++) {
> >   		if (fep->mii_bus->phy_map[phy_addr]) {
> > @@ -721,6 +723,11 @@ static int fec_enet_mii_probe(struct net_device *dev)
> >   	fep->link = 0;
> >   	fep->full_duplex = 0;
> >
> > +	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> > +		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
> > +		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> > +		fep->phy_dev->irq);
> > +
> >   	return 0;
> >   }
> >
> > @@ -768,13 +775,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> >   	if (mdiobus_register(fep->mii_bus))
> >   		goto err_out_free_mdio_irq;
> >
> > -	if (fec_enet_mii_probe(dev) != 0)
> > -		goto err_out_unregister_bus;
> > -
> >   	return 0;
> >
> > -err_out_unregister_bus:
> > -	mdiobus_unregister(fep->mii_bus);
> >   err_out_free_mdio_irq:
> >   	kfree(fep->mii_bus->irq);
> >   err_out_free_mdiobus:
> > @@ -928,7 +930,12 @@ fec_enet_open(struct net_device *dev)
> >   	if (ret)
> >   		return ret;
> >
> > -	/* schedule a link state check */
> > +	/* Probe and connect to PHY when open the interface */
> > +	ret = fec_enet_mii_probe(dev);
> > +	if (ret) {
> > +		fec_enet_free_buffers(dev);
> > +		return ret;
> > +	}
> >   	phy_start(fep->phy_dev);
> >   	netif_start_queue(dev);
> >   	fep->opened = 1;
> > @@ -942,10 +949,12 @@ fec_enet_close(struct net_device *dev)
> >
> >   	/* Don't know what to do yet. */
> >   	fep->opened = 0;
> > -	phy_stop(fep->phy_dev);
> >   	netif_stop_queue(dev);
> >   	fec_stop(dev);
> >
> > +	if (fep->phy_dev)
> > +		phy_disconnect(fep->phy_dev);
> > +
> >           fec_enet_free_buffers(dev);
> >   	clk_disable(fep->clk);
> >
> > @@ -1337,11 +1346,6 @@ fec_probe(struct platform_device *pdev)
> >
> >   	clk_disable(fep->clk);
> >
> > -	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
> > -		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
> > -		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
> > -		fep->phy_dev->irq);
> > -
> >   	return 0;
> >
> >   failed_register:
> 
> 
Apologies, I recall I asked about removal of phy_stop() and then forgot
to act on Bryan's response.

Acked-by: Colin King <colin.king@canonical.com>
Stefan Bader - May 7, 2010, 9:57 a.m.
Applied to Lucid fsl-imx51. Sorry I also forgot about that as it seemed to have
open questions.

Stefan

Patch

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 2280373..692d3c4 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -691,6 +691,8 @@  static int fec_enet_mii_probe(struct net_device *dev)
 	struct phy_device *phy_dev = NULL;
 	int phy_addr;
 
+	fep->phy_dev = NULL;
+
 	/* find the first phy */
 	for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
 		if (fep->mii_bus->phy_map[phy_addr]) {
@@ -721,6 +723,11 @@  static int fec_enet_mii_probe(struct net_device *dev)
 	fep->link = 0;
 	fep->full_duplex = 0;
 
+	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
+		"(mii_bus:phy_addr=%s, irq=%d)\n", dev->name,
+		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
+		fep->phy_dev->irq);
+
 	return 0;
 }
 
@@ -768,13 +775,8 @@  static int fec_enet_mii_init(struct platform_device *pdev)
 	if (mdiobus_register(fep->mii_bus))
 		goto err_out_free_mdio_irq;
 
-	if (fec_enet_mii_probe(dev) != 0)
-		goto err_out_unregister_bus;
-
 	return 0;
 
-err_out_unregister_bus:
-	mdiobus_unregister(fep->mii_bus);
 err_out_free_mdio_irq:
 	kfree(fep->mii_bus->irq);
 err_out_free_mdiobus:
@@ -928,7 +930,12 @@  fec_enet_open(struct net_device *dev)
 	if (ret)
 		return ret;
 
-	/* schedule a link state check */
+	/* Probe and connect to PHY when open the interface */
+	ret = fec_enet_mii_probe(dev);
+	if (ret) {
+		fec_enet_free_buffers(dev);
+		return ret;
+	}
 	phy_start(fep->phy_dev);
 	netif_start_queue(dev);
 	fep->opened = 1;
@@ -942,10 +949,12 @@  fec_enet_close(struct net_device *dev)
 
 	/* Don't know what to do yet. */
 	fep->opened = 0;
-	phy_stop(fep->phy_dev);
 	netif_stop_queue(dev);
 	fec_stop(dev);
 
+	if (fep->phy_dev)
+		phy_disconnect(fep->phy_dev);
+
         fec_enet_free_buffers(dev);
 	clk_disable(fep->clk);
 
@@ -1337,11 +1346,6 @@  fec_probe(struct platform_device *pdev)
 
 	clk_disable(fep->clk);
 
-	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
-		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
-		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
-		fep->phy_dev->irq);
-
 	return 0;
 
 failed_register: