usb-net/pegasus: fix pegasus carrier detection

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.

Quoted 4 lines · Jeff Garzik

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.

Quoted 7 lines

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.

Quoted 9 lines · Jeff Garzik

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 9 lines · On Tue, 2007-04-24 at 20:48 +0300, [email protected] wrote:

Quoted 9 lines · Jeff Garzik

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

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:

---

drivers/usb/net/pegasus.c +1 -7

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

Quoted 54 lines · Dan Williams

Quoted 9 lines · On Tue, 2007-04-24 at 20:48 +0300, [email protected] wrote:

Quoted 9 lines · Jeff Garzik

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 2 lines · 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()?

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

Quoted 93 lines

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:

---

(quoted diff) +1 -7

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

Quoted 54 lines · Dan Williams

Quoted 9 lines · On Tue, 2007-04-24 at 20:48 +0300, [email protected] wrote:

Quoted 9 lines · Jeff Garzik

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 5 lines · Dan Williams

Quoted 2 lines · 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()?

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

Quoted 95 lines

Quoted 93 lines

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:

---

(quoted diff) +1 -7

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

Quoted 54 lines · Dan Williams

Quoted 9 lines · On Tue, 2007-04-24 at 20:48 +0300, [email protected] wrote:

Quoted 9 lines · Jeff Garzik

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

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

Signed-off-by Dan Williams <[email protected]>

drivers/usb/net/pegasus.h +1 -2

@@ -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,

drivers/usb/net/pegasus.c +0 -10

@@ -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];

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

Signed-off-by Dan Williams <[email protected]>

drivers/usb/net/pegasus.h +1 -2

@@ -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,

drivers/usb/net/pegasus.c +0 -3

@@ -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);