Re: [PATCH net-next v3 1/7] dma: compile-out DMA sync op calls when not used

[Christoph Hellwig <hch@lst.de>]

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.