Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sricharan R]

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 V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Magnus Damm]

Hi Geert, everyone,

On Fri, May 5, 2017 at 10:23 PM, Geert Uytterhoeven
 wrote:
> Hi Sricharan, Robin,
>
> On Wed, May 3, 2017 at 12:24 PM, 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 
>
> 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 
> Signed-off-by: Laurent Pichart 
> Signed-off-by: Sricharan R 

 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 had a deeper look into the issue.
>
> What changed, is that of_dma_configure() now returns an error code,
> and dma_configure() looks at it.
>
> Actually there are two failure modes:
>   1. Devices with an iommus property pointing to a disabled IOMMU node.
>  These return -EPROBE_DEFER, and are now retried forever.
>   2. Devices that are blacklisted in the IPMMU driver, as we don't want to
>  use them with an IOMMU yet.
>  These return -ENODEV, due to ipmmu_of_xlate_dma().
>
>> 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 
>> 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 
>> ---
>>  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;
>
> Thanks, this fixes the first class of failures.
>
> The second class can be worked around using:
>
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -196,6 +196,11 @@ static const struct iommu_ops
> ops = of_iommu_xlate(dev, _spec);
> of_node_put(iommu_spec.np);
> idx++;
> +   if (PTR_ERR(ops) == -ENODEV) {
> +   dev_info(dev, "%s: Ignoring -ENODEV => NULL\n",
> +__func__);

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Robin Murphy]

On 16/05/17 15:10, 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 
 Reviewed-by: Robin Murphy 
 ---

   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 
> 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.

Ah, sorry, I see now - it was taking a long time to page the 32-bit code
back in, and I'd forgotten the specifics of the mess of competing
"default domain" notions. Indeed, it's not the device's driver expecting
any state to be preserved as I got stuck on, it's the IOMMU driver,
which does "know better" to an extent, expecting its changes to the
struct device to stick for the lifetime of that structure.

I agree there shouldn't be a disparity - arch_setup_dma_ops() only does
things given certain circumstances, so arch_teardown_dma_ops() should
only undo them under the same. With probe-deferral in place I'll be
reviving my work to convert this path over to IOMMU API default domains,
which will make some of these issues go away again, but in the meantime
I also agree that the most expedient fix is indeed to add a flag to say
whether the dma ops were automatically set or not (this is implicitly
true on arm64, which was partly what was tripping me up).

Robin.

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Laurent Pinchart]

Hi Sricharan,

On Tuesday 16 May 2017 19:59:01 sricha...@codeaurora.org wrote:
> On 2017-05-16 19:40, Laurent Pinchart wrote:
> > 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 
>  Reviewed-by: Robin Murphy 
>  ---
>  
>    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 
> > 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.

I've had a look at the code in more details, and I'm not sure how we've 
reached the current situation (as I haven't followed the multiple versions of 
this patch series due to lack of time), but it's a very big mess.

arch_setup_dma_ops() is currently not the only way to create a mapping and 
attach it to a device. Not only that, but with the current ARM32 dma-mapping 
implementation, it can't be.

arch_setup_dma_ops() will create a separate 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[sricharan]

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 
>> Reviewed-by: Robin Murphy 
>> ---
>>
>>   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 
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

[Laurent Pinchart]

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 
> >> Reviewed-by: Robin Murphy 
> >> ---
> >> 
> >>   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 
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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Laurent Pinchart]

Hi Sricharan,

On Tuesday 16 May 2017 19:10:03 sricha...@codeaurora.org wrote:
> On 2017-05-16 12:47, Laurent Pinchart wrote:
> > 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 
>  
>  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 
>  Signed-off-by: Laurent Pichart
>  
>  Signed-off-by: Sricharan R 
> >>> 
> >>> 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 
> > 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 
> > ---
> > 
> >  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 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Robin Murphy]

Hi Laurent,

On 16/05/17 08:17, 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 

 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 
 Signed-off-by: Laurent Pichart
 
 Signed-off-by: Sricharan R 
