[PATCH v4 00/16] iommu: retire bus_set_iommu()

[PATCH v4 00/16] iommu: retire bus_set_iommu()

[Robin Murphy]

v3: https://lore.kernel.org/linux-iommu/cover.1657034827.git.robin.murphy@arm.com/

Hi all,

As promised, here's the rebased v4 which I hope is ready to go now.
Besides the new patch for s390, there are just a few small improvements
over v3 to make things even simpler and clearer.

Thanks,
Robin.


Matthew Rosato (1):
  iommu/s390: Fail probe for non-PCI devices

Robin Murphy (15):
  iommu/vt-d: Handle race between registration and device probe
  iommu/amd: Handle race between registration and device probe
  iommu: Always register bus notifiers
  iommu: Move bus setup to IOMMU device registration
  iommu/amd: Clean up bus_set_iommu()
  iommu/arm-smmu: Clean up bus_set_iommu()
  iommu/arm-smmu-v3: Clean up bus_set_iommu()
  iommu/dart: Clean up bus_set_iommu()
  iommu/exynos: Clean up bus_set_iommu()
  iommu/ipmmu-vmsa: Clean up bus_set_iommu()
  iommu/mtk: Clean up bus_set_iommu()
  iommu/omap: Clean up bus_set_iommu()
  iommu/tegra-smmu: Clean up bus_set_iommu()
  iommu/virtio: Clean up bus_set_iommu()
  iommu: Clean up bus_set_iommu()

 drivers/iommu/amd/amd_iommu.h               |   1 -
 drivers/iommu/amd/init.c                    |   9 +-
 drivers/iommu/amd/iommu.c                   |  25 +---
 drivers/iommu/apple-dart.c                  |  30 +----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  53 +-------
 drivers/iommu/arm/arm-smmu/arm-smmu.c       |  84 +-----------
 drivers/iommu/arm/arm-smmu/qcom_iommu.c     |   4 -
 drivers/iommu/exynos-iommu.c                |   9 --
 drivers/iommu/fsl_pamu_domain.c             |   4 -
 drivers/iommu/intel/iommu.c                 |   4 +-
 drivers/iommu/iommu.c                       | 137 +++++++++-----------
 drivers/iommu/ipmmu-vmsa.c                  |  35 +----
 drivers/iommu/msm_iommu.c                   |   2 -
 drivers/iommu/mtk_iommu.c                   |  24 +---
 drivers/iommu/mtk_iommu_v1.c                |  13 +-
 drivers/iommu/omap-iommu.c                  |   6 -
 drivers/iommu/rockchip-iommu.c              |   2 -
 drivers/iommu/s390-iommu.c                  |  13 +-
 drivers/iommu/sprd-iommu.c                  |   5 -
 drivers/iommu/sun50i-iommu.c                |   2 -
 drivers/iommu/tegra-smmu.c                  |  29 +----
 drivers/iommu/virtio-iommu.c                |  25 ----
 include/linux/iommu.h                       |   1 -
 23 files changed, 86 insertions(+), 431 deletions(-)

-- 
2.36.1.dirty

[PATCH v4 01/16] iommu/vt-d: Handle race between registration and device probe

[Robin Murphy]

Currently we rely on registering all our instances before initially
allowing any .probe_device calls via bus_set_iommu(). In preparation for
phasing out the latter, make sure we won't inadvertently return success
for a device associated with a known but not yet registered instance,
otherwise we'll run straight into iommu_group_get_for_dev() trying to
use NULL ops.

That also highlights an issue with intel_iommu_get_resv_regions() taking
dmar_global_lock from within a section where intel_iommu_init() already
holds it, which already exists via probe_acpi_namespace_devices() when
an ANDD device is probed, but gets more obvious with the upcoming change
to iommu_device_register(). Since they are both read locks it manages
not to deadlock in practice, and a more in-depth rework of this locking
is underway, so no attempt is made to address it here.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: Update commit message

 drivers/iommu/intel/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7cca030a508e..65c340e2003c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4449,7 +4449,7 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 	u8 bus, devfn;
 
 	iommu = device_to_iommu(dev, &bus, &devfn);
-	if (!iommu)
+	if (!iommu || !iommu->iommu.ops)
 		return ERR_PTR(-ENODEV);
 
 	info = kzalloc(sizeof(*info), GFP_KERNEL);
-- 
2.36.1.dirty

[PATCH v4 02/16] iommu/amd: Handle race between registration and device probe

[Robin Murphy]

As for the Intel driver, make sure the AMD driver can cope with seeing
.probe_device calls without having to wait for all known instances to
register first.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/amd/iommu.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 65b8e4fd8217..5ed9fd40d2a7 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1851,6 +1851,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 	if (!iommu)
 		return ERR_PTR(-ENODEV);
 
+	/* Not registered yet? */
+	if (!iommu->iommu.ops)
+		return ERR_PTR(-ENODEV);
+
 	if (dev_iommu_priv_get(dev))
 		return &iommu->iommu;
 
-- 
2.36.1.dirty

[PATCH v4 03/16] iommu/s390: Fail probe for non-PCI devices

[Robin Murphy]

From: Matthew Rosato <mjrosato@linux.ibm.com>

s390-iommu only supports pci_bus_type today.

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: New

 drivers/iommu/s390-iommu.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index c898bcbbce11..8158a4ed0c60 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -185,7 +185,12 @@ static void s390_iommu_detach_device(struct iommu_domain *domain,
 
 static struct iommu_device *s390_iommu_probe_device(struct device *dev)
 {
-	struct zpci_dev *zdev = to_zpci_dev(dev);
+	struct zpci_dev *zdev;
+
+	if (!dev_is_pci(dev))
+		return ERR_PTR(-ENODEV);
+
+	zdev = to_zpci_dev(dev);
 
 	return &zdev->iommu_dev;
 }
-- 
2.36.1.dirty

[PATCH v4 04/16] iommu: Always register bus notifiers

[Robin Murphy]

The number of bus types that the IOMMU subsystem deals with is small and
manageable, so pull that list into core code as a first step towards
cleaning up all the boilerplate bus-awareness from drivers. Calling
iommu_probe_device() before bus->iommu_ops is set will simply return
-ENODEV and not break the notifier call chain, so there should be no
harm in proactively registering all our bus notifiers at init time.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: Squash iommu_bus_init() entirely, for maximum simplicity. Ignoring
    the return from bus_register_notifier() is common, so hopefully it's
    all pretty self-explanatory now.

 drivers/iommu/iommu.c | 72 ++++++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 780fb7071577..a8d14f2a1035 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt)    "iommu: " fmt
 
+#include <linux/amba/bus.h>
 #include <linux/device.h>
 #include <linux/dma-iommu.h>
 #include <linux/kernel.h>
@@ -16,11 +17,13 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/errno.h>
+#include <linux/host1x_context_bus.h>
 #include <linux/iommu.h>
 #include <linux/idr.h>
 #include <linux/err.h>
 #include <linux/pci.h>
 #include <linux/bitops.h>
+#include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/fsl/mc.h>
 #include <linux/module.h>
@@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {
 #define IOMMU_CMD_LINE_DMA_API		BIT(0)
 #define IOMMU_CMD_LINE_STRICT		BIT(1)
 
+static int iommu_bus_notifier(struct notifier_block *nb,
+			      unsigned long action, void *data);
 static int iommu_alloc_default_domain(struct iommu_group *group,
 				      struct device *dev);
 static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
@@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name =		\
 static LIST_HEAD(iommu_device_list);
 static DEFINE_SPINLOCK(iommu_device_lock);
 
+static struct bus_type * const iommu_buses[] = {
+	&platform_bus_type,
+#ifdef CONFIG_PCI
+	&pci_bus_type,
+#endif
+#ifdef CONFIG_ARM_AMBA
+	&amba_bustype,
+#endif
+#ifdef CONFIG_FSL_MC_BUS
+	&fsl_mc_bus_type,
+#endif
+#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS
+	&host1x_context_device_bus_type,
+#endif
+};
+
 /*
  * Use a function instead of an array here because the domain-type is a
  * bit-field, so an array would waste memory.
@@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)
 
 static int __init iommu_subsys_init(void)
 {
+	struct notifier_block *nb;
+
 	if (!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API)) {
 		if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
 			iommu_set_default_passthrough(false);
@@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void)
 			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
 				"(set via kernel command line)" : "");
 
+	nb = kcalloc(ARRAY_SIZE(iommu_buses), sizeof(*nb), GFP_KERNEL);
+	if (!nb)
+		return -ENOMEM;
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		nb[i].notifier_call = iommu_bus_notifier;
+		bus_register_notifier(iommu_buses[i], &nb[i]);
+	}
+
 	return 0;
 }
 subsys_initcall(iommu_subsys_init);
@@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
-{
-	struct notifier_block *nb;
-	int err;
-
-	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
-	if (!nb)
-		return -ENOMEM;
-
-	nb->notifier_call = iommu_bus_notifier;
-
-	err = bus_register_notifier(bus, nb);
-	if (err)
-		goto out_free;
-
-	err = bus_iommu_probe(bus);
-	if (err)
-		goto out_err;
-
-
-	return 0;
-
-out_err:
-	/* Clean up */
-	bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-	bus_unregister_notifier(bus, nb);
-
-out_free:
-	kfree(nb);
-
-	return err;
-}
-
 /**
  * bus_set_iommu - set iommu-callbacks for the bus
  * @bus: bus.
@@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
 	bus->iommu_ops = ops;
 
 	/* Do IOMMU specific setup for this bus-type */
-	err = iommu_bus_init(bus, ops);
-	if (err)
+	err = bus_iommu_probe(bus);
+	if (err) {
+		/* Clean up */
+		bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
 		bus->iommu_ops = NULL;
+	}
 
 	return err;
 }
-- 
2.36.1.dirty

[PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

Move the bus setup to iommu_device_register(). This should allow
bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.

At this point we can also handle cleanup better than just rolling back
the most-recently-touched bus upon failure - which may release devices
owned by other already-registered instances, and still leave devices on
other buses with dangling pointers to the failed instance. Now it's easy
to clean up the exact footprint of a given instance, no more, no less.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Reviewed-By: Krishna Reddy <vdumpa@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: Factor out the ops check in iommu_device_register() to keep the loop
even simpler, and comment the nominal change in behaviour

 drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a8d14f2a1035..4db0874a5ed6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -188,6 +188,14 @@ static int __init iommu_subsys_init(void)
 }
 subsys_initcall(iommu_subsys_init);
 
