[PATCH] usb-net/pegasus: fix pegasus carrier detection

[PATCH] usb-net/pegasus: fix pegasus carrier detection

[Dan Williams]

Broken by 4a1728a28a193aa388900714bbb1f375e08a6d8e which switched the
return semantics of read_mii_word() but didn't fix usage of
read_mii_word() to conform to the new semantics.

Setting carrier to off based on the NO_CARRIER flag is also incorrect as
that flag only triggers on TX failure and therefore isn't correct when
no frames are being transmitted.  Since there is already a 2*HZ MII
carrier check going on, defer to that.

Add a TRUST_LINK_STATUS feature flag for adapters where the LINK_STATUS
flag is actually correct, and use that rather than the NO_CARRIER flag.

Signed-off-by: Dan Williams <dcbw@redhat.com>

diff --git a/drivers/usb/net/pegasus.c b/drivers/usb/net/pegasus.c
index d48c024..6d12961 100644
--- a/drivers/usb/net/pegasus.c
+++ b/drivers/usb/net/pegasus.c
@@ -316,6 +316,7 @@ static int update_eth_regs_async(pegasus_t * pegasus)
 	return ret;
 }
 
+/* Returns 0 on success, error on failure */
 static int read_mii_word(pegasus_t * pegasus, __u8 phy, __u8 indx, __u16 * regd)
 {
 	int i;
@@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
 		 * d[0].NO_CARRIER kicks in only with failed TX.
 		 * ... so monitoring with MII may be safest.
 		 */
-		if (d[0] & NO_CARRIER)
-			netif_carrier_off(net);	
-		else
-			netif_carrier_on(net);
+		if (pegasus->features & TRUST_LINK_STATUS) {
+			if (d[5] & LINK_STATUS)
+				netif_carrier_on(net);
+			else
+				netif_carrier_off(net);
+		} else {
+			/* Never set carrier _on_ based on ! NO_CARRIER */
+			if (d[0] & NO_CARRIER)
+				netif_carrier_off(net);	
+		}
 
 		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
 		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
@@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
 	pegasus_t *pegasus = netdev_priv(net);
 	u16 tmp;
 
-	if (!read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
+	if (read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
 		return;
 
 	if (tmp & BMSR_LSTATUS)
diff --git a/drivers/usb/net/pegasus.h b/drivers/usb/net/pegasus.h
index c746782..c7aadb4 100644
--- a/drivers/usb/net/pegasus.h
+++ b/drivers/usb/net/pegasus.h
@@ -11,6 +11,7 @@
 
 #define	PEGASUS_II		0x80000000
 #define	HAS_HOME_PNA		0x40000000
+#define	TRUST_LINK_STATUS	0x20000000
 
 #define	PEGASUS_MTU		1536
 #define	RX_SKBS			4
@@ -203,7 +204,7 @@ PEGASUS_DEV( "AEI USB Fast Ethernet Adapter", VENDOR_AEILAB, 0x1701,
 PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
 		DEFAULT_GPIO_RESET | PEGASUS_II )
 PEGASUS_DEV( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121,
-		DEFAULT_GPIO_RESET | PEGASUS_II )
+		DEFAULT_GPIO_RESET | PEGASUS_II | TRUST_LINK_STATUS )
 PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
 		DEFAULT_GPIO_RESET )
 PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Jeff Garzik]

Dan Williams wrote:
> Broken by 4a1728a28a193aa388900714bbb1f375e08a6d8e which switched the
> return semantics of read_mii_word() but didn't fix usage of
> read_mii_word() to conform to the new semantics.
> 
> Setting carrier to off based on the NO_CARRIER flag is also incorrect as
> that flag only triggers on TX failure and therefore isn't correct when
> no frames are being transmitted.  Since there is already a 2*HZ MII
> carrier check going on, defer to that.
> 
> Add a TRUST_LINK_STATUS feature flag for adapters where the LINK_STATUS
> flag is actually correct, and use that rather than the NO_CARRIER flag.
> 
> Signed-off-by: Dan Williams <dcbw@redhat.com>
> 
> diff --git a/drivers/usb/net/pegasus.c b/drivers/usb/net/pegasus.c
> index d48c024..6d12961 100644
> --- a/drivers/usb/net/pegasus.c
> +++ b/drivers/usb/net/pegasus.c

