From owner-freebsd-hackers@FreeBSD.ORG Fri May 21 06:58:09 2004 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0AAE016A4CF; Fri, 21 May 2004 06:58:09 -0700 (PDT) Received: from comp.chem.msu.su (comp.chem.msu.su [158.250.32.97]) by mx1.FreeBSD.org (Postfix) with ESMTP id 331EF43D31; Fri, 21 May 2004 06:58:05 -0700 (PDT) (envelope-from yar@comp.chem.msu.su) Received: from comp.chem.msu.su (localhost [127.0.0.1]) by comp.chem.msu.su (8.12.9p2/8.12.9) with ESMTP id i4LDw03F060447; Fri, 21 May 2004 17:58:00 +0400 (MSD) (envelope-from yar@comp.chem.msu.su) Received: (from yar@localhost) by comp.chem.msu.su (8.12.9p2/8.12.9/Submit) id i4LDw04F060442; Fri, 21 May 2004 17:58:00 +0400 (MSD) (envelope-from yar) Date: Fri, 21 May 2004 17:58:00 +0400 From: Yar Tikhiy To: hackers@freebsd.org, net@freebsd.org Message-ID: <20040521135759.GE57132@comp.chem.msu.su> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.6i Subject: Bugfix for checksum offload in bge(4) X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 21 May 2004 13:58:09 -0000 Hi folks, While sweeping network interface drivers for incorrect usage of the capabilities framework, I noticed some bugs in bge(4). Unfortunately, I have no such card and I don't know its internals. Therefore I made a patch fixing hw-independent bugs and marking some questionable spots. It would be nice if somebody possessing the knowledge on bge(4) reviewed this patch and used it to really fix the things. If nobody undertakes that, I'll just commit my change since it doesn't seem to affect the functionality while it brings the code into the shape. Thanks! The issues addressed by the patch are as follows: 1. The code responsible for initiating RX csum offload is ifdef'd out with a comment reading there were problems with that. Despite this fact, the RX csum offload is marked as supported and active in the if_cap* fields, and a user is allowed to toggle the bit, which is misleading. 2. The code for RX csum offload should not consult ifp->if_hwassist. The latter field is for informing the TCP/IP stack about available _TX_ offload features. The driver itself should use ifp->if_capenable to see which capabilities are active. 3. Presently bge(4) doesn't advertise its ability to compute the TCP/UDP checksum over a fragmented packet. However, there's code handling this case. Consequently, that code is ineffective. Since I knew nothing about the availability of the feature in bge(4), I just marked the spot with an XXX comment. 4. A network interface driver should keep if_hwassist in sync with if_capenable when the latter is altered through ioctl(); bge(4) fails to do so. -- Yar Index: if_bge.c =================================================================== RCS file: /home/ncvs/src/sys/dev/bge/if_bge.c,v retrieving revision 1.63 diff -u -p -r1.63 if_bge.c --- if_bge.c 13 Jan 2004 11:31:09 -0000 1.63 +++ if_bge.c 21 May 2004 13:23:36 -0000 @@ -2371,7 +2371,8 @@ bge_attach(dev) ifp->if_mtu = ETHERMTU; ifp->if_snd.ifq_maxlen = BGE_TX_RING_CNT - 1; ifp->if_hwassist = BGE_CSUM_FEATURES; - ifp->if_capabilities = IFCAP_HWCSUM | IFCAP_VLAN_HWTAGGING | + /* XXX the code for RX csum offload is disabled for now */ + ifp->if_capabilities = IFCAP_TXCSUM | IFCAP_VLAN_HWTAGGING | IFCAP_VLAN_MTU; ifp->if_capenable = ifp->if_capabilities; @@ -2722,7 +2723,7 @@ bge_rxeof(sc) m->m_pkthdr.rcvif = ifp; #if 0 /* currently broken for some packets, possibly related to TCP options */ - if (ifp->if_hwassist) { + if (ifp->if_capenable & IFCAP_RXCSUM) { m->m_pkthdr.csum_flags |= CSUM_IP_CHECKED; if ((cur_rx->bge_ip_csum ^ 0xffff) == 0) m->m_pkthdr.csum_flags |= CSUM_IP_VALID; @@ -3136,6 +3137,11 @@ bge_start_locked(ifp) /* * XXX + * The code inside the if() block is never reached since we + * must mark CSUM_IP_FRAGS in our if_hwassist to start getting + * requests to checksum TCP/UDP in a fragmented packet. + * + * XXX * safety overkill. If this is a fragmented packet chain * with delayed TCP/UDP checksums, then only encapsulate * it if we have enough descriptors to handle the entire @@ -3478,11 +3484,13 @@ bge_ioctl(ifp, command, data) break; case SIOCSIFCAP: mask = ifr->ifr_reqcap ^ ifp->if_capenable; - if (mask & IFCAP_HWCSUM) { - if (IFCAP_HWCSUM & ifp->if_capenable) - ifp->if_capenable &= ~IFCAP_HWCSUM; + /* XXX the code for RX csum offload is disabled for now */ + if (mask & IFCAP_TXCSUM) { + ifp->if_capenable ^= IFCAP_TXCSUM; + if (IFCAP_TXCSUM & ifp->if_capenable) + ifp->if_hwassist = BGE_CSUM_FEATURES; else - ifp->if_capenable |= IFCAP_HWCSUM; + ifp->if_hwassist = 0; } error = 0; break;