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]

Message-ID: <Pine.LNX.4.64.0704251808210.16498@bender.nucleusys.com>
Date:	Wed, 25 Apr 2007 18:09:11 +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

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