[7/8] iommu: of: Handle IOMMU lookup failure with deferred probing or error

Commit Message

Sricharan Ramabadhran Aug. 8, 2016, 10:49 p.m. UTC

From: Laurent Pinchart <[e-mail]>

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.

Signed-off-by: Laurent Pinchart <[e-mail]>
Signed-off-by: Sricharan R <[e-mail]>
---
 drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
 drivers/of/device.c      |  2 ++
 2 files changed, 19 insertions(+), 4 deletions(-)

Comments

Tomasz Figa Aug. 12, 2016, 7:46 a.m. UTC | #1

Hi,

On Tue, Aug 9, 2016 at 7:49 AM, Sricharan R <[e-mail]> wrote:
> +
> +               if (ops->add_device)
> +                       ops = ops->add_device(dev) ? ops : NULL;

Patch description fails to mention anything about this change. Also it
looks slightly incorrect to lose the error condition here. I think we
should at least print some error message here and tell the user that
we are falling back to non-IOMMU setup.

>
>                 of_node_put(np);
>                 idx++;
> @@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>
>  err_put_node:
>         of_node_put(np);
> -       return NULL;
> +       return ops;
>  }
>
>  void __init of_iommu_init(void)
> @@ -211,7 +224,7 @@ void __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))

When is it possible to have NULL init_fn?

>                         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 e1fad50..92e02dc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np)
>                 coherent ? " " : " not ");
>
>         iommu = of_iommu_configure(dev, np);
> +       if (IS_ERR(iommu))
> +               return PTR_ERR(iommu);

nit: Would be good to add a blank line here for improved readability.

>         dev_dbg(dev, "device is%sbehind an iommu\n",
>                 iommu ? " " : " not ");
>

Best regards,
Tomasz

Sricharan Ramabadhran Aug. 12, 2016, 3:40 p.m. UTC | #2

Hi Tomaz,

>> +               if (ops->add_device)
>> +                       ops = ops->add_device(dev) ? ops : NULL;
>
>Patch description fails to mention anything about this change. Also it
>looks slightly incorrect to lose the error condition here. I think we
>should at least print some error message here and tell the user that
>we are falling back to non-IOMMU setup.
    Ok, will have to improve the patch description to add this and will
    fix the error value as well. Will also add the remove_device during
     deconfigure as well.

>
>>
>>                 of_node_put(np);
>>                 idx++;
>> @@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>
>>  err_put_node:
>>         of_node_put(np);
>> -       return NULL;
>> +       return ops;
>>  }
>>
>>  void __init of_iommu_init(void)
>> @@ -211,7 +224,7 @@ void __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))
>
>When is it possible to have NULL init_fn?
   ya wrong, should not be needed.
 
>
>>                         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 e1fad50..92e02dc 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np)
>>                 coherent ? " " : " not ");
>>
>>         iommu = of_iommu_configure(dev, np);
>> +       if (IS_ERR(iommu))
>> +               return PTR_ERR(iommu);
>
>nit: Would be good to add a blank line here for improved readability.
   ok, will add.

Regards,
 Sricharan

Marek Szyprowski Sept. 2, 2016, 8:09 a.m. UTC | #3

Hi Sricharan,


On 2016-08-12 17:40, Sricharan wrote:
> Hi Tomaz,
>
>>> +               if (ops->add_device)
>>> +                       ops = ops->add_device(dev) ? ops : NULL;
>> Patch description fails to mention anything about this change. Also it
>> looks slightly incorrect to lose the error condition here. I think we
>> should at least print some error message here and tell the user that
>> we are falling back to non-IOMMU setup.
>      Ok, will have to improve the patch description to add this and will
>      fix the error value as well. Will also add the remove_device during
>       deconfigure as well.
>
>>
>>>                  of_node_put(np);
>>>                  idx++;
>>> @@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>>
>>>   err_put_node:
>>>          of_node_put(np);
>>> -       return NULL;
>>> +       return ops;
>>>   }
>>>
>>>   void __init of_iommu_init(void)
>>> @@ -211,7 +224,7 @@ void __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))
>> When is it possible to have NULL init_fn?
>     ya wrong, should not be needed.

init_fn can be NULL if you convert IOMMU driver to use platform device
infrastructure based initialization and you don't need to do anything
before the driver gets probed, so please keep this check.

I used this approach here:
https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/a30735973573128b14bb4a25cf4debaa0979a655
(this commit is a part of v4.8-clocks-pm branch)

IOMMU_OF_DECLARE() with NULL init_fn is needed to notify IOMMU core that
this IOMMU driver is available in the system and core has to defer
probing of drivers before the IOMMU driver gets initialized from its
controller's platform device.

