[PATCH 0/4 v2] Consolidate iommu_ops->add/remove_device() calls

[PATCH 0/4 v2] Consolidate iommu_ops->add/remove_device() calls

[Joerg Roedel]

Hi,

here is the second version of the patch-set to wrap the
invocation of iommu_ops->add/remove_device() into functions.
The functions will do more setup stuff later when the the
iommu-related pointers in 'struct device' are consolidated.

Since version one this patch-set was rebased to v4.20-rc6
and I removed the pointer checks for the function pointers,
as suggested by Robin. I checked all 16 drivers and all of
them implement the add/remove_device call-backs.

Please review, if there are no objections I plan to queue
these patches in the IOMMU tree.

Thanks,

	Joerg

Joerg Roedel (4):
  iommu/sysfs: Rename iommu_release_device()
  iommu: Consolitate ->add/remove_device() calls
  iommu/of: Don't call iommu_ops->add_device directly
  ACPI/IORT: Don't call iommu_ops->add_device directly

 drivers/acpi/arm64/iort.c   |  4 +--
 drivers/iommu/iommu-sysfs.c | 12 ++++-----
 drivers/iommu/iommu.c       | 51 ++++++++++++++++++-------------------
 drivers/iommu/of_iommu.c    |  6 ++---
 include/linux/iommu.h       |  3 +++
 5 files changed, 39 insertions(+), 37 deletions(-)

-- 
2.17.1

[PATCH 1/4] iommu/sysfs: Rename iommu_release_device()

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Remove the iommu_ prefix from the function and a few other
static data structures so that the iommu_release_device name
can be re-used in iommu core code.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu-sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index 36d1a7ce7fc4..71c2249d3260 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -22,25 +22,25 @@ static struct attribute *devices_attr[] = {
 	NULL,
 };
 