+static int remove_iommu_group(struct device *dev, void *data)
+{
+	if (dev->iommu && dev->iommu->iommu_dev == data)
+		iommu_release_device(dev);
+
+	return 0;
+}
+
 /**
  * iommu_device_register() - Register an IOMMU hardware instance
  * @iommu: IOMMU handle for the instance
@@ -199,9 +207,18 @@ subsys_initcall(iommu_subsys_init);
 int iommu_device_register(struct iommu_device *iommu,
 			  const struct iommu_ops *ops, struct device *hwdev)
 {
+	int err = 0;
+
 	/* We need to be able to take module references appropriately */
 	if (WARN_ON(is_module_address((unsigned long)ops) && !ops->owner))
 		return -EINVAL;
+	/*
+	 * Temporarily enforce global restriction to a single driver. This was
+	 * already the de-facto behaviour, since any possible combination of
+	 * existing drivers would compete for at least the PCI or platform bus.
+	 */
+	if (iommu_buses[0]->iommu_ops && iommu_buses[0]->iommu_ops != ops)
+		return -EBUSY;
 
 	iommu->ops = ops;
 	if (hwdev)
@@ -210,12 +227,22 @@ int iommu_device_register(struct iommu_device *iommu,
 	spin_lock(&iommu_device_lock);
 	list_add_tail(&iommu->list, &iommu_device_list);
 	spin_unlock(&iommu_device_lock);
-	return 0;
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++) {
+		iommu_buses[i]->iommu_ops = ops;
+		err = bus_iommu_probe(iommu_buses[i]);
+	}
+	if (err)
+		iommu_device_unregister(iommu);
+	return err;
 }
 EXPORT_SYMBOL_GPL(iommu_device_register);
 
 void iommu_device_unregister(struct iommu_device *iommu)
 {
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+		bus_for_each_dev(iommu_buses[i], NULL, iommu, remove_iommu_group);
+
 	spin_lock(&iommu_device_lock);
 	list_del(&iommu->list);
 	spin_unlock(&iommu_device_lock);
@@ -1644,13 +1671,6 @@ static int probe_iommu_group(struct device *dev, void *data)
 	return ret;
 }
 
-static int remove_iommu_group(struct device *dev, void *data)
-{
-	iommu_release_device(dev);
-
-	return 0;
-}
-
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data)
 {
@@ -1822,27 +1842,12 @@ int bus_iommu_probe(struct bus_type *bus)
  */
 int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
 {
-	int err;
-
-	if (ops == NULL) {
-		bus->iommu_ops = NULL;
-		return 0;
-	}
-
-	if (bus->iommu_ops != NULL)
+	if (bus->iommu_ops && ops && bus->iommu_ops != ops)
 		return -EBUSY;
 
 	bus->iommu_ops = ops;
 
-	/* Do IOMMU specific setup for this bus-type */
-	err = bus_iommu_probe(bus);
-	if (err) {
-		/* Clean up */
-		bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
-		bus->iommu_ops = NULL;
-	}
-
-	return err;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(bus_set_iommu);
 
-- 
2.36.1.dirty

[PATCH v4 06/16] iommu/amd: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and
garbage-collect the last remnants of amd_iommu_init_api().

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/amd/amd_iommu.h |  1 -
 drivers/iommu/amd/init.c      |  9 +--------
 drivers/iommu/amd/iommu.c     | 21 ---------------------
 3 files changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 84e5bb1bf01b..c160a332ce33 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -18,7 +18,6 @@ extern void amd_iommu_restart_event_logging(struct amd_iommu *iommu);
 extern int amd_iommu_init_devices(void);
 extern void amd_iommu_uninit_devices(void);
 extern void amd_iommu_init_notifier(void);
-extern int amd_iommu_init_api(void);
 extern void amd_iommu_set_rlookup_table(struct amd_iommu *iommu, u16 devid);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index fdc642362c14..79e0286da6ce 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2168,20 +2168,13 @@ static int __init amd_iommu_init_pci(void)
 	/*
 	 * Order is important here to make sure any unity map requirements are
 	 * fulfilled. The unity mappings are created and written to the device
-	 * table during the amd_iommu_init_api() call.
+	 * table during the iommu_init_pci() call.
 	 *
 	 * After that we call init_device_table_dma() to make sure any
 	 * uninitialized DTE will block DMA, and in the end we flush the caches
 	 * of all IOMMUs to make sure the changes to the device table are
 	 * active.
 	 */
-	ret = amd_iommu_init_api();
-	if (ret) {
-		pr_err("IOMMU: Failed to initialize IOMMU-API interface (error=%d)!\n",
-		       ret);
-		goto out;
-	}
-
 	for_each_pci_segment(pci_seg)
 		init_device_table_dma(pci_seg);
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 5ed9fd40d2a7..7cf532470389 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -11,8 +11,6 @@
 #include <linux/ratelimit.h>
 #include <linux/pci.h>
 #include <linux/acpi.h>
-#include <linux/amba/bus.h>
-#include <linux/platform_device.h>
 #include <linux/pci-ats.h>
 #include <linux/bitmap.h>
 #include <linux/slab.h>
@@ -1941,25 +1939,6 @@ void amd_iommu_domain_update(struct protection_domain *domain)
 	amd_iommu_domain_flush_complete(domain);
 }
 
-int __init amd_iommu_init_api(void)
-{
-	int err;
-
-	err = bus_set_iommu(&pci_bus_type, &amd_iommu_ops);
-	if (err)
-		return err;
-#ifdef CONFIG_ARM_AMBA
-	err = bus_set_iommu(&amba_bustype, &amd_iommu_ops);
-	if (err)
-		return err;
-#endif
-	err = bus_set_iommu(&platform_bus_type, &amd_iommu_ops);
-	if (err)
-		return err;
-
-	return 0;
-}
-
 /*****************************************************************************
  *
  * The following functions belong to the exported interface of AMD IOMMU
-- 
2.36.1.dirty

[PATCH v4 07/16] iommu/arm-smmu: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary. With device
probes now replayed for every IOMMU instance registration, the whole
sorry ordering workaround for legacy DT bindings goes too, hooray!

Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 84 +--------------------------
 1 file changed, 2 insertions(+), 82 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index dfa82df00342..90433b61400b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -37,7 +37,6 @@
 #include <linux/ratelimit.h>
 #include <linux/slab.h>
 
-#include <linux/amba/bus.h>
 #include <linux/fsl/mc.h>
 
 #include "arm-smmu.h"
@@ -93,8 +92,6 @@ static struct platform_driver arm_smmu_driver;
 static struct iommu_ops arm_smmu_ops;
 
 #ifdef CONFIG_ARM_SMMU_LEGACY_DT_BINDINGS
-static int arm_smmu_bus_init(struct iommu_ops *ops);
-
 static struct device_node *dev_get_dev_node(struct device *dev)
 {
 	if (dev_is_pci(dev)) {
@@ -180,20 +177,6 @@ static int arm_smmu_register_legacy_master(struct device *dev,
 	kfree(sids);
 	return err;
 }
-
-/*
- * With the legacy DT binding in play, we have no guarantees about
- * probe order, but then we're also not doing default domains, so we can
- * delay setting bus ops until we're sure every possible SMMU is ready,
- * and that way ensure that no probe_device() calls get missed.
- */
-static int arm_smmu_legacy_bus_init(void)
-{
-	if (using_legacy_binding)
-		return arm_smmu_bus_init(&arm_smmu_ops);
-	return 0;
-}
-device_initcall_sync(arm_smmu_legacy_bus_init);
 #else
 static int arm_smmu_register_legacy_master(struct device *dev,
 					   struct arm_smmu_device **smmu)
@@ -2016,52 +1999,6 @@ static int arm_smmu_device_dt_probe(struct arm_smmu_device *smmu,
 	return 0;
 }
 
-static int arm_smmu_bus_init(struct iommu_ops *ops)
-{
-	int err;
-
-	/* Oh, for a proper bus abstraction */
-	if (!iommu_present(&platform_bus_type)) {
-		err = bus_set_iommu(&platform_bus_type, ops);
-		if (err)
-			return err;
-	}
-#ifdef CONFIG_ARM_AMBA
-	if (!iommu_present(&amba_bustype)) {
-		err = bus_set_iommu(&amba_bustype, ops);
-		if (err)
-			goto err_reset_platform_ops;
-	}
-#endif
-#ifdef CONFIG_PCI
-	if (!iommu_present(&pci_bus_type)) {
-		err = bus_set_iommu(&pci_bus_type, ops);
-		if (err)
-			goto err_reset_amba_ops;
-	}
-#endif
-#ifdef CONFIG_FSL_MC_BUS
-	if (!iommu_present(&fsl_mc_bus_type)) {
-		err = bus_set_iommu(&fsl_mc_bus_type, ops);
-		if (err)
-			goto err_reset_pci_ops;
-	}
-#endif
-	return 0;
-
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-	bus_set_iommu(&pci_bus_type, NULL);
-#endif
-err_reset_amba_ops: __maybe_unused;
-#ifdef CONFIG_ARM_AMBA
-	bus_set_iommu(&amba_bustype, NULL);
-#endif
-err_reset_platform_ops: __maybe_unused;
-	bus_set_iommu(&platform_bus_type, NULL);
-	return err;
-}
-
 static void arm_smmu_rmr_install_bypass_smr(struct arm_smmu_device *smmu)
 {
 	struct list_head rmr_list;
@@ -2226,7 +2163,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
-		goto err_sysfs_remove;
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return err;
 	}
 
 	platform_set_drvdata(pdev, smmu);
@@ -2248,24 +2186,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		pm_runtime_enable(dev);
 	}
 
-	/*
-	 * For ACPI and generic DT bindings, an SMMU will be probed before
-	 * any device which might need it, so we want the bus ops in place
-	 * ready to handle default domain setup as soon as any SMMU exists.
-	 */
-	if (!using_legacy_binding) {
-		err = arm_smmu_bus_init(&arm_smmu_ops);
-		if (err)
-			goto err_unregister_device;
-	}
-
 	return 0;
