Re: [EXTERNAL] Re: [PATCH 1/1] swiotlb: add a KUnit test suite

[Petr Tesařík]

On Wed, 3 Apr 2024 17:58:47 +0100
Robin Murphy <robin.murphy@xxxxxxx> wrote:

> On 2024-04-03 3:19 pm, Linu Cherian wrote:
> [...]
> >>> Should we not try this on a buffer that is mapped with DMA_FROM_DEVICE ?  
> >>
> >> I'm afraid I don't follow.
> >>
> >> AFAICT the direction is a property of the sync operation. In fact,
> >> swiotlb_tbl_map_single() does not even use its direction parameter at all.
> >> Removing that parameter is already on my TODO list of cleanups.
> >>  
> > 
> > Okay. Got it.
> >   
> >> swiotlb_map() uses its direction parameter only to perform the initial arch
> >> sync if DMA is non-coherent.
> >>
> >> OTOH I may be missing some high-level logical concepts which do not
> >> correspond to any actual code in the swiotlb implementation, so my use is still
> >> wrong.
> >>  
> > 
> > Just thought that the keeping the DMA direction consistent for the map and sync would be more aligned to typical use case.
> > For example, a buffer used for transmit in case of networking. OTOH, since the API by itself doesn't have such constraints on the direction parameter, may be it makes sense to test those scenarios.  
> 
> Right, SWIOTLB exists to serve the DMA API, so it makes more sense to me 
> to test it in the context of valid DMA API usage than to make up 
> scenarios that aren't representative of real-world usage. The direction 
> is a property of the whole DMA mapping itself, and thus must be passed 
> consistently for every operation on a given mapping.
> 
> Yes, there is some internal trickery once we get down to the level of 
> calling swiotlb_bounce() itself, but if we're driving the tests through 
> the higher-level public interface then I'd prefer to see that used as 
> expected. Given the whole partial-write-transparency business, the most 
> significant effect of direction should just be that of DMA_TO_DEVICE 
> skipping the copy-out for unmap and sync_for_cpu, so for the sake of 
> coverage you may as well just use DMA_BIDIRECTIONAL everywhere.

Thank you for the explanation. Got it now.

I'm now going to take all comments into account for a v2 series, plus I
want to add a test for partial sync to cover Michael's fix.

Stay tuned,
Petr T