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

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 <sricharan@codeaurora.org> wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Sricharan R <sricharan@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.

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, &match) {
>                 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 ? " " : " not ");
>
>         arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_dma_configure);
>
> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
> index 3cb2288..9499861 100644
> --- a/include/linux/of_device.h
> +++ b/include/linux/of_device.h
> @@ -56,7 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>         return of_node_get(cpu_dev->of_node);
>  }
>
> -void of_dma_configure(struct device *dev, struct device_node *np);
> +int of_dma_configure(struct device *dev, struct device_node *np);
>  void of_dma_deconfigure(struct device *dev);
>  #else /* CONFIG_OF */
>
> @@ -105,8 +105,11 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>  {
>         return NULL;
>  }
> -static inline void of_dma_configure(struct device *dev, struct device_node *np)
> -{}
> +
> +static inline int of_dma_configure(struct device *dev, struct device_node *np)
> +{
> +       return 0;
> +}
>  static inline void of_dma_deconfigure(struct device *dev)
>  {}
>  #endif /* CONFIG_OF */

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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 <sricharan@codeaurora.org> wrote:
>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>> Signed-off-by: Sricharan R <sricharan@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.

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, &match) {
>>                 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 ? " " : " not ");
>>
>>         arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
>> +
>> +       return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(of_dma_configure);
>>
>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>> index 3cb2288..9499861 100644
>> --- a/include/linux/of_device.h
>> +++ b/include/linux/of_device.h
>> @@ -56,7 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>>         return of_node_get(cpu_dev->of_node);
>>  }
>>
>> -void of_dma_configure(struct device *dev, struct device_node *np);
>> +int of_dma_configure(struct device *dev, struct device_node *np);
>>  void of_dma_deconfigure(struct device *dev);
>>  #else /* CONFIG_OF */
>>
>> @@ -105,8 +105,11 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>>  {
>>         return NULL;
>>  }
>> -static inline void of_dma_configure(struct device *dev, struct device_node *np)
>> -{}
>> +
>> +static inline int of_dma_configure(struct device *dev, struct device_node *np)
>> +{
>> +       return 0;
>> +}
>>  static inline void of_dma_deconfigure(struct device *dev)
>>  {}
>>  #endif /* CONFIG_OF */
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
> 

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 <sricharan@codeaurora.org> wrote:
>>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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(-)
>>>
>>> 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, &match) {
>>>                 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 ? " " : " not ");
>>>
>>>         arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
>>> +
>>> +       return 0;
>>>  }
>>>  EXPORT_SYMBOL_GPL(of_dma_configure);
>>>
>>> diff --git a/include/linux/of_device.h b/include/linux/of_device.h
>>> index 3cb2288..9499861 100644
>>> --- a/include/linux/of_device.h
>>> +++ b/include/linux/of_device.h
>>> @@ -56,7 +56,7 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>>>         return of_node_get(cpu_dev->of_node);
>>>  }
>>>
>>> -void of_dma_configure(struct device *dev, struct device_node *np);
>>> +int of_dma_configure(struct device *dev, struct device_node *np);
>>>  void of_dma_deconfigure(struct device *dev);
>>>  #else /* CONFIG_OF */
>>>
>>> @@ -105,8 +105,11 @@ static inline struct device_node *of_cpu_device_node_get(int cpu)
>>>  {
>>>         return NULL;
>>>  }
>>> -static inline void of_dma_configure(struct device *dev, struct device_node *np)
>>> -{}
>>> +
>>> +static inline int of_dma_configure(struct device *dev, struct device_node *np)
>>> +{
>>> +       return 0;
>>> +}
>>>  static inline void of_dma_deconfigure(struct device *dev)
>>>  {}
>>>  #endif /* CONFIG_OF */
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
>> --
>> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>>
>> In personal conversations with technical people, I call myself a hacker. But
>> when I'm talking to journalists I just say "programmer" or something like that.
>>                                 -- Linus Torvalds
>>
> 

