From owner-freebsd-arm@FreeBSD.ORG Sat Apr 21 16:10:13 2012 Return-Path: Delivered-To: freebsd-arm@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E8005106566C for ; Sat, 21 Apr 2012 16:10:12 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id D2FFA8FC08 for ; Sat, 21 Apr 2012 16:10:12 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id q3LGAC5v067222 for ; Sat, 21 Apr 2012 16:10:12 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.5/8.14.5/Submit) id q3LGACYL067221; Sat, 21 Apr 2012 16:10:12 GMT (envelope-from gnats) Date: Sat, 21 Apr 2012 16:10:12 GMT Message-Id: <201204211610.q3LGACYL067221@freefall.freebsd.org> To: freebsd-arm@FreeBSD.org From: Ian Lepore Cc: Subject: Re: arm/160431: [busdma] [patch] Disable interrupts during busdma cache sync operations. X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Ian Lepore List-Id: Porting FreeBSD to the StrongARM Processor List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Apr 2012 16:10:13 -0000 The following reply was made to PR arm/160431; it has been noted by GNATS. From: Ian Lepore To: bug-followup@FreeBSD.org Cc: Subject: Re: arm/160431: [busdma] [patch] Disable interrupts during busdma cache sync operations. Date: Sat, 21 Apr 2012 10:01:06 -0600 --=-I62BfnVhv+9haFCqsTdw Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Here is the latest and most-tested patch for this problem. Mark Tinguely had suggested that the scope of having interrupts disabled be narrowed to just the time spent doing a partial cacheline flush, as opposed to disabling them for the entire bus_dmamap_sync_buf() function as my original patch did. I made that change and we've been shipping products using this new patch since September 2011, but I apparently neglected to submit the updated patch (which also fixes some line wrapping and other style(9) issues). -- Ian --=-I62BfnVhv+9haFCqsTdw Content-Description: Content-Disposition: inline; filename="arm_busdma.diff" Content-Type: text/x-patch; name="arm_busdma.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit Index: sys/arm/arm/busdma_machdep.c =================================================================== --- sys/arm/arm/busdma_machdep.c (revision 234543) +++ sys/arm/arm/busdma_machdep.c (working copy) @@ -1106,28 +1106,45 @@ cpu_l2cache_wbinv_range((vm_offset_t)buf, len); } } + /* + * Interrupts must be disabled while 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); + 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)); + } } - 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); + } } } --=-I62BfnVhv+9haFCqsTdw--