ibm_newemac: emac_close() needs to call netif_carrier_off()

PATCH Applied Petri Gynther 4 participants 9 messages Aug 19, 2009 – Aug 20, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Petri Gynther

author

2009-08-19 21:00 UTC #0 lore raw

PATCH 1/1 ibm_newemac: emac_close() needs to call netif_carrier_off()

When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".

The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.

Signed-off-by Petri Gynther <[email protected]>

---
 drivers/net/ibm_newemac/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/net/ibm_newemac/core.c +2 -0

index beb8421..f0f8908 100644
@@ -1305,6 +1305,8 @@ static int emac_close(struct net_device *ndev)
 
 	free_irq(dev->emac_irq, dev);
 
+	netif_carrier_off(ndev);
+
 	return 0;
 }

Signature · 1 line

1.5.4.3

Stephen Hemminger

2009-08-19 21:11 UTC #1 lore raw

Quoted 9 lines · On Wed, 19 Aug 2009 14:00:00 -0700 (PDT) Petri Gynther <[email protected]>…

When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.

Bonding should be testing for netif_running() && netif_carrier_ok().

In many devices state of carrier is undefined when device is down.

David Miller

maintainer

2009-08-19 21:33 UTC #2 lore raw

From: Stephen Hemminger <[email protected]>
Date: Wed, 19 Aug 2009 14:11:36 -0700

Quoted 13 lines

Quoted 9 lines · On Wed, 19 Aug 2009 14:00:00 -0700 (PDT) Petri Gynther <[email protected]>…
When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.

Quoted 3 lines

Bonding should be testing for netif_running() && netif_carrier_ok().
In many devices state of carrier is undefined when device is down.

But if you check all of the drivers, ethernet in particular, the convention is to call netif_carrier_off() in foo_close().

Petri Gynther

author

2009-08-19 21:40 UTC #3 lore raw

I agree with David. And, that's why I propose this diff for ibm_newemac driver as well.

Quoted 23 lines · David Miller

From: Stephen Hemminger <[email protected]>
Date: Wed, 19 Aug 2009 14:11:36 -0700
Quoted 13 lines
Quoted 9 lines · On Wed, 19 Aug 2009 14:00:00 -0700 (PDT) Petri Gynther <[email protected]>…
When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.
Quoted 3 lines
Bonding should be testing for netif_running() && netif_carrier_ok().
In many devices state of carrier is undefined when device is down.
But if you check all of the drivers, ethernet in particular, the convention is to call netif_carrier_off() in foo_close().

Petri Gynther

author

2009-08-19 22:32 UTC #4 lore raw

Stephen,

I think your suggestion of adding netif_running() check to bond_check_dev_link() is valid and a good fix to the bonding driver. We can do this in a separate patch.

However, I think that the change to ibm_newemac: emac_close() is needed as well. ibm_newemac netdevs should not return netif_carrier_ok() == TRUE when they have been shut down.

Quoted 28 lines · Petri Gynther

I agree with David. And, that's why I propose this diff for ibm_newemac driver as well.
Quoted 23 lines · David Miller
From: Stephen Hemminger <[email protected]>
Date: Wed, 19 Aug 2009 14:11:36 -0700
Quoted 13 lines
Quoted 9 lines · On Wed, 19 Aug 2009 14:00:00 -0700 (PDT) Petri Gynther <[email protected]>…
When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.
Quoted 3 lines
Bonding should be testing for netif_running() && netif_carrier_ok().
In many devices state of carrier is undefined when device is down.
But if you check all of the drivers, ethernet in particular, the convention is to call netif_carrier_off() in foo_close().

Stephen Hemminger

2009-08-19 23:49 UTC #5 lore raw

Quoted 9 lines · On Wed, 19 Aug 2009 15:32:41 -0700 Petri Gynther <[email protected]> wrote:

Stephen,
I think your suggestion of adding netif_running() check to bond_check_dev_link() is valid and a good fix to the bonding driver. We can do this in a separate patch.
However, I think that the change to ibm_newemac: emac_close() is needed as well. ibm_newemac netdevs should not return netif_carrier_ok() == TRUE when they have been shut down.

I concur. Fixing the possible problems in both places is best.

Benjamin Herrenschmidt

2009-08-20 06:02 UTC #6 lore raw

In reply to Petri Gynther

Quoted 9 lines · Petri Gynther

Stephen,
I think your suggestion of adding netif_running() check to bond_check_dev_link() is valid and a good fix to the bonding driver. We can do this in a separate patch.
However, I think that the change to ibm_newemac: emac_close() is needed as well. ibm_newemac netdevs should not return netif_carrier_ok() == TRUE when they have been shut down.

Well, we definitely don't do that in sungem either, since we continue the link polling while the interface is down... IE. interface up/down is the data path and is orthogonal to the PHY polling in sungem. I suppose we -could- stop the polling while the interface is down, though I think my initial implementation did only poll the link while the interface was up and that was causing endless problems with various laptop-net style tools (however that was years and years ago).

Cheers,
Ben.

Quoted 29 lines · Petri Gynther

Quoted 28 lines
I agree with David. And, that's why I propose this diff for ibm_newemac driver as well.
Quoted 23 lines · David Miller
From: Stephen Hemminger <[email protected]>
Date: Wed, 19 Aug 2009 14:11:36 -0700
Quoted 13 lines
Quoted 9 lines · On Wed, 19 Aug 2009 14:00:00 -0700 (PDT) Petri Gynther <[email protected]>…
When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.
Quoted 3 lines
Bonding should be testing for netif_running() && netif_carrier_ok().
In many devices state of carrier is undefined when device is down.
But if you check all of the drivers, ethernet in particular, the convention is to call netif_carrier_off() in foo_close().

David Miller

maintainer

2009-08-20 09:20 UTC #7 lore raw

From: Benjamin Herrenschmidt <[email protected]>
Date: Thu, 20 Aug 2009 16:02:22 +1000

Quoted 11 lines · Petri Gynther

Quoted 9 lines
Stephen,
I think your suggestion of adding netif_running() check to bond_check_dev_link() is valid and a good fix to the bonding driver. We can do this in a separate patch.
However, I think that the change to ibm_newemac: emac_close() is needed as well. ibm_newemac netdevs should not return netif_carrier_ok() == TRUE when they have been shut down.

Quoted 7 lines

Well, we definitely don't do that in sungem either, since we continue the link polling while the interface is down... IE. interface up/down is the data path and is orthogonal to the PHY polling in sungem. I suppose we -could- stop the polling while the interface is down, though I think my initial implementation did only poll the link while the interface was up and that was causing endless problems with various laptop-net style tools (however that was years and years ago).

It just shows how few people use sungem with bonding :-)

Short term I'm going to add the ibm_newemac change.

Longer term we should probably add the netif_running() check to bond_check_dev_link().

David Miller

maintainer Applied

2009-08-20 09:21 UTC #8 lore raw

In reply to Petri Gynther

From: Petri Gynther <[email protected]>
Date: Wed, 19 Aug 2009 14:00:00 -0700 (PDT)

Quoted 11 lines

When ibm_newemac netdev instance is shutdown with "ifconfig down", the netdev interface does not go properly down. netif_carrier_ok() keeps returning TRUE even after "ifconfig down".
The problem can be seen when ibm_newemac instances are slaves of a bonding interface. The bonding interface code uses netif_carrier_ok() to determine the link status of its slaves. When ibm_newemac slave is shutdown with "ifconfig down", the bonding interface won't detect any link status change because netif_carrier_ok() keeps returning TRUE.
Signed-off-by Petri Gynther <[email protected]>

Applied, thanks.