-- 
"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,

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 <sricharan@codeaurora.org> wrote:
>>>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>>>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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

[Geert Uytterhoeven]

Hi Sricharan, Robin,

On Wed, May 3, 2017 at 12:24 PM, Sricharan R <sricharan@codeaurora.org> 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 <sricharan@codeaurora.org> wrote:
>>>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>>>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>>>> Signed-off-by: Sricharan R <sricharan@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 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 <sricharan@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 <sricharan@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;

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, &iommu_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!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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 <sricharan@codeaurora.org> wrote:
> >>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
> >>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
> >>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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.deacon@arm.com>
Acked-by: Will Deacon <will.deacon@arm.com>

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

[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 <laurent.pinchart+renesas@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.szyprowski@samsung.com>
> >>> Signed-off-by: Laurent Pichart
> >>> <laurent.pinchart+renesas@ideasonboard.com>
> >>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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...

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

[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 <laurent.pinchart+renesas@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.szyprowski@samsung.com>
> >>>> Signed-off-by: Laurent Pichart
> >>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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.

I might be missing something obvious, but I don't see how this can work.

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

[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 <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>> >>>> Signed-off-by: Laurent Pichart
>> >>>> <laurent.pinchart+renesas@ideasonboard.com>
>> >>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I 
missed
to send in the original series, but now have added to Russell's patch 
tracking
system.

[1] https://patchwork.kernel.org/patch/9434105/

Regards,
  Sricharan

> 
> I might be missing something obvious, but I don't see how this can 
> work.
> 
>> >>> 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.

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 <sricharan@codeaurora.org> wrote:
>> >>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>> >>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>> >>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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.deacon@arm.com>
> Acked-by: Will Deacon <will.deacon@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

[Laurent Pinchart]

Hi Sricharan,

On Tuesday 16 May 2017 07:53:57 sricharan@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+renesas@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.szyprowski@samsung.com>
> >>>>>> Signed-off-by: Laurent Pichart
> >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I
> missed to send in the original series, but now have added to Russell's patch
> tracking system.

Thank you. I fear that won't be enough though.

> [1] https://patchwork.kernel.org/patch/9434105/

Quoting the patch:

> 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 <sricharan@codeaurora.org>
> Reviewed-by: Robin Murphy <robin.murphy@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(). One option would be 
to add a field to struct dev_archdata to store that information. To avoid 
growing the structure, which is embedded in every struct device, you could 
possibly turn the dma_coherent bool into a bitfield.

@@ -19,7 +19,8 @@ struct dev_archdata {
 #ifdef CONFIG_XEN
 	const struct dma_map_ops *dev_dma_ops;
 #endif
-	bool dma_coherent;
+	bool dma_coherent:1;
+	bool dma_ops_setup:1;
 };
 
 struct omap_device;

I haven't checked, however, whether the dma_coherent field would need to be 
accessed atomically, so this might be a bad idea.

Last but not least, a fix must be merged in v4.12, and the sooner the better.

> > I might be missing something obvious, but I don't see how this can
> > work.
> > 
> >>>>> 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

[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 sricharan@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+renesas@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.szyprowski@samsung.com>
> > >>>>>> Signed-off-by: Laurent Pichart
> > >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> > >>>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I
> > missed to send in the original series, but now have added to Russell's patch
> > tracking system.
> 
> Thank you. I fear that won't be enough though.
> 
> > [1] https://patchwork.kernel.org/patch/9434105/
> 
> Quoting the patch:
> 
> > 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 <sricharan@codeaurora.org>
> > Reviewed-by: Robin Murphy <robin.murphy@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(). One option would be 
> to add a field to struct dev_archdata to store that information. To avoid 
> growing the structure, which is embedded in every struct device, you could 
> possibly turn the dma_coherent bool into a bitfield.
> 
> @@ -19,7 +19,8 @@ struct dev_archdata {
>  #ifdef CONFIG_XEN
>  	const struct dma_map_ops *dev_dma_ops;
>  #endif
> -	bool dma_coherent;
> +	bool dma_coherent:1;
> +	bool dma_ops_setup:1;
>  };
>  
>  struct omap_device;
> 
> I haven't checked, however, whether the dma_coherent field would need to be 
> accessed atomically, so this might be a bad idea.

A bool bit field? :-)

I think I'd just use bool for both. I wouldn't expect dma_coherent change
once it has been set before device driver probe though.

If you like a bit field, then I'd propose making it unsigned int.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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 sricharan@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+renesas@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.szyprowski@samsung.com>
>> >>>>>> Signed-off-by: Laurent Pichart
>> >>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>> >>>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I
>> missed to send in the original series, but now have added to Russell's 
>> patch
>> tracking system.
> 
> Thank you. I fear that won't be enough though.
> 
>> [1] https://patchwork.kernel.org/patch/9434105/
> 
> Quoting the patch:
> 
>> 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 <sricharan@codeaurora.org>
>> Reviewed-by: Robin Murphy <robin.murphy@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(). One option 
> would be
> to add a field to struct dev_archdata to store that information. To 
> avoid
> growing the structure, which is embedded in every struct device, you 
> could
> possibly turn the dma_coherent bool into a bitfield.
> 
> @@ -19,7 +19,8 @@ struct dev_archdata {
>  #ifdef CONFIG_XEN
>  	const struct dma_map_ops *dev_dma_ops;
>  #endif
> -	bool dma_coherent;
> +	bool dma_coherent:1;
> +	bool dma_ops_setup:1;
>  };
> 
>  struct omap_device;
> 
> I haven't checked, however, whether the dma_coherent field would need 
> to be
> accessed atomically, so this might be a bad idea.
> 
> Last but not least, a fix must be merged in v4.12, and the sooner the 
> better.
> 

ho, yet another combination. This seems to be a problem with 
exynos_iommu,
ipmmu-vmsa, mtk_iommu_v1 which calls the arm_iommu_attach_device with 
its
own custom mapping. They are calling arm_iommu_attach_device from the
add_device callback and that is not always replayed when the reprobe 
happens
and these archs are storing the old mapping data in private structures 
which
might not be cleared in the teardown path. I will post the fix that you 
have
suggested.

Regards,
  Sricharan


>> > I might be missing something obvious, but I don't see how this can
>> > work.
>> >
>> >>>>> 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.

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 sricharan@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+renesas@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.szyprowski@samsung.com>
>>>>>>>> Signed-off-by: Laurent Pichart
>>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I
>> missed to send in the original series, but now have added to Russell's patch
>> tracking system.
> 
> Thank you. I fear that won't be enough though.
> 
>> [1] https://patchwork.kernel.org/patch/9434105/
> 
> Quoting the patch:
> 
>> 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 <sricharan@codeaurora.org>
>> Reviewed-by: Robin Murphy <robin.murphy@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, 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?

Robin.

> One option would be 
> to add a field to struct dev_archdata to store that information. To avoid 
> growing the structure, which is embedded in every struct device, you could 
> possibly turn the dma_coherent bool into a bitfield.
> 
> @@ -19,7 +19,8 @@ struct dev_archdata {
>  #ifdef CONFIG_XEN
>  	const struct dma_map_ops *dev_dma_ops;
>  #endif
> -	bool dma_coherent;
> +	bool dma_coherent:1;
> +	bool dma_ops_setup:1;
>  };
>  
>  struct omap_device;
> 
> I haven't checked, however, whether the dma_coherent field would need to be 
> accessed atomically, so this might be a bad idea.
> 
> Last but not least, a fix must be merged in v4.12, and the sooner the better.
> 
>>> I might be missing something obvious, but I don't see how this can
>>> work.
>>>
>>>>>>> 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.
> 

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 sricharan@codeaurora.org wrote:
> On 2017-05-16 12:47, Laurent Pinchart wrote:
> > On Tuesday 16 May 2017 07:53:57 sricharan@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+renesas@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.szyprowski@samsung.com>
> >>>>>>>> Signed-off-by: Laurent Pichart
> >>>>>>>> <laurent.pinchart+renesas@ideasonboard.com>
> >>>>>>>> Signed-off-by: Sricharan R <sricharan@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 <sricharan@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 <sricharan@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 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 patch [1], which I
> >> missed to send in the original series, but now have added to Russell's
> >> patch tracking system.
> > 
> > Thank you. I fear that won't be enough though.
> > 
> >> [1] https://patchwork.kernel.org/patch/9434105/
> > 
> > Quoting the patch:
> >
> >> 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 <sricharan@codeaurora.org>
> >> Reviewed-by: Robin Murphy <robin.murphy@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(). One option would be to add a field to struct
> > dev_archdata to store that information. To avoid growing the structure,
> > which is embedded in every struct device, you could possibly turn the
> > dma_coherent bool into a bitfield.
> > 
> > @@ -19,7 +19,8 @@ struct dev_archdata {
> >  #ifdef CONFIG_XEN
> >  	const struct dma_map_ops *dev_dma_ops;
> >  #endif
> > -	bool dma_coherent;
> > +	bool dma_coherent:1;
> > +	bool dma_ops_setup:1;
> >  };
> >  
> >  struct omap_device;
> > 
> > I haven't checked, however, whether the dma_coherent field would need
> > to be accessed atomically, so this might be a bad idea.
> > 
> > Last but not least, a fix must be merged in v4.12, and the sooner the
> > better.
> 
> ho, yet another combination. This seems to be a problem with exynos_iommu,
> ipmmu-vmsa, mtk_iommu_v1 which calls the arm_iommu_attach_device with its
> own custom mapping. They are calling arm_iommu_attach_device from the
> add_device callback and that is not always replayed when the reprobe happens
> and these archs are storing the old mapping data in private structures which
> might not be cleared in the teardown path.

Yes, I know, it's messy :-/ There's a handful of non-IOMMU drivers calling 
arm_iommu_attach_device() directly too. All these should be fixed, but in the 
meantime, let's try not to break them.

> I will post the fix that you have suggested.

Thank you. You might want to use an unsigned int bitfield instead of a bool 
bitfield as Sakari suggested. It would be nice to check the code setting the 
dma_coherent field to make sure there will be no race with code setting the 
new dma_ops_setup field (which might not be the best name, feel free to rename 
it).

I have successfully test the patch, let me know if there's anything else I can 
do to help.

> >>> I might be missing something obvious, but I don't see how this can
> >>> work.
> >>> 
> >>>>>>> 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

[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 sricharan@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 <sricharan@codeaurora.org>
> >> Reviewed-by: Robin Murphy <robin.murphy@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+renesas@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.

-- 
Regards,

Laurent Pinchart

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 sricharan@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 <sricharan@codeaurora.org>
>> >> Reviewed-by: Robin Murphy <robin.murphy@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+renesas@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

[Laurent Pinchart]

Hi Sricharan,

On Tuesday 16 May 2017 19:59:01 sricharan@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 sricharan@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 <sricharan@codeaurora.org>
> >>>> Reviewed-by: Robin Murphy <robin.murphy@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+renesas@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.

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 mapping for every device. That's 
certainly fine when every device has its own IOMMU instance (or at least its 
own TLB in a shared IOMMU), but that's far from being true in all cases. 
Renesas R-Car Gen2 hardware, for instance, share IOMMUs and TLBs between 
multiple devices. That's why the ipmmu-vmsa driver creates the mapping 
manually with arm_iommu_create_mapping() and attaches it to the device with 
arm_iommu_attach_device(). We need to keep supporting this mechanism until the 
ARM32 dma-mapping API is brought in line with the ARM64 implementation that 
lets the IOMMU core manage the IOVA address space.

We're unfortunately far from that, so, for the time being, we need to keep 
supporting arm_iommu_attach_device(). How that can work with deferred probing, 
I will let you figure it out, but we need to at least fix the breakage for 
v4.12.

-- 
Regards,

Laurent Pinchart

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 sricharan@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 <sricharan@codeaurora.org>
>>>> Reviewed-by: Robin Murphy <robin.murphy@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+renesas@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.

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.

[PATCH] ARM: dma-mapping: Don't tear third-party mappings

[Laurent Pinchart]

arch_setup_dma_ops() is used in device probe code paths to create an
IOMMU mapping and attach it to the device. The function assumes that the
device is attached to a device-specific IOMMU instance (or at least a
device-specific TLB in a shared IOMMU instance) and thus creates a
separate mapping for every device.

On several systems (Renesas R-Car Gen2 being one of them), that
assumption is not true, and IOMMU mappings must be shared between
multiple devices. In those cases the IOMMU driver knows better than the
generic ARM dma-mapping layer and attaches mapping to devices manually
with arm_iommu_attach_device(), which sets the DMA ops for the device.

The arch_setup_dma_ops() function takes this into account and bails out
immediately if the device already has DMA ops assigned. However, the
corresponding arch_teardown_dma_ops() function, called from driver
unbind code paths (including probe deferral), will tear the mapping down
regardless of who created it. When the device is reprobed
arch_setup_dma_ops() will be called again but won't perform any
operation as the DMA ops will still be set.

We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
However, we can't do so unconditionally, as then a new mapping would be
created by arch_setup_dma_ops() when the device is reprobed, regardless
of whether the device needs to share a mapping or not. We must thus keep
track of whether arch_setup_dma_ops() created the mapping, and only in
that case tear it down in arch_teardown_dma_ops().

Keep track of that information in the dev_archdata structure. As the
structure is embedded in all instances of struct device let's not grow
it, but turn the existing dma_coherent bool field into a bitfield that
can be used for other purposes.

Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@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;
+	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);
 }
-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings

[Robin Murphy]

On 16/05/17 16:14, Laurent Pinchart wrote:
> arch_setup_dma_ops() is used in device probe code paths to create an
> IOMMU mapping and attach it to the device. The function assumes that the
> device is attached to a device-specific IOMMU instance (or at least a
> device-specific TLB in a shared IOMMU instance) and thus creates a
> separate mapping for every device.
> 
> On several systems (Renesas R-Car Gen2 being one of them), that
> assumption is not true, and IOMMU mappings must be shared between
> multiple devices. In those cases the IOMMU driver knows better than the
> generic ARM dma-mapping layer and attaches mapping to devices manually
> with arm_iommu_attach_device(), which sets the DMA ops for the device.
> 
> The arch_setup_dma_ops() function takes this into account and bails out
> immediately if the device already has DMA ops assigned. However, the
> corresponding arch_teardown_dma_ops() function, called from driver
> unbind code paths (including probe deferral), will tear the mapping down
> regardless of who created it. When the device is reprobed
> arch_setup_dma_ops() will be called again but won't perform any
> operation as the DMA ops will still be set.
> 
> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
> However, we can't do so unconditionally, as then a new mapping would be
> created by arch_setup_dma_ops() when the device is reprobed, regardless
> of whether the device needs to share a mapping or not. We must thus keep
> track of whether arch_setup_dma_ops() created the mapping, and only in
> that case tear it down in arch_teardown_dma_ops().
> 
> Keep track of that information in the dev_archdata structure. As the
> structure is embedded in all instances of struct device let's not grow
> it, but turn the existing dma_coherent bool field into a bitfield that
> can be used for other purposes.
> 
> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred probing or error")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@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.

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

With that (or firm reassurance that it's OK not to),

Reviewed-by: Robin Murphy <robin.murphy@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?

Robin.

>  }
> 

Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings

[Laurent Pinchart]

Hi Robin,

On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
> On 16/05/17 16:14, Laurent Pinchart wrote:
> > arch_setup_dma_ops() is used in device probe code paths to create an
> > IOMMU mapping and attach it to the device. The function assumes that the
> > device is attached to a device-specific IOMMU instance (or at least a
> > device-specific TLB in a shared IOMMU instance) and thus creates a
> > separate mapping for every device.
> > 
> > On several systems (Renesas R-Car Gen2 being one of them), that
> > assumption is not true, and IOMMU mappings must be shared between
> > multiple devices. In those cases the IOMMU driver knows better than the
> > generic ARM dma-mapping layer and attaches mapping to devices manually
> > with arm_iommu_attach_device(), which sets the DMA ops for the device.
> > 
> > The arch_setup_dma_ops() function takes this into account and bails out
> > immediately if the device already has DMA ops assigned. However, the
> > corresponding arch_teardown_dma_ops() function, called from driver
> > unbind code paths (including probe deferral), will tear the mapping down
> > regardless of who created it. When the device is reprobed
> > arch_setup_dma_ops() will be called again but won't perform any
> > operation as the DMA ops will still be set.
> > 
> > We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
> > However, we can't do so unconditionally, as then a new mapping would be
> > created by arch_setup_dma_ops() when the device is reprobed, regardless
> > of whether the device needs to share a mapping or not. We must thus keep
> > track of whether arch_setup_dma_ops() created the mapping, and only in
> > that case tear it down in arch_teardown_dma_ops().
> > 
> > Keep track of that information in the dev_archdata structure. As the
> > structure is embedded in all instances of struct device let's not grow
> > it, but turn the existing dma_coherent bool field into a bitfield that
> > can be used for other purposes.
> > 
> > Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
> > probing or error") Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@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.

> With that (or firm reassurance that it's OK not to),
> 
> Reviewed-by: Robin Murphy <robin.murphy@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

Tested-by: Laurent Pinchart <laurent.pinchart@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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] ARM: dma-mapping: Don't tear third-party mappings

[Sricharan R]

Hi Laurent/Robin,

On 5/16/2017 10:14 PM, Laurent Pinchart wrote:
> Hi Robin,
> 
> On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
>> On 16/05/17 16:14, Laurent Pinchart wrote:
>>> arch_setup_dma_ops() is used in device probe code paths to create an
>>> IOMMU mapping and attach it to the device. The function assumes that the
>>> device is attached to a device-specific IOMMU instance (or at least a
>>> device-specific TLB in a shared IOMMU instance) and thus creates a
>>> separate mapping for every device.
>>>
>>> On several systems (Renesas R-Car Gen2 being one of them), that
>>> assumption is not true, and IOMMU mappings must be shared between
>>> multiple devices. In those cases the IOMMU driver knows better than the
>>> generic ARM dma-mapping layer and attaches mapping to devices manually
>>> with arm_iommu_attach_device(), which sets the DMA ops for the device.
>>>
>>> The arch_setup_dma_ops() function takes this into account and bails out
>>> immediately if the device already has DMA ops assigned. However, the
>>> corresponding arch_teardown_dma_ops() function, called from driver
>>> unbind code paths (including probe deferral), will tear the mapping down
>>> regardless of who created it. When the device is reprobed
>>> arch_setup_dma_ops() will be called again but won't perform any
>>> operation as the DMA ops will still be set.
>>>
>>> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
>>> However, we can't do so unconditionally, as then a new mapping would be
>>> created by arch_setup_dma_ops() when the device is reprobed, regardless
>>> of whether the device needs to share a mapping or not. We must thus keep
>>> track of whether arch_setup_dma_ops() created the mapping, and only in
>>> that case tear it down in arch_teardown_dma_ops().
>>>
>>> Keep track of that information in the dev_archdata structure. As the
>>> structure is embedded in all instances of struct device let's not grow
>>> it, but turn the existing dma_coherent bool field into a bitfield that
>>> can be used for other purposes.
>>>
>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with deferred
>>> probing or error") Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+renesas@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.

>> With that (or firm reassurance that it's OK not to),
>>
>> Reviewed-by: Robin Murphy <robin.murphy@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.pinchart@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 <sricharan@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 <sricharan@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

[Magnus Damm]

Hi Geert, everyone,

On Fri, May 5, 2017 at 10:23 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Sricharan, Robin,
>
> On Wed, May 3, 2017 at 12:24 PM, Sricharan R <sricharan@codeaurora.org> 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 <sricharan@codeaurora.org> wrote:
>>>>> From: Laurent Pinchart <laurent.pinchart+renesas@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.szyprowski@samsung.com>
>>>>> Signed-off-by: Laurent Pichart <laurent.pinchart+renesas@ideasonboard.com>
>>>>> Signed-off-by: Sricharan R <sricharan@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 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 <sricharan@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 <sricharan@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;
>
> 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, &iommu_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 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
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/

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

[Sricharan R]

Hi Magnus,

<snip..>

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

[sricharan]

On 2017-05-17 10:45, Sricharan R wrote:
> Hi Laurent/Robin,
> 
> On 5/16/2017 10:14 PM, Laurent Pinchart wrote:
>> Hi Robin,
>> 
>> On Tuesday 16 May 2017 16:47:36 Robin Murphy wrote:
>>> On 16/05/17 16:14, Laurent Pinchart wrote:
>>>> arch_setup_dma_ops() is used in device probe code paths to create an
>>>> IOMMU mapping and attach it to the device. The function assumes that 
>>>> the
>>>> device is attached to a device-specific IOMMU instance (or at least 
>>>> a
>>>> device-specific TLB in a shared IOMMU instance) and thus creates a
>>>> separate mapping for every device.
>>>> 
>>>> On several systems (Renesas R-Car Gen2 being one of them), that
>>>> assumption is not true, and IOMMU mappings must be shared between
>>>> multiple devices. In those cases the IOMMU driver knows better than 
>>>> the
>>>> generic ARM dma-mapping layer and attaches mapping to devices 
>>>> manually
>>>> with arm_iommu_attach_device(), which sets the DMA ops for the 
>>>> device.
>>>> 
>>>> The arch_setup_dma_ops() function takes this into account and bails 
>>>> out
>>>> immediately if the device already has DMA ops assigned. However, the
>>>> corresponding arch_teardown_dma_ops() function, called from driver
>>>> unbind code paths (including probe deferral), will tear the mapping 
>>>> down
>>>> regardless of who created it. When the device is reprobed
>>>> arch_setup_dma_ops() will be called again but won't perform any
>>>> operation as the DMA ops will still be set.
>>>> 
>>>> We need to reset the DMA ops in arch_teardown_dma_ops() to fix this.
>>>> However, we can't do so unconditionally, as then a new mapping would 
>>>> be
>>>> created by arch_setup_dma_ops() when the device is reprobed, 
>>>> regardless
>>>> of whether the device needs to share a mapping or not. We must thus 
>>>> keep
>>>> track of whether arch_setup_dma_ops() created the mapping, and only 
>>>> in
>>>> that case tear it down in arch_teardown_dma_ops().
>>>> 
>>>> Keep track of that information in the dev_archdata structure. As the
>>>> structure is embedded in all instances of struct device let's not 
>>>> grow
>>>> it, but turn the existing dma_coherent bool field into a bitfield 
>>>> that
>>>> can be used for other purposes.
>>>> 
>>>> Fixes: 7b07cbefb68d ("iommu: of: Handle IOMMU lookup failure with 
>>>> deferred
>>>> probing or error") Signed-off-by: Laurent Pinchart
>>>> <laurent.pinchart+renesas@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 the teardown on the driver
release path.

[1] https://lkml.org/lkml/2017/5/17/344

Regards,
  Sricharan


> 
>>> With that (or firm reassurance that it's OK not to),
>>> 
>>> Reviewed-by: Robin Murphy <robin.murphy@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.pinchart@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 <sricharan@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 <sricharan@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
> 
> 
> 
>>