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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 25 Apr 2007 17:58:35 +0300 (EEST)
From:	Petko Manolov <petkan@...leusys.com>
To:	Dan Williams <dcbw@...hat.com>
cc:	Greg KH <greg@...ah.com>, Jeff Garzik <jeff@...zik.org>,
	netdev@...r.kernel.org
Subject: Re: [PATCH] usb-net/pegasus: fix pegasus carrier detection

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@...leusys.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
>
>
>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html