Date: Tue, 06 Sep 2011 15:02:47 -0600 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: freebsd-arm <freebsd-arm@freebsd.org> Subject: Re: arm/160431: [patch] Disable interrupts during busdma cache sync operations. Message-ID: <1315342967.1671.21.camel@revolution.hippie.lan> In-Reply-To: <201109031940.p83Je9fo004190@freefall.freebsd.org> References: <201109031940.p83Je9fo004190@freefall.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
After thinking more about Mark's question I've come to the conclusion
that interrupts should only be disabled in the partial cachline flush
case. Basically it comes down to the fact that only that case is
special, in that we temporarily duplicate the state of the hardware into
a local buffer then restore it, and we can't let the state of the real
hardware change during that time.
It also occurs to me that if it's necessary to disable interrupts during
all cache operations done by the busdma code, then it should be just as
necessary during the operations done by the pmap code, and it's not
clear to me that those operations are all currently happening only while
interrupts are disabled.
So, here is a do-over on the patch that only disables interrupts in the
partial cacheline case. Considering that the partial case seems to be
pretty rare, and tends to involve buffers smaller a single cacheline,
this should result in having interrupts disabled a whole lot less often
during buffer syncs.
Index: busdma_machdep.c
===================================================================
RCS file: /local/base/FreeBSD-CVS/src/sys/arm/arm/busdma_machdep.c,v
retrieving revision 1.50
diff -u -p -r1.50 busdma_machdep.c
--- busdma_machdep.c 11 Mar 2010 21:16:54 -0000 1.50
+++ busdma_machdep.c 6 Sep 2011 19:06:32 -0000
@@ -1106,28 +1106,44 @@ bus_dmamap_sync_buf(void *buf, int len,
cpu_l2cache_wbinv_range((vm_offset_t)buf, len);
}
}
+ /*
+ * Interrupts must be disabled if handling a partial cacheline flush,
+ * otherwise the interrupt handling code could modify data in the
+ * non-DMA part of a cacheline while we have it stashed away in the
+ * temporary stack buffer, then we end up restoring the stale value. As
+ * unlikely as this seems, it has been observed in the real world.
+ */
if (op & BUS_DMASYNC_POSTREAD) {
- if ((vm_offset_t)buf & arm_dcache_align_mask) {
- memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~
- arm_dcache_align_mask),
- (vm_offset_t)buf & arm_dcache_align_mask);
- }
- if (((vm_offset_t)buf + len) & arm_dcache_align_mask) {
- memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len),
- arm_dcache_align - (((vm_offset_t)(buf) + len) &
- arm_dcache_align_mask));
+ uint32_t intr;
+ int partial = (((vm_offset_t)buf) | len) & arm_dcache_align_mask;
+
+ if (partial) {
+ intr = intr_disable();
+ if ((vm_offset_t)buf & arm_dcache_align_mask) {
+ memcpy(_tmp_cl, (void *)((vm_offset_t)buf & ~
+ arm_dcache_align_mask),
+ (vm_offset_t)buf & arm_dcache_align_mask);
+ }
+ if (((vm_offset_t)buf + len) & arm_dcache_align_mask) {
+ memcpy(_tmp_clend, (void *)((vm_offset_t)buf + len),
+ arm_dcache_align - (((vm_offset_t)(buf) + len) &
+ arm_dcache_align_mask));
+ }
}
cpu_dcache_inv_range((vm_offset_t)buf, len);
cpu_l2cache_inv_range((vm_offset_t)buf, len);
- if ((vm_offset_t)buf & arm_dcache_align_mask)
- memcpy((void *)((vm_offset_t)buf &
- ~arm_dcache_align_mask), _tmp_cl,
- (vm_offset_t)buf & arm_dcache_align_mask);
- if (((vm_offset_t)buf + len) & arm_dcache_align_mask)
- memcpy((void *)((vm_offset_t)buf + len), _tmp_clend,
- arm_dcache_align - (((vm_offset_t)(buf) + len) &
- arm_dcache_align_mask));
+ if (partial) {
+ if ((vm_offset_t)buf & arm_dcache_align_mask)
+ memcpy((void *)((vm_offset_t)buf &
+ ~arm_dcache_align_mask), _tmp_cl,
+ (vm_offset_t)buf & arm_dcache_align_mask);
+ if (((vm_offset_t)buf + len) & arm_dcache_align_mask)
+ memcpy((void *)((vm_offset_t)buf + len), _tmp_clend,
+ arm_dcache_align - (((vm_offset_t)(buf) + len) &
+ arm_dcache_align_mask));
+ intr_restore(intr);
+ }
}
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1315342967.1671.21.camel>