From owner-freebsd-bugs@FreeBSD.ORG Tue Nov 11 10:43:48 2008 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 7244A106567A for ; Tue, 11 Nov 2008 10:43:48 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: from rv-out-0506.google.com (rv-out-0506.google.com [209.85.198.235]) by mx1.freebsd.org (Postfix) with ESMTP id 2F7188FC19 for ; Tue, 11 Nov 2008 10:43:48 +0000 (UTC) (envelope-from pyunyh@gmail.com) Received: by rv-out-0506.google.com with SMTP id b25so2845556rvf.43 for ; Tue, 11 Nov 2008 02:43:47 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:received:received:date:from :to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=+8W75xwExA6N9KG8CZ7Tdp3eNtLf+q4/DSQeJmmSL3k=; b=MYBoj2lMorJVgL4e7g1jGtUcyj7xF/FpZMl0We9W4UmPsOnqtKVmhmRPv3azW4GcAG 09yzIKeP41bjSCjqVMCpLO5fIX5YE5s0IVWg41HJ3e5IeqzW7HKmMrfIiLoH2QfsOASR c9Rfw65QEwlh08t1r2Sl9loVIwKuYEGVpSfqA= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; b=HyzQ5RPDBau4Da0tzBSfgCIhITKWeBHYNKJP3ghAK9lOiwOMd1Q8VNT2pz82Gs4SDs ZaMfNAKB3doWgKB7Yh1vzgU+MVIMdukVZu3PwK6DBXbxSsoxqMG3BovPDuueIfmrHV0F H99EALPLIpCqb28goVPsrqDcMWYDfLuC55m9I= Received: by 10.140.165.21 with SMTP id n21mr4142996rve.240.1226398690369; Tue, 11 Nov 2008 02:18:10 -0800 (PST) Received: from michelle.cdnetworks.co.kr ([211.53.35.84]) by mx.google.com with ESMTPS id b8sm24603364rvf.3.2008.11.11.02.18.07 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 11 Nov 2008 02:18:09 -0800 (PST) Received: from michelle.cdnetworks.co.kr (localhost.cdnetworks.co.kr [127.0.0.1]) by michelle.cdnetworks.co.kr (8.13.5/8.13.5) with ESMTP id mABAG7N4027952 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 11 Nov 2008 19:16:07 +0900 (KST) (envelope-from pyunyh@gmail.com) Received: (from yongari@localhost) by michelle.cdnetworks.co.kr (8.13.5/8.13.5/Submit) id mABAG7Zg027951; Tue, 11 Nov 2008 19:16:07 +0900 (KST) (envelope-from pyunyh@gmail.com) Date: Tue, 11 Nov 2008 19:16:07 +0900 From: Pyun YongHyeon To: Dan Lukes Message-ID: <20081111101606.GB26349@cdnetworks.co.kr> References: <200811110127.mAB1RiQV095192@freefall.freebsd.org> <49194D6A.30807@obluda.cz> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <49194D6A.30807@obluda.cz> User-Agent: Mutt/1.4.2.1i Cc: freebsd-bugs@FreeBSD.org, yongari@FreeBSD.org Subject: Re: kern/128766: [ PATCH ] VGE's [rt]xcsum can't be switched off X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: pyunyh@gmail.com List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Nov 2008 10:43:48 -0000 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