Patchwork sky2: set carrier off in probe

login
register
mail settings
Submitter Brandon Philips
Date Oct. 29, 2009, 11:58 p.m.
Message ID <20091029235807.GF3228@jenkins.home.ifup.org>
Download mbox | patch
Permalink /patch/37232/
State Accepted
Delegated to: David Miller
Headers show

Comments

Brandon Philips - Oct. 29, 2009, 11:58 p.m.
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(+)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Hemminger - Oct. 30, 2009, 3:09 a.m.
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?
Brandon Philips - Oct. 30, 2009, 3:51 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 30, 2009, 4:12 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brandon Philips - Oct. 30, 2009, 4:30 a.m.
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 30, 2009, 4:38 a.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Oct. 30, 2009, 3:54 p.m.
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,
David Miller - Oct. 30, 2009, 6:10 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Oct. 30, 2009, 6:16 p.m.
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,
David Miller - Oct. 30, 2009, 6:20 p.m.
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 :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Herbert Xu - Oct. 30, 2009, 7:13 p.m.
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 :)
David Miller - Oct. 30, 2009, 7:28 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,