Patchwork netdev/fec: fix ifconfig eth0 down hang issue (v5)

login
register
mail settings
Submitter Bryan Wu
Date April 28, 2010, 1:29 a.m.
Message ID <1272418198-2971-1-git-send-email-bryan.wu@canonical.com>
Download mbox | patch
Permalink /patch/51141/
State Superseded
Delegated to: Stefan Bader
Headers show

Comments

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

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

v4:
 - don't touch suspend/resume functions in this patch
 - set fep->phy_dev = NULL in the fec_enet_mii_probe() entry

v3:
if fec_enet_mii_probe() fails, we should exit the open and resume functions
properly and give up further operations without a proper PHY connected.

v2:
Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev

v1:
In ethernet connection open/close function, we need to use phy_connect
and phy_disconnect operation before we start/stop phy. Otherwise it will
cause system hang. Also suspend/resume needs the same fix.

Signed-off-by: Bryan Wu <bryan.wu@canonical.com>
---
 drivers/net/fec.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)
Tim Gardner - April 28, 2010, 4:56 p.m.
On 04/28/2010 02:29 AM, Bryan Wu wrote:
> BugLink: http://bugs.launchpad.net/bugs/559065
>
> v5:
> Only call fec_enet_mii_probe() in open function, otherwise the first open
> action will cause NULL pointer error.
>
> v4:
>   - don't touch suspend/resume functions in this patch
>   - set fep->phy_dev = NULL in the fec_enet_mii_probe() entry
>
> v3:
> if fec_enet_mii_probe() fails, we should exit the open and resume functions
> properly and give up further operations without a proper PHY connected.
>
> v2:
> Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev
>
> v1:
> In ethernet connection open/close function, we need to use phy_connect
> and phy_disconnect operation before we start/stop phy. Otherwise it will
> cause system hang. Also suspend/resume needs the same fix.
>
> 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:

ACK - though I don't think you need all of the patch history in the 
commit log message. Only v5 is of interest.

Acked-by: <tim.gardner@canonical.com>
Colin King - April 28, 2010, 9:15 p.m.
On Wed, 2010-04-28 at 09:29 +0800, Bryan Wu wrote:
> BugLink: http://bugs.launchpad.net/bugs/559065
> 
> v5:
> Only call fec_enet_mii_probe() in open function, otherwise the first open
> action will cause NULL pointer error.
> 
> v4:
>  - don't touch suspend/resume functions in this patch
>  - set fep->phy_dev = NULL in the fec_enet_mii_probe() entry
> 
> v3:
> if fec_enet_mii_probe() fails, we should exit the open and resume functions
> properly and give up further operations without a proper PHY connected.
> 
> v2:
> Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev
> 
> v1:
> In ethernet connection open/close function, we need to use phy_connect
> and phy_disconnect operation before we start/stop phy. Otherwise it will
> cause system hang. Also suspend/resume needs the same fix.
> 
> 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:
> -- 
> 1.7.0.4
> 
> 
Minor question - it seems that fec_enet_close() is no longer doing a
phy_stop().  Is that intentional and why can we get away with not doing
a phy_stop()?
Bryan Wu - April 29, 2010, 1:32 a.m.
On 04/29/2010 12:56 AM, Tim Gardner wrote:
> On 04/28/2010 02:29 AM, Bryan Wu wrote:
>> BugLink: http://bugs.launchpad.net/bugs/559065
>>
>> v5:
>> Only call fec_enet_mii_probe() in open function, otherwise the first open
>> action will cause NULL pointer error.
>>
>> v4:
>> - don't touch suspend/resume functions in this patch
>> - set fep->phy_dev = NULL in the fec_enet_mii_probe() entry
>>
>> v3:
>> if fec_enet_mii_probe() fails, we should exit the open and resume
>> functions
>> properly and give up further operations without a proper PHY connected.
>>
>> v2:
>> Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev
>>
>> v1:
>> In ethernet connection open/close function, we need to use phy_connect
>> and phy_disconnect operation before we start/stop phy. Otherwise it will
>> cause system hang. Also suspend/resume needs the same fix.
>>
>> 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:
>
> ACK - though I don't think you need all of the patch history in the
> commit log message. Only v5 is of interest.
>

OK, I will fix this.

> Acked-by: <tim.gardner@canonical.com>
>

Thanks,
Bryan Wu - April 29, 2010, 1:42 a.m.
On 04/29/2010 05:15 AM, Colin Ian King wrote:
> On Wed, 2010-04-28 at 09:29 +0800, Bryan Wu wrote:
>> BugLink: http://bugs.launchpad.net/bugs/559065
>>
>> v5:
>> Only call fec_enet_mii_probe() in open function, otherwise the first open
>> action will cause NULL pointer error.
>>
>> v4:
>>   - don't touch suspend/resume functions in this patch
>>   - set fep->phy_dev = NULL in the fec_enet_mii_probe() entry
>>
>> v3:
>> if fec_enet_mii_probe() fails, we should exit the open and resume functions
>> properly and give up further operations without a proper PHY connected.
>>
>> v2:
>> Check fec_enet_mii_probe() return value directly, instead of fep->phy_dev
>>
>> v1:
>> In ethernet connection open/close function, we need to use phy_connect
>> and phy_disconnect operation before we start/stop phy. Otherwise it will
>> cause system hang. Also suspend/resume needs the same fix.
>>
>> 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:
>> --
>> 1.7.0.4
>>
>>
> Minor question - it seems that fec_enet_close() is no longer doing a
> phy_stop().  Is that intentional and why can we get away with not doing
> a phy_stop()?
>

The original design uses phy_stop() here, we faced an ifdown hang issue. 
phy_disconnect will disconnect from the phy and stop it via it's internal 
statues machine. And when we open fec, phy_connect will be called in 
fec_enet_mii_probe().

Actually, I was a little bit confused by this phy_stop() and phy_disconnect() at 
beginning. But from our test, phy_stop() will cause ifdown hang issue. After 
looking at other driver such as drivers/net/davinci_emac.c, I think 
phy_disconnect is the right way here. Or maybe we can call phy_stop then 
followed by phy_disconnect, but phy_disconnect here is good enough. Let me ping 
upstream for help with the patch later.

Thanks,

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: