[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Laurent Pinchart]

Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops
into arch_setup_dma_ops") moved the setting of the DMA operations from
arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA
operations to be used are selected based on whether the device is
connected to an IOMMU. However, the IOMMU detection scheme requires the
IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver
has been ported yet, this effectively breaks all IOMMU ARM users that
depend on the IOMMU being handled transparently by the DMA mapping API.

Fix this by restoring the setting of DMA IOMMU ops in
arm_iommu_attach_device() and splitting the rest of the function into a
new internal __arm_iommu_attach_device() function, called by
arch_setup_dma_ops().

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 arch/arm/mm/dma-mapping.c | 53 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 15 deletions(-)

This fixes a v3.19 regression that breaks IOMMU on the Exynos, OMAP3, Renesas
and Rockchip platforms.

The patch has been tested on a Renesas M2 platform with the ipmmu-vmsa driver.

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7864797609b3..a673c7f7e208 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1940,13 +1940,32 @@ void arm_iommu_release_mapping(struct dma_iommu_mapping *mapping)
 }
 EXPORT_SYMBOL_GPL(arm_iommu_release_mapping);
 
+static int __arm_iommu_attach_device(struct device *dev,
+				     struct dma_iommu_mapping *mapping)
+{
+	int err;
+
+	err = iommu_attach_device(mapping->domain, dev);
+	if (err)
+		return err;
+
+	kref_get(&mapping->kref);
+	dev->archdata.mapping = mapping;
+
+	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
+	return 0;
+}
+
 /**
  * arm_iommu_attach_device
  * @dev: valid struct device pointer
  * @mapping: io address space mapping structure (returned from
  *	arm_iommu_create_mapping)
  *
- * Attaches specified io address space mapping to the provided device,
+ * Attaches specified io address space mapping to the provided device.
+ * This replaces the dma operations (dma_map_ops pointer) with the
+ * IOMMU aware version.
+ *
  * More than one client might be attached to the same io address space
  * mapping.
  */
@@ -1955,25 +1974,16 @@ int arm_iommu_attach_device(struct device *dev,
 {
 	int err;
 
-	err = iommu_attach_device(mapping->domain, dev);
+	err = __arm_iommu_attach_device(dev, mapping);
 	if (err)
 		return err;
 
-	kref_get(&mapping->kref);
-	dev->archdata.mapping = mapping;
-
-	pr_debug("Attached IOMMU controller to %s device.\n", dev_name(dev));
+	set_dma_ops(dev, &iommu_ops);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(arm_iommu_attach_device);
 
-/**
- * arm_iommu_detach_device
- * @dev: valid struct device pointer
- *
- * Detaches the provided device from a previously attached map.
- */
-void arm_iommu_detach_device(struct device *dev)
+static void __arm_iommu_detach_device(struct device *dev)
 {
 	struct dma_iommu_mapping *mapping;
 
@@ -1989,6 +1999,19 @@ void arm_iommu_detach_device(struct device *dev)
 
 	pr_debug("Detached IOMMU controller from %s device.\n", dev_name(dev));
 }
+
+/**
+ * arm_iommu_detach_device
+ * @dev: valid struct device pointer
+ *
+ * Detaches the provided device from a previously attached map.
+ * This voids the dma operations (dma_map_ops pointer)
+ */
+void arm_iommu_detach_device(struct device *dev)
+{
+	__arm_iommu_detach_device(dev);
+	set_dma_ops(dev, NULL);
+}
 EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
 
 static struct dma_map_ops *arm_get_iommu_dma_map_ops(bool coherent)
@@ -2011,7 +2034,7 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		return false;
 	}
 
-	if (arm_iommu_attach_device(dev, mapping)) {
+	if (__arm_iommu_attach_device(dev, mapping)) {
 		pr_warn("Failed to attached device %s to IOMMU_mapping\n",
 				dev_name(dev));
 		arm_iommu_release_mapping(mapping);
@@ -2025,7 +2048,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev)
 {
 	struct dma_iommu_mapping *mapping = dev->archdata.mapping;
 
-	arm_iommu_detach_device(dev);
+	__arm_iommu_detach_device(dev);
 	arm_iommu_release_mapping(mapping);
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Arnd Bergmann]

On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote:
> +/**
> + * arm_iommu_detach_device
> + * @dev: valid struct device pointer
> + *
> + * Detaches the provided device from a previously attached map.
> + * This voids the dma operations (dma_map_ops pointer)
> + */
> +void arm_iommu_detach_device(struct device *dev)
> +{
> +       __arm_iommu_detach_device(dev);
> +       set_dma_ops(dev, NULL);
> +}
>  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
>  
> 

Would this introduce a regression in the case where the device is
cache coherent and needs arm_coherent_dma_ops set?

	Arnd

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Laurent Pinchart]

Hi Arnd,

On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote:
> On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote:
> > +/**
> > + * arm_iommu_detach_device
> > + * @dev: valid struct device pointer
> > + *
> > + * Detaches the provided device from a previously attached map.
> > + * This voids the dma operations (dma_map_ops pointer)
> > + */
> > +void arm_iommu_detach_device(struct device *dev)
> > +{
> > +       __arm_iommu_detach_device(dev);
> > +       set_dma_ops(dev, NULL);
> > +}
> > 
> >  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
> 
> Would this introduce a regression in the case where the device is
> cache coherent and needs arm_coherent_dma_ops set?

I think we need to handle that case (as well as the coherent IOMMU case in 
arm_iommu_attach_device), but this patch only tries to restore the previous 
behaviour to fix the bug detailed in the commit message. Would you prefer 
fixing both issues in one go ?

-- 
Regards,

Laurent Pinchart

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Arnd Bergmann]