-
-err_unregister_device:
-	iommu_device_unregister(&smmu->iommu);
-err_sysfs_remove:
-	iommu_device_sysfs_remove(&smmu->iommu);
-	return err;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
@@ -2278,7 +2199,6 @@ static int arm_smmu_device_remove(struct platform_device *pdev)
 	if (!bitmap_empty(smmu->context_map, ARM_SMMU_MAX_CBS))
 		dev_notice(&pdev->dev, "disabling translation\n");
 
-	arm_smmu_bus_init(NULL);
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
 
-- 
2.36.1.dirty

[PATCH v4 08/16] iommu/arm-smmu-v3: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Acked-by: Will Deacon <will@kernel.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 53 +--------------------
 1 file changed, 2 insertions(+), 51 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index d32b02336411..e17ed5870f77 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -28,8 +28,6 @@
 #include <linux/pci-ats.h>
 #include <linux/platform_device.h>
 
-#include <linux/amba/bus.h>
-
 #include "arm-smmu-v3.h"
 #include "../../iommu-sva-lib.h"
 
@@ -3673,43 +3671,6 @@ static unsigned long arm_smmu_resource_size(struct arm_smmu_device *smmu)
 		return SZ_128K;
 }
 
-static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
-{
-	int err;
-
-#ifdef CONFIG_PCI
-	if (pci_bus_type.iommu_ops != ops) {
-		err = bus_set_iommu(&pci_bus_type, ops);
-		if (err)
-			return err;
-	}
-#endif
-#ifdef CONFIG_ARM_AMBA
-	if (amba_bustype.iommu_ops != ops) {
-		err = bus_set_iommu(&amba_bustype, ops);
-		if (err)
-			goto err_reset_pci_ops;
-	}
-#endif
-	if (platform_bus_type.iommu_ops != ops) {
-		err = bus_set_iommu(&platform_bus_type, ops);
-		if (err)
-			goto err_reset_amba_ops;
-	}
-
-	return 0;
-
-err_reset_amba_ops:
-#ifdef CONFIG_ARM_AMBA
-	bus_set_iommu(&amba_bustype, NULL);
-#endif
-err_reset_pci_ops: __maybe_unused;
-#ifdef CONFIG_PCI
-	bus_set_iommu(&pci_bus_type, NULL);
-#endif
-	return err;
-}
-
 static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
 				      resource_size_t size)
 {
@@ -3848,27 +3809,17 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	ret = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (ret) {
 		dev_err(dev, "Failed to register iommu\n");
-		goto err_sysfs_remove;
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ret;
 	}
 
-	ret = arm_smmu_set_bus_ops(&arm_smmu_ops);
-	if (ret)
-		goto err_unregister_device;
-
 	return 0;
-
-err_unregister_device:
-	iommu_device_unregister(&smmu->iommu);
-err_sysfs_remove:
-	iommu_device_sysfs_remove(&smmu->iommu);
-	return ret;
 }
 
 static int arm_smmu_device_remove(struct platform_device *pdev)
 {
 	struct arm_smmu_device *smmu = platform_get_drvdata(pdev);
 
-	arm_smmu_set_bus_ops(NULL);
 	iommu_device_unregister(&smmu->iommu);
 	iommu_device_sysfs_remove(&smmu->iommu);
 	arm_smmu_device_disable(smmu);
-- 
2.36.1.dirty

[PATCH v4 09/16] iommu/dart: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Tested-by: Sven Peter <sven@svenpeter.dev>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/apple-dart.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
index 1b1725759262..437aed674fba 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -820,27 +820,6 @@ static irqreturn_t apple_dart_irq(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
-static int apple_dart_set_bus_ops(const struct iommu_ops *ops)
-{
-	int ret;
-
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type, ops);
-		if (ret)
-			return ret;
-	}
-#ifdef CONFIG_PCI
-	if (!iommu_present(&pci_bus_type)) {
-		ret = bus_set_iommu(&pci_bus_type, ops);
-		if (ret) {
-			bus_set_iommu(&platform_bus_type, NULL);
-			return ret;
-		}
-	}
-#endif
-	return 0;
-}
-
 static int apple_dart_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -895,14 +874,10 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, dart);
 
-	ret = apple_dart_set_bus_ops(&apple_dart_iommu_ops);
-	if (ret)
-		goto err_free_irq;
-
 	ret = iommu_device_sysfs_add(&dart->iommu, dev, NULL, "apple-dart.%s",
 				     dev_name(&pdev->dev));
 	if (ret)
-		goto err_remove_bus_ops;
+		goto err_free_irq;
 
 	ret = iommu_device_register(&dart->iommu, &apple_dart_iommu_ops, dev);
 	if (ret)
@@ -916,8 +891,6 @@ static int apple_dart_probe(struct platform_device *pdev)
 
 err_sysfs_remove:
 	iommu_device_sysfs_remove(&dart->iommu);
-err_remove_bus_ops:
-	apple_dart_set_bus_ops(NULL);
 err_free_irq:
 	free_irq(dart->irq, dart);
 err_clk_disable:
@@ -932,7 +905,6 @@ static int apple_dart_remove(struct platform_device *pdev)
 
 	apple_dart_hw_reset(dart);
 	free_irq(dart->irq, dart);
-	apple_dart_set_bus_ops(NULL);
 
 	iommu_device_unregister(&dart->iommu);
 	iommu_device_sysfs_remove(&dart->iommu);
-- 
2.36.1.dirty

[PATCH v4 10/16] iommu/exynos: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/exynos-iommu.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 8e18984a0c4f..45fd4850bacb 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1446,16 +1446,7 @@ static int __init exynos_iommu_init(void)
 		goto err_zero_lv2;
 	}
 
-	ret = bus_set_iommu(&platform_bus_type, &exynos_iommu_ops);
-	if (ret) {
-		pr_err("%s: Failed to register exynos-iommu driver.\n",
-								__func__);
-		goto err_set_iommu;
-	}
-
 	return 0;
-err_set_iommu:
-	kmem_cache_free(lv2table_kmem_cache, zero_lv2_table);
 err_zero_lv2:
 	platform_driver_unregister(&exynos_sysmmu_driver);
 err_reg_driver:
-- 
2.36.1.dirty

[PATCH v4 11/16] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary. This also
leaves the custom initcall effectively doing nothing but register
the driver, which no longer needs to happen early either, so convert
it to builtin_platform_driver().

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/ipmmu-vmsa.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index 1d42084d0276..3b30c0752274 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1090,11 +1090,6 @@ static int ipmmu_probe(struct platform_device *pdev)
 		ret = iommu_device_register(&mmu->iommu, &ipmmu_ops, &pdev->dev);
 		if (ret)
 			return ret;
-
-#if defined(CONFIG_IOMMU_DMA)
-		if (!iommu_present(&platform_bus_type))
-			bus_set_iommu(&platform_bus_type, &ipmmu_ops);
-#endif
 	}
 
 	/*
@@ -1168,32 +1163,4 @@ static struct platform_driver ipmmu_driver = {
 	.probe = ipmmu_probe,
 	.remove	= ipmmu_remove,
 };
-
-static int __init ipmmu_init(void)
-{
-	struct device_node *np;
-	static bool setup_done;
-	int ret;
-
-	if (setup_done)
-		return 0;
-
-	np = of_find_matching_node(NULL, ipmmu_of_ids);
-	if (!np)
-		return 0;
-
-	of_node_put(np);
-
-	ret = platform_driver_register(&ipmmu_driver);
-	if (ret < 0)
-		return ret;
-
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
-	if (!iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
-#endif
-
-	setup_done = true;
-	return 0;
-}
-subsys_initcall(ipmmu_init);
+builtin_platform_driver(ipmmu_driver);
-- 
2.36.1.dirty

[PATCH v4 12/16] iommu/mtk: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure paths accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/mtk_iommu.c    | 24 +-----------------------
 drivers/iommu/mtk_iommu_v1.c | 13 +------------
 2 files changed, 2 insertions(+), 35 deletions(-)

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 7e363b1f24df..552e4eb8c610 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1243,30 +1243,13 @@ static int mtk_iommu_probe(struct platform_device *pdev)
 		data->hw_list = &data->hw_list_head;
 	}
 
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);
-		if (ret)
-			goto out_list_del;
-	}
-
 	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, match);
 		if (ret)
-			goto out_bus_set_null;
-	} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
-		   MTK_IOMMU_HAS_FLAG(data->plat_data, IFA_IOMMU_PCIE_SUPPORT)) {
-#ifdef CONFIG_PCI
-		if (!iommu_present(&pci_bus_type)) {
-			ret = bus_set_iommu(&pci_bus_type, &mtk_iommu_ops);
-			if (ret) /* PCIe fail don't affect platform_bus. */
-				goto out_list_del;
-		}
-#endif
+			goto out_list_del;
 	}
 	return ret;
 
-out_bus_set_null:
-	bus_set_iommu(&platform_bus_type, NULL);
 out_list_del:
 	list_del(&data->list);
 	iommu_device_unregister(&data->iommu);
@@ -1294,11 +1277,6 @@ static int mtk_iommu_remove(struct platform_device *pdev)
 	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
 		device_link_remove(data->smicomm_dev, &pdev->dev);
 		component_master_del(&pdev->dev, &mtk_iommu_com_ops);
-	} else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
-		   MTK_IOMMU_HAS_FLAG(data->plat_data, IFA_IOMMU_PCIE_SUPPORT)) {
-#ifdef CONFIG_PCI
-		bus_set_iommu(&pci_bus_type, NULL);
-#endif
 	}
 	pm_runtime_disable(&pdev->dev);
 	for (i = 0; i < data->plat_data->banks_num; i++) {
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 128c7a3f1778..6e0e65831eb7 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -691,19 +691,11 @@ static int mtk_iommu_v1_probe(struct platform_device *pdev)
 	if (ret)
 		goto out_sysfs_remove;
 
-	if (!iommu_present(&platform_bus_type)) {
-		ret = bus_set_iommu(&platform_bus_type,  &mtk_iommu_v1_ops);
-		if (ret)
-			goto out_dev_unreg;
-	}
-
 	ret = component_master_add_with_match(dev, &mtk_iommu_v1_com_ops, match);
 	if (ret)
-		goto out_bus_set_null;
+		goto out_dev_unreg;
 	return ret;
 
-out_bus_set_null:
-	bus_set_iommu(&platform_bus_type, NULL);
 out_dev_unreg:
 	iommu_device_unregister(&data->iommu);
 out_sysfs_remove:
@@ -718,9 +710,6 @@ static int mtk_iommu_v1_remove(struct platform_device *pdev)
 	iommu_device_sysfs_remove(&data->iommu);
 	iommu_device_unregister(&data->iommu);
 
-	if (iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, NULL);
-
 	clk_disable_unprepare(data->bclk);
 	devm_free_irq(&pdev->dev, data->irq, data);
 	component_master_del(&pdev->dev, &mtk_iommu_v1_com_ops);
-- 
2.36.1.dirty

[PATCH v4 13/16] iommu/omap: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the init failure path accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/omap-iommu.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d9cf2820c02e..07ee2600113c 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1776,14 +1776,8 @@ static int __init omap_iommu_init(void)
 		goto fail_driver;
 	}
 
