svn commit: r308345 - head/sys/dev/e1000

[Sean Bruno]

On 11/05/16 17:16, John Baldwin wrote:
> On Saturday, November 05, 2016 04:30:43 PM Sean Bruno wrote:
>> Author: sbruno
>> Date: Sat Nov  5 16:30:42 2016
>> New Revision: 308345
>> URL: https://svnweb.freebsd.org/changeset/base/308345
>>
>> Log:
>>   r295133 attempted to deactivate TSO in the 100Mbit link case with this
>>   adapter to work around bugs in TSO handling at this speed.
>>   
>>   em_init_locked is called during first boot of the adapter and will
>>   see that link_speed is unitialized, effectively turning off tso for
>>   all cards at all speeds, which I believe was *not* the intent.
>>   
>>   Move the handling of TSO deactivation to the link handler where we can
>>   more effectively make the decision about what to do.  In addition,
>>   completely purge the TSO capabilities instead of disabling just CSUM_TSO.
>>   
>>   Thanks to jhb for explanation of the hw capabilites api.
>>   
>>   Thanks to royger and cognet for testing the 100Mbit failure case to
>>   ensure that their adapters do indeed still work.
>>   
>>   MFC after:	1 week
>>   Sponsored by:	Limelight Networks
>>
>> Modified:
>>   head/sys/dev/e1000/if_em.c
>>
>> Modified: head/sys/dev/e1000/if_em.c
>> ==============================================================================
>> --- head/sys/dev/e1000/if_em.c	Sat Nov  5 16:23:33 2016	(r308344)
>> +++ head/sys/dev/e1000/if_em.c	Sat Nov  5 16:30:42 2016	(r308345)
>> @@ -369,11 +369,6 @@ MODULE_DEPEND(em, netmap, 1, 1, 1);
>>  #define MAX_INTS_PER_SEC	8000
>>  #define DEFAULT_ITR		(1000000000/(MAX_INTS_PER_SEC * 256))
>>  
>> -/* Allow common code without TSO */
>> -#ifndef CSUM_TSO
>> -#define CSUM_TSO	0
>> -#endif
>> -
>>  #define TSO_WORKAROUND	4
>>  
>>  static SYSCTL_NODE(_hw, OID_AUTO, em, CTLFLAG_RD, 0, "EM driver parameters");
>> @@ -1396,15 +1391,9 @@ em_init_locked(struct adapter *adapter)
>>  	if_clearhwassist(ifp);
>>  	if (if_getcapenable(ifp) & IFCAP_TXCSUM)
>>  		if_sethwassistbits(ifp, CSUM_TCP | CSUM_UDP, 0);
>> -	/* 
>> -	** There have proven to be problems with TSO when not
>> -	** at full gigabit speed, so disable the assist automatically
>> -	** when at lower speeds.  -jfv
>> -	*/
>> -	if (if_getcapenable(ifp) & IFCAP_TSO4) {
>> -		if (adapter->link_speed == SPEED_1000)
>> -			if_sethwassistbits(ifp, CSUM_TSO, 0);
>> -	}
>> +
>> +	if (if_getcapenable(ifp) & IFCAP_TSO4)
>> +		if_sethwassistbits(ifp, CSUM_TSO, 0);
> 
> Does this always disable TSO?  Should this part be removed entirely?
> (That is, it seems like this would disable TSO even on Gigabit links).
> 

I was confused by this question.  The old code *always* disabled TSO
because link_speed was always 0 here on boot.  My intention is to ensure
that CSUM_TSO is set if IFCAP_TSO4 is set.


>>  	/* Configure for OS presence */
>>  	em_init_manageability(adapter);
>> @@ -2412,6 +2401,18 @@ em_update_link_status(struct adapter *ad
>>  	if (link_check && (adapter->link_active == 0)) {
>>  		e1000_get_speed_and_duplex(hw, &adapter->link_speed,
>>  		    &adapter->link_duplex);
>> +		/* 
>> +		** There have proven to be problems with TSO when not
>> +		** at full gigabit speed, so disable the assist automatically
>> +		** when at lower speeds.  -jfv
>> +		*/
>> +		if (adapter->link_speed != SPEED_1000) {
>> +			if_sethwassistbits(ifp, 0, CSUM_TSO);
>> +			if_setcapenablebit(ifp, 0, IFCAP_TSO4);
>> +        		if_setcapabilitiesbit(ifp, 0, IFCAP_TSO4);
>> +
>> +		}
> 
> Even though I suggested it, I wonder if it wouldn't be better to only
> modify if_capenable and not if_capabilities, that way the admin can
> decide to use 'ifconfig em0 tso' to force it back on (e.g. if moving
> an adapter from 100 to 1G).
> 

I spent several hours trying to come up with logic that would allow me
to allow the user to do this.  I am open to suggestions here, but it
would require quite a bit more finesse than my "big hammer" approach.

sean

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 585 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20161107/0a235cf8/attachment.sig>