On Friday 23 January 2015 17:55:25 Laurent Pinchart wrote:
> 
> On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote:
> > On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote:
> > > +/**
> > > + * arm_iommu_detach_device
> > > + * @dev: valid struct device pointer
> > > + *
> > > + * Detaches the provided device from a previously attached map.
> > > + * This voids the dma operations (dma_map_ops pointer)
> > > + */
> > > +void arm_iommu_detach_device(struct device *dev)
> > > +{
> > > +       __arm_iommu_detach_device(dev);
> > > +       set_dma_ops(dev, NULL);
> > > +}
> > > 
> > >  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
> > 
> > Would this introduce a regression in the case where the device is
> > cache coherent and needs arm_coherent_dma_ops set?
> 
> I think we need to handle that case (as well as the coherent IOMMU case in 
> arm_iommu_attach_device), but this patch only tries to restore the previous 
> behaviour to fix the bug detailed in the commit message. Would you prefer 
> fixing both issues in one go ?
> 

No, I was specifically trying to find out whether the new behavior would
be worse than what we had in 3.18. If it's a preexisting bug that nobody
has run into, there is no hurry.

It's quite likely that to date, all IOMMU users on ARM32 are not
cache coherent.

	Arnd

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Will Deacon]

On Fri, Jan 23, 2015 at 05:00:45PM +0000, Arnd Bergmann wrote:
> On Friday 23 January 2015 17:55:25 Laurent Pinchart wrote:
> > 
> > On Friday 23 January 2015 16:27:24 Arnd Bergmann wrote:
> > > On Friday 23 January 2015 16:21:49 Laurent Pinchart wrote:
> > > > +/**
> > > > + * arm_iommu_detach_device
> > > > + * @dev: valid struct device pointer
> > > > + *
> > > > + * Detaches the provided device from a previously attached map.
> > > > + * This voids the dma operations (dma_map_ops pointer)
> > > > + */
> > > > +void arm_iommu_detach_device(struct device *dev)
> > > > +{
> > > > +       __arm_iommu_detach_device(dev);
> > > > +       set_dma_ops(dev, NULL);
> > > > +}
> > > > 
> > > >  EXPORT_SYMBOL_GPL(arm_iommu_detach_device);
> > > 
> > > Would this introduce a regression in the case where the device is
> > > cache coherent and needs arm_coherent_dma_ops set?
> > 
> > I think we need to handle that case (as well as the coherent IOMMU case in 
> > arm_iommu_attach_device), but this patch only tries to restore the previous 
> > behaviour to fix the bug detailed in the commit message. Would you prefer 
> > fixing both issues in one go ?
> > 
> 
> No, I was specifically trying to find out whether the new behavior would
> be worse than what we had in 3.18. If it's a preexisting bug that nobody
> has run into, there is no hurry.

Agreed, I think Laurent's proposal here is the smallest thing that fixes
the issue without breaking the new of_xlate-based configuration.

  Acked-by: Will Deacon <will.deacon@arm.com>

> It's quite likely that to date, all IOMMU users on ARM32 are not
> cache coherent.

Highbank has an ARM SMMU, but I don't see the iommu_coherent_ops getting
used, so your assertion is probably right.

