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

[Pyun YongHyeon]

On Tue, Nov 11, 2008 at 01:42:31PM +0100, Dan Lukes wrote:
 > Pyun YongHyeon napsal/wrote, On 11/11/08 11:16:
 > > > 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.
 > 
 > It check it against if_capabilites, eg against capabilites supported by 
 > a instance of driver as if_capabilities are capabilities of such 
 > particular hardware.
 > 
 > >If
 > >Tx checksum offload was broken for specific revision, driver should
 > >check whether the capability is available against the hardware.
 > 
 > If txcsum was broken for specific revision then driver shall not mark 
 > the txcsum to be supported in if_capabilities.
 > 

Correct. That is one of reason why I check if_capabilities in
ioctl() without relying on ifhwioctl().(as you said ifhwioctl()
checks if_capabilities but I'd like more tight check due to other
part of kernel. Adding an additional if_capabilities check wouldn't
hurt and it's not in fast path.) 
This also separates chipset specific code from capability ioctl.
Without that I would have embedded another variable into softc.

 > >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)).
 > 
 > True. Either drivers or callers shall do such test. It's questionable if 
 > all drivers shall be corrected or bridge(4) code, but you are comitter, 
 > it's your decision.
 > 

[...]

 > > > 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.
 > 
 > I'm speaking about something diferent. if_capabilities are capabilities 
 > for such instance of driver. But i'm not speaking about the request to 
 > change of capability that are not supported by driver at all. I'm 
 > speaking about the request to change the capability that is supported 
 > but can't be turned off.
 > 

Ok. does the patch I posted work for you?
(I hoped I finish vge(4) cleanups but half-broken VT6122 I have
made it difficult to do that.)

-- 
Regards,
Pyun YongHyeon