Date: Fri, 21 May 2004 17:58:00 +0400 From: Yar Tikhiy <yar@comp.chem.msu.su> To: hackers@freebsd.org, net@freebsd.org Subject: Bugfix for checksum offload in bge(4) Message-ID: <20040521135759.GE57132@comp.chem.msu.su>
next in thread | raw e-mail | index | archive | help
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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20040521135759.GE57132>