-	ret = bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
-	if (ret)
-		goto fail_bus;
-
 	return 0;
 
-fail_bus:
-	platform_driver_unregister(&omap_iommu_driver);
 fail_driver:
 	kmem_cache_destroy(iopte_cachep);
 	return ret;
-- 
2.36.1.dirty

[PATCH v4 14/16] iommu/tegra-smmu: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/tegra-smmu.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 2a8de975fe63..5b1af40221ec 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1083,8 +1083,8 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 
 	/*
 	 * This is a bit of a hack. Ideally we'd want to simply return this
-	 * value. However the IOMMU registration process will attempt to add
-	 * all devices to the IOMMU when bus_set_iommu() is called. In order
+	 * value. However iommu_device_register() will attempt to add
+	 * all devices to the IOMMU before we get that far. In order
 	 * not to rely on global variables to track the IOMMU instance, we
 	 * set it here so that it can be looked up from the .probe_device()
 	 * callback via the IOMMU device's .drvdata field.
@@ -1138,32 +1138,15 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 		return ERR_PTR(err);
 
 	err = iommu_device_register(&smmu->iommu, &tegra_smmu_ops, dev);
-	if (err)
-		goto remove_sysfs;
-
-	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		goto unregister;
-
-#ifdef CONFIG_PCI
-	err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
-	if (err < 0)
-		goto unset_platform_bus;
-#endif
+	if (err) {
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return ERR_PTR(err);
+	}
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
 
 	return smmu;
-
-unset_platform_bus: __maybe_unused;
-	bus_set_iommu(&platform_bus_type, NULL);
-unregister:
-	iommu_device_unregister(&smmu->iommu);
-remove_sysfs:
-	iommu_device_sysfs_remove(&smmu->iommu);
-
-	return ERR_PTR(err);
 }
 
 void tegra_smmu_remove(struct tegra_smmu *smmu)
-- 
2.36.1.dirty

[PATCH v4 15/16] iommu/virtio: Clean up bus_set_iommu()

[Robin Murphy]

Stop calling bus_set_iommu() since it's now unnecessary, and simplify
the probe failure path accordingly.

Reviewed-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/virtio-iommu.c | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 08eeafc9529f..31ab9d622b67 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -7,7 +7,6 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/amba/bus.h>
 #include <linux/delay.h>
 #include <linux/dma-iommu.h>
 #include <linux/dma-map-ops.h>
@@ -17,7 +16,6 @@
 #include <linux/module.h>
 #include <linux/of_platform.h>
 #include <linux/pci.h>
-#include <linux/platform_device.h>
 #include <linux/virtio.h>
 #include <linux/virtio_config.h>
 #include <linux/virtio_ids.h>
@@ -1145,26 +1143,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	iommu_device_register(&viommu->iommu, &viommu_ops, parent_dev);
 
-#ifdef CONFIG_PCI
-	if (pci_bus_type.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-#endif
-#ifdef CONFIG_ARM_AMBA
-	if (amba_bustype.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&amba_bustype, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-#endif
-	if (platform_bus_type.iommu_ops != &viommu_ops) {
-		ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
-		if (ret)
-			goto err_unregister;
-	}
-
 	vdev->priv = viommu;
 
 	dev_info(dev, "input address: %u bits\n",
@@ -1173,9 +1151,6 @@ static int viommu_probe(struct virtio_device *vdev)
 
 	return 0;
 
-err_unregister:
-	iommu_device_sysfs_remove(&viommu->iommu);
-	iommu_device_unregister(&viommu->iommu);
 err_free_vqs:
 	vdev->config->del_vqs(vdev);
 
-- 
2.36.1.dirty

[PATCH v4 16/16] iommu: Clean up bus_set_iommu()

[Robin Murphy]

Clean up the remaining trivial bus_set_iommu() callsites along
with the implementation. Now drivers only have to know and care
about iommu_device instances, phew!

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v4: No change

 drivers/iommu/arm/arm-smmu/qcom_iommu.c |  4 ----
 drivers/iommu/fsl_pamu_domain.c         |  4 ----
 drivers/iommu/intel/iommu.c             |  2 --
 drivers/iommu/iommu.c                   | 24 ------------------------
 drivers/iommu/msm_iommu.c               |  2 --
 drivers/iommu/rockchip-iommu.c          |  2 --
 drivers/iommu/s390-iommu.c              |  6 ------
 drivers/iommu/sprd-iommu.c              |  5 -----
 drivers/iommu/sun50i-iommu.c            |  2 --
 include/linux/iommu.h                   |  1 -
 10 files changed, 52 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 17235116d3bb..a6aae2e20417 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -837,8 +837,6 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 		goto err_pm_disable;
 	}
 
-	bus_set_iommu(&platform_bus_type, &qcom_iommu_ops);
-
 	if (qcom_iommu->local_base) {
 		pm_runtime_get_sync(dev);
 		writel_relaxed(0xffffffff, qcom_iommu->local_base + SMMU_INTR_SEL_NS);
@@ -856,8 +854,6 @@ static int qcom_iommu_device_remove(struct platform_device *pdev)
 {
 	struct qcom_iommu_dev *qcom_iommu = platform_get_drvdata(pdev);
 
-	bus_set_iommu(&platform_bus_type, NULL);
-
 	pm_runtime_force_suspend(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&qcom_iommu->iommu);
diff --git a/drivers/iommu/fsl_pamu_domain.c b/drivers/iommu/fsl_pamu_domain.c
index 011f9ab7f743..e4a6883228fd 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -476,11 +476,7 @@ int __init pamu_domain_init(void)
 	if (ret) {
 		iommu_device_sysfs_remove(&pamu_iommu);
 		pr_err("Can't register iommu device\n");
-		return ret;
 	}
 
-	bus_set_iommu(&platform_bus_type, &fsl_pamu_ops);
-	bus_set_iommu(&pci_bus_type, &fsl_pamu_ops);
-
 	return ret;
 }
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 65c340e2003c..85b09c34dc12 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3890,7 +3890,6 @@ static int __init probe_acpi_namespace_devices(void)
 					continue;
 				}
 
-				pn->dev->bus->iommu_ops = &intel_iommu_ops;
 				ret = iommu_probe_device(pn->dev);
 				if (ret)
 					break;
@@ -4023,7 +4022,6 @@ int __init intel_iommu_init(void)
 	}
 	up_read(&dmar_global_lock);
 
-	bus_set_iommu(&pci_bus_type, &intel_iommu_ops);
 	if (si_domain && !hw_pass_through)
 		register_memory_notifier(&intel_iommu_memory_nb);
 
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4db0874a5ed6..daa4c7f17677 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1827,30 +1827,6 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
-/**
- * bus_set_iommu - set iommu-callbacks for the bus
- * @bus: bus.
- * @ops: the callbacks provided by the iommu-driver
- *
- * This function is called by an iommu driver to set the iommu methods
- * used for a particular bus. Drivers for devices on that bus can use
- * the iommu-api after these ops are registered.
- * This special function is needed because IOMMUs are usually devices on
- * the bus itself, so the iommu drivers are not initialized when the bus
- * is set up. With this function the iommu-driver can set the iommu-ops
- * afterwards.
- */
-int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
-{
-	if (bus->iommu_ops && ops && bus->iommu_ops != ops)
-		return -EBUSY;
-
-	bus->iommu_ops = ops;
-
-	return 0;
-}
-EXPORT_SYMBOL_GPL(bus_set_iommu);
-
 bool iommu_present(struct bus_type *bus)
 {
 	return bus->iommu_ops != NULL;
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 6a24aa804ea3..16179a9a7283 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -792,8 +792,6 @@ static int msm_iommu_probe(struct platform_device *pdev)
 		goto fail;
 	}
 
-	bus_set_iommu(&platform_bus_type, &msm_iommu_ops);
-
 	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
 		iommu->base, iommu->irq, iommu->ncb);
 
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index ab57c4b8fade..a3fc59b814ab 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1300,8 +1300,6 @@ static int rk_iommu_probe(struct platform_device *pdev)
 	if (!dma_dev)
 		dma_dev = &pdev->dev;
 
-	bus_set_iommu(&platform_bus_type, &rk_iommu_ops);
-
 	pm_runtime_enable(dev);
 
 	for (i = 0; i < iommu->num_irq; i++) {
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 8158a4ed0c60..762f892b4ec3 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -390,9 +390,3 @@ static const struct iommu_ops s390_iommu_ops = {
 		.free		= s390_domain_free,
 	}
 };
-
-static int __init s390_iommu_init(void)
-{
-	return bus_set_iommu(&pci_bus_type, &s390_iommu_ops);
-}
-subsys_initcall(s390_iommu_init);
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
index 511959c8a14d..fadd2c907222 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -496,9 +496,6 @@ static int sprd_iommu_probe(struct platform_device *pdev)
 	if (ret)
 		goto remove_sysfs;
 
-	if (!iommu_present(&platform_bus_type))
-		bus_set_iommu(&platform_bus_type, &sprd_iommu_ops);
-
 	ret = sprd_iommu_clk_enable(sdev);
 	if (ret)
 		goto unregister_iommu;
@@ -534,8 +531,6 @@ static int sprd_iommu_remove(struct platform_device *pdev)
 	iommu_group_put(sdev->group);
 	sdev->group = NULL;
 