applied

For drivers/usb/* you should at least CC GregKH.

Long term, Greg seemed OK with moving the net drivers from 
drivers/usb/net to drivers/usb/net, in line with the current policy of 
placing net drivers in drivers/net/*, bus agnostic.  After that move, 
sending to netdev and me (as you did here) would be the preferred avenue.

	Jeff

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Greg KH]

On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
>  Long term, Greg seemed OK with moving the net drivers from drivers/usb/net 
>  to drivers/usb/net, in line with the current policy of placing net drivers 
>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and me 
>  (as you did here) would be the preferred avenue.

Speaking of which, do you want me to do this in the 2.6.22-rc1
timeframe?  Usually big code moves like this are good to do right after
rc1 comes out as the major churn is usually completed then.

thanks,

greg k-h

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Jeff Garzik]

Greg KH wrote:
> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
>>  Long term, Greg seemed OK with moving the net drivers from drivers/usb/net 
>>  to drivers/usb/net, in line with the current policy of placing net drivers 
>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and me 
>>  (as you did here) would be the preferred avenue.
> 
> Speaking of which, do you want me to do this in the 2.6.22-rc1
> timeframe?  Usually big code moves like this are good to do right after
> rc1 comes out as the major churn is usually completed then.

Whatever works best for you.  There are few if any interlocking 
dependencies.

Or I could wait for you to do your 2.6.22 merge window USB push (sans 
this move), and then I can do the rename-across-directories in git in my 
normal netdev-2.6.git stream.

	Jeff

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[petkan]

> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
>>  Long term, Greg seemed OK with moving the net drivers from
>> drivers/usb/net
>>  to drivers/usb/net, in line with the current policy of placing net
>> drivers
>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
>> me
>>  (as you did here) would be the preferred avenue.
>
> Speaking of which, do you want me to do this in the 2.6.22-rc1
> timeframe?  Usually big code moves like this are good to do right after
> rc1 comes out as the major churn is usually completed then.

Sorry to interfere, but could you guys wait until tomorrow before applying
the patch to your respective GIT trees?  I'd like to check if the code is
doing the right thing and avoid patch reversal.

cheers,
Petko

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Dan Williams]

On Tue, 2007-04-24 at 20:48 +0300, petkan@nucleusys.com wrote:
> > On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
> >>  Long term, Greg seemed OK with moving the net drivers from
> >> drivers/usb/net
> >>  to drivers/usb/net, in line with the current policy of placing net
> >> drivers
> >>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
> >> me
> >>  (as you did here) would be the preferred avenue.
> >
> > Speaking of which, do you want me to do this in the 2.6.22-rc1
> > timeframe?  Usually big code moves like this are good to do right after
> > rc1 comes out as the major churn is usually completed then.
> 
> Sorry to interfere, but could you guys wait until tomorrow before applying
> the patch to your respective GIT trees?  I'd like to check if the code is
> doing the right thing and avoid patch reversal.

Original problem was that the patch I referenced in the commit message
from Jan 6 2006 switched the return value semantics from
read_mii_word().  Before the patch, read_mii_word returned 1 on success,
0 on error.  After the patch, it returns the generally accepted 0 on
success and !0 on error.

That causes set_carrier() to return immediately rather than fiddle with
netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
values, set_carrier() was not updated for the new return values.
Nothing else in the code cares about read_mii_word()'s return value
except set_carrier().

But when the card is brought up and no cable is plugged in,
intr_callback() gets called repeatedly, which itself repeatedly calls
netif_carrier_on() due to the NO_CARRIER check.  The comment there about
"NO_CARRIER kicks in on TX failure" seems accurate, because even with no
cable plugged in, and therefore no packets getting transmitted, the
NO_CARRIER check is never true on the Belkin part.  Therefore,
netif_carrier_on() is always called as a result of the failure of d[0] &
NO_CARRIER, turning carrier back on even if there is no cable plugged
in.  This bulldozes over the MII carrier_check routine too.

I don't think the intr_callback() code should ever turn the carrier
_on_, because there's that 2*HZ MII carrier check which can certainly
handle the carrier on/off stuff.

LINK_STATUS appears valid on the Belkin part too, so we can add that as
a reverse-quirk and use LINK_STATUS on parts where it works.  If you
think that the NO_CARRIER check should be in _addition_ to the
LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
check only turns carrier off.

Dan

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Petko Manolov]

In general i agree with the reasoning below.  However, isn't it better to 
remove the code that sets carrier on/off in intr_callback()?

There's a reliable way of getting the link status by reading the MII. 
After correct checking of the return value from read_mii_word(), 
set_carrier() is what is good enough.  If 2 seconds is too long of an 
interval we could reduce it to 1 second or, if needed, less.

I'd like to avoid adding additional flags per device as it will take 
forever to collect information about their "correct" behavior and update 
pegasus.h.  In short i think this part of your patch should be enough:

---

@@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
 		 * d[0].NO_CARRIER kicks in only with failed TX.
 		 * ... so monitoring with MII may be safest.
 		 */
