[PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[Petri Gynther]

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 <pgynther@google.com>
---
 drivers/net/ibm_newemac/core.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ibm_newemac/core.c b/drivers/net/ibm_newemac/core.c
index beb8421..f0f8908 100644
--- a/drivers/net/ibm_newemac/core.c
+++ b/drivers/net/ibm_newemac/core.c
@@ -1305,6 +1305,8 @@ static int emac_close(struct net_device *ndev)
 
 	free_irq(dev->emac_irq, dev);
 
+	netif_carrier_off(ndev);
+
 	return 0;
 }
 
-- 
1.5.4.3

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[Stephen Hemminger]

On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
Petri Gynther <pgynther@google.com> wrote:

> 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.

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 19 Aug 2009 14:11:36 -0700

> On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
> Petri Gynther <pgynther@google.com> wrote:
> 
>> 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.

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

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[Petri Gynther]

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

On Wed, Aug 19, 2009 at 2:34 PM, David Miller<davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 19 Aug 2009 14:11:36 -0700
>
>> On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
>> Petri Gynther <pgynther@google.com> wrote:
>>
>>> 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.
>
> But if you check all of the drivers, ethernet in particular, the
> convention is to call netif_carrier_off() in foo_close().
>

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[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.

On Wed, Aug 19, 2009 at 2:40 PM, Petri Gynther<pgynther@google.com> wrote:
> I agree with David. And, that's why I propose this diff for
> ibm_newemac driver as well.
>
> On Wed, Aug 19, 2009 at 2:34 PM, David Miller<davem@davemloft.net> wrote:
>> From: Stephen Hemminger <shemminger@vyatta.com>
>> Date: Wed, 19 Aug 2009 14:11:36 -0700
>>
>>> On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
>>> Petri Gynther <pgynther@google.com> wrote:
>>>
>>>> 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.
>>
>> But if you check all of the drivers, ethernet in particular, the
>> convention is to call netif_carrier_off() in foo_close().
>>
>

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[Stephen Hemminger]

On Wed, 19 Aug 2009 15:32:41 -0700
Petri Gynther <pgynther@google.com> 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.

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[Benjamin Herrenschmidt]

On Wed, 2009-08-19 at 15:32 -0700, Petri Gynther 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.

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.

> On Wed, Aug 19, 2009 at 2:40 PM, Petri Gynther<pgynther@google.com> wrote:
> > I agree with David. And, that's why I propose this diff for
> > ibm_newemac driver as well.
> >
> > On Wed, Aug 19, 2009 at 2:34 PM, David Miller<davem@davemloft.net> wrote:
> >> From: Stephen Hemminger <shemminger@vyatta.com>
> >> Date: Wed, 19 Aug 2009 14:11:36 -0700
> >>
> >>> On Wed, 19 Aug 2009 14:00:00 -0700 (PDT)
> >>> Petri Gynther <pgynther@google.com> wrote:
> >>>
> >>>> 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.
> >>
> >> But if you check all of the drivers, ethernet in particular, the
> >> convention is to call netif_carrier_off() in foo_close().
> >>
> >

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 20 Aug 2009 16:02:22 +1000

> On Wed, 2009-08-19 at 15:32 -0700, Petri Gynther 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.
> 
> 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().

Re: [PATCH] ibm_newemac: emac_close() needs to call netif_carrier_off()

[David Miller]

From: Petri Gynther <pgynther@google.com>
Date: Wed, 19 Aug 2009 14:00:00 -0700 (PDT)

> 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 <pgynther@google.com>

Applied, thanks.