>>>
>>> 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 
> 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 
> ---
>
>  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 the device is probed the arch_setup_dma_ops() function is called.
>>> It sets the device's dma_ops and the mapping (in
>>> 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[sricharan]

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 
>>
>> 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 
>> Signed-off-by: Laurent Pichart
>> 
>> Signed-off-by: Sricharan R 
>
> 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 
>>> 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 
>>> ---
>>>
>>>  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 the device is probed the arch_setup_dma_ops() function is called.
> It sets the device's dma_ops and the mapping (in
> __arm_iommu_attach_device()). If probe is deferred,
> arch_teardown_dma_ops() is called which in turn calls
> arch_teardown_dma_ops(). This removes the mapping but doesn't touch the
> dma_ops. The next time the 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sakari Ailus]

Hi Laurent,

On Tue, May 16, 2017 at 10:17:08AM +0300, 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 
> > >> 
> > >> 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 
> > >> Signed-off-by: Laurent Pichart
> > >> 
> > >> Signed-off-by: Sricharan R 
> > > 
> > > 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 
> > >>> 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 
> > >>> ---
> > >>> 
> > >>>  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...
> > 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Laurent Pinchart]

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 
> >> 
> >> 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 
> >> Signed-off-by: Laurent Pichart
> >> 
> >> Signed-off-by: Sricharan R 
> > 
> > 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 
> >>> 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 
> >>> ---
> >>> 
> >>>  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 the device is probed the arch_setup_dma_ops() function is called.
> > It sets the device's dma_ops and the mapping (in
> > __arm_iommu_attach_device()). If probe is 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[sricharan]

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  wrote:
>>> From: Laurent Pinchart 
>>>
>>> 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 
>>> Signed-off-by: Laurent Pichart 
>>> Signed-off-by: Sricharan R 
>>
>> 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 
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 
---
 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 
Acked-by: Will Deacon 

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

[sricharan]

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 

 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 
 Signed-off-by: Laurent Pichart
 
 Signed-off-by: Sricharan R 
>>>
>>> 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 
> 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 
> ---
>
>  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 the device is probed the arch_setup_dma_ops() function is called. 
It sets
the device's dma_ops and the mapping (in __arm_iommu_attach_device()). 
If
probe is deferred, arch_teardown_dma_ops() is called which in turn 
calls

arch_teardown_dma_ops(). This removes the mapping but doesn't touch the
dma_ops. The next time the device is probed, arch_setup_dma_ops() bails 
out
immediately as the dma_ops are already set, leaving us with a device 
bound to
IOMMU operations but with no mapping. This oopses later as soon as the 
kernel

tries to map memory for the device through the IOMMU.


Resetting the dma_ops for arm32 was added in this 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Laurent Pinchart]

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 
>  
>  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 
>  Signed-off-by: Laurent Pichart
>  
>  Signed-off-by: Sricharan R 
> >>> 
> >>> 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 
> > 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 
> > ---
> > 
> >  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 the device is probed the arch_setup_dma_ops() function is called. It sets 
the device's dma_ops and the mapping (in __arm_iommu_attach_device()). If 
probe is deferred, arch_teardown_dma_ops() is called which in turn calls 
arch_teardown_dma_ops(). This removes the mapping but doesn't touch the 
dma_ops. The next time the device is probed, arch_setup_dma_ops() bails out 
immediately as the dma_ops are already set, leaving us with a device bound to 
IOMMU operations but with no mapping. This oopses later as 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Laurent Pinchart]

Hi Sricharan,

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 
> >>> 
> >>> 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 
> >>> Signed-off-by: Laurent Pichart
> >>> 
> >>> Signed-off-by: Sricharan R 
> >> 
> >> 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 
> 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 
> ---
>  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...

> >> 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Will Deacon]

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  
> >> wrote:
> >>> From: Laurent Pinchart 
> >>>
> >>> 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 
> >>> Signed-off-by: Laurent Pichart 
> >>> Signed-off-by: Sricharan R 
> >>
> >> 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 
> 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 
> ---
>  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 
Acked-by: Will Deacon 