-	bus_set_iommu(&platform_bus_type, NULL);
-
 	platform_set_drvdata(pdev, NULL);
 	iommu_device_sysfs_remove(&sdev->iommu);
 	iommu_device_unregister(&sdev->iommu);
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index a84c63518773..cd9b74ee24de 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -965,8 +965,6 @@ static int sun50i_iommu_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_unregister;
 
-	bus_set_iommu(&platform_bus_type, &sun50i_iommu_ops);
-
 	return 0;
 
 err_unregister:
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ea30f00dc145..79b6fb3c6b84 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -416,7 +416,6 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
-extern int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops);
 extern int bus_iommu_probe(struct bus_type *bus);
 extern bool iommu_present(struct bus_type *bus);
 extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
-- 
2.36.1.dirty

Re: [PATCH v4 11/16] iommu/ipmmu-vmsa: Clean up bus_set_iommu()

[kernel test robot]

Hi Robin,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v6.0-rc1]
[also build test WARNING on linus/master next-20220815]
[cannot apply to joro-iommu/next krzk/for-next tegra/for-next rockchip/for-next sunxi/sunxi/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommu-retire-bus_set_iommu/20220816-002348
base:    568035b01cfb107af8d2e4bd2fb9aea22cf5b868
config: x86_64-buildonly-randconfig-r003-20220815 (https://download.01.org/0day-ci/archive/20220816/202208160803.PS5lvtSV-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/72fa5405b9cf32a9814ca7ed6d5b877cabf83efd
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Robin-Murphy/iommu-retire-bus_set_iommu/20220816-002348
        git checkout 72fa5405b9cf32a9814ca7ed6d5b877cabf83efd
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iommu/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/iommu/ipmmu-vmsa.c:946:34: warning: 'ipmmu_of_ids' defined but not used [-Wunused-const-variable=]
     946 | static const struct of_device_id ipmmu_of_ids[] = {
         |                                  ^~~~~~~~~~~~


vim +/ipmmu_of_ids +946 drivers/iommu/ipmmu-vmsa.c

7a62ced8ebd0e1 Yoshihiro Shimoda       2021-09-07  945  
33f3ac9b511612 Magnus Damm             2017-10-16 @946  static const struct of_device_id ipmmu_of_ids[] = {
33f3ac9b511612 Magnus Damm             2017-10-16  947  	{
33f3ac9b511612 Magnus Damm             2017-10-16  948  		.compatible = "renesas,ipmmu-vmsa",
33f3ac9b511612 Magnus Damm             2017-10-16  949  		.data = &ipmmu_features_default,
60fb0083c9d43b Fabrizio Castro         2018-08-23  950  	}, {
60fb0083c9d43b Fabrizio Castro         2018-08-23  951  		.compatible = "renesas,ipmmu-r8a774a1",
60fb0083c9d43b Fabrizio Castro         2018-08-23  952  		.data = &ipmmu_features_rcar_gen3,
757f26a3a9ec2c Biju Das                2019-09-27  953  	}, {
757f26a3a9ec2c Biju Das                2019-09-27  954  		.compatible = "renesas,ipmmu-r8a774b1",
757f26a3a9ec2c Biju Das                2019-09-27  955  		.data = &ipmmu_features_rcar_gen3,
b6d39cd82241bf Fabrizio Castro         2018-12-13  956  	}, {
b6d39cd82241bf Fabrizio Castro         2018-12-13  957  		.compatible = "renesas,ipmmu-r8a774c0",
b6d39cd82241bf Fabrizio Castro         2018-12-13  958  		.data = &ipmmu_features_rcar_gen3,
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14  959  	}, {
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14  960  		.compatible = "renesas,ipmmu-r8a774e1",
4b2aa7a6f9b793 Marian-Cristian Rotariu 2020-07-14  961  		.data = &ipmmu_features_rcar_gen3,
58b8e8bf409236 Magnus Damm             2017-10-16  962  	}, {
58b8e8bf409236 Magnus Damm             2017-10-16  963  		.compatible = "renesas,ipmmu-r8a7795",
0b8ac1409641e1 Magnus Damm             2018-06-14  964  		.data = &ipmmu_features_rcar_gen3,
0b8ac1409641e1 Magnus Damm             2018-06-14  965  	}, {
0b8ac1409641e1 Magnus Damm             2018-06-14  966  		.compatible = "renesas,ipmmu-r8a7796",
0b8ac1409641e1 Magnus Damm             2018-06-14  967  		.data = &ipmmu_features_rcar_gen3,
17fe1618163980 Yoshihiro Shimoda       2020-06-11  968  	}, {
17fe1618163980 Yoshihiro Shimoda       2020-06-11  969  		.compatible = "renesas,ipmmu-r8a77961",
17fe1618163980 Yoshihiro Shimoda       2020-06-11  970  		.data = &ipmmu_features_rcar_gen3,
98dbffd39a6513 Jacopo Mondi            2018-06-14  971  	}, {
98dbffd39a6513 Jacopo Mondi            2018-06-14  972  		.compatible = "renesas,ipmmu-r8a77965",
98dbffd39a6513 Jacopo Mondi            2018-06-14  973  		.data = &ipmmu_features_rcar_gen3,
3701c123e1c13c Simon Horman            2018-06-14  974  	}, {
3701c123e1c13c Simon Horman            2018-06-14  975  		.compatible = "renesas,ipmmu-r8a77970",
3701c123e1c13c Simon Horman            2018-06-14  976  		.data = &ipmmu_features_rcar_gen3,
1cdeb52e5c245b Nikita Yushchenko       2021-09-23  977  	}, {
1cdeb52e5c245b Nikita Yushchenko       2021-09-23  978  		.compatible = "renesas,ipmmu-r8a77980",
1cdeb52e5c245b Nikita Yushchenko       2021-09-23  979  		.data = &ipmmu_features_rcar_gen3,
b0c32912150565 Hai Nguyen Pham         2018-10-17  980  	}, {
b0c32912150565 Hai Nguyen Pham         2018-10-17  981  		.compatible = "renesas,ipmmu-r8a77990",
b0c32912150565 Hai Nguyen Pham         2018-10-17  982  		.data = &ipmmu_features_rcar_gen3,
3701c123e1c13c Simon Horman            2018-06-14  983  	}, {
3701c123e1c13c Simon Horman            2018-06-14  984  		.compatible = "renesas,ipmmu-r8a77995",
3701c123e1c13c Simon Horman            2018-06-14  985  		.data = &ipmmu_features_rcar_gen3,
7a62ced8ebd0e1 Yoshihiro Shimoda       2021-09-07  986  	}, {
7a62ced8ebd0e1 Yoshihiro Shimoda       2021-09-07  987  		.compatible = "renesas,ipmmu-r8a779a0",
ae684caf465b7d Yoshihiro Shimoda       2022-02-08  988  		.data = &ipmmu_features_rcar_gen4,
ae684caf465b7d Yoshihiro Shimoda       2022-02-08  989  	}, {
9f7d09fe23a011 Yoshihiro Shimoda       2022-06-17  990  		.compatible = "renesas,rcar-gen4-ipmmu-vmsa",
ae684caf465b7d Yoshihiro Shimoda       2022-02-08  991  		.data = &ipmmu_features_rcar_gen4,
33f3ac9b511612 Magnus Damm             2017-10-16  992  	}, {
33f3ac9b511612 Magnus Damm             2017-10-16  993  		/* Terminator */
33f3ac9b511612 Magnus Damm             2017-10-16  994  	},
33f3ac9b511612 Magnus Damm             2017-10-16  995  };
33f3ac9b511612 Magnus Damm             2017-10-16  996  

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

RE: [PATCH v4 04/16] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Tuesday, August 16, 2022 12:20 AM
> 
> The number of bus types that the IOMMU subsystem deals with is small and
> manageable, so pull that list into core code as a first step towards
> cleaning up all the boilerplate bus-awareness from drivers. Calling
> iommu_probe_device() before bus->iommu_ops is set will simply return
> -ENODEV and not break the notifier call chain, so there should be no
> harm in proactively registering all our bus notifiers at init time.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v4 00/16] iommu: retire bus_set_iommu()

[Joerg Roedel]

On Mon, Aug 15, 2022 at 05:20:01PM +0100, Robin Murphy wrote:
> Matthew Rosato (1):
>   iommu/s390: Fail probe for non-PCI devices
> 
> Robin Murphy (15):
>   iommu/vt-d: Handle race between registration and device probe
>   iommu/amd: Handle race between registration and device probe
>   iommu: Always register bus notifiers
>   iommu: Move bus setup to IOMMU device registration
>   iommu/amd: Clean up bus_set_iommu()
>   iommu/arm-smmu: Clean up bus_set_iommu()
>   iommu/arm-smmu-v3: Clean up bus_set_iommu()
>   iommu/dart: Clean up bus_set_iommu()
>   iommu/exynos: Clean up bus_set_iommu()
>   iommu/ipmmu-vmsa: Clean up bus_set_iommu()
>   iommu/mtk: Clean up bus_set_iommu()
>   iommu/omap: Clean up bus_set_iommu()
>   iommu/tegra-smmu: Clean up bus_set_iommu()
>   iommu/virtio: Clean up bus_set_iommu()
>   iommu: Clean up bus_set_iommu()

Applied, thanks Robin.

Re: [PATCH v4 04/16] iommu: Always register bus notifiers

[Saravana Kannan]

On Mon, Aug 15, 2022 at 9:20 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> The number of bus types that the IOMMU subsystem deals with is small and
> manageable, so pull that list into core code as a first step towards
> cleaning up all the boilerplate bus-awareness from drivers. Calling
> iommu_probe_device() before bus->iommu_ops is set will simply return
> -ENODEV and not break the notifier call chain, so there should be no
> harm in proactively registering all our bus notifiers at init time.
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>
> v4: Squash iommu_bus_init() entirely, for maximum simplicity. Ignoring
>     the return from bus_register_notifier() is common, so hopefully it's
>     all pretty self-explanatory now.
>
>  drivers/iommu/iommu.c | 72 ++++++++++++++++++++++---------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 780fb7071577..a8d14f2a1035 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -6,6 +6,7 @@
>
>  #define pr_fmt(fmt)    "iommu: " fmt
>
> +#include <linux/amba/bus.h>
>  #include <linux/device.h>
>  #include <linux/dma-iommu.h>
>  #include <linux/kernel.h>
> @@ -16,11 +17,13 @@
>  #include <linux/export.h>
>  #include <linux/slab.h>
>  #include <linux/errno.h>
> +#include <linux/host1x_context_bus.h>
>  #include <linux/iommu.h>
>  #include <linux/idr.h>
>  #include <linux/err.h>
>  #include <linux/pci.h>
>  #include <linux/bitops.h>
> +#include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/fsl/mc.h>
>  #include <linux/module.h>
> @@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {
>  #define IOMMU_CMD_LINE_DMA_API         BIT(0)
>  #define IOMMU_CMD_LINE_STRICT          BIT(1)
>
> +static int iommu_bus_notifier(struct notifier_block *nb,
> +                             unsigned long action, void *data);
>  static int iommu_alloc_default_domain(struct iommu_group *group,
>                                       struct device *dev);
>  static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> @@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name =            \
>  static LIST_HEAD(iommu_device_list);
>  static DEFINE_SPINLOCK(iommu_device_lock);
>
> +static struct bus_type * const iommu_buses[] = {
> +       &platform_bus_type,
> +#ifdef CONFIG_PCI
> +       &pci_bus_type,
> +#endif
> +#ifdef CONFIG_ARM_AMBA
> +       &amba_bustype,
> +#endif
> +#ifdef CONFIG_FSL_MC_BUS
> +       &fsl_mc_bus_type,
> +#endif
> +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS
> +       &host1x_context_device_bus_type,
> +#endif
> +};
> +
>  /*
>   * Use a function instead of an array here because the domain-type is a
>   * bit-field, so an array would waste memory.
> @@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)
>
>  static int __init iommu_subsys_init(void)
>  {
> +       struct notifier_block *nb;
> +
>         if (!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API)) {
>                 if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
>                         iommu_set_default_passthrough(false);
> @@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void)
>                         (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>                                 "(set via kernel command line)" : "");
>
> +       nb = kcalloc(ARRAY_SIZE(iommu_buses), sizeof(*nb), GFP_KERNEL);
> +       if (!nb)
> +               return -ENOMEM;
> +
> +       for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +               nb[i].notifier_call = iommu_bus_notifier;
> +               bus_register_notifier(iommu_buses[i], &nb[i]);
> +       }
> +

Carrying on the community's general disdain for notifiers, can we drop
the bus notifier and just call iommu_probe_device() directly from
device_add()? That way, you also won't need to keep an ifdef-ed array
of buses and it'll be easy to tell from driver core code that iommu
stuff is happening as devices are added. And I'd probably move that
call to be AFTER some of the fw_devlink stuff is done too.

Also, would it be possible to delay this until probe (sorry, don't
have enough of IOMMU details in my head) and call iommu_probe_device()
from really_probe() like we do to set up pinctrl, etc? That'd be
ideal.

-Saravana


>         return 0;
>  }
>  subsys_initcall(iommu_subsys_init);
> @@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)
>         return ret;
>  }
>
> -static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
> -{
> -       struct notifier_block *nb;
> -       int err;
> -
> -       nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> -       if (!nb)
> -               return -ENOMEM;
> -
> -       nb->notifier_call = iommu_bus_notifier;
> -
> -       err = bus_register_notifier(bus, nb);
> -       if (err)
> -               goto out_free;
> -
> -       err = bus_iommu_probe(bus);
> -       if (err)
> -               goto out_err;
> -
> -
> -       return 0;
> -
> -out_err:
> -       /* Clean up */
> -       bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
> -       bus_unregister_notifier(bus, nb);
> -
> -out_free:
> -       kfree(nb);
> -
> -       return err;
> -}
> -
>  /**
>   * bus_set_iommu - set iommu-callbacks for the bus
>   * @bus: bus.
> @@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
>         bus->iommu_ops = ops;
>
>         /* Do IOMMU specific setup for this bus-type */
> -       err = iommu_bus_init(bus, ops);
> -       if (err)
> +       err = bus_iommu_probe(bus);
> +       if (err) {
> +               /* Clean up */
> +               bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
>                 bus->iommu_ops = NULL;
> +       }
>
>         return err;
>  }
> --
> 2.36.1.dirty
>
>

Re: [PATCH v4 04/16] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-09-07 19:50, Saravana Kannan wrote:
> On Mon, Aug 15, 2022 at 9:20 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> The number of bus types that the IOMMU subsystem deals with is small and
>> manageable, so pull that list into core code as a first step towards
>> cleaning up all the boilerplate bus-awareness from drivers. Calling
>> iommu_probe_device() before bus->iommu_ops is set will simply return
>> -ENODEV and not break the notifier call chain, so there should be no
>> harm in proactively registering all our bus notifiers at init time.
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
>> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v4: Squash iommu_bus_init() entirely, for maximum simplicity. Ignoring
>>      the return from bus_register_notifier() is common, so hopefully it's
>>      all pretty self-explanatory now.
>>
>>   drivers/iommu/iommu.c | 72 ++++++++++++++++++++++---------------------
>>   1 file changed, 37 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 780fb7071577..a8d14f2a1035 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -6,6 +6,7 @@
>>
>>   #define pr_fmt(fmt)    "iommu: " fmt
>>
>> +#include <linux/amba/bus.h>
>>   #include <linux/device.h>
>>   #include <linux/dma-iommu.h>
>>   #include <linux/kernel.h>
>> @@ -16,11 +17,13 @@
>>   #include <linux/export.h>
>>   #include <linux/slab.h>
>>   #include <linux/errno.h>
>> +#include <linux/host1x_context_bus.h>
>>   #include <linux/iommu.h>
>>   #include <linux/idr.h>
>>   #include <linux/err.h>
>>   #include <linux/pci.h>
>>   #include <linux/bitops.h>
>> +#include <linux/platform_device.h>
>>   #include <linux/property.h>
>>   #include <linux/fsl/mc.h>
>>   #include <linux/module.h>
>> @@ -75,6 +78,8 @@ static const char * const iommu_group_resv_type_string[] = {
>>   #define IOMMU_CMD_LINE_DMA_API         BIT(0)
>>   #define IOMMU_CMD_LINE_STRICT          BIT(1)
>>
>> +static int iommu_bus_notifier(struct notifier_block *nb,
>> +                             unsigned long action, void *data);
>>   static int iommu_alloc_default_domain(struct iommu_group *group,
>>                                        struct device *dev);
>>   static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>> @@ -103,6 +108,22 @@ struct iommu_group_attribute iommu_group_attr_##_name =            \
>>   static LIST_HEAD(iommu_device_list);
>>   static DEFINE_SPINLOCK(iommu_device_lock);
>>
>> +static struct bus_type * const iommu_buses[] = {
>> +       &platform_bus_type,
>> +#ifdef CONFIG_PCI
>> +       &pci_bus_type,
>> +#endif
>> +#ifdef CONFIG_ARM_AMBA
>> +       &amba_bustype,
>> +#endif
>> +#ifdef CONFIG_FSL_MC_BUS
>> +       &fsl_mc_bus_type,
>> +#endif
>> +#ifdef CONFIG_TEGRA_HOST1X_CONTEXT_BUS
>> +       &host1x_context_device_bus_type,
>> +#endif
>> +};
>> +
>>   /*
>>    * Use a function instead of an array here because the domain-type is a
>>    * bit-field, so an array would waste memory.
>> @@ -126,6 +147,8 @@ static const char *iommu_domain_type_str(unsigned int t)
>>
>>   static int __init iommu_subsys_init(void)
>>   {
>> +       struct notifier_block *nb;
>> +
>>          if (!(iommu_cmd_line & IOMMU_CMD_LINE_DMA_API)) {
>>                  if (IS_ENABLED(CONFIG_IOMMU_DEFAULT_PASSTHROUGH))
>>                          iommu_set_default_passthrough(false);
>> @@ -152,6 +175,15 @@ static int __init iommu_subsys_init(void)
>>                          (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>                                  "(set via kernel command line)" : "");
>>
>> +       nb = kcalloc(ARRAY_SIZE(iommu_buses), sizeof(*nb), GFP_KERNEL);
>> +       if (!nb)
>> +               return -ENOMEM;
>> +
>> +       for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +               nb[i].notifier_call = iommu_bus_notifier;
>> +               bus_register_notifier(iommu_buses[i], &nb[i]);
>> +       }
>> +
> 
> Carrying on the community's general disdain for notifiers, can we drop
> the bus notifier and just call iommu_probe_device() directly from
> device_add()? That way, you also won't need to keep an ifdef-ed array
> of buses and it'll be easy to tell from driver core code that iommu
> stuff is happening as devices are added. And I'd probably move that
> call to be AFTER some of the fw_devlink stuff is done too.

Yup, we're working in that general direction, this is just big and moves 
slow :)

One of the next steps after this is unpicking 
{of,acpi}_iommu_configure() for iommu_probe_device() to take ownership 
of firmware parsing, after which it should be sufficiently 
self-contained that the notifier might indeed become the worst 
impediment to further reasoning. We'll still want to limit things to the 
small subset of buses where IOMMUs are at all relevant (one of the 
reasons that I stopped short of trying to grovel the whole list of 
registered buses out of the driver core here), but since there's a 
bus-specific element involved in parsing the firmware bindings, I'm 
expecting it to work out quite neatly in the end.

> Also, would it be possible to delay this until probe (sorry, don't
> have enough of IOMMU details in my head) and call iommu_probe_device()
> from really_probe() like we do to set up pinctrl, etc? That'd be
> ideal.

No, that's not so good, and in fact the stuff that can currently happen 
at driver probe time is already problematic and responsible for various 
subtle breakage. There are things about the IOMMU topology that need to 
be known regardless of whether drivers exist for all the devices, so now 
that fw_devlink can take care of waiting for IOMMU drivers to load, the 
rest of IOMMU setup (other than DMA ops) really does want to move back 
to device_add() time.

Thanks,
Robin.

> 
> -Saravana
> 
> 
>>          return 0;
>>   }
>>   subsys_initcall(iommu_subsys_init);
>> @@ -1775,39 +1807,6 @@ int bus_iommu_probe(struct bus_type *bus)
>>          return ret;
>>   }
>>
>> -static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
>> -{
>> -       struct notifier_block *nb;
>> -       int err;
>> -
>> -       nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
>> -       if (!nb)
>> -               return -ENOMEM;
>> -
>> -       nb->notifier_call = iommu_bus_notifier;
>> -
>> -       err = bus_register_notifier(bus, nb);
>> -       if (err)
>> -               goto out_free;
>> -
>> -       err = bus_iommu_probe(bus);
>> -       if (err)
>> -               goto out_err;
>> -
>> -
>> -       return 0;
>> -
>> -out_err:
>> -       /* Clean up */
>> -       bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
>> -       bus_unregister_notifier(bus, nb);
>> -
>> -out_free:
>> -       kfree(nb);
>> -
>> -       return err;
>> -}
>> -
>>   /**
>>    * bus_set_iommu - set iommu-callbacks for the bus
>>    * @bus: bus.
>> @@ -1836,9 +1835,12 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
>>          bus->iommu_ops = ops;
>>
>>          /* Do IOMMU specific setup for this bus-type */
>> -       err = iommu_bus_init(bus, ops);
>> -       if (err)
>> +       err = bus_iommu_probe(bus);
>> +       if (err) {
>> +               /* Clean up */
>> +               bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
>>                  bus->iommu_ops = NULL;
>> +       }
>>
>>          return err;
>>   }
>> --
>> 2.36.1.dirty
>>
>>

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Jon Hunter]