-		if (d[0] & NO_CARRIER)
-			netif_carrier_off(net);
-		else
-			netif_carrier_on(net);
-
 		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
 		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
@@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
 	pegasus_t *pegasus = netdev_priv(net);
 	u16 tmp;

-	if (!read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
+	if (read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
 		return;

---


cheers,
Petko


On Tue, 24 Apr 2007, Dan Williams wrote:

> On Tue, 2007-04-24 at 20:48 +0300, petkan@nucleusys.com wrote:
>>> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
>>>>  Long term, Greg seemed OK with moving the net drivers from
>>>> drivers/usb/net
>>>>  to drivers/usb/net, in line with the current policy of placing net
>>>> drivers
>>>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
>>>> me
>>>>  (as you did here) would be the preferred avenue.
>>>
>>> Speaking of which, do you want me to do this in the 2.6.22-rc1
>>> timeframe?  Usually big code moves like this are good to do right after
>>> rc1 comes out as the major churn is usually completed then.
>>
>> Sorry to interfere, but could you guys wait until tomorrow before applying
>> the patch to your respective GIT trees?  I'd like to check if the code is
>> doing the right thing and avoid patch reversal.
>
> Original problem was that the patch I referenced in the commit message
> from Jan 6 2006 switched the return value semantics from
> read_mii_word().  Before the patch, read_mii_word returned 1 on success,
> 0 on error.  After the patch, it returns the generally accepted 0 on
> success and !0 on error.
>
> That causes set_carrier() to return immediately rather than fiddle with
> netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
> values, set_carrier() was not updated for the new return values.
> Nothing else in the code cares about read_mii_word()'s return value
> except set_carrier().
>
> But when the card is brought up and no cable is plugged in,
> intr_callback() gets called repeatedly, which itself repeatedly calls
> netif_carrier_on() due to the NO_CARRIER check.  The comment there about
> "NO_CARRIER kicks in on TX failure" seems accurate, because even with no
> cable plugged in, and therefore no packets getting transmitted, the
> NO_CARRIER check is never true on the Belkin part.  Therefore,
> netif_carrier_on() is always called as a result of the failure of d[0] &
> NO_CARRIER, turning carrier back on even if there is no cable plugged
> in.  This bulldozes over the MII carrier_check routine too.
>
> I don't think the intr_callback() code should ever turn the carrier
> _on_, because there's that 2*HZ MII carrier check which can certainly
> handle the carrier on/off stuff.
>
> LINK_STATUS appears valid on the Belkin part too, so we can add that as
> a reverse-quirk and use LINK_STATUS on parts where it works.  If you
> think that the NO_CARRIER check should be in _addition_ to the
> LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
> check only turns carrier off.
>
> Dan
>
>
>

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Dan Williams]

On Wed, 2007-04-25 at 17:58 +0300, Petko Manolov wrote:
> In general i agree with the reasoning below.  However, isn't it better to 
> remove the code that sets carrier on/off in intr_callback()?

I'm fine with this; whatever makes carrier status work makes me happy :)

Dan

> There's a reliable way of getting the link status by reading the MII. 
> After correct checking of the return value from read_mii_word(), 
> set_carrier() is what is good enough.  If 2 seconds is too long of an 
> interval we could reduce it to 1 second or, if needed, less.
> 
> I'd like to avoid adding additional flags per device as it will take 
> forever to collect information about their "correct" behavior and update 
> pegasus.h.  In short i think this part of your patch should be enough:
> 
> ---
> 
> @@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
>  		 * d[0].NO_CARRIER kicks in only with failed TX.
>  		 * ... so monitoring with MII may be safest.
>  		 */
> -		if (d[0] & NO_CARRIER)
> -			netif_carrier_off(net);
> -		else
> -			netif_carrier_on(net);
> -
>  		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
>  		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
> @@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
>  	pegasus_t *pegasus = netdev_priv(net);
>  	u16 tmp;
> 
> -	if (!read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
> +	if (read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
>  		return;
> 
> ---
> 
> 
> cheers,
> Petko
> 
> 
> On Tue, 24 Apr 2007, Dan Williams wrote:
> 
> > On Tue, 2007-04-24 at 20:48 +0300, petkan@nucleusys.com wrote:
> >>> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
> >>>>  Long term, Greg seemed OK with moving the net drivers from
> >>>> drivers/usb/net
> >>>>  to drivers/usb/net, in line with the current policy of placing net
> >>>> drivers
> >>>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
> >>>> me
> >>>>  (as you did here) would be the preferred avenue.
> >>>
> >>> Speaking of which, do you want me to do this in the 2.6.22-rc1
> >>> timeframe?  Usually big code moves like this are good to do right after
> >>> rc1 comes out as the major churn is usually completed then.
> >>
> >> Sorry to interfere, but could you guys wait until tomorrow before applying
> >> the patch to your respective GIT trees?  I'd like to check if the code is
> >> doing the right thing and avoid patch reversal.
> >
> > Original problem was that the patch I referenced in the commit message
> > from Jan 6 2006 switched the return value semantics from
> > read_mii_word().  Before the patch, read_mii_word returned 1 on success,
> > 0 on error.  After the patch, it returns the generally accepted 0 on
> > success and !0 on error.
> >
> > That causes set_carrier() to return immediately rather than fiddle with
> > netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
> > values, set_carrier() was not updated for the new return values.
> > Nothing else in the code cares about read_mii_word()'s return value
> > except set_carrier().
> >
> > But when the card is brought up and no cable is plugged in,
> > intr_callback() gets called repeatedly, which itself repeatedly calls
> > netif_carrier_on() due to the NO_CARRIER check.  The comment there about
> > "NO_CARRIER kicks in on TX failure" seems accurate, because even with no
> > cable plugged in, and therefore no packets getting transmitted, the
> > NO_CARRIER check is never true on the Belkin part.  Therefore,
> > netif_carrier_on() is always called as a result of the failure of d[0] &
> > NO_CARRIER, turning carrier back on even if there is no cable plugged
> > in.  This bulldozes over the MII carrier_check routine too.
> >
> > I don't think the intr_callback() code should ever turn the carrier
> > _on_, because there's that 2*HZ MII carrier check which can certainly
> > handle the carrier on/off stuff.
> >
> > LINK_STATUS appears valid on the Belkin part too, so we can add that as
> > a reverse-quirk and use LINK_STATUS on parts where it works.  If you
> > think that the NO_CARRIER check should be in _addition_ to the
> > LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
> > check only turns carrier off.
> >
> > Dan
> >
> >
> >

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Petko Manolov]

On Wed, 25 Apr 2007, Dan Williams wrote:

> On Wed, 2007-04-25 at 17:58 +0300, Petko Manolov wrote:
>> In general i agree with the reasoning below.  However, isn't it better to
>> remove the code that sets carrier on/off in intr_callback()?
>
> I'm fine with this; whatever makes carrier status work makes me happy :)

Great.  Are you going to submit the new patch or this hard labor will lay 
on my shoulders? :)


 		Petko



>> There's a reliable way of getting the link status by reading the MII.
>> After correct checking of the return value from read_mii_word(),
>> set_carrier() is what is good enough.  If 2 seconds is too long of an
>> interval we could reduce it to 1 second or, if needed, less.
>>
>> I'd like to avoid adding additional flags per device as it will take
>> forever to collect information about their "correct" behavior and update
>> pegasus.h.  In short i think this part of your patch should be enough:
>>
>> ---
>>
>> @@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
>>  		 * d[0].NO_CARRIER kicks in only with failed TX.
>>  		 * ... so monitoring with MII may be safest.
>>  		 */
>> -		if (d[0] & NO_CARRIER)
>> -			netif_carrier_off(net);
>> -		else
>> -			netif_carrier_on(net);
>> -
>>  		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
>>  		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
>> @@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
>>  	pegasus_t *pegasus = netdev_priv(net);
>>  	u16 tmp;
>>
>> -	if (!read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
>> +	if (read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
>>  		return;
>>
>> ---
>>
>>
>> cheers,
>> Petko
>>
>>
>> On Tue, 24 Apr 2007, Dan Williams wrote:
>>
>>> On Tue, 2007-04-24 at 20:48 +0300, petkan@nucleusys.com wrote:
>>>>> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
>>>>>>  Long term, Greg seemed OK with moving the net drivers from
>>>>>> drivers/usb/net
>>>>>>  to drivers/usb/net, in line with the current policy of placing net
>>>>>> drivers
>>>>>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
>>>>>> me
>>>>>>  (as you did here) would be the preferred avenue.
>>>>>
>>>>> Speaking of which, do you want me to do this in the 2.6.22-rc1
>>>>> timeframe?  Usually big code moves like this are good to do right after
>>>>> rc1 comes out as the major churn is usually completed then.
>>>>
>>>> Sorry to interfere, but could you guys wait until tomorrow before applying
>>>> the patch to your respective GIT trees?  I'd like to check if the code is
>>>> doing the right thing and avoid patch reversal.
>>>
>>> Original problem was that the patch I referenced in the commit message
>>> from Jan 6 2006 switched the return value semantics from
>>> read_mii_word().  Before the patch, read_mii_word returned 1 on success,
>>> 0 on error.  After the patch, it returns the generally accepted 0 on
>>> success and !0 on error.
>>>
>>> That causes set_carrier() to return immediately rather than fiddle with
>>> netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
>>> values, set_carrier() was not updated for the new return values.
>>> Nothing else in the code cares about read_mii_word()'s return value
>>> except set_carrier().
>>>
>>> But when the card is brought up and no cable is plugged in,
>>> intr_callback() gets called repeatedly, which itself repeatedly calls
>>> netif_carrier_on() due to the NO_CARRIER check.  The comment there about
>>> "NO_CARRIER kicks in on TX failure" seems accurate, because even with no
>>> cable plugged in, and therefore no packets getting transmitted, the
>>> NO_CARRIER check is never true on the Belkin part.  Therefore,
>>> netif_carrier_on() is always called as a result of the failure of d[0] &
>>> NO_CARRIER, turning carrier back on even if there is no cable plugged
>>> in.  This bulldozes over the MII carrier_check routine too.
>>>
>>> I don't think the intr_callback() code should ever turn the carrier
>>> _on_, because there's that 2*HZ MII carrier check which can certainly
>>> handle the carrier on/off stuff.
>>>
>>> LINK_STATUS appears valid on the Belkin part too, so we can add that as
>>> a reverse-quirk and use LINK_STATUS on parts where it works.  If you
>>> think that the NO_CARRIER check should be in _addition_ to the
>>> LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
>>> check only turns carrier off.
>>>
>>> Dan
>>>
>>>
>>>
>
>

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Jeff Garzik]

The patch went upstream ~24 hours ago: 
c43c49bd61fdb9bb085ddafcaadb17d06f95ec43

Upstream is the base for any new patches.

	Jeff

Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

[Dan Williams]

On Wed, 2007-04-25 at 18:09 +0300, Petko Manolov wrote:
> On Wed, 25 Apr 2007, Dan Williams wrote:
> 
> > On Wed, 2007-04-25 at 17:58 +0300, Petko Manolov wrote:
> >> In general i agree with the reasoning below.  However, isn't it better to
> >> remove the code that sets carrier on/off in intr_callback()?
> >
> > I'm fine with this; whatever makes carrier status work makes me happy :)
> 
> Great.  Are you going to submit the new patch or this hard labor will lay 
> on my shoulders? :)

Well, it looked like you already had one; but if you'd like I'll whip up
a new one.

Dan

> 
>  		Petko
> 
> 
> 
> >> There's a reliable way of getting the link status by reading the MII.
> >> After correct checking of the return value from read_mii_word(),
> >> set_carrier() is what is good enough.  If 2 seconds is too long of an
> >> interval we could reduce it to 1 second or, if needed, less.
> >>
> >> I'd like to avoid adding additional flags per device as it will take
> >> forever to collect information about their "correct" behavior and update
> >> pegasus.h.  In short i think this part of your patch should be enough:
> >>
> >> ---
> >>
> >> @@ -847,10 +848,16 @@ static void intr_callback(struct urb *urb)
> >>  		 * d[0].NO_CARRIER kicks in only with failed TX.
> >>  		 * ... so monitoring with MII may be safest.
> >>  		 */
> >> -		if (d[0] & NO_CARRIER)
> >> -			netif_carrier_off(net);
> >> -		else
> >> -			netif_carrier_on(net);
> >> -
> >>  		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
> >>  		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
> >> @@ -950,7 +957,7 @@ static void set_carrier(struct net_device *net)
> >>  	pegasus_t *pegasus = netdev_priv(net);
> >>  	u16 tmp;
> >>
> >> -	if (!read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
> >> +	if (read_mii_word(pegasus, pegasus->phy, MII_BMSR, &tmp))
> >>  		return;
> >>
> >> ---
> >>
> >>
> >> cheers,
> >> Petko
> >>
> >>
> >> On Tue, 24 Apr 2007, Dan Williams wrote:
> >>
> >>> On Tue, 2007-04-24 at 20:48 +0300, petkan@nucleusys.com wrote:
> >>>>> On Tue, Apr 24, 2007 at 12:49:12PM -0400, Jeff Garzik wrote:
> >>>>>>  Long term, Greg seemed OK with moving the net drivers from
> >>>>>> drivers/usb/net
> >>>>>>  to drivers/usb/net, in line with the current policy of placing net
> >>>>>> drivers
> >>>>>>  in drivers/net/*, bus agnostic.  After that move, sending to netdev and
> >>>>>> me
> >>>>>>  (as you did here) would be the preferred avenue.
> >>>>>
> >>>>> Speaking of which, do you want me to do this in the 2.6.22-rc1
> >>>>> timeframe?  Usually big code moves like this are good to do right after
> >>>>> rc1 comes out as the major churn is usually completed then.
> >>>>
> >>>> Sorry to interfere, but could you guys wait until tomorrow before applying
> >>>> the patch to your respective GIT trees?  I'd like to check if the code is
> >>>> doing the right thing and avoid patch reversal.
> >>>
> >>> Original problem was that the patch I referenced in the commit message
> >>> from Jan 6 2006 switched the return value semantics from
> >>> read_mii_word().  Before the patch, read_mii_word returned 1 on success,
> >>> 0 on error.  After the patch, it returns the generally accepted 0 on
> >>> success and !0 on error.
> >>>
> >>> That causes set_carrier() to return immediately rather than fiddle with
> >>> netif_carrier_*.  When the Jan 6 2006 patch went in changing the return
> >>> values, set_carrier() was not updated for the new return values.
> >>> Nothing else in the code cares about read_mii_word()'s return value
> >>> except set_carrier().
> >>>
> >>> But when the card is brought up and no cable is plugged in,
> >>> intr_callback() gets called repeatedly, which itself repeatedly calls
> >>> netif_carrier_on() due to the NO_CARRIER check.  The comment there about
> >>> "NO_CARRIER kicks in on TX failure" seems accurate, because even with no
> >>> cable plugged in, and therefore no packets getting transmitted, the
> >>> NO_CARRIER check is never true on the Belkin part.  Therefore,
> >>> netif_carrier_on() is always called as a result of the failure of d[0] &
> >>> NO_CARRIER, turning carrier back on even if there is no cable plugged
> >>> in.  This bulldozes over the MII carrier_check routine too.
> >>>
> >>> I don't think the intr_callback() code should ever turn the carrier
> >>> _on_, because there's that 2*HZ MII carrier check which can certainly
> >>> handle the carrier on/off stuff.
> >>>
> >>> LINK_STATUS appears valid on the Belkin part too, so we can add that as
> >>> a reverse-quirk and use LINK_STATUS on parts where it works.  If you
> >>> think that the NO_CARRIER check should be in _addition_ to the
> >>> LINK_STATUS check, that's fine with me, provided that the NO_CARRIER
> >>> check only turns carrier off.
> >>>
> >>> Dan
> >>>
> >>>
> >>>
> >
> >

[PATCH] usb-net/pegasus: simplify carrier detection

[Dan Williams]

Simplify pegasus carrier detection; rely only on the periodic MII
polling.  Reverts pieces of c43c49bd61fdb9bb085ddafcaadb17d06f95ec43.

Signed-off-by: Dan Williams <dcbw@redhat.com>

--- a/drivers/usb/net/pegasus.h	2007-04-25 21:21:00.000000000 -0400
+++ b/drivers/usb/net/pegasus.h	2007-04-25 21:21:13.000000000 -0400
@@ -11,7 +11,6 @@
 
 #define	PEGASUS_II		0x80000000
 #define	HAS_HOME_PNA		0x40000000
-#define	TRUST_LINK_STATUS	0x20000000
 
 #define	PEGASUS_MTU		1536
 #define	RX_SKBS			4
@@ -204,7 +203,7 @@
 PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
 		DEFAULT_GPIO_RESET | PEGASUS_II )
 PEGASUS_DEV( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121,
-		DEFAULT_GPIO_RESET | PEGASUS_II | TRUST_LINK_STATUS )
+		DEFAULT_GPIO_RESET | PEGASUS_II )
 PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
 		DEFAULT_GPIO_RESET )
 PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,
--- a/drivers/usb/net/pegasus.c	2007-04-25 21:20:32.000000000 -0400
+++ b/drivers/usb/net/pegasus.c	2007-04-25 21:22:15.000000000 -0400
@@ -848,16 +848,6 @@
 		 * d[0].NO_CARRIER kicks in only with failed TX.
 		 * ... so monitoring with MII may be safest.
 		 */
-		if (pegasus->features & TRUST_LINK_STATUS) {
-			if (d[5] & LINK_STATUS)
-				netif_carrier_on(net);
-			else
-				netif_carrier_off(net);
-		} else {
-			/* Never set carrier _on_ based on ! NO_CARRIER */
-			if (d[0] & NO_CARRIER)
-				netif_carrier_off(net);	
-		}
 
 		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
 		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];

Re: [PATCH] usb-net/pegasus: simplify carrier detection

[Petko Manolov]

Good man, I owe you a beer. :)


