Subject: Re: bus_dmamap_sync() in ti(4) or the lack thereof
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
From: Tobias Nygren <tnn+nbsd@nygren.pp.se>
List: tech-kern
Date: 02/28/2007 00:52:21
This is a multi-part message in MIME format.
--------------000704070401020300050203
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Izumi Tsutsui wrote:
> tnn+nbsd@nygren.pp.se wrote:
>
>   
>> ti(4) doesn't work right on sparc64. With DIAGNOSTIC i get tons
>> of "iommu_dvmamap_load: map still in use".
>>     
>
> This is usually caused by missing bus_dmamap_unload(9).
> (IMHO other ports should have the similar assersions)
>
>   
>> I took a glance at the ti(4) code and noticed the absence of any
>> preread/postread dma syncs, despite the fact that the driver relies
>> heavily on reuse of dma maps. This can't be right, can it?
>>     
>
> It should have proper sync calls otherwise it won't work on
> non DMA coherent architecture (like sgimips O2) or architectures
> which require bounce buffer (like some alpha), but many FreeBSD's
> drivers often lack them since they are not required by "major" ports ;-p
>
>   
>> The fact that the driver appears to reload maps without unloading
>> them first might be caused by something else, though.
>>     
>
> It seems bus_dmamap_unload(9) is needed before calling ti_newbuf_jumbo()
> or ti_newbuf_mini() in ti_rxeof().
> ---
> Izumi Tsutsui
>   

For what it's worth, this patch fixes the problems I had.
I'm not sure what to do about the jumbo case, because it uses a single
dmamap for all descriptors.


--------------000704070401020300050203
Content-Type: text/plain;
 name="if_ti.c.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="if_ti.c.diff"

Index: if_ti.c
===================================================================
RCS file: /cvsroot/src/sys/dev/pci/if_ti.c,v
retrieving revision 1.73
diff -u -r1.73 if_ti.c
--- if_ti.c	16 Nov 2006 01:33:09 -0000	1.73
+++ if_ti.c	27 Feb 2007 23:51:12 -0000
@@ -752,6 +752,9 @@
 		m_new->m_len = m_new->m_pkthdr.len = MCLBYTES;
 		m_adj(m_new, ETHER_ALIGN);
 
+		if (dmamap->dm_nsegs > 0)
+			bus_dmamap_unload(sc->sc_dmat, dmamap);
+
 		if ((error = bus_dmamap_load(sc->sc_dmat, dmamap,
 				mtod(m_new, caddr_t), m_new->m_len, NULL,
 				BUS_DMA_READ|BUS_DMA_NOWAIT)) != 0) {
@@ -766,6 +769,8 @@
 		m_adj(m_new, ETHER_ALIGN);
 
 		/* reuse the dmamap */
+		bus_dmamap_sync(sc->sc_dmat, dmamap, 0, dmamap->dm_mapsize,
+		    BUS_DMASYNC_PREREAD);
 	}
 
 	sc->ti_cdata.ti_rx_std_chain[i] = m_new;
@@ -821,6 +826,9 @@
 		m_new->m_len = m_new->m_pkthdr.len = MHLEN;
 		m_adj(m_new, ETHER_ALIGN);
 
+		if (dmamap->dm_nsegs > 0)
+			bus_dmamap_unload(sc->sc_dmat, dmamap);
+
 		if ((error = bus_dmamap_load(sc->sc_dmat, dmamap,
 				mtod(m_new, caddr_t), m_new->m_len, NULL,
 				BUS_DMA_READ|BUS_DMA_NOWAIT)) != 0) {
@@ -835,6 +843,8 @@
 		m_adj(m_new, ETHER_ALIGN);
 
 		/* reuse the dmamap */
+		bus_dmamap_sync(sc->sc_dmat, dmamap, 0, dmamap->dm_mapsize,
+		    BUS_DMASYNC_PREREAD);
 	}
 
 	r = &sc->ti_rdata->ti_rx_mini_ring[i];
@@ -896,6 +906,8 @@
 		m_new->m_ext.ext_size = ETHER_MAX_LEN_JUMBO;
 	}
 
+	/* XXX tnn: dmasync preread for jumbo case? */
+
 	m_adj(m_new, ETHER_ALIGN);
 	/* Set up the descriptor. */
 	r = &sc->ti_rdata->ti_rx_jumbo_ring[i];
@@ -1946,6 +1958,9 @@
 		TI_INC(sc->ti_rx_saved_considx, TI_RETURN_RING_CNT);
 
 		if (cur_rx->ti_flags & TI_BDFLAG_JUMBO_RING) {
+
+			/* XXX tnn: dmamap sync postread for jumbo case? */
+
 			TI_INC(sc->ti_jumbo, TI_JUMBO_RX_RING_CNT);
 			m = sc->ti_cdata.ti_rx_jumbo_chain[rxidx];
 			sc->ti_cdata.ti_rx_jumbo_chain[rxidx] = NULL;
@@ -1965,6 +1980,8 @@
 			m = sc->ti_cdata.ti_rx_mini_chain[rxidx];
 			sc->ti_cdata.ti_rx_mini_chain[rxidx] = NULL;
 			dmamap = sc->mini_dmamap[rxidx];
+			bus_dmamap_sync(sc->sc_dmat, dmamap, 0,
+			    dmamap->dm_mapsize, BUS_DMASYNC_POSTREAD);
 			sc->mini_dmamap[rxidx] = 0;
 			if (cur_rx->ti_flags & TI_BDFLAG_ERROR) {
 				ifp->if_ierrors++;
@@ -1982,6 +1999,8 @@
 			m = sc->ti_cdata.ti_rx_std_chain[rxidx];
 			sc->ti_cdata.ti_rx_std_chain[rxidx] = NULL;
 			dmamap = sc->std_dmamap[rxidx];
+			bus_dmamap_sync(sc->sc_dmat, dmamap, 0,
+			    dmamap->dm_mapsize, BUS_DMASYNC_POSTREAD);
 			sc->std_dmamap[rxidx] = 0;
 			if (cur_rx->ti_flags & TI_BDFLAG_ERROR) {
 				ifp->if_ierrors++;

--------------000704070401020300050203--