[PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

Subsystems: iommu subsystem, the rest

STALE3804d

9 messages, 4 authors, 2016-03-01 · open the first message on its own page

[PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-02-29 09:20:20

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)
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?

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
 	select IOMMU_API
 	select IOMMU_DMA
 	select IOMMU_IO_PGTABLE_ARMV7S
-- 
2.7.0

[PATCH 2/2] iommu/mediatek: mark PM functions as __maybe_unused

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-02-29 09:20:28

When CONFIG_PM is unset, we get a harmless warning for this driver:

drivers/iommu/mtk_iommu.c:665:12: error: 'mtk_iommu_suspend' defined but not used [-Werror=unused-function]
drivers/iommu/mtk_iommu.c:680:12: error: 'mtk_iommu_resume' defined but not used [-Werror=unused-function]

Marking the functions as __maybe_unused gits rid of the two functions
and lets the compiler silently drop the object code, while still
doing syntax checking on them for build-time verification.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
---
 drivers/iommu/mtk_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 721ffdb296d6..f3c160e4c25d 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -662,7 +662,7 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int mtk_iommu_suspend(struct device *dev)
+static int __maybe_unused mtk_iommu_suspend(struct device *dev)
 {
 	struct mtk_iommu_data *data = dev_get_drvdata(dev);
 	struct mtk_iommu_suspend_reg *reg = &data->reg;
@@ -677,7 +677,7 @@ static int mtk_iommu_suspend(struct device *dev)
 	return 0;
 }
 
-static int mtk_iommu_resume(struct device *dev)
+static int __maybe_unused mtk_iommu_resume(struct device *dev)
 {
 	struct mtk_iommu_data *data = dev_get_drvdata(dev);
 	struct mtk_iommu_suspend_reg *reg = &data->reg;
-- 
2.7.0

Re: [PATCH 2/2] iommu/mediatek: mark PM functions as __maybe_unused

From: Yong Wu <yong.wu@mediatek.com>
Date: 2016-03-01 05:05:11

On Mon, 2016-02-29 at 10:19 +0100, Arnd Bergmann wrote:
When CONFIG_PM is unset, we get a harmless warning for this driver:

drivers/iommu/mtk_iommu.c:665:12: error: 'mtk_iommu_suspend' defined but not used [-Werror=unused-function]
drivers/iommu/mtk_iommu.c:680:12: error: 'mtk_iommu_resume' defined but not used [-Werror=unused-function]

Marking the functions as __maybe_unused gits rid of the two functions
and lets the compiler silently drop the object code, while still
doing syntax checking on them for build-time verification.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 0df4fabe208d ("iommu/mediatek: Add mt8173 IOMMU driver")
Hi Arnd,
  Thanks very much for both fixes.

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Robin Murphy <robin.murphy@arm.com>
Date: 2016-02-29 10:37:46

Hi Arnd,

On 29/02/16 09:19, Arnd Bergmann wrote:
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?
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.
quoted hunk
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.
  	select IOMMU_API
  	select IOMMU_DMA
As above, this is already selected on arm64, and isn't used on 32-bit*, 
so could probably just be removed, especially if it leads to build issues.

Robin.

*yet, of course. I need to have a proper look over Marek's RFC ;)
  	select IOMMU_IO_PGTABLE_ARMV7S

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-02-29 10:54:45

On Monday 29 February 2016 10:37:40 Robin Murphy wrote:
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.
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
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?

	Arnd

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Robin Murphy <robin.murphy@arm.com>
Date: 2016-02-29 11:22:30

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.

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Arnd Bergmann <arnd@arndb.de>
Date: 2016-02-29 11:31:41

On Monday 29 February 2016 11:22:24 Robin Murphy wrote:
quoted
quoted
quoted
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.
Nevermind, I didn't notice the dependency on the architecture.

What is keeping us from having 'depends on ARM || ARM64 || COMPILE_TEST'?

I assume it doesn't work yet, but it would be nice to get that done
at some point so we can take advantage of automated build testing like
coverity.

	Arnd

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Robin Murphy <robin.murphy@arm.com>
Date: 2016-02-29 11:47:34

On 29/02/16 11:29, Arnd Bergmann wrote:
On Monday 29 February 2016 11:22:24 Robin Murphy wrote:
quoted
quoted
quoted
quoted
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.
Nevermind, I didn't notice the dependency on the architecture.

What is keeping us from having 'depends on ARM || ARM64 || COMPILE_TEST'?
IIRC it got changed because the kbuild test robot was consistently 
blowing up due to missing definitions on things like parisc.

Robin.
I assume it doesn't work yet, but it would be nice to get that done
at some point so we can take advantage of automated build testing like
coverity.

	Arnd

Re: [PATCH 1/2] iommu/mediatek: select ARM_DMA_USE_IOMMU

From: Joerg Roedel <joro@8bytes.org>
Date: 2016-02-29 15:48:27

On Mon, Feb 29, 2016 at 10:19:06AM +0100, Arnd Bergmann wrote:
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)
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?

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(+)
Applied both, thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help