Will

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Heiko Stübner]

Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart:
> Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops
> into arch_setup_dma_ops") moved the setting of the DMA operations from
> arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA
> operations to be used are selected based on whether the device is
> connected to an IOMMU. However, the IOMMU detection scheme requires the
> IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver
> has been ported yet, this effectively breaks all IOMMU ARM users that
> depend on the IOMMU being handled transparently by the DMA mapping API.
> 
> Fix this by restoring the setting of DMA IOMMU ops in
> arm_iommu_attach_device() and splitting the rest of the function into a
> new internal __arm_iommu_attach_device() function, called by
> arch_setup_dma_ops().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

on a rk3288 board with the currently failing drm driver this patch brought it 
back to display something :-) , so

Tested-by: Heiko Stuebner <heiko@sntech.de>


Thanks
Heiko

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Joerg Roedel]

On Fri, Jan 23, 2015 at 04:21:49PM +0200, Laurent Pinchart wrote:
> Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops
> into arch_setup_dma_ops") moved the setting of the DMA operations from
> arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA
> operations to be used are selected based on whether the device is
> connected to an IOMMU. However, the IOMMU detection scheme requires the
> IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver
> has been ported yet, this effectively breaks all IOMMU ARM users that
> depend on the IOMMU being handled transparently by the DMA mapping API.
> 
> Fix this by restoring the setting of DMA IOMMU ops in
> arm_iommu_attach_device() and splitting the rest of the function into a
> new internal __arm_iommu_attach_device() function, called by
> arch_setup_dma_ops().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  arch/arm/mm/dma-mapping.c | 53 +++++++++++++++++++++++++++++++++--------------
>  1 file changed, 38 insertions(+), 15 deletions(-)

I guess this patch will be merged by one of the ARM trees?


	Joerg

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Heiko Stübner]

Hi Arnd, Olof,

Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart:
> Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops
> into arch_setup_dma_ops") moved the setting of the DMA operations from
> arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA
> operations to be used are selected based on whether the device is
> connected to an IOMMU. However, the IOMMU detection scheme requires the
> IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver
> has been ported yet, this effectively breaks all IOMMU ARM users that
> depend on the IOMMU being handled transparently by the DMA mapping API.
> 
> Fix this by restoring the setting of DMA IOMMU ops in
> arm_iommu_attach_device() and splitting the rest of the function into a
> new internal __arm_iommu_attach_device() function, called by
> arch_setup_dma_ops().
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

in the original submission arm at kernel.org was not included, but it looks like 
the patch should go through arm-soc.

We have two tags
	Acked-by: Will Deacon <will.deacon@arm.com>
	Tested-by: Heiko Stuebner <heiko@sntech.de>

Can you find the original patch somehow or should it be resend to include 
arm at kernel.org ?


Thanks
Heiko

[PATCH URGENT] arm: dma-mapping: Set DMA IOMMU ops in arm_iommu_attach_device()

[Olof Johansson]

On Wed, Jan 28, 2015 at 01:25:35AM +0100, Heiko St?bner wrote:
> Hi Arnd, Olof,
> 
> Am Freitag, 23. Januar 2015, 16:21:49 schrieb Laurent Pinchart:
> > Commit 4bb25789ed28228a ("arm: dma-mapping: plumb our iommu mapping ops
> > into arch_setup_dma_ops") moved the setting of the DMA operations from
> > arm_iommu_attach_device() to arch_setup_dma_ops() where the DMA
> > operations to be used are selected based on whether the device is
> > connected to an IOMMU. However, the IOMMU detection scheme requires the
> > IOMMU driver to be ported to the new IOMMU of_xlate API. As no driver
> > has been ported yet, this effectively breaks all IOMMU ARM users that
> > depend on the IOMMU being handled transparently by the DMA mapping API.
> > 
> > Fix this by restoring the setting of DMA IOMMU ops in
> > arm_iommu_attach_device() and splitting the rest of the function into a
> > new internal __arm_iommu_attach_device() function, called by
> > arch_setup_dma_ops().
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> in the original submission arm at kernel.org was not included, but it looks like 
> the patch should go through arm-soc.
> 
> We have two tags
> 	Acked-by: Will Deacon <will.deacon@arm.com>
> 	Tested-by: Heiko Stuebner <heiko@sntech.de>
> 
> Can you find the original patch somehow or should it be resend to include 
> arm at kernel.org ?

The patch was posted on a list that I am subscribed to, so it popped up in
the same thread this time. I've applied it to fixes for 3.19.

Thanks,

-Olof