From owner-freebsd-bugs@FreeBSD.ORG Tue Nov 11 09:16:30 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 53EA91065674; Tue, 11 Nov 2008 09:16:30 +0000 (UTC) (envelope-from dan@obluda.cz) Received: from smtp1.kolej.mff.cuni.cz (smtp1.kolej.mff.cuni.cz [IPv6:2001:718:1e03:a01::a]) by mx1.freebsd.org (Postfix) with ESMTP id 961DA8FC1E; Tue, 11 Nov 2008 09:16:29 +0000 (UTC) (envelope-from dan@obluda.cz) X-Envelope-From: dan@obluda.cz Received: from kgw.obluda.cz (openvpn.ms.mff.cuni.cz [195.113.20.87]) by smtp1.kolej.mff.cuni.cz (8.14.2/8.14.2) with ESMTP id mAB9GQrI060443; Tue, 11 Nov 2008 10:16:27 +0100 (CET) (envelope-from dan@obluda.cz) Message-ID: <49194D6A.30807@obluda.cz> Date: Tue, 11 Nov 2008 10:16:26 +0100 From: Dan Lukes User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.8.1.17) Gecko/20081021 SeaMonkey/1.1.12 MIME-Version: 1.0 To: yongari@FreeBSD.org References: <200811110127.mAB1RiQV095192@freefall.freebsd.org> In-Reply-To: <200811110127.mAB1RiQV095192@freefall.freebsd.org> Content-Type: multipart/mixed; boundary="------------040005090307080703020404" Cc: freebsd-bugs@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 List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Nov 2008 09:16:30 -0000 This is a multi-part message in MIME format. --------------040005090307080703020404 Content-Type: text/plain; charset=ISO-8859-2; format=flowed Content-Transfer-Encoding: 7bit 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; } =================================================================== ... 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). In advance, we shall catch attempt to modify capabilities that are supported by driver, but that driver doesn't support to be switched off. 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. The resulting patch is attached. Dan --------------040005090307080703020404 Content-Type: text/plain; name="x" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="x" --- /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: --------------040005090307080703020404--