Could you resend it as a proper patch, please?

Will

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Geert Uytterhoeven]

Hi Sricharan, Robin,

On Wed, May 3, 2017 at 12:24 PM, 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 

 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 
 Signed-off-by: Laurent Pichart 
 Signed-off-by: Sricharan R 
>>>
>>> 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 had a deeper look into the issue.

What changed, is that of_dma_configure() now returns an error code,
and dma_configure() looks at it.

Actually there are two failure modes:
  1. Devices with an iommus property pointing to a disabled IOMMU node.
 These return -EPROBE_DEFER, and are now retried forever.
  2. Devices that are blacklisted in the IPMMU driver, as we don't want to
 use them with an IOMMU yet.
 These return -ENODEV, due to ipmmu_of_xlate_dma().

> 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 
> 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 
> ---
>  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;

Thanks, this fixes the first class of failures.

The second class can be worked around using:

--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -196,6 +196,11 @@ static const struct iommu_ops
ops = of_iommu_xlate(dev, _spec);
of_node_put(iommu_spec.np);
idx++;
+   if (PTR_ERR(ops) == -ENODEV) {
+   dev_info(dev, "%s: Ignoring -ENODEV => NULL\n",
+__func__);
+   return NULL;
+   }
if (IS_ERR_OR_NULL(ops))
break;
}

but obviously that's too hackish to apply...
Magnus, do you have a suggestion?

Thanks!

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Sricharan R]

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  
>>> wrote:
 From: Laurent Pinchart 

 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 
 Signed-off-by: Laurent Pichart 
 Signed-off-by: Sricharan R 
>>>
>>> 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 
> 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 
> ---
>  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

[Sricharan R]

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  wrote:
>>> From: Laurent Pinchart 
>>>
>>> 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 
>>> Signed-off-by: Laurent Pichart 
>>> Signed-off-by: Sricharan R 
>>
>> 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 
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 
---
 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(-)
>>>
>>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
>>> index 449b948..82bd45c 100644
>>> --- a/drivers/base/dma-mapping.c
>>> +++ b/drivers/base/dma-mapping.c
>>> @@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
>>>  {
>>> struct device *bridge = NULL, *dma_dev = dev;
>>> enum dev_dma_attr attr;
>>> +   int ret = 0;
>>>
>>> if (dev_is_pci(dev)) {
>>> bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>>> @@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
>>> }

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Robin Murphy]

Hi Geert,

