On 2017-05-17 10:45, Sricharan R wrote:
Hi Laurent/Robin,
On 5/16/2017 10:14 PM, Laurent Pinchart wrote:
Hi Robin,
On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
On 16/05/17 16:14, Laurent Pinchart wrote:
arch_setup_dma_ops() is used in device probe code paths to create an
IOMMU mapping and attach it to the device. The function assumes that
the
device is attached to a device-specific IOMMU instance (or at least
a
device-specific TLB in a shared IOMMU instance) and thus creates a
separate mapping for every device.
On several systems (Renesas R-Car Gen2 being one of them), that
assumption is not true, and IOMMU mappings must be shared between
multiple devices. In those cases the IOMMU driver knows better than
the
generic ARM dma-mapping layer and attaches mapping to devices
manually
with arm_iommu_attach_device(), which sets the DMA ops for the
device.
The arch_setup_dma_ops() function takes this into account and bails
out
immediately if the device already has DMA ops assigned. However, the
corresponding arch_teardown_dma_ops() function, called from driver
unbind code paths (including probe deferral), will tear the mapping
down
regardless of who created it. When the device is reprobed
arch_setup_dma_ops() will be called again but won't perform any
operation as the DMA ops will still be set.
We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
However, we can't do so unconditionally, as then a new mapping would
be
created by arch_setup_dma_ops() when the device is reprobed,
regardless
of whether the device needs to share a mapping or not. We must thus
keep
track of whether arch_setup_dma_ops() created the mapping, and only
in
that case tear it down in arch_teardown_dma_ops().
Keep track of that information in the dev_archdata structure. As the
structure is embedded in all instances of struct device let's not
grow
it, but turn the existing dma_coherent bool field into a bitfield
that
can be used for other purposes.
Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with
deferred
probing or error") Signed-off-by: Laurent Pinchart
<laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> ---
arch/arm/include/asm/device.h | 3 ++-
arch/arm/mm/dma-mapping.c | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm/include/asm/device.h
b/arch/arm/include/asm/device.h
index 36ec9c8f6e16..3234fe9bba6e 100644
--- a/arch/arm/include/asm/device.h
+++ b/arch/arm/include/asm/device.h
@@ -19,7 +19,8 @@ struct dev_archdata {
#ifdef CONFIG_XEN
const struct dma_map_ops *dev_dma_ops;
#endif
- bool dma_coherent;
+ unsigned int dma_coherent:1;
This should only ever be accessed by the Xen DMA code via the
is_device_dma_coherent() helper, so I can't see the change of storage
type causing any problems.
Thank you for double-checking. I agree with your analysis.
+ unsigned int dma_ops_setup:1;
};
struct omap_device;
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index c742dfd2967b..e0272f9140e2 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2430,9 +2430,14 @@ void arch_setup_dma_ops(struct device *dev,
u64
dma_base, u64 size,
dev->dma_ops = xen_dma_ops;
}
#endif
+ dev->archdata.dma_ops_setup = true;
}
void arch_teardown_dma_ops(struct device *dev)
{
+ if (!dev->archdata.dma_ops_setup)
+ return;
+
arm_teardown_iommu_dma_ops(dev);
+ set_dma_ops(dev, NULL);
Should we clear dma_ops_setup here for symmetry? I guess in practice
it's down to the IOMMU driver so will never change after the first
probe, but it still feels like a bit of a nagging loose end.
To make a difference, we would need an IOMMU driver that creates a
mapping
after a first round of arch_setup_dma_ops() / arch_teardown_dma_ops()
calls,
follow by a second round. I don't think this could happen, but if it
did, I
believe we'd be screwed already, as there would be a time were an
incorrect
mapping (created by arch_setup_dma_ops() while the IOMMU driver needs
to take
care of mapping creation) exists.
Feels correct not to reset this, the iommu drivers in question, seems
to
creating mapping/attaching in add_device path (which gets called before
the
clients gets probed) and when the iommu client gets deferred/reprobed
that
does not happen again even after the first round.
Please ignore the above comment. I said that because I was doing the
dma_ops_setup in arm_iommu_attach_device. I posted the three fixes now
[1].
Accidentally removed you from CC, sorry for that.
Applied those patches on top of 8674/1 that Robin mentioned
below. So removed setting set_dma_ops(dev, NULL) from your patch.
Also please note that, I changed the Fixes: commit msg in your patch to
("of/acpi: Configure dma operations at probe time for platform/amba/pci
bus devices")
because that was one which started to invoke the teardown on the driver
release path.
[1] https://lkml.org/lkml/2017/5/17/344
Regards,
Sricharan
With that (or firm reassurance that it's OK not to),
Reviewed-by: Robin Murphy <robin.murphy@xxxxxxx>
Apologies for being too arm64-focused in the earlier reviews and
overlooking this. Should the patch supersede 8674/1 currently in
Russell's incoming box?
Yes I think it should. Could you please take care of that ?
You can also add my
was
Tested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
as I've tested that this paptch restores proper IOMMU operation on the
Renesas
R-Car H2 Lager board. I believe the problem related to Sricharan's
patch
reported by Geert still affects us and needs to be addressed
separately.
Thanks for the above, i had the same thing to be posted, was just
testing it once.
There are three patches [1][2], already posted and third one for the
issue that Geert
pointed i did below (Geert had a patch little differently to ignore
-ENODEV).
I had this question previously for not propagating errors apart from
EPROBE_DEFER,
did not have an issue reported at that time. Anyways if the below is
ok, i will
just send the 3 patches in one set for easy picking up ?
[1] https://lkml.org/lkml/2017/5/16/25
[2] The above one that you have.
[3] The below one, if its fine ?
From 4b379d4b852c41d7b5904c9a9e53deda94039f0a Mon Sep 17 00:00:00 2001
From: Sricharan R <sricharan@xxxxxxxxxxxxxx>
Date: Wed, 3 May 2017 14:54:11 +0530
Subject: [PATCH] of: iommu: Ignore all errors except EPROBE_DEFER
While deferring the probe of iommu masters,
xlate and add_device callback can passback error values
like -ENODEV, which means iommu cannot be connected
with that master for real reasons. So rather than
killing the master's probe for such errors, just
ignore the errors and let the master work without
an iommu.
Signed-off-by: Sricharan R <sricharan@xxxxxxxxxxxxxx>
---
drivers/iommu/of_iommu.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index e6e9bec..750ab07 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -237,6 +237,10 @@ const struct iommu_ops *of_iommu_configure(struct
device *dev,
ops = ERR_PTR(err);
}
+ /* Ignore all other errors apart from EPROBE_DEFER */
+ if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER))
+ ops = NULL;
+
return ops;
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