Hi Robin,

On 15/08/2022 17:20, Robin Murphy wrote:
> Move the bus setup to iommu_device_register(). This should allow
> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
> 
> At this point we can also handle cleanup better than just rolling back
> the most-recently-touched bus upon failure - which may release devices
> owned by other already-registered instances, and still leave devices on
> other buses with dangling pointers to the failed instance. Now it's easy
> to clean up the exact footprint of a given instance, no more, no less.


Since this change, I have noticed that the DRM driver on Tegra20 is 
failing to probe and I am seeing ...

  tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
  drm drm: failed to initialize 54140000.gr2d: -19

Bisect points to this change and reverting it fixes it. Let me know if 
you have any thoughts.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-10-06 15:01, Jon Hunter wrote:
> Hi Robin,
> 
> On 15/08/2022 17:20, Robin Murphy wrote:
>> Move the bus setup to iommu_device_register(). This should allow
>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>
>> At this point we can also handle cleanup better than just rolling back
>> the most-recently-touched bus upon failure - which may release devices
>> owned by other already-registered instances, and still leave devices on
>> other buses with dangling pointers to the failed instance. Now it's easy
>> to clean up the exact footprint of a given instance, no more, no less.
> 
> 
> Since this change, I have noticed that the DRM driver on Tegra20 is 
> failing to probe and I am seeing ...
> 
>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>   drm drm: failed to initialize 54140000.gr2d: -19
> 
> Bisect points to this change and reverting it fixes it. Let me know if 
> you have any thoughts.

