[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 2/4] iommu: Consolitate ->add/remove_device() calls

[Joerg Roedel]

On Wed, Dec 05, 2018 at 06:20:50PM +0000, Robin Murphy wrote:
> Is there any good reason to let .add_device/.remove_device be optional
> still? Everyone's implemented them for a while now, and on a practical level
> I don't really see how else we could expect devices to be taken in and out
> of their appropriate groups correctly.

There is probably no reason anymore to keep add_device/remove_device
option. I'll check the users and remove the pointer checks if possible.


Regards,

	Joerg

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

[Robin Murphy]

On 05/12/2018 14:36, Joerg Roedel wrote:
> 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 | 54 ++++++++++++++++++++++---------------------
>   include/linux/iommu.h |  3 +++
>   2 files changed, 31 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index edbdf5d6962c..ad4dc51eb69c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -110,6 +110,26 @@ 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;
> +
> +	if (!ops->add_device)
> +		return 0;

Is there any good reason to let .add_device/.remove_device be optional 
still? Everyone's implemented them for a while now, and on a practical 
level I don't really see how else we could expect devices to be taken in 
and out of their appropriate groups correctly.

Robin.

> +
> +	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 (ops->remove_device && 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 +1137,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 +1152,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 +1160,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..2357841845bb 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 {};
> 

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

[Joerg Roedel]

On Wed, Dec 05, 2018 at 06:54:18AM -0800, Christoph Hellwig wrote:
> > +void  iommu_release_device(struct device *dev)
> 
> Nitpick: there seems to be a double space here.
> 
> > +int iommu_probe_device(struct device *dev);
> > +void  iommu_release_device(struct device *dev);
> 
> .. and here.

Right, thanks. I fixed it in my development branch.

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

[Christoph Hellwig]

> +void  iommu_release_device(struct device *dev)

Nitpick: there seems to be a double space here.

> +int iommu_probe_device(struct device *dev);
> +void  iommu_release_device(struct device *dev);

.. and here.

[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 | 54 ++++++++++++++++++++++---------------------
 include/linux/iommu.h |  3 +++
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index edbdf5d6962c..ad4dc51eb69c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -110,6 +110,26 @@ 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;
+
+	if (!ops->add_device)
+		return 0;
+
+	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 (ops->remove_device && 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 +1137,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 +1152,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 +1160,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..2357841845bb 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