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@xxxxxxxx>
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@xxxxxxxxxxxx/
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@xxxxxxxxxxxxx>
Thanks!
Arnd