-static const struct attribute_group iommu_devices_attr_group = {
+static const struct attribute_group devices_attr_group = {
 	.name = "devices",
 	.attrs = devices_attr,
 };
 
-static const struct attribute_group *iommu_dev_groups[] = {
-	&iommu_devices_attr_group,
+static const struct attribute_group *dev_groups[] = {
+	&devices_attr_group,
 	NULL,
 };
 
-static void iommu_release_device(struct device *dev)
+static void release_device(struct device *dev)
 {
 	kfree(dev);
 }
 
 static struct class iommu_class = {
 	.name = "iommu",
-	.dev_release = iommu_release_device,
-	.dev_groups = iommu_dev_groups,
+	.dev_release = release_device,
+	.dev_groups = dev_groups,
 };
 
 static int __init iommu_dev_init(void)
-- 
2.17.1

[PATCH 2/4] iommu: Consolitate ->add/remove_device() calls

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Put them into separate functions and call those where the
plain ops have been called before.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/iommu.c | 51 +++++++++++++++++++++----------------------
 include/linux/iommu.h |  3 +++
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index edbdf5d6962c..1e8f4e0c9198 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -110,6 +110,23 @@ void iommu_device_unregister(struct iommu_device *iommu)
 	spin_unlock(&iommu_device_lock);
 }
 
+int iommu_probe_device(struct device *dev)
+{
+	const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+	WARN_ON(dev->iommu_group);
+
+	return ops->add_device(dev);
+}
+
+void iommu_release_device(struct device *dev)
+{
+	const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+	if (dev->iommu_group)
+		ops->remove_device(dev);
+}
+
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
 						 unsigned type);
 static int __iommu_attach_device(struct iommu_domain *domain,
@@ -1117,16 +1134,7 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
 
 static int add_iommu_group(struct device *dev, void *data)
 {
-	struct iommu_callback_data *cb = data;
-	const struct iommu_ops *ops = cb->ops;
-	int ret;
-
-	if (!ops->add_device)
-		return 0;
-
-	WARN_ON(dev->iommu_group);
-
-	ret = ops->add_device(dev);
+	int ret = iommu_probe_device(dev);
 
 	/*
 	 * We ignore -ENODEV errors for now, as they just mean that the
@@ -1141,11 +1149,7 @@ static int add_iommu_group(struct device *dev, void *data)
 
 static int remove_iommu_group(struct device *dev, void *data)
 {
-	struct iommu_callback_data *cb = data;
-	const struct iommu_ops *ops = cb->ops;
-
-	if (ops->remove_device && dev->iommu_group)
-		ops->remove_device(dev);
+	iommu_release_device(dev);
 
 	return 0;
 }
@@ -1153,27 +1157,22 @@ static int remove_iommu_group(struct device *dev, void *data)
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data)
 {
+	unsigned long group_action = 0;
 	struct device *dev = data;
-	const struct iommu_ops *ops = dev->bus->iommu_ops;
 	struct iommu_group *group;
-	unsigned long group_action = 0;
 
 	/*
 	 * ADD/DEL call into iommu driver ops if provided, which may
 	 * result in ADD/DEL notifiers to group->notifier
 	 */
 	if (action == BUS_NOTIFY_ADD_DEVICE) {
-		if (ops->add_device) {
-			int ret;
+		int ret;
 
-			ret = ops->add_device(dev);
-			return (ret) ? NOTIFY_DONE : NOTIFY_OK;
-		}
+		ret = iommu_probe_device(dev);
+		return (ret) ? NOTIFY_DONE : NOTIFY_OK;
 	} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
-		if (ops->remove_device && dev->iommu_group) {
-			ops->remove_device(dev);
-			return 0;
-		}
+		iommu_release_device(dev);
+		return NOTIFY_OK;
 	}
 
 	/*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a1d28f42cb77..cb36f32cd3c2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -398,6 +398,9 @@ void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
 const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
 
+int iommu_probe_device(struct device *dev);
+void iommu_release_device(struct device *dev);
+
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
-- 
2.17.1

[PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Make sure to invoke this call-back through the proper
function of the IOMMU-API.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..4d4847de727e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 		ops = dev->iommu_fwspec->ops;
 	/*
 	 * If we have reason to believe the IOMMU driver missed the initial
-	 * add_device callback for dev, replay it to get things in order.
+	 * probe for dev, replay it to get things in order.
 	 */
-	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
-		err = ops->add_device(dev);
+	if (dev->bus && !dev->iommu_group)
+		err = iommu_probe_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
-- 
2.17.1

[PATCH 4/4] ACPI/IORT: Don't call iommu_ops->add_device directly

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Make sure to invoke this call-back through the proper
function of the IOMMU-API.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/acpi/arm64/iort.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 70f4e80b9246..d4f7c1adc048 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -805,8 +805,8 @@ static inline int iort_add_device_replay(const struct iommu_ops *ops,
 {
 	int err = 0;
 
-	if (ops->add_device && dev->bus && !dev->iommu_group)
-		err = ops->add_device(dev);
+	if (dev->bus && !dev->iommu_group)
+		err = iommu_probe_device(dev);
 
 	return err;
 }
-- 
2.17.1

Re: [PATCH 4/4] ACPI/IORT: Don't call iommu_ops->add_device directly

[Hanjun Guo]

On 2018/12/11 23:05, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/acpi/arm64/iort.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Robin and Lorenzo know this better than me, but it's
pretty straight forward to me,

Acked-by: Hanjun Guo <hanjun.guo@linaro.org>

Thanks
Hanjun

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Marek Szyprowski]

Hi Joerg,

This patch landed in today's linux-next and causes a regression.

On 2018-12-11 16:05, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
>
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
>
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  drivers/iommu/of_iommu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..4d4847de727e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>  		ops = dev->iommu_fwspec->ops;
>  	/*
>  	 * If we have reason to believe the IOMMU driver missed the initial
> -	 * add_device callback for dev, replay it to get things in order.
> +	 * probe for dev, replay it to get things in order.
>  	 */
> -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> -		err = ops->add_device(dev);
> +	if (dev->bus && !dev->iommu_group)
> +		err = iommu_probe_device(dev);

This change removes a check for NULL ops, what causes NULL pointer
exception on first device without IOMMU.

I'm also not sure if this is a good idea to call iommu_probe_device(),
which comes from dev->bus->iommu_ops, which might be different from ops
from local variable.

>  
>  	/* Ignore all other errors apart from EPROBE_DEFER */
>  	if (err == -EPROBE_DEFER) {

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

Hi Marek,

thanks for the report!

On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
> On 2018-12-11 16:05, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel@suse.de>
> >
> > Make sure to invoke this call-back through the proper
> > function of the IOMMU-API.
> >
> > Signed-off-by: Joerg Roedel <jroedel@suse.de>
> > ---
> >  drivers/iommu/of_iommu.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index c5dd63072529..4d4847de727e 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >  		ops = dev->iommu_fwspec->ops;
> >  	/*
> >  	 * If we have reason to believe the IOMMU driver missed the initial
> > -	 * add_device callback for dev, replay it to get things in order.
> > +	 * probe for dev, replay it to get things in order.
> >  	 */
> > -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> > -		err = ops->add_device(dev);
> > +	if (dev->bus && !dev->iommu_group)
> > +		err = iommu_probe_device(dev);
> 
> This change removes a check for NULL ops, what causes NULL pointer
> exception on first device without IOMMU.

Bummer, this check was supposed to be in iommu_probe_device(), but
apparently it got lost. Does the attached patch fix it?

> I'm also not sure if this is a good idea to call iommu_probe_device(),
> which comes from dev->bus->iommu_ops, which might be different from ops
> from local variable.

The local variable comes from dev->iommu_fwspec->ops, which should be
exactly the same as dev->bus->iommu_ops. I'll leave that for now until
it turns out to be a problem (which I don't expect).


Regards,

	Joerg

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a2131751dcff..3ed4db334341 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
 int iommu_probe_device(struct device *dev)
 {
 	const struct iommu_ops *ops = dev->bus->iommu_ops;
+	int ret = -EINVAL;
 
 	WARN_ON(dev->iommu_group);
 
-	return ops->add_device(dev);
+	if (ops)
+		ret = ops->add_device(dev);
+
+	return ret;
 }
 
 void iommu_release_device(struct device *dev)

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Marek Szyprowski]

Hi Joerg,

On 2018-12-19 15:34, Joerg Roedel wrote:
> Hi Marek,
>
> thanks for the report!
>
> On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
>> On 2018-12-11 16:05, Joerg Roedel wrote:
>>> From: Joerg Roedel <jroedel@suse.de>
>>>
>>> Make sure to invoke this call-back through the proper
>>> function of the IOMMU-API.
>>>
>>> Signed-off-by: Joerg Roedel <jroedel@suse.de>
>>> ---
>>>  drivers/iommu/of_iommu.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>> index c5dd63072529..4d4847de727e 100644
>>> --- a/drivers/iommu/of_iommu.c
>>> +++ b/drivers/iommu/of_iommu.c
>>> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>>  		ops = dev->iommu_fwspec->ops;
>>>  	/*
>>>  	 * If we have reason to believe the IOMMU driver missed the initial
>>> -	 * add_device callback for dev, replay it to get things in order.
>>> +	 * probe for dev, replay it to get things in order.
>>>  	 */
>>> -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
>>> -		err = ops->add_device(dev);
>>> +	if (dev->bus && !dev->iommu_group)
>>> +		err = iommu_probe_device(dev);
>> This change removes a check for NULL ops, what causes NULL pointer
>> exception on first device without IOMMU.
> Bummer, this check was supposed to be in iommu_probe_device(), but
> apparently it got lost. Does the attached patch fix it?

Yes, it fixes this issue.

>> I'm also not sure if this is a good idea to call iommu_probe_device(),
>> which comes from dev->bus->iommu_ops, which might be different from ops
>> from local variable.
> The local variable comes from dev->iommu_fwspec->ops, which should be
> exactly the same as dev->bus->iommu_ops. I'll leave that for now until
> it turns out to be a problem (which I don't expect).
>
>
> Regards,
>
> 	Joerg
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a2131751dcff..3ed4db334341 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
>  int iommu_probe_device(struct device *dev)
>  {
>  	const struct iommu_ops *ops = dev->bus->iommu_ops;
> +	int ret = -EINVAL;
>  
>  	WARN_ON(dev->iommu_group);
>  
> -	return ops->add_device(dev);
> +	if (ops)
> +		ret = ops->add_device(dev);
> +
> +	return ret;
>  }
>  
>  void iommu_release_device(struct device *dev)
>
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

Hi Marek,

On Wed, Dec 19, 2018 at 03:53:51PM +0100, Marek Szyprowski wrote:
> Yes, it fixes this issue.

Thanks a lot, patch is sent out and in iommu tree.

Regards,

	Joerg

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Geert Uytterhoeven]

Hi Jörg,

On Wed, Dec 19, 2018 at 5:51 PM Joerg Roedel <joro@8bytes.org> wrote:
> On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
> > On 2018-12-11 16:05, Joerg Roedel wrote:
> > > From: Joerg Roedel <jroedel@suse.de>
> > >
> > > Make sure to invoke this call-back through the proper
> > > function of the IOMMU-API.
> > >
> > > Signed-off-by: Joerg Roedel <jroedel@suse.de>
> > > ---
> > >  drivers/iommu/of_iommu.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > > index c5dd63072529..4d4847de727e 100644
> > > --- a/drivers/iommu/of_iommu.c
> > > +++ b/drivers/iommu/of_iommu.c
> > > @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> > >             ops = dev->iommu_fwspec->ops;
> > >     /*
> > >      * If we have reason to believe the IOMMU driver missed the initial
> > > -    * add_device callback for dev, replay it to get things in order.
> > > +    * probe for dev, replay it to get things in order.
> > >      */
> > > -   if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> > > -           err = ops->add_device(dev);
> > > +   if (dev->bus && !dev->iommu_group)
> > > +           err = iommu_probe_device(dev);
> >
> > This change removes a check for NULL ops, what causes NULL pointer
> > exception on first device without IOMMU.
>
> Bummer, this check was supposed to be in iommu_probe_device(), but
> apparently it got lost. Does the attached patch fix it?
>
> > I'm also not sure if this is a good idea to call iommu_probe_device(),
> > which comes from dev->bus->iommu_ops, which might be different from ops
> > from local variable.
>
> The local variable comes from dev->iommu_fwspec->ops, which should be
> exactly the same as dev->bus->iommu_ops. I'll leave that for now until
> it turns out to be a problem (which I don't expect).
>
>
> Regards,
>
>         Joerg
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a2131751dcff..3ed4db334341 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
>  int iommu_probe_device(struct device *dev)
>  {
>         const struct iommu_ops *ops = dev->bus->iommu_ops;
> +       int ret = -EINVAL;
>
>         WARN_ON(dev->iommu_group);
>
> -       return ops->add_device(dev);
> +       if (ops)

Is this sufficient? The old code checked for ops->add_device != NULL,
too.

> +               ret = ops->add_device(dev);
> +
> +       return ret;
>  }
>
>  void iommu_release_device(struct device *dev)


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 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

Hi Geert,

On Thu, Dec 20, 2018 at 04:42:17PM +0100, Geert Uytterhoeven wrote:
> > -       return ops->add_device(dev);
> > +       if (ops)
> 
> Is this sufficient? The old code checked for ops->add_device != NULL,
> too.

Robin brought up that all iommu-ops implementations support the
add_device call-back, so we can remove that check and make the call-back
mandatory for new implementations too.

Regards,

	Joerg

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

On Wed, Dec 05, 2018 at 06:49:26PM +0000, Robin Murphy wrote:
> 	if (ops && dev->bus && !dev->iommu_group)
> 
> What I can't quite remember just now is whether it's actually valid to get
> here with err == 0 but dev->iommu_fwspec->ops == NULL, so it *might* be OK
> to use "!err" instead of "ops" to make things slightly more obvious - I'll
> work through the flow tomorrow to double-check.

Yeah, adding an err == 0 check seems to be the best option here, so that
iommu-drivers don't get confused when we try to add a device that is not
managed by an iommu.


Regards,

	Joerg

Re: [PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Robin Murphy]

On 05/12/2018 14:36, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
> 
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>   drivers/iommu/of_iommu.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..4d4847de727e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>   		ops = dev->iommu_fwspec->ops;
>   	/*
>   	 * If we have reason to believe the IOMMU driver missed the initial
> -	 * add_device callback for dev, replay it to get things in order.
> +	 * probe for dev, replay it to get things in order.
>   	 */
> -	if (ops && ops->add_device && dev->bus && !dev->iommu_group)

Ugh, code I wrote...

I think that first ops test is serving double-duty - it's not just an 
"is ops->add_device safe to call?" check, but also the specific "do we 
need to try doing this at all?" check, since ops is only (potentially) 
set in the !err case at the top of this context.

> -		err = ops->add_device(dev);
> +	if (dev->bus && !dev->iommu_group)

Thus to avoid calling add_device erroneously in either of the "no valid 
IOMMU" cases (since dev->bus->iommu_ops may well be non-NULL), this 
still needs to be at least:

	if (ops && dev->bus && !dev->iommu_group)

What I can't quite remember just now is whether it's actually valid to 
get here with err == 0 but dev->iommu_fwspec->ops == NULL, so it *might* 
be OK to use "!err" instead of "ops" to make things slightly more 
obvious - I'll work through the flow tomorrow to double-check.

Robin.

> +		err = iommu_probe_device(dev);
>   
>   	/* Ignore all other errors apart from EPROBE_DEFER */
>   	if (err == -EPROBE_DEFER) {
> 

[PATCH 3/4] iommu/of: Don't call iommu_ops->add_device directly

[Joerg Roedel]

From: Joerg Roedel <jroedel@suse.de>

Make sure to invoke this call-back through the proper
function of the IOMMU-API.

Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..4d4847de727e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
 		ops = dev->iommu_fwspec->ops;
 	/*
 	 * If we have reason to believe the IOMMU driver missed the initial
-	 * add_device callback for dev, replay it to get things in order.
+	 * probe for dev, replay it to get things in order.
 	 */
-	if (ops && ops->add_device && dev->bus && !dev->iommu_group)
-		err = ops->add_device(dev);
+	if (dev->bus && !dev->iommu_group)
+		err = iommu_probe_device(dev);
 
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {
-- 
2.17.1