Subject: Re: CVS commit: src/sys/arch/sgimips
To: None <rafal@pobox.com>
From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
List: tech-kern
Date: 10/08/2003 21:29:15
(moved to port-mips and tech-kern)

In article <200310072239.h97Mdb76005190@fearless-vampire-killer.waterside.net>
rafal@pobox.com wrote:

> -> Log Message:
> -> Use proper mips_dcache_{wb,inv,wbinv}_range() ops in dmamap sync function
> -> if possible. Tested on my O2 (with r5ksc patch).
> -> 
> -> XXX should we apply similar changes to mips/bus_dma.c and use it?
> 
> Hmm, I'm interested in how you found this and/or how it manifested itself.
> I had lots and lots of cache-looking problems with the Intel GigE card in
> my O2 and trying the obvious things (using the generic arch/mips/mips/bus_
> dma.c code, disabling L2) didn't make it go away.  I guess I should give
> it a try again.

Well, my changes are mostly based on mips/bus_dma.c code which
uses wb op (without inv) for PREWRITE, and wbinv op for PREREAD.
(which was mostly written by thorpej and others ;-)

The only change I added is trying to use inv op (without wb)
for PREREAD more aggressively. We don't have to write back
cache against memory which will soon be overwriten by bus-master,
but if regions to be flushed are not cacheline aligned,
both cachelines at start and end of regions need to be written back
otherwise data in the same cachelines near the both boundaries
will be corrupted.

On the other hand, old sgimips' bus_dmamap_sync() in sgimips/bus.c
always used wbinv op, so it was most conservative.
If wm(4) fails with it, I guess my cache op changes
won't help and the driver might have some bugs.

With a quick glance, structure of wm(4) actually smells Intel-ism ;-p
uint8_t/uint16_t values in 32 bit DMA descriptors always cause
endian issue (and might cause race conditions on RISC CPUs.)

How about the attached patch?
---
Izumi Tsutsui
tsutsui@ceres.dti.ne.jp

Index: if_wm.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wm.c,v
retrieving revision 1.41
diff -u -r1.41 if_wm.c
--- if_wm.c	10 Sep 2003 04:02:17 -0000	1.41
+++ if_wm.c	8 Oct 2003 12:25:00 -0000
@@ -1074,36 +1074,36 @@
 
 	if (m0->m_pkthdr.csum_flags & M_CSUM_IPv4) {
 		WM_EVCNT_INCR(&sc->sc_ev_txipsum);
-		fields |= htole32(WTX_IXSM);
-		ipcs = htole32(WTX_TCPIP_IPCSS(offset) |
+		fields |= WTX_IXSM;
+		ipcs = WTX_TCPIP_IPCSS(offset) |
 		    WTX_TCPIP_IPCSO(offset + offsetof(struct ip, ip_sum)) |
-		    WTX_TCPIP_IPCSE(offset + iphl - 1));
+		    WTX_TCPIP_IPCSE(offset + iphl - 1);
 	} else if (__predict_true(sc->sc_txctx_ipcs != 0xffffffff)) {
 		/* Use the cached value. */
 		ipcs = sc->sc_txctx_ipcs;
 	} else {
 		/* Just initialize it to the likely value anyway. */
-		ipcs = htole32(WTX_TCPIP_IPCSS(offset) |
+		ipcs = WTX_TCPIP_IPCSS(offset) |
 		    WTX_TCPIP_IPCSO(offset + offsetof(struct ip, ip_sum)) |
-		    WTX_TCPIP_IPCSE(offset + iphl - 1));
+		    WTX_TCPIP_IPCSE(offset + iphl - 1);
 	}
 
 	offset += iphl;
 
 	if (m0->m_pkthdr.csum_flags & (M_CSUM_TCPv4|M_CSUM_UDPv4)) {
 		WM_EVCNT_INCR(&sc->sc_ev_txtusum);
-		fields |= htole32(WTX_TXSM);
-		tucs = htole32(WTX_TCPIP_TUCSS(offset) |
+		fields |= WTX_TXSM;
+		tucs = WTX_TCPIP_TUCSS(offset) |
 		    WTX_TCPIP_TUCSO(offset + m0->m_pkthdr.csum_data) |
-		    WTX_TCPIP_TUCSE(0) /* rest of packet */);
+		    WTX_TCPIP_TUCSE(0) /* rest of packet */;
 	} else if (__predict_true(sc->sc_txctx_tucs != 0xffffffff)) {
 		/* Use the cached value. */
 		tucs = sc->sc_txctx_tucs;
 	} else {
 		/* Just initialize it to a valid TCP context. */
-		tucs = htole32(WTX_TCPIP_TUCSS(offset) |
+		tucs = WTX_TCPIP_TUCSS(offset) |
 		    WTX_TCPIP_TUCSO(offset + offsetof(struct tcphdr, th_sum)) |
-		    WTX_TCPIP_TUCSE(0) /* rest of packet */);
+		    WTX_TCPIP_TUCSE(0) /* rest of packet */;
 	}
 
 	if (sc->sc_txctx_ipcs == ipcs &&
@@ -1121,8 +1121,8 @@
 #endif
 		t = (struct livengood_tcpip_ctxdesc *)
 		    &sc->sc_txdescs[sc->sc_txnext];
-		t->tcpip_ipcs = ipcs;
-		t->tcpip_tucs = tucs;
+		t->tcpip_ipcs = htole32(ipcs);
+		t->tcpip_tucs = htole32(tucs);
 		t->tcpip_cmdlen =
 		    htole32(WTX_CMD_DEXT | WTX_DTYP_C);
 		t->tcpip_seg = 0;
@@ -1297,7 +1297,7 @@
 			cksumfields = 0;
 		}
 