cheers,
Petko



On Wed, 25 Apr 2007, Dan Williams wrote:

> Simplify pegasus carrier detection; rely only on the periodic MII
> polling.  Reverts pieces of c43c49bd61fdb9bb085ddafcaadb17d06f95ec43.
>
> Signed-off-by: Dan Williams <dcbw@redhat.com>
>
> --- a/drivers/usb/net/pegasus.h	2007-04-25 21:21:00.000000000 -0400
> +++ b/drivers/usb/net/pegasus.h	2007-04-25 21:21:13.000000000 -0400
> @@ -11,7 +11,6 @@
>
> #define	PEGASUS_II		0x80000000
> #define	HAS_HOME_PNA		0x40000000
> -#define	TRUST_LINK_STATUS	0x20000000
>
> #define	PEGASUS_MTU		1536
> #define	RX_SKBS			4
> @@ -204,7 +203,7 @@
> PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
> 		DEFAULT_GPIO_RESET | PEGASUS_II )
> PEGASUS_DEV( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121,
> -		DEFAULT_GPIO_RESET | PEGASUS_II | TRUST_LINK_STATUS )
> +		DEFAULT_GPIO_RESET | PEGASUS_II )
> PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
> 		DEFAULT_GPIO_RESET )
> PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,
> --- a/drivers/usb/net/pegasus.c	2007-04-25 21:20:32.000000000 -0400
> +++ b/drivers/usb/net/pegasus.c	2007-04-25 21:22:15.000000000 -0400
> @@ -848,16 +848,6 @@
> 		 * d[0].NO_CARRIER kicks in only with failed TX.
> 		 * ... so monitoring with MII may be safest.
> 		 */
> -		if (pegasus->features & TRUST_LINK_STATUS) {
> -			if (d[5] & LINK_STATUS)
> -				netif_carrier_on(net);
> -			else
> -				netif_carrier_off(net);
> -		} else {
> -			/* Never set carrier _on_ based on ! NO_CARRIER */
> -			if (d[0] & NO_CARRIER)
> -				netif_carrier_off(net);
> -		}
>
> 		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
> 		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
>
>
>

