On Fri, 25 Mar 2022 11:27:41 +0000 Robin Murphy <robin.murphy@xxxxxxx> wrote: > What muddies the waters a bit is that the opposite combination > sync_for_cpu(DMA_TO_DEVICE) really *should* always be a no-op, and I for > one have already made the case for eliding that in code elsewhere, but > it doesn't necessarily hold for the inverse here, hence why I'm not sure > there even is a robust common solution for peeking at a live > DMA_FROM_DEVICE buffer. In https://lkml.org/lkml/2022/3/24/739 I also argued, that a robust common solution for a peeking at a live DMA_FROM_DEVICE buffer is probably not possible, at least not with the current programming model as described by Documentation/core-api/dma-api.rst. Namely AFAIU the programming model is based on exclusive ownership: the buffer is either owned by the device, which means CPU(s) are not allowed to *access* it, or it is owned by the CPU(s), and the device is not allowed to *access* it. Do we agree on this? Considering what Linus said here https://lkml.org/lkml/2022/3/24/775 I understand that: if the idea that dma_sync_*_for_{cpu,device} always transfers ownership to the cpu and device respectively is abandoned, and we re-define ownership in a sense that only the owner may write, but non-owner is allowed to read, then it may be possible to make the scenario under discussion work. The scenario in pseudo code: /* when invoked device might be doing DMA into buf */ rx_buf_complete(buf) { prepare_peek(buf, DMA_FROM_DEVICE); if (!is_ready(buf)) { /*let device gain the buffer again*/ peek_done_not_ready(buf, DMA_FROM_DEVICE); return false; } peek_done_ready(buf, DMA_FROM_DEVICE); process_buff(buf, DMA_FROM_DEVICE); is } IMHO it is pretty obvious, that prepare_peek() has to update the cpu copy of the data *without* transferring ownership to the CPU. Since the owner is still the device, it is legit for the device to keep modifying the buffer via DMA. In case of the swiotlb, we would copy the content of the bounce buffer to the orig buffer possibly after invalidating caches, and for non-swiotlb we would do invalidate caches. So prepare_peek() could be actually something like, dma_sync_single_for_cpu(buf, DMA_FROM_DEVICE, DMA_ATTR_NO_OWNERSHIP_TRANSFER) which would most end up being functionally the same, as without the flag, since my guess is that the ownership is only tracked in our heads. For peek_done_not_ready() there is conceptually nothing to do, because the device retained ownership. Thus would either have to mandate peek_done_not_ready() being a nop, or non-existent, (that is what Toke's patch does in the specific case), or we would have to mandate that dma_sync_*_for_*() has no side effects under certain. The former looks simpler to me, especially with swiotlb. But we are also fine if the cache ain't dirty, because the CPU didn't write (as pointed out by Linus) and we were to detect that, and avoid flushing a clean cache, or if we were to track ownership and to avoid flushing caches because no ownership transfer. But to avoid these bad flushes, at least for swiotlb, we would either have to track cache ownership, or even worse track dirtiness (for which we would have to extend the API, and make the drivers tell us that the cache, i.e. the original buffer got dirtied). Since the device has ownership when peek_done_not_ready() is invoked, we might need to transfer ownership to the CPU in peek_done_ready(). This could again be a dma_sync_for_cpu() with a flag, which when supplied tells the dma API that no sync (cache invalidate) is needed because the driver guarantees, that the whole mapping was sufficiently sync-ed by prepare_peek(). Please notice, that the whole scheme is based on the driver knowing that the whole DMA is done by examining the buffer, and it decides based on whatever it sees. Some of the ongoing discussion seem so ignore this whole ownership biz. My feeling is: the notion of ownership useful. If both sides end up modifying (and eventually flushing) we are in trouble IMHO, an ownership avoids that. But if the conclusion ends up being, that ownership does not matter, then we should make sure it is purged from the documentation, because otherwise it will confuse the hell out of people who read documentations and care about programming models. People like me. Regards, Halil
