Re: [PATCH 2/2] dma: Add IOMMU static calls with clear default ops

[Robin Murphy <robin.murphy@arm.com>]

On 2024-07-11 7:57 pm, Leon Romanovsky wrote:
> On Thu, Jul 11, 2024 at 07:23:20PM +0100, Robin Murphy wrote:
>> On 11/07/2024 11:38 am, Leon Romanovsky wrote:
>>> From: Leon Romanovsky <leonro@nvidia.com>
>>>
>>> Most of the IOMMU drivers are using the same DMA operations, which are
>>> default ones implemented in drivers/iomem/dma-iomem.c. So it makes sense
>>> to properly set them as a default with direct call without need to
>>> perform function pointer dereference.
>>>
>>> During system initialization, the IOMMU driver can set its own DMA and
>>> in such case, the default DMA operations will be overridden.
>>
>> I'm going to guess you don't actually mean "IOMMU drivers" in the usual
>> sense of drivers/iommu/, but rather "arch DMA ops (which often, but not
>> always, involve some sort of IOMMU)."
> 
> Yes, you are right. I used word "drivers" as a general term to describe
> everything that implements their own ->map_page() callback.
> 
>>
>> If so, I'd much rather see this done properly, i.e. hook everything up
>> similarly to dma-direct and be able to drop CONFIG_DMA_OPS for "modern"
>> dma-direct/iommu-dma architectures entirely. Furthermore the implementation
>> here isn't right - not only is it not conceptually appropriate to make
>> iommu-dma responsibile for proxying random arch DMA ops, but in practial
>> terms it's just plain broken, since the architectures which still have their
>> own DMA ops also don't use iommu-dma, so this is essentially disabling the
>> entire streaming DMA API on ARM/PowerPC/etc.
> 
> Sorry but I'm not sure that I understood your reply. Can you please clarify
> what does it mean broken in this context?
> 
> All archs have one of the following options:
> 1. No DMA ops -> for them dma_map_direct() will return True and they
> never will enter into IOMMU path.
> 2. Have DMA ops but without .map_page() -> they will use default IOMMU
> 3. Have DMA ops with .map_page() -> they will use their own implementation

Urgh, sorry, let that instead be a lesson in not adding needless layers 
of indirection that are named as confusingly as possible, then. Seems I 
saw stubs plus static inline wrappers, managed to misread dma_iommu_* 
vs. iommu_dma_*, and jump to the wrong conclusion of what was stubbed 
out, not least since that was the only interpretation in which adding an 
extra layer of inline wrappers would seem necessary in the first place. 
If these dma_iommu_* functions serve no purpose other than to make the 
code even more of a maze of twisty little passages, all alike, then 
please just feed them to a grue instead.

Thanks,
Robin.