kern/128766: [ PATCH ] VGE's [rt]xcsum can't be switched off

Pyun YongHyeon pyunyh at gmail.com
Tue Nov 11 02:43:48 PST 2008


On Tue, Nov 11, 2008 at 10:16:26AM +0100, Dan Lukes wrote:
 > yongari at FreeBSD.org napsal/wrote, On 11/11/08 02:27:
 > >It seems that the hardware does not require reinitialization when
 > >checksum offload configuration is changed.
 > >Would you try patch at the followng URL?
 > 
 > True, vge_init() is not necesarry. Your code ...
 > 
 > ===================================================================
 > 	if ((mask & IFCAP_TXCSUM) != 0 &&
 > 	    (ifp->if_capabilities & IFCAP_TXCSUM) != 0) {
 > 		ifp->if_capenable ^= IFCAP_TXCSUM;
 > 		if ((ifp->if_capenable & IFCAP_TXCSUM) != 0)
 > 			ifp->if_hwassist |= VGE_CSUM_FEATURES;
 > 		else
 > 			ifp->if_hwassist &= ~VGE_CSUM_FEATURES;
 > 	}
 > 	if ((mask & IFCAP_RXCSUM) != 0 &&
 > 	    (ifp->if_capabilities & IFCAP_RXCSUM) != 0)
 > 		ifp->if_capenable ^= IFCAP_RXCSUM;
 > 	}
 > ===================================================================
 > 
 > ... will work, but is suboptimal.
 > 
 > The tests (ifp->if_capabilities & IFCAP_TXCSUM) and 
 > (ifp->if_capabilities & IFCAP_RXCSUM) is true everytime because the test 
 > is done already in upper layer (see net.c:ifhwioctl(), it return EINVAL 
 > when someone try to set capabilities not offered by driver)
 > 
 > Code with such tests removed ...
 > 
 > ===================================================================
 > 	if (mask & IFCAP_TXCSUM) != 0) {
 > 		ifp->if_capenable ^= IFCAP_TXCSUM;
 > 		if ((ifp->if_capenable & IFCAP_TXCSUM) != 0)
 > 			ifp->if_hwassist |= VGE_CSUM_FEATURES;
 > 		else
 > 			ifp->if_hwassist &= ~VGE_CSUM_FEATURES;
 > 	}
 > 	if (mask & IFCAP_RXCSUM != 0)
 > 		ifp->if_capenable ^= IFCAP_RXCSUM;
 > 	}
 > ===================================================================
 > 

Yeah, ATM your code is correct for userland side. But there is no
guarantee that all vge(4) hardwares have the same capabilities. If
Tx checksum offload was broken for specific revision, driver should
check whether the capability is available against the hardware.
There are still a couple of drivers in tree that misuses this kind
of checking.
Also remember some subsystem still directly calls driver ioctls
without relying on ifhwioctl() so it still needs the check
(e.g. bridge(4)).

 > ... is OK but ~VGE_CSUM_FEATURES shall be ~VGE_CSUM_FEATURE instead 
 > (-VGE_CSUM_FEATURES may not be equal to ~VGE_CSUM_FEATURES althought it 
 > is on most platforms).
 > 

Sorry I don't see the point. vge(4) supports Tx IP/TCP/UDP checksum
offload and it's just represention of the feature in driver. Why do
you need to specify -VGE_CSUM_FEATURES?

 > In advance, we shall catch attempt to modify capabilities that are 
 > supported by driver, but that driver doesn't support to be switched off.
 > 

As you already might know vge(4) is in pretty bad shape. It doesn't
even allow VLAN tag control modification and does not inform
updated capability to VLAN layer as well as bus_dma(9) related bugs.
I didn't touch DEVICE_POLLING case as it may require more complete
rewrite of ioctl handler which in turn requires more driver
fix/cleanup.

 > If we shall do
 > mask ^= IFCAP_POLLING
 > on the end of IFCAP_POLLING handling and
 > mask ^= (IFCAP_TXCSUM | IFCAP_RXCSUM)
 > on the end of CSUM handling then we can do
 > 
 > if (mask != 0)
 > 	return(ENOTSUPP)
 > on the end of SIOCSIFCAP.
 > 

See above. I think you can't do that. Drivers should always check
against its usable capabilities. And I'm not sure ENOTSUPP is
right error code. I guess if ioctl itself is not available,
ENOTSUPP would be correct one otherwise it can return EINVAL for
handled (invalid) ioctl call, ENOTTY for unhandled ioctl call.

 > The resulting patch is attached.
 > 
 > 					Dan
 > 

 > --- /usr/src/sys/dev/vge/if_vgevar.h.orig	2008-11-10 23:53:04.000000000 +0100
 > +++ /usr/src/sys/dev/vge/if_vgevar.h	2008-11-11 02:19:20.000000000 +0100
 > @@ -2228,16 +2228,21 @@
 >  				VGE_UNLOCK(sc);
 >  			}
 >  		}
 > +		mask ^= IFCAP_POLLING;
 >  #endif /* DEVICE_POLLING */
 > -		if (mask & IFCAP_HWCSUM) {
 > -			ifp->if_capenable |= ifr->ifr_reqcap & (IFCAP_HWCSUM);
 > +		if (mask & IFCAP_TXCSUM) {
 > +			ifp->if_capenable ^= IFCAP_TXCSUM;
 >  			if (ifp->if_capenable & IFCAP_TXCSUM)
 > -				ifp->if_hwassist = VGE_CSUM_FEATURES;
 > +				ifp->if_hwassist |= VGE_CSUM_FEATURES;
 >  			else
 > -				ifp->if_hwassist = 0;
 > -			if (ifp->if_drv_flags & IFF_DRV_RUNNING)
 > -				vge_init(sc);
 > +				ifp->if_hwassist &= ~VGE_CSUM_FEATURES;
 >  		}
 > +		if (mask & IFCAP_RXCSUM) {
 > +			ifp->if_capenable ^= IFCAP_RXCSUM;
 > +		}
 > +		mask ^= (IFCAP_TXCSUM | IFCAP_RXCSUM);
 > +		if (mask != 0)
 > +			return(ENODEV);
 >  	    }
 >  		break;
 >  	default:


-- 
Regards,
Pyun YongHyeon


More information about the freebsd-bugs mailing list