Messages in this thread | | | Date | Thu, 30 Mar 2023 09:10:57 +0200 | From | "Arnd Bergmann" <> | Subject | Re: [PATCH 08/21] riscv: dma-mapping: only invalidate after DMA, not flush |
| |
On Wed, Mar 29, 2023, at 22:48, Conor Dooley wrote: > On Mon, Mar 27, 2023 at 02:13:04PM +0200, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> No other architecture intentionally writes back dirty cache lines into >> a buffer that a device has just finished writing into. If the cache is >> clean, this has no effect at all, but > >> if a cacheline in the buffer has >> actually been written by the CPU, there is a drive bug that is likely >> made worse by overwriting that buffer. > > So does this need a > Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using > zicbom extension") > then, even if the cacheline really should not have been touched by the > CPU? > Also, minor typo, s/drive/driver/.
done
> In the thread we had that sparked this, I went digging for the source of > the flushes, and it came from a review comment: > https://lore.kernel.org/linux-riscv/342e3c12-ebb0-badf-7d4c-c444a2b842b2@sholland.org/
Ah, so the comment that led to it was
"For arch_sync_dma_for_cpu(DMA_BIDIRECTIONAL), we expect the CPU to have written to the buffer, so this should flush, not invalidate."
which sounds like Samuel just misunderstood what "bidirectional" means: the comment implies that both the cpu and the device access the buffer before arch_sync_dma_for_cpu(DMA_BIDIRECTIONAL), but this is not allowed. Instead, the point is that the device may both read and write the buffer, requiring that we must do a writeback at arch_sync_dma_for_device(DMA_BIDIRECTIONAL) and an invalidate at arch_sync_dma_for_cpu(DMA_BIDIRECTIONAL).
The comment about arch_sync_dma_for_device(DMA_FROM_DEVICE) (in the same email) seems equally confused. It's of course easy to misunderstand these, and many others have gotten confused in similar ways before.
> But *surely* if no other arch needs to do that, then we are safe to also > not do it... Your logic seems right by me at least, especially given the > lack of flushes elsewhere.
Right, I remove the extra writeback from powerpc, parisc and microblaze for the same reason. Those appear to only be there because they used the same function for _for_device() as for _for_cpu(), not because someone thought they were required.
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Thanks!
Arnd
| |