[PATCH] sky2: set carrier off in probe

[PATCH] sky2: set carrier off in probe

[Brandon Philips]

Before bringing up a sky2 interface up ethtool reports 
"Link detected: yes". Do as ixgbe does and netif_carrier_off() on
probe().

Signed-off-by: Brandon Philips <bphilips@suse.de>

---
 drivers/net/sky2.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/net/sky2.c
===================================================================
--- linux-2.6.orig/drivers/net/sky2.c
+++ linux-2.6/drivers/net/sky2.c
@@ -4538,6 +4538,8 @@ static int __devinit sky2_probe(struct p
 		goto err_out_free_netdev;
 	}
 
+	netif_carrier_off(dev);
+
 	netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
 
 	err = request_irq(pdev->irq, sky2_intr,

Re: [PATCH] sky2: set carrier off in probe

[Stephen Hemminger]

On Thu, 29 Oct 2009 16:58:07 -0700
Brandon Philips <bphilips@suse.de> wrote:

> Before bringing up a sky2 interface up ethtool reports 
> "Link detected: yes". Do as ixgbe does and netif_carrier_off() on
> probe().
> 
> Signed-off-by: Brandon Philips <bphilips@suse.de>
> 
> ---
>  drivers/net/sky2.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6/drivers/net/sky2.c
> ===================================================================
> --- linux-2.6.orig/drivers/net/sky2.c
> +++ linux-2.6/drivers/net/sky2.c
> @@ -4538,6 +4538,8 @@ static int __devinit sky2_probe(struct p
>  		goto err_out_free_netdev;
>  	}
>  
> +	netif_carrier_off(dev);
> +
>  	netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
>  
>  	err = request_irq(pdev->irq, sky2_intr,

IMHO carrier is meaningless until device is up? What software
cares?

-- 

Re: [PATCH] sky2: set carrier off in probe

[Brandon Philips]

On 20:09 Thu 29 Oct 2009, Stephen Hemminger wrote:
> On Thu, 29 Oct 2009 16:58:07 -0700
> Brandon Philips <bphilips@suse.de> wrote:
> 
> > Before bringing up a sky2 interface up ethtool reports 
> > "Link detected: yes". Do as ixgbe does and netif_carrier_off() on
> > probe().
> > 
> > Signed-off-by: Brandon Philips <bphilips@suse.de>
> > 
> > ---
> >  drivers/net/sky2.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > Index: linux-2.6/drivers/net/sky2.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/net/sky2.c
> > +++ linux-2.6/drivers/net/sky2.c
> > @@ -4538,6 +4538,8 @@ static int __devinit sky2_probe(struct p
> >  		goto err_out_free_netdev;
> >  	}
> >  
> > +	netif_carrier_off(dev);
> > +
> >  	netif_napi_add(dev, &hw->napi, sky2_poll, NAPI_WEIGHT);
> >  
> >  	err = request_irq(pdev->irq, sky2_intr,
> 
> IMHO carrier is meaningless until device is up? What software
> cares?

A customer had a script that was testing for ethtool reporting "Link
detected: yes" and taking some sort of action. They found other
drivers reported "Link detected: No" until the first interface up.

The right thing to do is up the interface first before looking at the
the Link state, and I told them to do that, but I figured that this
patch made sense too to fix the initial buglet.

Cheers,

	Brandon

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Brandon Philips <bphilips@suse.de>
Date: Thu, 29 Oct 2009 20:51:28 -0700

> The right thing to do is up the interface first before looking at the
> the Link state, and I told them to do that, but I figured that this
> patch made sense too to fix the initial buglet.

It is not valid to expect link status before bringing the interface
up.

Otherwise, if link status is expected in this case, drivers cannot
fully power down all elements of the chip when the device is not even
brought up.

Re: [PATCH] sky2: set carrier off in probe

[Brandon Philips]

On 21:12 Thu 29 Oct 2009, David Miller wrote:
> From: Brandon Philips <bphilips@suse.de>
> Date: Thu, 29 Oct 2009 20:51:28 -0700
> 
> > The right thing to do is up the interface first before looking at the
> > the Link state, and I told them to do that, but I figured that this
> > patch made sense too to fix the initial buglet.
> 
> It is not valid to expect link status before bringing the interface
> up.

I agree the link state makes no sense when the interface isn't up.
The buglet is that other drivers seem to report "Link detected: no"
after probe while sky2 doesn't.

I don't have a strong feeling on this patch since the customer fixed
their script to do the right thing.

Cheers,

	Brandon

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Brandon Philips <bphilips@suse.de>
Date: Thu, 29 Oct 2009 21:30:50 -0700

> The buglet is that other drivers seem to report "Link detected: no"
> after probe while sky2 doesn't.

I agree that we should report something consistent before interface
up and 'no' is probably the best.

I remember fixing something similar in other drivers a few months
ago.

Re: [PATCH] sky2: set carrier off in probe

[Stephen Hemminger]

Why not fix the problem in a generic way?


---
Subject: ethtool: link is only up if device is running


Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/net/core/ethtool.c	2009-10-30 08:32:52.584728610 -0700
+++ b/net/core/ethtool.c	2009-10-30 08:33:31.806667877 -0700
@@ -27,7 +27,7 @@
 
 u32 ethtool_op_get_link(struct net_device *dev)
 {
-	return netif_carrier_ok(dev) ? 1 : 0;
+	return netif_running(dev) && netif_carrier_ok(dev);
 }
 
 u32 ethtool_op_get_rx_csum(struct net_device *dev)

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Stephen Hemminger <shemminger@linux-foundation.org>
Date: Fri, 30 Oct 2009 08:34:52 -0700

> Why not fix the problem in a generic way?

Drivers still need to make sure carrier is off when their
->open() routine runs, so that the transition event from
link down to link up occurs properly when the device is brought
up.

So mucking around with this carrier test will only hide the
bugs, not make things easier.

Re: [PATCH] sky2: set carrier off in probe

[Herbert Xu]

David Miller <davem@davemloft.net> wrote:
> 
> I agree that we should report something consistent before interface
> up and 'no' is probably the best.
> 
> I remember fixing something similar in other drivers a few months
> ago.

Can't we do this in one spot rather than having every driver
duplicate this?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 30 Oct 2009 11:54:20 -0400

> David Miller <davem@davemloft.net> wrote:
>> 
>> I agree that we should report something consistent before interface
>> up and 'no' is probably the best.
>> 
>> I remember fixing something similar in other drivers a few months
>> ago.
> 
> Can't we do this in one spot rather than having every driver
> duplicate this?

It doesn't matter if we do.

Because the driver must start in state with carrier off anyways,
so that we get the transition event when the device comes up
from link down to link up.

So many things depend upon that link state transition event, that
we're not saving anything by just mucking with the carrier test.

Re: [PATCH] sky2: set carrier off in probe

[Herbert Xu]

On Fri, Oct 30, 2009 at 11:10:27AM -0700, David Miller wrote:
. 
> Because the driver must start in state with carrier off anyways,
> so that we get the transition event when the device comes up
> from link down to link up.

Well we want all drivers to start in state NOCARRIER.  However,
a freshly allocated netdev has the NOCARRIER bit off.  This means
every single driver has to set the NOCARRIER bit.

It would seem much easier to ensure that the NOCARRIER bit is set
in a newly allocated netdev.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Fri, 30 Oct 2009 14:16:32 -0400

> On Fri, Oct 30, 2009 at 11:10:27AM -0700, David Miller wrote:
> . 
>> Because the driver must start in state with carrier off anyways,
>> so that we get the transition event when the device comes up
>> from link down to link up.
> 
> Well we want all drivers to start in state NOCARRIER.  However,
> a freshly allocated netdev has the NOCARRIER bit off.  This means
> every single driver has to set the NOCARRIER bit.
> 
> It would seem much easier to ensure that the NOCARRIER bit is set
> in a newly allocated netdev.

Since many drivers (especially virtual software ones) do not manage
carrier state and therefore that's why we start in state carrier on.

We've had this discussion a few times before, most recently with
Rusty wrt. virtio :-)

Re: [PATCH] sky2: set carrier off in probe

[Herbert Xu]

On Fri, Oct 30, 2009 at 11:20:51AM -0700, David Miller wrote:
>
> Since many drivers (especially virtual software ones) do not manage
> carrier state and therefore that's why we start in state carrier on.
> 
> We've had this discussion a few times before, most recently with
> Rusty wrt. virtio :-)

You're right, modifying the driver is the best way.  Oh well,
at least it's easy to find them by searching for netif_carrier :)
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] sky2: set carrier off in probe

[David Miller]

From: Brandon Philips <bphilips@suse.de>
Date: Thu, 29 Oct 2009 16:58:07 -0700

> Before bringing up a sky2 interface up ethtool reports 
> "Link detected: yes". Do as ixgbe does and netif_carrier_off() on
> probe().
> 
> Signed-off-by: Brandon Philips <bphilips@suse.de>

Applied to net-2.6, thanks.