Oh, apparently what's happened is that I've inadvertently enabled the 
tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu() 
before. Looking at the history, it appears to have been that way since 
c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so 
essentially that driver has been broken and useless for close to 8 years 
now :(

Given that, I'd be inclined to "fix" it as below, or just give up and 
delete the whole thing.

Thanks,
Robin.

----->8-----
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 5c5cb5bee8b6..7b3f7fd6e527 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
  	bool "Tegra GART IOMMU Support"
  	depends on ARCH_TEGRA_2x_SOC
  	depends on TEGRA_MC
+	depends on BROKEN
  	select IOMMU_API
  	help
  	  Enables support for remapping discontiguous physical memory

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2611 bytes --]

On Thu, Oct 06, 2022 at 04:27:39PM +0100, Robin Murphy wrote:
> On 2022-10-06 15:01, Jon Hunter wrote:
> > Hi Robin,
> > 
> > On 15/08/2022 17:20, Robin Murphy wrote:
> > > Move the bus setup to iommu_device_register(). This should allow
> > > bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
> > > and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
> > > 
> > > At this point we can also handle cleanup better than just rolling back
> > > the most-recently-touched bus upon failure - which may release devices
> > > owned by other already-registered instances, and still leave devices on
> > > other buses with dangling pointers to the failed instance. Now it's easy
> > > to clean up the exact footprint of a given instance, no more, no less.
> > 
> > 
> > Since this change, I have noticed that the DRM driver on Tegra20 is
> > failing to probe and I am seeing ...
> > 
> >   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
> >   drm drm: failed to initialize 54140000.gr2d: -19
> > 
> > Bisect points to this change and reverting it fixes it. Let me know if
> > you have any thoughts.
> 
> Oh, apparently what's happened is that I've inadvertently enabled the
> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu()
> before. Looking at the history, it appears to have been that way since
> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so essentially
> that driver has been broken and useless for close to 8 years now :(
> 
> Given that, I'd be inclined to "fix" it as below, or just give up and delete
> the whole thing.

I'm inclined to agree. GART is severely limited: it provides a single
IOMMU domain with an aperture of 32 MiB. It's close to useless for
anything we would want to do and my understanding is that people have
been falling back to CMA for any graphics/display stuff that the GART
would've been useful for.

Given that nobody's felt the urge to fix this for the past 8 years, I
don't think there's enough interest in this to keep it going.

Dmitry, any thoughts?

Thierry

> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 5c5cb5bee8b6..7b3f7fd6e527 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>  	bool "Tegra GART IOMMU Support"
>  	depends on ARCH_TEGRA_2x_SOC
>  	depends on TEGRA_MC
> +	depends on BROKEN
>  	select IOMMU_API
>  	help
>  	  Enables support for remapping discontiguous physical memory

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Dmitry Osipenko]

On 10/6/22 20:12, Thierry Reding wrote:
> On Thu, Oct 06, 2022 at 04:27:39PM +0100, Robin Murphy wrote:
>> On 2022-10-06 15:01, Jon Hunter wrote:
>>> Hi Robin,
>>>
>>> On 15/08/2022 17:20, Robin Murphy wrote:
>>>> Move the bus setup to iommu_device_register(). This should allow
>>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>>>
>>>> At this point we can also handle cleanup better than just rolling back
>>>> the most-recently-touched bus upon failure - which may release devices
>>>> owned by other already-registered instances, and still leave devices on
>>>> other buses with dangling pointers to the failed instance. Now it's easy
>>>> to clean up the exact footprint of a given instance, no more, no less.
>>>
>>>
>>> Since this change, I have noticed that the DRM driver on Tegra20 is
>>> failing to probe and I am seeing ...
>>>
>>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>>   drm drm: failed to initialize 54140000.gr2d: -19

The upstream Tegra20 device-tree doesn't have IOMMU phandle for
54140000.gr2d. In this case IOMMU domain shouldn't be available for the
DRM driver [1]. Sounds like IOMMU core has a bug.

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/iommu/tegra-gart.c#L243

>>> Bisect points to this change and reverting it fixes it. Let me know if
>>> you have any thoughts.
>>
>> Oh, apparently what's happened is that I've inadvertently enabled the
>> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu()
>> before. Looking at the history, it appears to have been that way since
>> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so essentially
>> that driver has been broken and useless for close to 8 years now :(
>>
>> Given that, I'd be inclined to "fix" it as below, or just give up and delete
>> the whole thing.
> 
> I'm inclined to agree. GART is severely limited: it provides a single
> IOMMU domain with an aperture of 32 MiB. It's close to useless for
> anything we would want to do and my understanding is that people have
> been falling back to CMA for any graphics/display stuff that the GART
> would've been useful for.
> 
> Given that nobody's felt the urge to fix this for the past 8 years, I
> don't think there's enough interest in this to keep it going.
> 
> Dmitry, any thoughts?

This GART driver is used by a community kernel fork that has alternative
DRM driver supporting IOMMU/GART on Tegra20. The fork is periodically
synced with the latest upstream, it's used by postmarketOS. Hence it
wasn't a completely dead driver.

The 32M aperture works well for 2d/3d engines because it fits multiple
textures at once. Tegra DRM driver needs to remap buffers dynamically,
but this is easy to implement because DRM core has nice helpers for
that. We haven't got to the point where upstream DRM driver is ready to
support this feature.

CMA is hard to use for anything other than display framebuffers. It's
slow and fails to allocate memory if CMA area is "shared" due to
fragmentation and pinned pages. Reserved CMA isn't an option for GPU
because then there is no memory for the rest of system.

I don't see any problems with removing GART driver. It's not going to be
used soon in upstream and only adds maintenance burden. We can always
re-add it in the future.

-- 
Best regards,
Dmitry

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Alex Williamson]

On Mon, 15 Aug 2022 17:20:06 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> Move the bus setup to iommu_device_register(). This should allow
> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
> 
> At this point we can also handle cleanup better than just rolling back
> the most-recently-touched bus upon failure - which may release devices
> owned by other already-registered instances, and still leave devices on
> other buses with dangling pointers to the failed instance. Now it's easy
> to clean up the exact footprint of a given instance, no more, no less.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Reviewed-By: Krishna Reddy <vdumpa@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Tested-by: Matthew Rosato <mjrosato@linux.ibm.com> # s390
> Tested-by: Niklas Schnelle <schnelle@linux.ibm.com> # s390
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v4: Factor out the ops check in iommu_device_register() to keep the loop
> even simpler, and comment the nominal change in behaviour
> 
>  drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)

This introduces the below lockdep spat regression, bisected to commit:

57365a04c921 ("iommu: Move bus setup to IOMMU device registration")

This can be reproduced with simple vfio-pci device assignment to a VM
on x86_64 with VT-d.  Thanks,

Alex

======================================================
WARNING: possible circular locking dependency detected
6.0.0-rc4+ #127 Tainted: G            E     
------------------------------------------------------
qemu-system-x86/1726 is trying to acquire lock:
ffffffffacf8a7d0 (dmar_global_lock){++++}-{3:3}, at: intel_iommu_get_resv_regions+0x21/0x2a0

but task is already holding lock:
ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&group->mutex){+.+.}-{3:3}:
       __mutex_lock+0x6d/0x8c0
       iommu_group_add_device+0xfb/0x330
       __iommu_probe_device+0x150/0x270
       probe_iommu_group+0x31/0x50
       bus_for_each_dev+0x67/0xa0
       bus_iommu_probe+0x38/0x2a0
       iommu_device_register+0xc1/0x130
       intel_iommu_init+0xfd9/0x120d
       pci_iommu_init+0xe/0x36
       do_one_initcall+0x5b/0x310
       kernel_init_freeable+0x275/0x2c1
       kernel_init+0x16/0x140
       ret_from_fork+0x22/0x30

