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

[John Baldwin]

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

>  	/* 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).

-- 
John Baldwin