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

[John Baldwin]

On Monday, November 07, 2016 07:02:23 AM Sean Bruno wrote:
> 
> 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.

Oh, I misread the code.   It is setting CSUM_TSO and clearing 0.  I think you
probably don't need this as the flag can only be set in capenable by an
ioctl handler that sets the flag to true (and the calling ioctl code should
update hwassist for you).

Oh, the driver does 'if_clearhwassist()' above this.  How... unfortunate.
I think that since you are now handling this in 'update_link_status' that
you can instead remove this whole block starting with 'if_clearhwassist()'
and ending with the 'if_sethwassist' for CSUM_TSO.  That is, you can now
leave if_hwasssist alone in em_init_locked() and not change it.

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

I think you just need to remove 'if_setcapabilities' here and leave the rest
of the logic as-is.

-- 
John Baldwin