-		cksumcmd |= htole32(WTX_CMD_IDE);
+		cksumcmd |= WTX_CMD_IDE;
 
 		/*
 		 * Initialize the transmit descriptor.
@@ -1312,17 +1312,17 @@
 			sc->sc_txdescs[nexttx].wtx_addr.wa_high = 0;
 			sc->sc_txdescs[nexttx].wtx_addr.wa_low =
 			    htole32(dmamap->dm_segs[seg].ds_addr);
-			sc->sc_txdescs[nexttx].wtx_cmdlen = cksumcmd |
-			    htole32(dmamap->dm_segs[seg].ds_len);
+			sc->sc_txdescs[nexttx].wtx_cmdlen =
+			    htole32(cksumcmd | dmamap->dm_segs[seg].ds_len);
 			sc->sc_txdescs[nexttx].wtx_fields.wtxu_bits =
-			    cksumfields;
+			    htole32(cksumfields);
 			lasttx = nexttx;
 
 			DPRINTF(WM_DEBUG_TX,
 			    ("%s: TX: desc %d: low 0x%08x, len 0x%04x\n",
 			    sc->sc_dev.dv_xname, nexttx,
-			    (uint32_t) dmamap->dm_segs[seg].ds_addr,
-			    (uint32_t) dmamap->dm_segs[seg].ds_len));
+			    le32toh(dmamap->dm_segs[seg].ds_addr),
+			    le32toh(dmamap->dm_segs[seg].ds_len)));
 		}
 
 		/*
Index: if_wmreg.h
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_wmreg.h,v
retrieving revision 1.5
diff -u -r1.5 if_wmreg.h
--- if_wmreg.h	30 May 2003 02:08:55 -0000	1.5
+++ if_wmreg.h	8 Oct 2003 12:25:01 -0000
@@ -59,12 +59,23 @@
 typedef struct wiseman_rxdesc {
 	wiseman_addr_t	wrx_addr;	/* buffer address */
 
+#if BYTE_ORDER == LITTLE_ENDIGN
 	uint16_t	wrx_len;	/* buffer length */
 	uint16_t	wrx_cksum;	/* checksum (starting at PCSS) */
+#else
+	uint16_t	wrx_cksum;	/* checksum (starting at PCSS) */
+	uint16_t	wrx_len;	/* buffer length */
+#endif
 
+#if BYTE_ORDER == LITTLE_ENDIGN
 	uint8_t		wrx_status;	/* Rx status */
 	uint8_t		wrx_errors;	/* Rx errors */
 	uint16_t	wrx_special;	/* special field (VLAN, etc.) */
+#else
+	uint16_t	wrx_special;	/* special field (VLAN, etc.) */
+	uint8_t		wrx_errors;	/* Rx errors */
+	uint8_t		wrx_status;	/* Rx status */
+#endif
 } __attribute__((__packed__)) wiseman_rxdesc_t;
 
 /* wrx_status bits */
@@ -101,9 +112,15 @@
 typedef union wiseman_tx_fields {
 	uint32_t	wtxu_bits;	/* bits; see below; */
 	struct {
+#if BYTE_ORDER == LITTLE_ENDIGN
 		uint8_t wtxu_status;		/* Tx status */
 		uint8_t wtxu_options;		/* options */
 		uint16_t wtxu_vlan;		/* VLAN info */
+#else
+		uint16_t wtxu_vlan;		/* VLAN info */
+		uint8_t wtxu_options;		/* options */
+		uint8_t wtxu_status;		/* Tx status */
+#endif
 	} __attribute__((__packed__)) wtxu_fields;
 } __attribute__((__packed__)) wiseman_txfields_t;
 typedef struct wiseman_txdesc {