On 29/02/16 10:53, Arnd Bergmann wrote:

On Monday 29 February 2016 10:37:40 Robin Murphy wrote:

quoted

Hi Arnd,

On 29/02/16 09:19, Arnd Bergmann wrote:

quoted

The newly added Mediatek IOMMU driver uses the IOMMU_DMA infrastructure,
but unlike other such drivers, it does not select 'ARM_DMA_USE_IOMMU',
which is a prerequisite, leading to a link error:

warning: (MTK_IOMMU) selects IOMMU_DMA which has unmet direct dependencies (IOMMU_SUPPORT && NEED_SG_DMA_LENGTH)

Going off on a tangent, is it actually right for that to depend on a
NEED_* symbol, or should that really be a select instead?

ARM_DMA_USE_IOMMU gets this right, it selects NEED_SG_DMA_LENGTH as it
actually needs it.

The IOMMU_DMA symbol is a bit strange, and the dependency can probably
get dropped altogether, but at least here it told us what went wrong.

IOMMU_DMA uses sg_dma_len() unconditionally all over the place, hence 
the "dependency". Thanks for the clarification; fix sent.

quoted

quoted

drivers/iommu/built-in.o: In function `iommu_put_dma_cookie':
mtk_iommu.c:(.text+0x11fe): undefined reference to `put_iova_domain'
drivers/iommu/built-in.o: In function `iommu_dma_init_domain':
mtk_iommu.c:(.text+0x1316): undefined reference to `init_iova_domain'
drivers/iommu/built-in.o: In function `__iommu_dma_unmap':
mtk_iommu.c:(.text+0x1380): undefined reference to `find_iova'

This adds the same select that the other drivers have. On a related
note, I wonder if we should just always select ARM_DMA_USE_IOMMU
whenever any IOMMU driver is enabled. Are there any cases where
we would enable an IOMMU but not use it?

You could use one solely for VFIO without caring about DMA ops - I think
that's mostly how ARM SMMUs are being used in practice at the moment -
but DMA-focused 'media' IOMMUs vastly outnumber 'virtualisation' IOMMUs
on ARM, so it would probably make sense. We already have the equivalent
"select IOMMU_DMA if IOMMU_SUPPORT" on arm64.

Ok.

quoted

quoted

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
---
   drivers/iommu/Kconfig | 1 +
   1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index b325954cf8f8..ea0998921702 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig

@@ -341,6 +341,7 @@ config MTK_IOMMU
   	bool "MTK IOMMU Support"
   	depends on ARM || ARM64
   	depends on ARCH_MEDIATEK || COMPILE_TEST
+	select ARM_DMA_USE_IOMMU

If going down this route, I'd be inclined to add an "if ARM" there, just
for clarity.

That would run into the NEED_SG_DMA_LENGTH problem on other architectures
that don't already set it, right?

Sorry, I'm lost - wouldn't "depends on ARM || ARM64" make other 
architectures moot? arm64 already has NEED_SG_DMA_LENGTH=y by default.

Robin.