On Wed, Feb 14, 2024 at 05:20:50PM +0000, Robin Murphy wrote:
>> +config DMA_NEED_SYNC
>> + def_bool ARCH_HAS_SYNC_DMA_FOR_DEVICE || ARCH_HAS_SYNC_DMA_FOR_CPU || \
>> + ARCH_HAS_SYNC_DMA_FOR_CPU_ALL || DMA_OPS || SWIOTLB
>
> I'm not sure DMA_OPS belongs here - several architectures have non-trivial
> ops without syncs, e.g. Alpha.
True, but peeking through the ops is a bit hard. And I don't think it's
worth optimizing the dma sync performance on Alpha :)
>> +static inline void __dma_sync_single_for_cpu(struct device *dev,
>> + dma_addr_t addr, size_t size, enum dma_data_direction dir)
>
> To me it would feel more logical to put all the wrappers inside the #ifdef
> CONFIG_HAS_DMA and not touch these stubs at all (what does it mean to skip
> an inline no-op?). Or in fact, if dma_skip_sync() is constant false for
> !HAS_DMA, then we could also just make the external function declarations
> unconditional and remove the stubs. Not a critical matter though, and I
> defer to whatever Christoph thinks is most maintainable.
Your idea sounds reasonable to me, but I don't have a strong preference.
>> +static inline bool dma_need_sync(struct device *dev, dma_addr_t dma_addr)
>> +{
>> + return !dma_skip_sync(dev) ? __dma_need_sync(dev, dma_addr) : false;
>> +}
>
> That's a bit of a mind-bender... is it actually just
>
> return !dma_skip_sync(dev) && __dma_need_sync(dev, dma_addr);
>
> ?
That looks a lot more readable for sure.
> (I do still think the negative flag makes it all a little harder to follow
> in general than a positive "device needs to consider syncs" flag would.)
Probably.
