| Message ID | 20091029235807.GF3228@jenkins.home.ifup.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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?
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
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
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
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
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,
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
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,
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
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 :)
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
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,
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