Skip to contentSkip to conversation

Consolidate iommu_ops->add/remove_device() calls

PATCH In review Joerg Roedel 4 participants 12 messages Dec 11, 2018 – Jan 11, 2019

Subsystems (3)
ACPI IOMMU ACPI arm64
lore
Share & tools
lore

Joerg Roedel

author maintainer
#0 lore raw

v2 cover Consolidate iommu_ops->add/remove_device() calls

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(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH v2 1/4 iommu/sysfs: Rename iommu_release_device()

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
 drivers/iommu/iommu-sysfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/iommu/iommu-sysfs.c +6 -6
index 36d1a7ce7fc4..71c2249d3260 100644
@@ -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)
Signature · 1 line
2.17.1
author maintainer
#2 lore raw

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

In reply to Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/iommu.c | 51 +++++++++++++++++++++----------------------
 include/linux/iommu.h |  3 +++
 2 files changed, 28 insertions(+), 26 deletions(-)
drivers/iommu/iommu.c +25 -26
index edbdf5d6962c..1e8f4e0c9198 100644
@@ -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;
 	}
 
 	/*
include/linux/iommu.h +3 -0
index a1d28f42cb77..cb36f32cd3c2 100644
@@ -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 {};
Signature · 1 line
2.17.1
author maintainer
#3 lore raw

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

In reply to Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/of_iommu.c +3 -3
index c5dd63072529..4d4847de727e 100644
@@ -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) {
Signature · 1 line
2.17.1
#4 lore raw

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

Hi Joerg,

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

Quoted 15 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/of_iommu.c +0 -0
index c5dd63072529..4d4847de727e 100644

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 224

Quoted 10 lines
drivers/iommu/of_iommu.c +3 -3
@@ -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.

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 227

Quoted 3 lines
drivers/iommu/of_iommu.c +0 -0
 	/* Ignore all other errors apart from EPROBE_DEFER */
 	if (err == -EPROBE_DEFER) {

Best regards

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author maintainer
#5 lore raw

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

Hi Marek,

thanks for the report!

Quoted 21 lines · Marek Szyprowski
Quoted 20 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/of_iommu.c +1 -1
index c5dd63072529..4d4847de727e 100644
@@ -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.
Quoted 8 lines

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 224

Quoted 5 lines
drivers/iommu/of_iommu.c +2 -2
 	 */
-	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?

Quoted 3 lines

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

drivers/iommu/iommu.c +5 -1
index a2131751dcff..3ed4db334341 100644
@@ -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)
#6 lore raw

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

Hi Joerg,

Quoted 33 lines · Joerg Roedel

Hi Marek,

thanks for the report!

Quoted 20 lines · Marek Szyprowski
Quoted 19 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/of_iommu.c +0 -1
index c5dd63072529..4d4847de727e 100644
@@ -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.
Quoted 8 lines

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 224

Quoted 6 lines
drivers/iommu/of_iommu.c +3 -2
+	 * 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.

Quoted 2 lines

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.

Quoted 24 lines
Quoted 3 lines

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

drivers/iommu/iommu.c +1 -0
index a2131751dcff..3ed4db334341 100644
@@ -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;

drivers/iommu/iommu.c void iommu_device_unregister(struct iommu_device *iommu) · line 127

Quoted 10 lines
drivers/iommu/iommu.c +4 -1
 	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

Signature · 2 lines
Marek Szyprowski, PhD
Samsung R&D Institute Poland
author maintainer
#7 lore raw

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

Hi Marek,

Quoted 1 line · Marek Szyprowski

Yes, it fixes this issue.

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

Regards,

Joerg

#8 lore raw

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

In reply to Joerg Roedel

Hi Jörg,

Quoted 52 lines · Joerg Roedel
Quoted 21 lines · Marek Szyprowski
Quoted 20 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/of_iommu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iommu/of_iommu.c +1 -1
index c5dd63072529..4d4847de727e 100644
@@ -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.
Quoted 8 lines

drivers/iommu/of_iommu.c const struct iommu_ops *of_iommu_configure(struct device *dev, · line 224

Quoted 5 lines
drivers/iommu/of_iommu.c +2 -2
     */
-   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?

Quoted 3 lines

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

drivers/iommu/iommu.c +0 -0
index a2131751dcff..3ed4db334341 100644

drivers/iommu/iommu.c void iommu_device_unregister(struct iommu_device *iommu) · line 121

Quoted 9 lines
drivers/iommu/iommu.c +2 -1
@@ -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.

drivers/iommu/iommu.c void iommu_device_unregister(struct iommu_device *iommu) · line 127

Quoted 6 lines
drivers/iommu/iommu.c +3 -0
+               ret = ops->add_device(dev);
+
+       return ret;
 }
 
 void iommu_release_device(struct device *dev)

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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
author maintainer
#9 lore raw

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

Hi Geert,

Quoted 5 lines · Geert Uytterhoeven

drivers/iommu/iommu.c void iommu_device_unregister(struct iommu_device *iommu) · line 121

Quoted 2 lines
drivers/iommu/iommu.c +1 -1
-       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

author maintainer
#10 lore raw

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

In reply to Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/acpi/arm64/iort.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/acpi/arm64/iort.c +2 -2
index 70f4e80b9246..d4f7c1adc048 100644
@@ -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;
 }
Signature · 1 line
2.17.1
maintainer Acked-by on v2 4/4
#11 lore raw

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

Quoted 9 lines · Joerg Roedel

From: Joerg Roedel <[email protected]>

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

Signed-off-by Joerg Roedel <[email protected]>
---
 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 <[email protected]>

Thanks
Hanjun