>
>>
>>>                          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 e1fad50..92e02dc 100644
>>> --- a/drivers/of/device.c
>>> +++ b/drivers/of/device.c
>>> @@ -149,6 +149,8 @@ int of_dma_configure_ops(struct device *dev, struct device_node *np)
>>>                  coherent ? " " : " not ");
>>>
>>>          iommu = of_iommu_configure(dev, np);
>>> +       if (IS_ERR(iommu))
>>> +               return PTR_ERR(iommu);
>> nit: Would be good to add a blank line here for improved readability.
>     ok, will add.

Best regards

Marek Szyprowski Sept. 2, 2016, 12:52 p.m. UTC | #4

Hi Sricharan,

On 2016-08-09 00:49, Sricharan R wrote:
 > From: Laurent Pinchart <[e-mail]>
 >
 > 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.
 >
 > Signed-off-by: Laurent Pinchart 
<[e-mail]>

It is a common practice to briefly describe here what has been changed
since the original patch if you have modified it (see commit
855ed04a3758b205e84b269f92d26ab36ed8e2f7 for the example).

 > Signed-off-by: Sricharan R <[e-mail]>
 > ---
 >  drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
 >  drivers/of/device.c      |  2 ++
 >  2 files changed, 19 insertions(+), 4 deletions(-)
 >
 > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
 > index 4c4219d..3994cf5 100644
 > --- a/drivers/iommu/of_iommu.c
 > +++ b/drivers/iommu/of_iommu.c
 > @@ -149,7 +149,7 @@ const struct iommu_ops *of_iommu_configure(struct 
device *dev,
 >  {
 >      struct of_phandle_args iommu_spec;
 >      struct device_node *np = NULL;
 > -    struct iommu_ops *ops = NULL;
 > +    const struct iommu_ops *ops = NULL;
 >      int idx = 0;
 >
 >      if (dev_is_pci(dev)) {
 > @@ -189,8 +189,21 @@ const struct iommu_ops 
*of_iommu_configure(struct device *dev,
 >          np = iommu_spec.np;
 >          ops = of_iommu_get_ops(np);
 >
 > -        if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
 > +        if (!ops) {
 > +            const struct of_device_id *oid;
 > +
 > +            oid = of_match_node(&__iommu_of_table, np);
 > +            ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
 >              goto err_put_node;
 > +        }
 > +
 > +        if (!ops->of_xlate || ops->of_xlate(dev, &iommu_spec)) {
 > +            ops = NULL;
 > +            goto err_put_node;
 > +        }
 > +
 > +        if (ops->add_device)
 > +            ops = ops->add_device(dev) ? ops : NULL;

ops->add_device() returns ZERO on success or error code on failure, so 
the above
line should be changed to:
             ops = (ops->add_device(dev) == 0) ? ops : NULL;


 >          of_node_put(np);
 >          idx++;
 > @@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct 
device *dev,
 >
 >  err_put_node:
 >      of_node_put(np);
 > -    return NULL;
 > +    return ops;
 >  }
 >
 >  void __init of_iommu_init(void)
 > @@ -211,7 +224,7 @@ void __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 e1fad50..92e02dc 100644
 > --- a/drivers/of/device.c
 > +++ b/drivers/of/device.c
 > @@ -149,6 +149,8 @@ int of_dma_configure_ops(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 ");
 >

Best regards

Sricharan Ramabadhran Sept. 7, 2016, 6:16 a.m. UTC | #5

Hi Marek,

>Hi Sricharan,
>
>
>On 2016-08-12 17:40, Sricharan wrote:
>> Hi Tomaz,
>>
>>>> +               if (ops->add_device)
>>>> +                       ops = ops->add_device(dev) ? ops : NULL;
>>> Patch description fails to mention anything about this change. Also it
>>> looks slightly incorrect to lose the error condition here. I think we
>>> should at least print some error message here and tell the user that
>>> we are falling back to non-IOMMU setup.
>>      Ok, will have to improve the patch description to add this and will
>>      fix the error value as well. Will also add the remove_device during
>>       deconfigure as well.
>>
>>>
>>>>                  of_node_put(np);
>>>>                  idx++;
>>>> @@ -200,7 +213,7 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>>>
>>>>   err_put_node:
>>>>          of_node_put(np);
>>>> -       return NULL;
>>>> +       return ops;
>>>>   }
>>>>
>>>>   void __init of_iommu_init(void)
>>>> @@ -211,7 +224,7 @@ void __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))
>>> When is it possible to have NULL init_fn?
>>     ya wrong, should not be needed.
>
>init_fn can be NULL if you convert IOMMU driver to use platform device
>infrastructure based initialization and you don't need to do anything
>before the driver gets probed, so please keep this check.
>
>I used this approach here:
>https://git.linaro.org/people/marek.szyprowski/linux-srpol.git/commitdiff/a30735973573128b14bb4a25cf4debaa0979a655
>(this commit is a part of v4.8-clocks-pm branch)
>
>IOMMU_OF_DECLARE() with NULL init_fn is needed to notify IOMMU core that
>this IOMMU driver is available in the system and core has to defer
>probing of drivers before the IOMMU driver gets initialized from its
>controller's platform device.
>
   ok, thanks, understand. I was not thinking about the non-dt case.
I will keep this check then.

Regards,
 Sricharan

Sricharan Ramabadhran Sept. 7, 2016, 6:29 a.m. UTC | #6

Hi Marek,

>Hi Sricharan,
>
>On 2016-08-09 00:49, Sricharan R wrote:
> > From: Laurent Pinchart <[e-mail]>
> >
> > 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.
> >
> > Signed-off-by: Laurent Pinchart
><[e-mail]>
>
>It is a common practice to briefly describe here what has been changed
>since the original patch if you have modified it (see commit
>855ed04a3758b205e84b269f92d26ab36ed8e2f7 for the example).
>
 Sorry i missed updating it. i will update the log.

> > Signed-off-by: Sricharan R <[e-mail]>
> > ---
> >  drivers/iommu/of_iommu.c | 21 +++++++++++++++++----
> >  drivers/of/device.c      |  2 ++
> >  2 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index 4c4219d..3994cf5 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -149,7 +149,7 @@ const struct iommu_ops *of_iommu_configure(struct
>device *dev,
> >  {
> >      struct of_phandle_args iommu_spec;
> >      struct device_node *np = NULL;
> > -    struct iommu_ops *ops = NULL;
> > +    const struct iommu_ops *ops = NULL;
> >      int idx = 0;
> >
> >      if (dev_is_pci(dev)) {
> > @@ -189,8 +189,21 @@ const struct iommu_ops
>*of_iommu_configure(struct device *dev,
> >          np = iommu_spec.np;
> >          ops = of_iommu_get_ops(np);
> >
> > -        if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
> > +        if (!ops) {
> > +            const struct of_device_id *oid;
> > +
> > +            oid = of_match_node(&__iommu_of_table, np);
> > +            ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
> >              goto err_put_node;
> > +        }
> > +
> > +        if (!ops->of_xlate || ops->of_xlate(dev, &iommu_spec)) {
> > +            ops = NULL;
> > +            goto err_put_node;
> > +        }
> > +
> > +        if (ops->add_device)
> > +            ops = ops->add_device(dev) ? ops : NULL;
>
>ops->add_device() returns ZERO on success or error code on failure, so
>the above
>line should be changed to:
>             ops = (ops->add_device(dev) == 0) ? ops : NULL;

 Yes, i fixed it while testing, but somehow have missed it while sending it.
  will fix it.

Regards,
 Sricharan

Patch

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 4c4219d..3994cf5 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -149,7 +149,7 @@  const struct iommu_ops *of_iommu_configure(struct device *dev,
 {
 	struct of_phandle_args iommu_spec;
 	struct device_node *np = NULL;
-	struct iommu_ops *ops = NULL;
+	const struct iommu_ops *ops = NULL;
 	int idx = 0;
 
 	if (dev_is_pci(dev)) {
@@ -189,8 +189,21 @@  const struct iommu_ops *of_iommu_configure(struct device *dev,
 		np = iommu_spec.np;
 		ops = of_iommu_get_ops(np);
 
-		if (!ops || !ops->of_xlate || ops->of_xlate(dev, &iommu_spec))
+		if (!ops) {
+			const struct of_device_id *oid;
+
+			oid = of_match_node(&__iommu_of_table, np);
+			ops = oid ? ERR_PTR(-EPROBE_DEFER) : NULL;
 			goto err_put_node;
+		}
+
+		if (!ops->of_xlate || ops->of_xlate(dev, &iommu_spec)) {
+			ops = NULL;
+			goto err_put_node;
+		}
+
+		if (ops->add_device)
+			ops = ops->add_device(dev) ? ops : NULL;
 
 		of_node_put(np);
 		idx++;
@@ -200,7 +213,7 @@  const struct iommu_ops *of_iommu_configure(struct device *dev,
 
 err_put_node:
 	of_node_put(np);
-	return NULL;
+	return ops;
 }
 
 void __init of_iommu_init(void)
@@ -211,7 +224,7 @@  void __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 e1fad50..92e02dc 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -149,6 +149,8 @@  int of_dma_configure_ops(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 ");