Date: Tue, 11 Nov 2008 19:16:07 +0900 From: Pyun YongHyeon <pyunyh@gmail.com> To: Dan Lukes <dan@obluda.cz> Cc: freebsd-bugs@FreeBSD.org, yongari@FreeBSD.org Subject: Re: kern/128766: [ PATCH ] VGE's [rt]xcsum can't be switched off Message-ID: <20081111101606.GB26349@cdnetworks.co.kr> In-Reply-To: <49194D6A.30807@obluda.cz> References: <200811110127.mAB1RiQV095192@freefall.freebsd.org> <49194D6A.30807@obluda.cz>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Nov 11, 2008 at 10:16:26AM +0100, Dan Lukes wrote: > yongari@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20081111101606.GB26349>