On 02/05/17 19:35, Geert Uytterhoeven wrote:
> Hi Sricharan,
> 
> On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R  wrote:
>> From: Laurent Pinchart 
>>
>> 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 
>> Signed-off-by: Laurent Pichart 
>> Signed-off-by: Sricharan R 
> 
> 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.

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(-)
>>
>> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
>> index 449b948..82bd45c 100644
>> --- a/drivers/base/dma-mapping.c
>> +++ b/drivers/base/dma-mapping.c
>> @@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
>>  {
>> struct device *bridge = NULL, *dma_dev = dev;
>> enum dev_dma_attr attr;
>> +   int ret = 0;
>>
>> if (dev_is_pci(dev)) {
>> bridge = pci_get_host_bridge_device(to_pci_dev(dev));
>> @@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
>> }
>>
>> if (dma_dev->of_node) {
>> -   of_dma_configure(dev, dma_dev->of_node);
>> +   ret = of_dma_configure(dev, dma_dev->of_node);
>> } else if (has_acpi_companion(dma_dev)) {
>> attr = 
>> acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
>> if (attr != DEV_DMA_NOT_SUPPORTED)
>> @@ -373,7 +374,7 @@ int dma_configure(struct device *dev)
>> if (bridge)
>> pci_put_host_bridge_device(bridge);
>>
>> -   return 0;
>> +   return ret;
>>  }
>>
>>  void dma_deconfigure(struct device *dev)
>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>> index 1f92d98..2d04663 100644
>> --- a/drivers/iommu/of_iommu.c
>> +++ b/drivers/iommu/of_iommu.c
>> @@ -236,7 +236,7 @@ const struct iommu_ops *of_iommu_configure(struct device 
>> *dev,
>> ops = ERR_PTR(err);
>> }
>>
>> -   return IS_ERR(ops) ? NULL : ops;
>> +   return ops;
>>  }
>>
>>  static int __init of_iommu_init(void)
>> @@ -247,7 +247,7 @@ static int __init of_iommu_init(void)
>> for_each_matching_node_and_match(np, matches, ) {
>> const of_iommu_init_fn init_fn = match->data;
>>
>> -   if (init_fn(np))
>> +   if (init_fn && init_fn(np))
>> pr_err("Failed to initialise IOMMU %s\n",
>> 

Re: [PATCH V8 07/11] iommu: of: Handle IOMMU lookup failure with deferred probing or error

[Geert Uytterhoeven]

Hi Sricharan,

On Fri, Feb 3, 2017 at 4:48 PM, Sricharan R  wrote:
> From: Laurent Pinchart 
>
> 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 
> Signed-off-by: Laurent Pichart 
> Signed-off-by: Sricharan R 

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.

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(-)
>
> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> index 449b948..82bd45c 100644
> --- a/drivers/base/dma-mapping.c
> +++ b/drivers/base/dma-mapping.c
> @@ -353,6 +353,7 @@ int dma_configure(struct device *dev)
>  {
> struct device *bridge = NULL, *dma_dev = dev;
> enum dev_dma_attr attr;
> +   int ret = 0;
>
> if (dev_is_pci(dev)) {
> bridge = pci_get_host_bridge_device(to_pci_dev(dev));
> @@ -363,7 +364,7 @@ int dma_configure(struct device *dev)
> }
>
> if (dma_dev->of_node) {
> -   of_dma_configure(dev, dma_dev->of_node);
> +   ret = of_dma_configure(dev, dma_dev->of_node);
> } else if (has_acpi_companion(dma_dev)) {
> attr = 
> acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
> if (attr != DEV_DMA_NOT_SUPPORTED)
> @@ -373,7 +374,7 @@ int dma_configure(struct device *dev)
> if (bridge)
> pci_put_host_bridge_device(bridge);
>
> -   return 0;
> +   return ret;
>  }
>
>  void dma_deconfigure(struct device *dev)
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index 1f92d98..2d04663 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -236,7 +236,7 @@ const struct iommu_ops *of_iommu_configure(struct device 
> *dev,
> ops = ERR_PTR(err);
> }
>
> -   return IS_ERR(ops) ? NULL : ops;
> +   return ops;
>  }
>
>  static int __init of_iommu_init(void)
> @@ -247,7 +247,7 @@ static int __init of_iommu_init(void)
> for_each_matching_node_and_match(np, matches, ) {
> const of_iommu_init_fn init_fn = match->data;
>
> -   if (init_fn(np))
> +   if (init_fn && init_fn(np))
> pr_err("Failed to initialise IOMMU %s\n",
> of_node_full_name(np));
> }
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index c17c19d..ba51ca6 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -82,7 +82,7 @@ int of_device_add(struct platform_device *ofdev)
>   * can use a platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE events
>   * to fix up DMA configuration.
>   */
> -void of_dma_configure(struct device *dev, struct device_node *np)
> +int of_dma_configure(struct device *dev, struct device_node *np)
>  {
> u64 dma_addr, paddr, size;
> int ret;
> @@ -129,10 +129,15 @@ void of_dma_configure(struct device *dev, struct 
> device_node *np)
> coherent ? " " : " not ");
>
> iommu = of_iommu_configure(dev, np);
> +   if (IS_ERR(iommu))
> +   return PTR_ERR(iommu);
> +
> dev_dbg(dev, "device is%sbehind an iommu\n",
> iommu ?