Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in modules

[Robin Murphy]

On 06/07/17 03:25, Tomasz Figa wrote:
> On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy <robin.murphy@xxxxxxx> wrote:
>> On 05/07/17 08:12, Tomasz Figa wrote:
>>> There is nothing wrong in having a loadable module implementing DMA API,
>>> for example to be used for sub-devices registered by the module. However,
>>> most of the functions from dma-iommu do not have their symbols exported,
>>> making it impossible to use them from loadable modules.
>>>
>>> Export all the non-static functions in the file, so that loadable modules
>>> can benefit from them. Use EXPORT_SYMBOL() for consistency with other
>>> exports in the file.
>>
>> To echo what Christoph said, everything not already exported here
>> shouldn't in any way be considered a driver-facing API in the general
>> sense, it's horrible glue code to sit behind an arch-specific DMA
>> mapping implementation (and frankly I'd consider even the current
>> exports more of an unfortunate abstraction leakage).
> 
> Well, if I remember correctly, we agreed that the IPU3 driver would
> benefit from using all the iommu_dma_*() helpers in its DMA ops,
> similarly to ARM64. This is IMHO much better than re-implementing them
> again internally just for this driver. However almost none of
> necessary helpers are currently exported...

Oh, for sure - I don't personally have much objection to arch code being
modular (even as part of a driver subsystem), I just don't want anyone
to get the impression that this layer is something that any old driver
can dip into as it fancies.

>>> Signed-off-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
>>> ---
>>
>> [...]
>>
>>> @@ -829,17 +838,20 @@ dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>>>       return __iommu_dma_map(dev, phys, size,
>>>                       dma_info_to_prot(dir, false, attrs) | IOMMU_MMIO);
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_map_resource);
>>>
>>>  void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>>>               size_t size, enum dma_data_direction dir, unsigned long attrs)
>>>  {
>>>       __iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle, size);
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_unmap_resource);
>>
>> Do you need these two? Unless your custom DMA ops really have to support
>> slave DMA or other peer-to-peer traffic through their IOMMU, I'd be more
>> inclined to implement dma_map_resource as "return 0;" and ignore
>> dma_unmap_resource.
> 
> I don't need them. Getting an idea what is desirable to export and
> what not is actually one of the goals of this RFC.
> 
>>
>>> @@ -913,3 +925,4 @@ void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg)
>>>               msg->address_lo += lower_32_bits(msi_page->iova);
>>>       }
>>>  }
>>> +EXPORT_SYMBOL(iommu_dma_map_msi_msg);
>>
>> Given the nature of the kind of irqchip drivers this exists for, the
>> chances of one ever being modular seem vanishingly small.
> 
> Agreed. The IPU3 driver does not need it either.
> 
> Let me list the (not yet exported) helpers it requires:
> 
> dma-iommu.c
>  - iommu_dma_init,
>  - dma_info_to_prot,
>  - iommu_dma_free,
>  - iommu_dma_alloc,
>  - iommu_dma_mmap,
>  - iommu_dma_map_page,
>  - iommu_dma_unmap_page,
>  - iommu_dma_map_sg,
>  - iommu_dma_unmap_sg,
>  - iommu_dma_mapping_error,
> (added by my patch) iommu_dma_cleanup,
> 
> iommu.c
>  - iommu_group_get_for_dev,
> 
> base/dma-mapping.c
>  - dma_common_pages_remap,
>  - dma_common_free_remap,
> (added by my patch) dma_common_get_mapped_pages (OR find_vm_area),

I suppose another option is to just make the IOMMU and DMA ops a
self-contained non-modular driver mirroring the VT-d/AMD-Vi IOMMUs -
AFAICS it shouldn't have to be all that tightly coupled to the IPU bus
code, the latter more or less just needs to create the appropriate IOMMU
device for the driver to find.

Robin.

> 
> Best regards,
> Tomasz
>