Skip to contentSkip to conversation

Consolitate iommu_ops->add/remove_device() calls

PATCH Superseded Joerg Roedel 3 participants 11 messages Dec 5, 2018 – Dec 6, 2018

Subsystems (3)
ACPI IOMMU ACPI arm64

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Joerg Roedel

author maintainer
#0 lore raw

cover Consolitate iommu_ops->add/remove_device() calls

Hi,

this patch-set consolidates the calls to iommu_ops->add_device() and remove_device() and the necessary checks into probe/release functions that be extended later with more setup work.

Regards,

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       | 54 +++++++++++++++++++------------------
 drivers/iommu/of_iommu.c    |  6 ++---
 include/linux/iommu.h       |  3 +++
 5 files changed, 42 insertions(+), 37 deletions(-)
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 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 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 | 54 ++++++++++++++++++++++---------------------
 include/linux/iommu.h |  3 +++
 2 files changed, 31 insertions(+), 26 deletions(-)
drivers/iommu/iommu.c +28 -26
index edbdf5d6962c..ad4dc51eb69c 100644
@@ -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;
 	}
 
 	/*
include/linux/iommu.h +3 -0
index a1d28f42cb77..2357841845bb 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
#3 lore raw

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

Quoted 1 line

+void iommu_release_device(struct device *dev)

Nitpick: there seems to be a double space here.

include/linux/iommu.h void iommu_fwspec_free(struct device *dev); · line 402

Quoted 2 lines
include/linux/iommu.h +2 -0
+int iommu_probe_device(struct device *dev);
+void  iommu_release_device(struct device *dev);

.. and here.

author maintainer
#4 lore raw

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

Quoted 8 lines · Christoph Hellwig
Quoted 1 line

+void iommu_release_device(struct device *dev)

Nitpick: there seems to be a double space here.

include/linux/iommu.h void iommu_fwspec_free(struct device *dev); · line 402

Quoted 2 lines
include/linux/iommu.h +2 -0
+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.

reviewer
#5 lore raw

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

In reply to Joerg Roedel
Quoted 16 lines · 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 | 54 ++++++++++++++++++++++---------------------
  include/linux/iommu.h |  3 +++
  2 files changed, 31 insertions(+), 26 deletions(-)
drivers/iommu/iommu.c +0 -0
index edbdf5d6962c..ad4dc51eb69c 100644

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

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

Quoted 97 lines
drivers/iommu/iommu.c +22 -26
+
+	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;
  	}
 
  	/*
include/linux/iommu.h +3 -0
index a1d28f42cb77..2357841845bb 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 {};
author maintainer
#6 lore raw

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

Quoted 4 lines · Robin Murphy

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

author maintainer
#7 lore raw

PATCH 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
reviewer
#8 lore raw

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

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 223

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

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

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

Quoted 5 lines
drivers/iommu/of_iommu.c +1 -0
+		err = iommu_probe_device(dev);
 
  	/* Ignore all other errors apart from EPROBE_DEFER */
  	if (err == -EPROBE_DEFER) {
author maintainer
#9 lore raw

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

Quoted 6 lines · Robin Murphy
	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

author maintainer
#10 lore raw

PATCH 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