Re: [PATCH] usb-net/pegasus: simplify carrier detection

[Jeff Garzik]

Dan Williams wrote:
> Simplify pegasus carrier detection; rely only on the periodic MII
> polling.  Reverts pieces of c43c49bd61fdb9bb085ddafcaadb17d06f95ec43.
> 
> Signed-off-by: Dan Williams <dcbw@redhat.com>
> 
> --- a/drivers/usb/net/pegasus.h	2007-04-25 21:21:00.000000000 -0400
> +++ b/drivers/usb/net/pegasus.h	2007-04-25 21:21:13.000000000 -0400
> @@ -11,7 +11,6 @@
>  
>  #define	PEGASUS_II		0x80000000
>  #define	HAS_HOME_PNA		0x40000000
> -#define	TRUST_LINK_STATUS	0x20000000
>  
>  #define	PEGASUS_MTU		1536
>  #define	RX_SKBS			4
> @@ -204,7 +203,7 @@
>  PEGASUS_DEV( "Allied Telesyn Int. AT-USB100", VENDOR_ALLIEDTEL, 0xb100,
>  		DEFAULT_GPIO_RESET | PEGASUS_II )
>  PEGASUS_DEV( "Belkin F5D5050 USB Ethernet", VENDOR_BELKIN, 0x0121,
> -		DEFAULT_GPIO_RESET | PEGASUS_II | TRUST_LINK_STATUS )
> +		DEFAULT_GPIO_RESET | PEGASUS_II )
>  PEGASUS_DEV( "Billionton USB-100", VENDOR_BILLIONTON, 0x0986,
>  		DEFAULT_GPIO_RESET )
>  PEGASUS_DEV( "Billionton USBLP-100", VENDOR_BILLIONTON, 0x0987,
> --- a/drivers/usb/net/pegasus.c	2007-04-25 21:20:32.000000000 -0400
> +++ b/drivers/usb/net/pegasus.c	2007-04-25 21:22:15.000000000 -0400
> @@ -848,16 +848,6 @@
>  		 * d[0].NO_CARRIER kicks in only with failed TX.
>  		 * ... so monitoring with MII may be safest.
>  		 */
> -		if (pegasus->features & TRUST_LINK_STATUS) {
> -			if (d[5] & LINK_STATUS)
> -				netif_carrier_on(net);
> -			else
> -				netif_carrier_off(net);
> -		} else {
> -			/* Never set carrier _on_ based on ! NO_CARRIER */
> -			if (d[0] & NO_CARRIER)
> -				netif_carrier_off(net);	
> -		}
>  
>  		/* bytes 3-4 == rx_lostpkt, reg 2E/2F */
>  		pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];

applied