-> #0 (dmar_global_lock){++++}-{3:3}:
       __lock_acquire+0x10dc/0x1da0
       lock_acquire+0xc2/0x2d0
       down_read+0x2d/0x40
       intel_iommu_get_resv_regions+0x21/0x2a0
       iommu_get_group_resv_regions+0x88/0x3b0
       vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1]
       vfio_fops_unl_ioctl+0x19d/0x270 [vfio]
       __x64_sys_ioctl+0x8b/0xc0
       do_syscall_64+0x3b/0x90
       entry_SYSCALL_64_after_hwframe+0x63/0xcd

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&group->mutex);
                               lock(dmar_global_lock);
                               lock(&group->mutex);
  lock(dmar_global_lock);

 *** DEADLOCK ***

4 locks held by qemu-system-x86/1726:
 #0: ffff9811b5546c88 (&container->group_lock){++++}-{3:3}, at: vfio_fops_unl_ioctl+0xbb/0x270 [vfio]
 #1: ffffffffc058c720 (&vfio.iommu_drivers_lock){+.+.}-{3:3}, at: vfio_fops_unl_ioctl+0xeb/0x270 [vfio]
 #2: ffff9811d865ba88 (&iommu->lock#2){+.+.}-{3:3}, at: vfio_iommu_type1_attach_group+0x51/0xce1 [vfio_iommu_type1]
 #3: ffff981240efb0c0 (&group->mutex){+.+.}-{3:3}, at: iommu_get_group_resv_regions+0x2c/0x3b0

stack backtrace:
CPU: 0 PID: 1726 Comm: qemu-system-x86 Tainted: G            E      6.0.0-rc4+ #127
Hardware name: System manufacturer System Product Name/P8H67-M PRO, BIOS 3904 04/27/2013
Call Trace:
 <TASK>
 dump_stack_lvl+0x56/0x73
 check_noncircular+0xd6/0x100
 ? __lock_acquire+0x374/0x1da0
 __lock_acquire+0x10dc/0x1da0
 lock_acquire+0xc2/0x2d0
 ? intel_iommu_get_resv_regions+0x21/0x2a0
 ? trace_contention_end+0x2d/0xd0
 ? __mutex_lock+0xdf/0x8c0
 ? iommu_get_group_resv_regions+0x2c/0x3b0
 ? lock_is_held_type+0xe2/0x140
 down_read+0x2d/0x40
 ? intel_iommu_get_resv_regions+0x21/0x2a0
 intel_iommu_get_resv_regions+0x21/0x2a0
 iommu_get_group_resv_regions+0x88/0x3b0
 ? iommu_attach_group+0x76/0xa0
 vfio_iommu_type1_attach_group+0x19d/0xce1 [vfio_iommu_type1]
 ? rcu_read_lock_sched_held+0x43/0x70
 ? __module_address.part.0+0x2b/0xa0
 ? is_module_address+0x43/0x70
 ? __init_waitqueue_head+0x4a/0x60
 vfio_fops_unl_ioctl+0x19d/0x270 [vfio]
 __x64_sys_ioctl+0x8b/0xc0
 do_syscall_64+0x3b/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f803853a17b
Code: 0f 1e fa 48 8b 05 1d ad 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d ed ac 0c 00 f7 d8 64 89 01 48
RSP: 002b:00007ffd8128c2e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000020 RCX: 00007f803853a17b
RDX: 0000000000000003 RSI: 0000000000003b66 RDI: 000000000000001c
RBP: 00007ffd8128c320 R08: 000055f59d8ff8d0 R09: 00007f8038605a40
R10: 0000000000000008 R11: 0000000000000246 R12: 000055f599aed1d0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
 </TASK>

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

Hi Alex,

On 2022/10/13 0:28, Alex Williamson wrote:
> On Mon, 15 Aug 2022 17:20:06 +0100
> Robin Murphy<robin.murphy@arm.com>  wrote:
> 
>> Move the bus setup to iommu_device_register(). This should allow
>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>
>> At this point we can also handle cleanup better than just rolling back
>> the most-recently-touched bus upon failure - which may release devices
>> owned by other already-registered instances, and still leave devices on
>> other buses with dangling pointers to the failed instance. Now it's easy
>> to clean up the exact footprint of a given instance, no more, no less.
>>
>> Tested-by: Marek Szyprowski<m.szyprowski@samsung.com>
>> Reviewed-By: Krishna Reddy<vdumpa@nvidia.com>
>> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
>> Tested-by: Matthew Rosato<mjrosato@linux.ibm.com>  # s390
>> Tested-by: Niklas Schnelle<schnelle@linux.ibm.com>  # s390
>> Signed-off-by: Robin Murphy<robin.murphy@arm.com>
>> ---
>>
>> v4: Factor out the ops check in iommu_device_register() to keep the loop
>> even simpler, and comment the nominal change in behaviour
>>
>>   drivers/iommu/iommu.c | 55 +++++++++++++++++++++++--------------------
>>   1 file changed, 30 insertions(+), 25 deletions(-)
> This introduces the below lockdep spat regression, bisected to commit:
> 
> 57365a04c921 ("iommu: Move bus setup to IOMMU device registration")
> 
> This can be reproduced with simple vfio-pci device assignment to a VM
> on x86_64 with VT-d.  Thanks,

Thank you for reporting this. I have proposed below fix:

https://lore.kernel.org/all/20220927053109.4053662-1-baolu.lu@linux.intel.com/

Does it work for you?

Best regards,
baolu

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Jon Hunter]

On 06/10/2022 16:27, Robin Murphy wrote:
> On 2022-10-06 15:01, Jon Hunter wrote:
>> Hi Robin,
>>
>> On 15/08/2022 17:20, Robin Murphy wrote:
>>> Move the bus setup to iommu_device_register(). This should allow
>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU instances,
>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>>
>>> At this point we can also handle cleanup better than just rolling back
>>> the most-recently-touched bus upon failure - which may release devices
>>> owned by other already-registered instances, and still leave devices on
>>> other buses with dangling pointers to the failed instance. Now it's easy
>>> to clean up the exact footprint of a given instance, no more, no less.
>>
>>
>> Since this change, I have noticed that the DRM driver on Tegra20 is 
>> failing to probe and I am seeing ...
>>
>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>   drm drm: failed to initialize 54140000.gr2d: -19
>>
>> Bisect points to this change and reverting it fixes it. Let me know if 
>> you have any thoughts.
> 
> Oh, apparently what's happened is that I've inadvertently enabled the 
> tegra-gart driver, since it seems that *wasn't* calling bus_set_iommu() 
> before. Looking at the history, it appears to have been that way since 
> c7e3ca515e78 ("iommu/tegra: gart: Do not register with bus"), so 
> essentially that driver has been broken and useless for close to 8 years 
> now :(
> 
> Given that, I'd be inclined to "fix" it as below, or just give up and 
> delete the whole thing.
> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 5c5cb5bee8b6..7b3f7fd6e527 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>       bool "Tegra GART IOMMU Support"
>       depends on ARCH_TEGRA_2x_SOC
>       depends on TEGRA_MC
> +    depends on BROKEN
>       select IOMMU_API
>       help
>         Enables support for remapping discontiguous physical memory


Thanks Robin. This works for me.

Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?

Jon

-- 
nvpublic

Re: [PATCH v4 05/16] iommu: Move bus setup to IOMMU device registration

[Dmitry Osipenko]

18.10.2022 09:13, Jon Hunter пишет:
> 
> On 06/10/2022 16:27, Robin Murphy wrote:
>> On 2022-10-06 15:01, Jon Hunter wrote:
>>> Hi Robin,
>>>
>>> On 15/08/2022 17:20, Robin Murphy wrote:
>>>> Move the bus setup to iommu_device_register(). This should allow
>>>> bus_iommu_probe() to be correctly replayed for multiple IOMMU
>>>> instances,
>>>> and leaves bus_set_iommu() as a glorified no-op to be cleaned up next.
>>>>
>>>> At this point we can also handle cleanup better than just rolling back
>>>> the most-recently-touched bus upon failure - which may release devices
>>>> owned by other already-registered instances, and still leave devices on
>>>> other buses with dangling pointers to the failed instance. Now it's
>>>> easy
>>>> to clean up the exact footprint of a given instance, no more, no less.
>>>
>>>
>>> Since this change, I have noticed that the DRM driver on Tegra20 is
>>> failing to probe and I am seeing ...
>>>
>>>   tegra-gr2d 54140000.gr2d: failed to attach to domain: -19
>>>   drm drm: failed to initialize 54140000.gr2d: -19
>>>
>>> Bisect points to this change and reverting it fixes it. Let me know
>>> if you have any thoughts.
>>
>> Oh, apparently what's happened is that I've inadvertently enabled the
>> tegra-gart driver, since it seems that *wasn't* calling
>> bus_set_iommu() before. Looking at the history, it appears to have
>> been that way since c7e3ca515e78 ("iommu/tegra: gart: Do not register
>> with bus"), so essentially that driver has been broken and useless for
>> close to 8 years now :(
>>
>> Given that, I'd be inclined to "fix" it as below, or just give up and
>> delete the whole thing.
>>
>> Thanks,
>> Robin.
>>
>> ----->8-----
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 5c5cb5bee8b6..7b3f7fd6e527 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -230,6 +230,7 @@ config TEGRA_IOMMU_GART
>>       bool "Tegra GART IOMMU Support"
>>       depends on ARCH_TEGRA_2x_SOC
>>       depends on TEGRA_MC
>> +    depends on BROKEN
>>       select IOMMU_API
>>       help
>>         Enables support for remapping discontiguous physical memory
> 
> 
> Thanks Robin. This works for me.
> 
> Thierry, Dmitry, we need a fix for v6.1 and so OK with the above?

To me it is more a problem of the DRM driver that it doesn't support
GART. GART will require a special handling from the DRM driver anyways [1].

[1]
https://github.com/grate-driver/linux/blob/master/drivers/gpu/drm/grate/drm.c#L460

The GART driver itself isn't broken, it's working perfectly fine. It's
the DRM driver that should start caring about the GART presence, IMO.