Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
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+rene...@ideasonboard.com> ---
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
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Magnus, >> Magnus, do you have a suggestion? > > Thanks for your efforts guys! > > I've recently been working on up-porting the IPMMU patches and > addressing review comments. Now with my local driver stack on top of > v4.12-rc (a95cfad) I did not notice these issues initially since I > only tested devices with IPMMU enabled. Once these issues were pointed > out to me by Geert I have now reproduced the 64-bit ARM specific ones > on r8a7796 Salvator-X. > > On my r8a7796 platform I'm using the following IOMMU and DMA Engine devices: > > IOMMU device IPMMU-DS0 - Connected to SYS-DMAC0 > IOMMU device IPMMU-DS1 - Connected to SYS-DMAC1 and SYS-DMAC2 > IOMMU device IPMMU-MM - Root device serving IPMMU-DS0 and IPMMU-DS1 > > For testing the serial port SCIF1 is used with DMA Engine devices > SYS-DMAC1 or SYS-DMAC2. > > The software environment is configured as follows: > - The DTS comes with all devices above enabled except IPMMU-DS0 which > comes with status = "disabled". > - The IPMMU driver is during run time only allowing use of SYS-DMAC2. > For other devices ->of_xlate() returns -ENODEV. > > The above used to work just fine with v4.11 or earlier. > > My observations for v4.12-rc: > > 1) Default state for a95cfad > > The devices SYS-DMAC0 and SYS-DMAC1 will never probe. However SYS-DMAC2 is > fine. > > 2) After applying "[PATCH] iommu: of: Fix check for returning EPROBE_DEFER" > [1] > > This fixes SYS-DMAC0 that now probes and operates without IPMMU-DS0 as > expected. Same as v4.11 or earlier. > > 3) After also applying "[PATCH] of: iommu: Ignore all errors except > EPROBE_DEFER" [2] > > This fixes SYS-DMAC1 that now probes and operates without IPMMU-DS1 as > expected. Same as v4.11 or earlier. > > With fix [1] and [2] things seem back to normal. Unless I'm mistaken Thanks, will use this as your Tested-by. Regards, Sricharan > it also seems that [1] allows me to drop the similar patch "[PATCH/RFC > v2 1/4] iommu/of: Skip IOMMU devices disabled in DT". > > Thanks for your help. > > Cheers, > > / magnus > > > [1] https://lkml.org/lkml/2017/5/16/25 > [2] https://www.spinics.net/lists/arm-kernel/msg581485.html > [3] https://patchwork.kernel.org/patch/9540605/ > -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings
ime 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. >> With that (or firm reassurance that it's OK not to), >> >> Reviewed-by: Robin Murphy <robin.mur...@arm.com> >> >> 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.pinch...@ideasonboard.com> > > 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 <sricha...@codeaurora.org> 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 <sricha...@codeaurora.org> --- 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 > -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation Regards, Sricharan
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi, On 2017-05-16 19:40, Laurent Pinchart wrote: Hi Robin, On Tuesday 16 May 2017 15:04:55 Robin Murphy wrote: On 16/05/17 08:17, Laurent Pinchart wrote: > On Tuesday 16 May 2017 07:53:57 sricha...@codeaurora.org wrote: [snip] >> arch_teardown_dma_ops() being the inverse of arch_setup_dma_ops() >> ,dma_ops should be cleared in the teardown path. Otherwise >> this causes problem when the probe of device is retried after >> being deferred. The device's iommu structures are cleared >> after EPROBEDEFER error, but on the next try dma_ops will still >> be set to old value, which is not right. >> >> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >> Reviewed-by: Robin Murphy <robin.mur...@arm.com> >> --- >> >> arch/arm/mm/dma-mapping.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c >> index ab4f745..a40f03e 100644 >> --- a/arch/arm/mm/dma-mapping.c >> +++ b/arch/arm/mm/dma-mapping.c >> @@ -2358,6 +2358,7 @@ static void arm_teardown_iommu_dma_ops(struct >> device *dev) > >>__arm_iommu_detach_device(dev); >>arm_iommu_release_mapping(mapping); >> + set_dma_ops(dev, NULL); >> } >> #else > > The subject mentions arch_teardown_dma_ops(), which I think is correct, > but the patch adds the set_dma_ops() call to arm_teardown_iommu_dma_ops(). > > However, the situation is perhaps more complex. Note the check at the > beginning of arch_setup_dma_ops(): >/* > * Don't override the dma_ops if they have already been set. Ideally > * this should be the only location where dma_ops are set, remove this > * check when all other callers of set_dma_ops will have disappeared. > */ >if (dev->dma_ops) >return; > > If you set the dma_ops to NULL in arm_teardown_iommu_dma_ops() or > arch_teardown_dma_ops(), the next call to arch_setup_dma_ops() will > override them. To be safe you should only set them to NULL if they have > been set by arch_setup_dma_ops(). More than that, arch_teardown_dma_ops() > should probably not call arm_teardown_iommu_dma_ops() at all if the > dma_ops were set by arm_iommu_attach_device() and not > arch_teardown_dma_ops(). Under what circumstances is that an issue? We'll only be tearing down the DMA ops when unbinding the driver, Or when deferring probe. and I think it would be erroneous to expect the device to retain much state after that. Everything else would be set up from scratch again if it get reprobed later, so why not the DMA ops? Because the DMA ops might have been set elsewhere than arch_setup_dma_ops(). If you look at the patch that added the above warning, its commit message states commit 26b37b946a5c2658dbc37dd5d6df40aaa9685d70 (iommu-joerg/arm/core) Author: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> Date: Fri May 15 02:00:02 2015 +0300 arm: dma-mapping: Don't override dma_ops in arch_setup_dma_ops() The arch_setup_dma_ops() function is in charge of setting dma_ops with a call to set_dma_ops(). set_dma_ops() is also called from - highbank and mvebu bus notifiers - dmabounce (to be replaced with swiotlb) - arm_iommu_attach_device (arm_iommu_attach_device is itself called from IOMMU and bus master device drivers) To allow the arch_setup_dma_ops() call to be moved from device add time to device probe time we must ensure that dma_ops already setup by any of the above callers will not be overriden. Aftering replacing dmabounce with swiotlb, converting IOMMU drivers to of_xlate and taking care of highbank and mvebu, the workaround should be removed. I'm concerned about potentially breaking these if we unconditionally remove the DMA ops and mapping. arch_teardown_dma_ops does nothing if there is no mapping (not behind iommu), dma_ops without iommu is ok. But when the arm_iommu_create_mapping/arm_iommu_attach_device was called previously in the iommu driver, after we teardown, that path in the iommu driver which called those functions is not replayed. Regards,
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Laurent, On 2017-05-16 12:47, Laurent Pinchart wrote: Hi Sricharan, On Tuesday 16 May 2017 07:53:57 sricha...@codeaurora.org wrote: On 2017-05-16 03:04, Laurent Pinchart wrote: > On Monday 15 May 2017 23:37:16 Laurent Pinchart wrote: >> On Wednesday 03 May 2017 15:54:59 Sricharan R wrote: >>> On 5/3/2017 3:24 PM, Robin Murphy wrote: >>>> On 02/05/17 19:35, Geert Uytterhoeven wrote: >>>>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R wrote: >>>>>> From: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> >>>>>> >>>>>> Failures to look up an IOMMU when parsing the DT iommus property >>>>>> need to be handled separately from the .of_xlate() failures to >>>>>> support deferred probing. >>>>>> >>>>>> The lack of a registered IOMMU can be caused by the lack of a driver >>>>>> for the IOMMU, the IOMMU device probe not having been performed yet, >>>>>> having been deferred, or having failed. >>>>>> >>>>>> The first case occurs when the device tree describes the bus master >>>>>> and IOMMU topology correctly but no device driver exists for the >>>>>> IOMMU yet or the device driver has not been compiled in. Return NULL, >>>>>> the caller will configure the device without an IOMMU. >>>>>> >>>>>> The second and third cases are handled by deferring the probe of the >>>>>> bus master device which will eventually get reprobed after the >>>>>> IOMMU. >>>>>> >>>>>> The last case is currently handled by deferring the probe of the bus >>>>>> master device as well. A mechanism to either configure the bus >>>>>> master device without an IOMMU or to fail the bus master device probe >>>>>> depending on whether the IOMMU is optional or mandatory would be a >>>>>> good enhancement. >>>>>> >>>>>> Tested-by: Marek Szyprowski <m.szyprow...@samsung.com> >>>>>> Signed-off-by: Laurent Pichart >>>>>> <laurent.pinchart+rene...@ideasonboard.com> >>>>>> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >>>>> >>>>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. >>>>> As the IOMMU nodes in DT are not yet enabled, all devices having >>>>> iommus properties in DT now fail to probe. >>>> >>>> How exactly do they fail to probe? Per d7b0558230e4, if there are no >>>> ops registered then they should merely defer until we reach the point >>>> of giving up and ignoring the IOMMU. Is it just that you have no other >>>> late-probing drivers or post-init module loads to kick the deferred >>>> queue after that point? I did try to find a way to explicitly kick it >>>> from a suitably late initcall, but there didn't seem to be any obvious >>>> public interface - anyone have any suggestions? >>>> >>>> I think that's more of a general problem with the probe deferral >>>> mechanism itself (I've seen the same thing happen with some of the >>>> CoreSight stuff on Juno due to the number of inter-component >>>> dependencies) rather than any specific fault of this series. >>> >>> I was thinking of an additional check like below to avoid the >>> situation ? >>> >>> From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 >>> From: Sricharan R <sricha...@codeaurora.org> >>> Date: Wed, 3 May 2017 13:16:59 +0530 >>> Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER >>> >>> While returning EPROBE_DEFER for iommu masters >>> take in to account of iommu nodes that could be >>> marked in DT as 'status=disabled', in which case >>> simply return NULL and let the master's probe >>> continue rather than deferring. >>> >>> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >>> --- >>> >>> drivers/iommu/of_iommu.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c >>> index 9f44ee8..e6e9bec 100644 >>> --- a/drivers/iommu/of_iommu.c >>> +++ b/drivers/iommu/of_iommu.c >>> @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct >>> device_node *np) >>> >>> ops = iommu_
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Will, On 2017-05-15 19:52, Will Deacon wrote: Hi Sricharan, On Wed, May 03, 2017 at 03:54:59PM +0530, Sricharan R wrote: On 5/3/2017 3:24 PM, Robin Murphy wrote: > On 02/05/17 19:35, Geert Uytterhoeven wrote: >> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R <sricha...@codeaurora.org> wrote: >>> From: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> >>> >>> Failures to look up an IOMMU when parsing the DT iommus property need to >>> be handled separately from the .of_xlate() failures to support deferred >>> probing. >>> >>> The lack of a registered IOMMU can be caused by the lack of a driver for >>> the IOMMU, the IOMMU device probe not having been performed yet, having >>> been deferred, or having failed. >>> >>> The first case occurs when the device tree describes the bus master and >>> IOMMU topology correctly but no device driver exists for the IOMMU yet >>> or the device driver has not been compiled in. Return NULL, the caller >>> will configure the device without an IOMMU. >>> >>> The second and third cases are handled by deferring the probe of the bus >>> master device which will eventually get reprobed after the IOMMU. >>> >>> The last case is currently handled by deferring the probe of the bus >>> master device as well. A mechanism to either configure the bus master >>> device without an IOMMU or to fail the bus master device probe depending >>> on whether the IOMMU is optional or mandatory would be a good >>> enhancement. >>> >>> Tested-by: Marek Szyprowski <m.szyprow...@samsung.com> >>> Signed-off-by: Laurent Pichart <laurent.pinchart+rene...@ideasonboard.com> >>> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >> >> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. >> As the IOMMU nodes in DT are not yet enabled, all devices having iommus >> properties in DT now fail to probe. > > How exactly do they fail to probe? Per d7b0558230e4, if there are no ops > registered then they should merely defer until we reach the point of > giving up and ignoring the IOMMU. Is it just that you have no other > late-probing drivers or post-init module loads to kick the deferred > queue after that point? I did try to find a way to explicitly kick it > from a suitably late initcall, but there didn't seem to be any obvious > public interface - anyone have any suggestions? > > I think that's more of a general problem with the probe deferral > mechanism itself (I've seen the same thing happen with some of the > CoreSight stuff on Juno due to the number of inter-component > dependencies) rather than any specific fault of this series. > I was thinking of an additional check like below to avoid the situation ? From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 From: Sricharan R <sricha...@codeaurora.org> Date: Wed, 3 May 2017 13:16:59 +0530 Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER While returning EPROBE_DEFER for iommu masters take in to account of iommu nodes that could be marked in DT as 'status=disabled', in which case simply return NULL and let the master's probe continue rather than deferring. Signed-off-by: Sricharan R <sricha...@codeaurora.org> --- drivers/iommu/of_iommu.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 9f44ee8..e6e9bec 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node *np) ops = iommu_ops_from_fwnode(fwnode); if ((ops && !ops->of_xlate) || + !of_device_is_available(iommu_spec->np) || (!ops && !of_iommu_driver_present(iommu_spec->np))) return NULL; Without this patch, v4.12-rc1 hangs on my Juno waiting to mount the root filesystem. The problem is that the USB controller is behind an SMMU which is marked as 'status = "disabled"' in the devicetree. Whilst there was a separate thread with Ard about exactly what this means in terms of the DMA ops used by upstream devices, your patch above fixes the regression and I think should go in regardless. The DMA ops issue will likely require an additional DT binding anyway, to advertise the behaviour of the IOMMU when it is disabled. Tested-by: Will Deacon <will.dea...@arm.com> Acked-by: Will Deacon <will.dea...@arm.com> Could you resend it as a proper patch, please? Sure, will send this as a separate patch. Regards, Sricharan
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Laurent, On 2017-05-16 03:04, Laurent Pinchart wrote: Hi Sricharan, On Monday 15 May 2017 23:37:16 Laurent Pinchart wrote: On Wednesday 03 May 2017 15:54:59 Sricharan R wrote: > On 5/3/2017 3:24 PM, Robin Murphy wrote: >> On 02/05/17 19:35, Geert Uytterhoeven wrote: >>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R wrote: >>>> From: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> >>>> >>>> Failures to look up an IOMMU when parsing the DT iommus property need >>>> to be handled separately from the .of_xlate() failures to support >>>> deferred probing. >>>> >>>> The lack of a registered IOMMU can be caused by the lack of a driver >>>> for the IOMMU, the IOMMU device probe not having been performed yet, >>>> having been deferred, or having failed. >>>> >>>> The first case occurs when the device tree describes the bus master >>>> and IOMMU topology correctly but no device driver exists for the IOMMU >>>> yet or the device driver has not been compiled in. Return NULL, the >>>> caller will configure the device without an IOMMU. >>>> >>>> The second and third cases are handled by deferring the probe of the >>>> bus master device which will eventually get reprobed after the IOMMU. >>>> >>>> The last case is currently handled by deferring the probe of the bus >>>> master device as well. A mechanism to either configure the bus master >>>> device without an IOMMU or to fail the bus master device probe >>>> depending on whether the IOMMU is optional or mandatory would be a good >>>> enhancement. >>>> >>>> Tested-by: Marek Szyprowski <m.szyprow...@samsung.com> >>>> Signed-off-by: Laurent Pichart >>>> <laurent.pinchart+rene...@ideasonboard.com> >>>> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >>> >>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. >>> As the IOMMU nodes in DT are not yet enabled, all devices having iommus >>> properties in DT now fail to probe. >> >> How exactly do they fail to probe? Per d7b0558230e4, if there are no ops >> registered then they should merely defer until we reach the point of >> giving up and ignoring the IOMMU. Is it just that you have no other >> late-probing drivers or post-init module loads to kick the deferred >> queue after that point? I did try to find a way to explicitly kick it >> from a suitably late initcall, but there didn't seem to be any obvious >> public interface - anyone have any suggestions? >> >> I think that's more of a general problem with the probe deferral >> mechanism itself (I've seen the same thing happen with some of the >> CoreSight stuff on Juno due to the number of inter-component >> dependencies) rather than any specific fault of this series. > > I was thinking of an additional check like below to avoid the > situation ? > > From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 > From: Sricharan R <sricha...@codeaurora.org> > Date: Wed, 3 May 2017 13:16:59 +0530 > Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER > > While returning EPROBE_DEFER for iommu masters > take in to account of iommu nodes that could be > marked in DT as 'status=disabled', in which case > simply return NULL and let the master's probe > continue rather than deferring. > > Signed-off-by: Sricharan R <sricha...@codeaurora.org> > --- > > drivers/iommu/of_iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 9f44ee8..e6e9bec 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node > *np) > > ops = iommu_ops_from_fwnode(fwnode); > if ((ops && !ops->of_xlate) || > + !of_device_is_available(iommu_spec->np) || > (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; This looks good to me, but won't be enough. The ipmmu-vmsa driver in v4.12-rc1 doesn't call iommu_device_register() and thus won't be found by iommu_ops_from_fwnode(). Furthermore, it doesn't IOMMU_OF_DECLARE(), and thus will always be considered as absent. I agree that the ipmmu-vmsa driver needs to be fixed, but it would have been nice to check existing IOMMU drivers before merging this patch series... Please pardon the question, but has this patch series been tested on ARM32 ? When th
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi, On 5/3/2017 3:54 PM, Sricharan R wrote: > Hi Robin, > > On 5/3/2017 3:24 PM, Robin Murphy wrote: >> Hi Geert, >> >> On 02/05/17 19:35, Geert Uytterhoeven wrote: >>> Hi Sricharan, >>> >>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R <sricha...@codeaurora.org> >>> wrote: >>>> From: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> >>>> >>>> Failures to look up an IOMMU when parsing the DT iommus property need to >>>> be handled separately from the .of_xlate() failures to support deferred >>>> probing. >>>> >>>> The lack of a registered IOMMU can be caused by the lack of a driver for >>>> the IOMMU, the IOMMU device probe not having been performed yet, having >>>> been deferred, or having failed. >>>> >>>> The first case occurs when the device tree describes the bus master and >>>> IOMMU topology correctly but no device driver exists for the IOMMU yet >>>> or the device driver has not been compiled in. Return NULL, the caller >>>> will configure the device without an IOMMU. >>>> >>>> The second and third cases are handled by deferring the probe of the bus >>>> master device which will eventually get reprobed after the IOMMU. >>>> >>>> The last case is currently handled by deferring the probe of the bus >>>> master device as well. A mechanism to either configure the bus master >>>> device without an IOMMU or to fail the bus master device probe depending >>>> on whether the IOMMU is optional or mandatory would be a good >>>> enhancement. >>>> >>>> Tested-by: Marek Szyprowski <m.szyprow...@samsung.com> >>>> Signed-off-by: Laurent Pichart <laurent.pinchart+rene...@ideasonboard.com> >>>> Signed-off-by: Sricharan R <sricha...@codeaurora.org> >>> >>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers. >>> As the IOMMU nodes in DT are not yet enabled, all devices having iommus >>> properties in DT now fail to probe. >> >> How exactly do they fail to probe? Per d7b0558230e4, if there are no ops >> registered then they should merely defer until we reach the point of >> giving up and ignoring the IOMMU. Is it just that you have no other >> late-probing drivers or post-init module loads to kick the deferred >> queue after that point? I did try to find a way to explicitly kick it >> from a suitably late initcall, but there didn't seem to be any obvious >> public interface - anyone have any suggestions? >> >> I think that's more of a general problem with the probe deferral >> mechanism itself (I've seen the same thing happen with some of the >> CoreSight stuff on Juno due to the number of inter-component >> dependencies) rather than any specific fault of this series. >> > > I was thinking of an additional check like below to avoid the > situation ? > > From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001 > From: Sricharan R <sricha...@codeaurora.org> > Date: Wed, 3 May 2017 13:16:59 +0530 > Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER > > While returning EPROBE_DEFER for iommu masters > take in to account of iommu nodes that could be > marked in DT as 'status=disabled', in which case > simply return NULL and let the master's probe > continue rather than deferring. > > Signed-off-by: Sricharan R <sricha...@codeaurora.org> > --- > drivers/iommu/of_iommu.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 9f44ee8..e6e9bec 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node > *np) > > ops = iommu_ops_from_fwnode(fwnode); > if ((ops && !ops->of_xlate) || > + !of_device_is_available(iommu_spec->np) || > (!ops && !of_iommu_driver_present(iommu_spec->np))) > return NULL; > While same as the other 'status=disabled' patch [1], better not to defer the probe itself in the case ? [1] https://patchwork.kernel.org/patch/9681211/ Regards, Sricharan -- "QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error
Hi Robin,
On 5/3/2017 3:24 PM, Robin Murphy wrote:
> Hi Geert,
>
> On 02/05/17 19:35, Geert Uytterhoeven wrote:
>> Hi Sricharan,
>>
>> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R <sricha...@codeaurora.org> wrote:
>>> From: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
>>>
>>> Failures to look up an IOMMU when parsing the DT iommus property need to
>>> be handled separately from the .of_xlate() failures to support deferred
>>> probing.
>>>
>>> The lack of a registered IOMMU can be caused by the lack of a driver for
>>> the IOMMU, the IOMMU device probe not having been performed yet, having
>>> been deferred, or having failed.
>>>
>>> The first case occurs when the device tree describes the bus master and
>>> IOMMU topology correctly but no device driver exists for the IOMMU yet
>>> or the device driver has not been compiled in. Return NULL, the caller
>>> will configure the device without an IOMMU.
>>>
>>> The second and third cases are handled by deferring the probe of the bus
>>> master device which will eventually get reprobed after the IOMMU.
>>>
>>> The last case is currently handled by deferring the probe of the bus
>>> master device as well. A mechanism to either configure the bus master
>>> device without an IOMMU or to fail the bus master device probe depending
>>> on whether the IOMMU is optional or mandatory would be a good
>>> enhancement.
>>>
>>> Tested-by: Marek Szyprowski <m.szyprow...@samsung.com>
>>> Signed-off-by: Laurent Pichart <laurent.pinchart+rene...@ideasonboard.com>
>>> Signed-off-by: Sricharan R <sricha...@codeaurora.org>
>>
>> This patch broke Renesas R-Car Gen3 platforms in renesas-drivers.
>> As the IOMMU nodes in DT are not yet enabled, all devices having iommus
>> properties in DT now fail to probe.
>
> How exactly do they fail to probe? Per d7b0558230e4, if there are no ops
> registered then they should merely defer until we reach the point of
> giving up and ignoring the IOMMU. Is it just that you have no other
> late-probing drivers or post-init module loads to kick the deferred
> queue after that point? I did try to find a way to explicitly kick it
> from a suitably late initcall, but there didn't seem to be any obvious
> public interface - anyone have any suggestions?
>
> I think that's more of a general problem with the probe deferral
> mechanism itself (I've seen the same thing happen with some of the
> CoreSight stuff on Juno due to the number of inter-component
> dependencies) rather than any specific fault of this series.
>
I was thinking of an additional check like below to avoid the
situation ?
>From 499b6e662f60f23740b8880882b0a16f16434501 Mon Sep 17 00:00:00 2001
From: Sricharan R <sricha...@codeaurora.org>
Date: Wed, 3 May 2017 13:16:59 +0530
Subject: [PATCH] iommu: of: Fix check for returning EPROBE_DEFER
While returning EPROBE_DEFER for iommu masters
take in to account of iommu nodes that could be
marked in DT as 'status=disabled', in which case
simply return NULL and let the master's probe
continue rather than deferring.
Signed-off-by: Sricharan R <sricha...@codeaurora.org>
---
drivers/iommu/of_iommu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 9f44ee8..e6e9bec 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -118,6 +118,7 @@ static bool of_iommu_driver_present(struct device_node *np)
ops = iommu_ops_from_fwnode(fwnode);
if ((ops && !ops->of_xlate) ||
+ !of_device_is_available(iommu_spec->np) ||
(!ops && !of_iommu_driver_present(iommu_spec->np)))
return NULL;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Regards,
Sricharan
> Robin.
>
>> This can be fixed by either:
>> - Disabling CONFIG_IPMMU_VMSA, or
>> - Reverting commit 7b07cbefb68d486f (but keeping "int ret = 0;").
>>
>> Note that this was a bit hard to investigate, as R-Car Gen3 support wasn't
>> upstreamed yet, so bisection pointed to a merge commit.
>>
>>> ---
>>> [*] Fixed minor comment from Bjorn for removing the pci.h header inclusion
>>> in of_iommu.c
>>>
>>> drivers/base/dma-mapping.c | 5 +++--
>>> drivers/iommu/of_iommu.c | 4 ++--
>>> drivers/of/device.c| 7 ++-
>>> include/linux/of_device.h | 9 ++---
>>> 4 files changed, 17 insertions(+), 8 deletions(-)
&
RE: [PATCH/RFC 1/2] arm64: mm: Silently allow devices lacking IOMMU group
Hi Robin,
>> Consider failure of iommu_get_domain_for_dev() as non-critical and
>> get rid of the warning printout. This allows IOMMU properties to be
>> included in the DTB even though the kernel is configured with
>> CONFIG_IOMMU_API=n or in case a particular IOMMU driver refuses to
>> enable IOMMU support for a certain slave device and returns error
>> from the ->add_device() callback.
>>
>> This is only a cosmetic change that removes console warning printouts.
>
>The warning is there for a reason - at this point, we *expected* the
>device to be using an IOMMU for DMA, so a failure is significant. Rather
>than masking genuine failures in other cases because your case
>deliberately breaks that expectation, simply change the expectation -
>i.e. rather than letting of_xlate() succeed then failing add_device()
>later, reject the of_xlate() call up-front such that the DMA layer never
>gets told about the IOMMU in the first place.
>
>Robin.
>
With the iommu probe deferral patches, this behavior would change
where the arch_setup_dma_ops would never be called if there is
an error from xlate or add_device. But also the error value from
xlate/add_device is returned back and the probe of the device
would fail for any error. So if there can be cases like above, where
the xlate/add_device callbacks can return error for specific reasons,
should only EPROBE_DEFER be considered and rest of the errors
be filtered out with a WARN probably ?
Regards,
Sricharan
>> Signed-off-by: Magnus Damm <damm+rene...@opensource.se>
>> ---
>>
>> arch/arm64/mm/dma-mapping.c | 10 +-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> --- 0001/arch/arm64/mm/dma-mapping.c
>> +++ work/arch/arm64/mm/dma-mapping.c 2017-01-23 20:54:40.060607110 +0900
>> @@ -827,11 +827,19 @@ static bool do_iommu_attach(struct devic
>> struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
>>
>> /*
>> + * In case IOMMU support is excluded from the kernel or if the device
>> + * is not hooked up to any IOMMU group then be silent and keep the
>> + * old dma_ops.
>> + */
>> +if (!domain)
>> +return false;
>> +
>> +/*
>> * If the IOMMU driver has the DMA domain support that we require,
>> * then the IOMMU core will have already configured a group for this
>> * device, and allocated the default domain for that group.
>> */
>> -if (!domain || iommu_dma_init_domain(domain, dma_base, size, dev)) {
>> +if (iommu_dma_init_domain(domain, dma_base, size, dev)) {
>> pr_warn("Failed to set up IOMMU for device %s; retaining
>> platform DMA ops\n",
>> dev_name(dev));
>> return false;
>> ___
>> iommu mailing list
>> io...@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu
>>
>
>___
>iommu mailing list
>io...@lists.linux-foundation.org
>https://lists.linuxfoundation.org/mailman/listinfo/iommu
