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

From: Tomasz Figa
Date: Wed Jul 05 2017 - 22:25:42 EST

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...

>> 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:

- 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_group_get_for_dev,

- dma_common_pages_remap,
- dma_common_free_remap,
(added by my patch) dma_common_get_mapped_pages (OR find_vm_area),

Best regards,