[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] swiotlb: Remove alloc_size argument to swiotlb_tbl_map_single()
On Mon, 15 Apr 2024 12:23:22 +0000 Michael Kelley <mhklinux@xxxxxxxxxxx> wrote: > From: Petr Tesařík <petr@xxxxxxxxxxx> Sent: Monday, April 15, 2024 4:46 AM > > > > Hi Michael, > > > > sorry for taking so long to answer. Yes, there was no agreement on the > > removal of the "dir" parameter, but I'm not sure it's because of > > symmetry with swiotlb_sync_*(), because the topic was not really > > discussed. > > > > The discussion was about the KUnit test suite and whether direction is > > a property of the bounce buffer or of each sync operation. Since DMA API > > defines associates each DMA buffer with a direction, the direction > > parameter passed to swiotlb_sync_*() should match what was passed to > > swiotlb_tbl_map_single(), because that's how it is used by the generic > > DMA code. In other words, if the parameter is kept, it should be kept > > to match dma_map_*(). > > > > However, there is also symmetry with swiotlb_tbl_unmap_single(). This > > function does use the parameter for the final sync. I believe there > > should be a matching initial sync in swiotlb_tbl_map_single(). In > > short, the buffer sync for DMA non-coherent devices should be moved from > > swiotlb_map() to swiotlb_tbl_map_single(). If this sync is not needed, > > then the caller can (and should) include DMA_ATTR_SKIP_CPU_SYNC in > > the flags parameter. > > > > To sum it up: > > > > * Do *NOT* remove the "dir" parameter. > > * Let me send a patch which moves the initial buffer sync. > > > > I'm not seeing the need to move the initial buffer sync. All > callers of swiotlb_tbl_map_single() already have a subsequent > check for a non-coherent device, and a call to > arch_sync_dma_for_device(). And the Xen code has some > special handling that probably shouldn't go in > swiotlb_tbl_map_single(). Or am I missing something? Oh, sure, there's nothing broken ATM. It's merely a cleanup. The API is asymmetric and thus confusing. You get a final sync by default if you call swiotlb_tbl_unmap_single(), but you don't get an initial sync by default if you call swiotlb_tbl_map_single(). This is difficult to remember, so potential new users of the API may incorrectly assume that an initial sync is done, or that a final sync is not done. And yes, when moving the code, all current users of swiotlb_tbl_map_single() should specify DMA_ATTR_SKIP_CPU_SYNC. Petr T
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |