Date:   Thu, 6 Jul 2017 11:25:09 +0900
From:   Tomasz Figa <tfiga@...omium.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     "open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Christoph Hellwig <hch@....de>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Joerg Roedel <joro@...tes.org>,
        Will Deacon <will.deacon@....com>,
        Vineet Gupta <vgupta@...opsys.com>,
        Hans-Christian Noren Egtvedt <egtvedt@...fundet.no>,
        Mitchel Humpherys <mitchelh@...eaurora.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [RFC PATCH 4/5] iommu/dma: Export non-static functions to use in modules

On Thu, Jul 6, 2017 at 1:22 AM, Robin Murphy <robin.murphy@....com> 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@...omium.org>
>> ---
>
> [...]
>
>> @@ -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),

Best regards,
Tomasz

Powered by blists - more mailing lists