[PATCH v3 00/15] iommu: Retire bus_set_iommu()

[PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Robin Murphy]

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

Hi all,

Here's v3, now with working x86! Having finally made sense of how I
broke Intel, I've given AMD the same fix by inspection. I'm still not
100% sure about s390, but it looks like it should probably be OK since
it seems to register an IOMMU instance for each PCI device (?!) before
disappearing into PCI hotplug code, wherein I assume we should never see
a PCI device appear without its IOMMU already registered.

Otherwise, the only other updates are hooking up the new host1x context
bus (noting that it now takes all of 4 lines to support a whole new bus,
yay!), and a slight tweak to make sure we keep rejecting registration of
conflicting iommu_ops rather than needlessly change that just yet.

Thanks,
Robin.


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                       | 117 +++++++++-----------
 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                  |   6 -
 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, 75 insertions(+), 415 deletions(-)

-- 
2.36.1.dirty

[PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Robin Murphy]

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

Hi all,

Here's v3, now with working x86! Having finally made sense of how I
broke Intel, I've given AMD the same fix by inspection. I'm still not
100% sure about s390, but it looks like it should probably be OK since
it seems to register an IOMMU instance for each PCI device (?!) before
disappearing into PCI hotplug code, wherein I assume we should never see
a PCI device appear without its IOMMU already registered.

Otherwise, the only other updates are hooking up the new host1x context
bus (noting that it now takes all of 4 lines to support a whole new bus,
yay!), and a slight tweak to make sure we keep rejecting registration of
conflicting iommu_ops rather than needlessly change that just yet.

Thanks,
Robin.


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                       | 117 +++++++++-----------
 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                  |   6 -
 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, 75 insertions(+), 415 deletions(-)

-- 
2.36.1.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 01/15] 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, so I'm leaving it here for someone with
more confidence to tackle a larger rework of the locking.

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

v3: New

 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 44016594831d..3e02c08802a0 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4600,7 +4600,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 v3 01/15] 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, so I'm leaving it here for someone with
more confidence to tackle a larger rework of the locking.

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

v3: New

 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 44016594831d..3e02c08802a0 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4600,7 +4600,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 02/15] 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>
---

v3: New

 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 840831d5d2ad..2f8e8f818a6c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1757,6 +1757,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 	devid = get_device_id(dev);
 	iommu = amd_iommu_rlookup_table[devid];
 
+	/* 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 v3 02/15] 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>
---

v3: New

 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 840831d5d2ad..2f8e8f818a6c 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1757,6 +1757,10 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 	devid = get_device_id(dev);
 	iommu = amd_iommu_rlookup_table[devid];
 
+	/* Not registered yet? */
+	if (!iommu->iommu.ops)
+		return ERR_PTR(-ENODEV);
+
 	if (dev_iommu_priv_get(dev))
 		return &iommu->iommu;
 
-- 
2.36.1.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 03/15] 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>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: Add host1x_context_bus

 drivers/iommu/iommu.c | 53 ++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..514edc0eaa94 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,7 @@ 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_init(struct bus_type *bus);
 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 +107,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.
@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
 			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
 				"(set via kernel command line)" : "");
 
+	/* If the system is so broken that this fails, it will WARN anyway */
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+		iommu_bus_init(iommu_buses[i]);
+
 	return 0;
 }
 subsys_initcall(iommu_subsys_init);
@@ -1772,35 +1796,19 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
+static int iommu_bus_init(struct bus_type *bus)
 {
 	struct notifier_block *nb;
 	int err;
 
-	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
+	nb = kzalloc(sizeof(*nb), 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);
+		kfree(nb);
 
 	return err;
 }
@@ -1833,9 +1841,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 v3 03/15] 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>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: Add host1x_context_bus

 drivers/iommu/iommu.c | 53 ++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 847ad47a2dfd..514edc0eaa94 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,7 @@ 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_init(struct bus_type *bus);
 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 +107,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.
@@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
 			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
 				"(set via kernel command line)" : "");
 
+	/* If the system is so broken that this fails, it will WARN anyway */
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
+		iommu_bus_init(iommu_buses[i]);
+
 	return 0;
 }
 subsys_initcall(iommu_subsys_init);
@@ -1772,35 +1796,19 @@ int bus_iommu_probe(struct bus_type *bus)
 	return ret;
 }
 
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
+static int iommu_bus_init(struct bus_type *bus)
 {
 	struct notifier_block *nb;
 	int err;
 
-	nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
+	nb = kzalloc(sizeof(*nb), 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);
+		kfree(nb);
 
 	return err;
 }
@@ -1833,9 +1841,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 04/15] 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>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: Have iommu_device_register() return -EBUSY for conflicting ops
    rather than change that behaviour just yet.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 514edc0eaa94..acb7b2ab0b79 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -180,6 +180,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
@@ -202,12 +210,32 @@ 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);
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		struct bus_type *bus = iommu_buses[i];
+		int err;
+
+		if (bus->iommu_ops && bus->iommu_ops != ops) {
+			err = -EBUSY;
+		} else {
+			bus->iommu_ops = ops;
+			err = bus_iommu_probe(bus);
+		}
+		if (err) {
+			iommu_device_unregister(iommu);
+			return err;
+		}
+	}
+
 	return 0;
 }
 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);
@@ -1633,13 +1661,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)
 {
@@ -1828,27 +1849,12 @@ static int iommu_bus_init(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 v3 04/15] 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>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v3: Have iommu_device_register() return -EBUSY for conflicting ops
    rather than change that behaviour just yet.

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

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 514edc0eaa94..acb7b2ab0b79 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -180,6 +180,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
@@ -202,12 +210,32 @@ 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);
+
+	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
+		struct bus_type *bus = iommu_buses[i];
+		int err;
+
+		if (bus->iommu_ops && bus->iommu_ops != ops) {
+			err = -EBUSY;
+		} else {
+			bus->iommu_ops = ops;
+			err = bus_iommu_probe(bus);
+		}
+		if (err) {
+			iommu_device_unregister(iommu);
+			return err;
+		}
+	}
+
 	return 0;
 }
 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);
@@ -1633,13 +1661,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)
 {
@@ -1828,27 +1849,12 @@ static int iommu_bus_init(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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 05/15] 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>
---

v3: 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 1ab31074f5b3..384393ce57fb 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);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 1d08f87e734b..9bd63511173f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1969,20 +1969,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;
-	}
-
 	init_device_table_dma();
 
 	for_each_iommu(iommu)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f8e8f818a6c..e67141bb255f 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>
@@ -1842,25 +1840,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 v3 05/15] 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>
---

v3: 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 1ab31074f5b3..384393ce57fb 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);
 
 #ifdef CONFIG_AMD_IOMMU_DEBUGFS
 void amd_iommu_debugfs_setup(struct amd_iommu *iommu);
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 1d08f87e734b..9bd63511173f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -1969,20 +1969,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;
-	}
-
 	init_device_table_dma();
 
 	for_each_iommu(iommu)
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 2f8e8f818a6c..e67141bb255f 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>
@@ -1842,25 +1840,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 06/15] 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>
---

v3: 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 2ed3594f384e..83c9f2144541 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)
@@ -2025,52 +2008,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 int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -2187,7 +2124,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);
@@ -2205,24 +2143,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)
@@ -2235,7 +2156,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 v3 06/15] 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>
---

v3: 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 2ed3594f384e..83c9f2144541 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)
@@ -2025,52 +2008,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 int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -2187,7 +2124,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);
@@ -2205,24 +2143,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)
@@ -2235,7 +2156,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 07/15] 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>
---

v3: 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 88817a3376ef..8b7936095bec 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"
 
@@ -3698,43 +3696,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)
 {
@@ -3840,27 +3801,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 v3 07/15] 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>
---

v3: 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 88817a3376ef..8b7936095bec 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"
 
@@ -3698,43 +3696,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)
 {
@@ -3840,27 +3801,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 08/15] 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>
---

v3: 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 8af0242a90d9..d7379e12d623 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -824,27 +824,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;
@@ -899,14 +878,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)
@@ -920,8 +895,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:
@@ -936,7 +909,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 v3 08/15] 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>
---

v3: 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 8af0242a90d9..d7379e12d623 100644
--- a/drivers/iommu/apple-dart.c
+++ b/drivers/iommu/apple-dart.c
@@ -824,27 +824,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;
@@ -899,14 +878,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)
@@ -920,8 +895,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:
@@ -936,7 +909,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 09/15] 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>
---

v3: 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 71f2018e23fe..359b255b3924 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1356,16 +1356,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 v3 09/15] 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>
---

v3: 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 71f2018e23fe..359b255b3924 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -1356,16 +1356,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 10/15] 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>
---

v3: 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 8fdb84b3642b..2549d32f0ddd 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 v3 10/15] 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>
---

v3: 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 8fdb84b3642b..2549d32f0ddd 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 11/15] 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>
---

v3: Rebase around recent probe/remove changes

 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 bb9dd92c9898..1a0780b65961 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1239,30 +1239,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);
@@ -1290,11 +1273,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 e1cb51b9866c..e60e8890e45d 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -696,19 +696,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:
@@ -723,9 +715,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 v3 11/15] 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>
---

v3: Rebase around recent probe/remove changes

 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 bb9dd92c9898..1a0780b65961 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -1239,30 +1239,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);
@@ -1290,11 +1273,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 e1cb51b9866c..e60e8890e45d 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -696,19 +696,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:
@@ -723,9 +715,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 12/15] 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>
---

v3: 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 v3 12/15] 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>
---

v3: 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 13/15] 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>
---

v3: 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 1fea68e551f1..2e4d2e4c65bb 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1086,8 +1086,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.
@@ -1141,32 +1141,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 v3 13/15] 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>
---

v3: 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 1fea68e551f1..2e4d2e4c65bb 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -1086,8 +1086,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.
@@ -1141,32 +1141,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 14/15] 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>
---

v3: 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 25be4b822aa0..bcbd10ec4ccb 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>
@@ -1146,26 +1144,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",
@@ -1174,9 +1152,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 v3 14/15] 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>
---

v3: 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 25be4b822aa0..bcbd10ec4ccb 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>
@@ -1146,26 +1144,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",
@@ -1174,9 +1152,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 15/15] 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!

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

v3: Also catch Intel's cheeky open-coded assignment

 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 4c077c38fbd6..80af00f468b4 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -845,8 +845,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);
@@ -864,8 +862,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 94b4589dc67c..09aa90c27723 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -481,11 +481,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 3e02c08802a0..388dd0e22fb9 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4031,7 +4031,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;
@@ -4153,7 +4152,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 acb7b2ab0b79..c10ecb87fd9c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1834,30 +1834,6 @@ static int iommu_bus_init(struct bus_type *bus)
 	return err;
 }
 
-/**
- * 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 f09aedfdd462..4a71989406dc 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -797,8 +797,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 c898bcbbce11..dd957145fb81 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -385,9 +385,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 bd409bab6286..6770e6a72283 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -507,9 +507,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;
@@ -545,8 +542,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 c54ab477b8fd..e104543b78d9 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -968,8 +968,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 5e1afe169549..2cb05ff89f03 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -412,7 +412,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

[PATCH v3 15/15] 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!

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

v3: Also catch Intel's cheeky open-coded assignment

 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 4c077c38fbd6..80af00f468b4 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -845,8 +845,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);
@@ -864,8 +862,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 94b4589dc67c..09aa90c27723 100644
--- a/drivers/iommu/fsl_pamu_domain.c
+++ b/drivers/iommu/fsl_pamu_domain.c
@@ -481,11 +481,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 3e02c08802a0..388dd0e22fb9 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4031,7 +4031,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;
@@ -4153,7 +4152,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 acb7b2ab0b79..c10ecb87fd9c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1834,30 +1834,6 @@ static int iommu_bus_init(struct bus_type *bus)
 	return err;
 }
 
-/**
- * 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 f09aedfdd462..4a71989406dc 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -797,8 +797,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 c898bcbbce11..dd957145fb81 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -385,9 +385,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 bd409bab6286..6770e6a72283 100644
--- a/drivers/iommu/sprd-iommu.c
+++ b/drivers/iommu/sprd-iommu.c
@@ -507,9 +507,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;
@@ -545,8 +542,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 c54ab477b8fd..e104543b78d9 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -968,8 +968,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 5e1afe169549..2cb05ff89f03 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -412,7 +412,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.

Thanks for highlighting this. I will look into it later.

Best regards,
baolu

> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v3: New
> 
>   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 44016594831d..3e02c08802a0 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4600,7 +4600,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);

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.

Thanks for highlighting this. I will look into it later.

Best regards,
baolu

> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v3: New
> 
>   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 44016594831d..3e02c08802a0 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4600,7 +4600,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);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
>   /*
>    * Use a function instead of an array here because the domain-type is a
>    * bit-field, so an array would waste memory.
> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>   			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>   				"(set via kernel command line)" : "");
>   
> +	/* If the system is so broken that this fails, it will WARN anyway */

Can you please elaborate a bit on this? iommu_bus_init() still return
errors.

> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
> +		iommu_bus_init(iommu_buses[i]);
> +
>   	return 0;

Best regards,
baolu

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
>   /*
>    * Use a function instead of an array here because the domain-type is a
>    * bit-field, so an array would waste memory.
> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>   			(iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>   				"(set via kernel command line)" : "");
>   
> +	/* If the system is so broken that this fails, it will WARN anyway */

Can you please elaborate a bit on this? iommu_bus_init() still return
errors.

> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
> +		iommu_bus_init(iommu_buses[i]);
> +
>   	return 0;

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> @@ -202,12 +210,32 @@ 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);
> +
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +		struct bus_type *bus = iommu_buses[i];
> +		int err;
> +
> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> +			err = -EBUSY;
> +		} else {
> +			bus->iommu_ops = ops;
> +			err = bus_iommu_probe(bus);
> +		}
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(iommu_device_register);

With bus_set_iommu() retired, my understanding is that now we embrace
the first-come-first-serve policy for bus->iommu_ops setting. This will
lead to problem in different iommu_ops for different bus case. Did I
overlook anything?

Best regards,
baolu

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> @@ -202,12 +210,32 @@ 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);
> +
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +		struct bus_type *bus = iommu_buses[i];
> +		int err;
> +
> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> +			err = -EBUSY;
> +		} else {
> +			bus->iommu_ops = ops;
> +			err = bus_iommu_probe(bus);
> +		}
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(iommu_device_register);

With bus_set_iommu() retired, my understanding is that now we embrace
the first-come-first-serve policy for bus->iommu_ops setting. This will
lead to problem in different iommu_ops for different bus case. Did I
overlook anything?

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-06 02:53, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>>   /*
>>    * Use a function instead of an array here because the domain-type is a
>>    * bit-field, so an array would waste memory.
>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>                   "(set via kernel command line)" : "");
>> +    /* If the system is so broken that this fails, it will WARN 
>> anyway */
> 
> Can you please elaborate a bit on this? iommu_bus_init() still return
> errors.

Indeed, it's commenting on the fact that we don't try to clean up or 
propagate an error value further even if it did ever manage to return 
one. I feared that if I strip the error handling out of iommu_bus_init() 
itself on the same reasoning, we'll just get constant patches from the 
static checker brigade trying to add it back, so it seemed like the 
neatest compromise to keep that decision where it's obviously in an 
early initcall, rather than in the helper function which can be viewed 
out of context. However, I'm happy to either expand this comment or go 
the whole way and make iommu_bus_init() return void if you think it's 
worthwhile.

Cheers,
Robin.

> 
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
>> +        iommu_bus_init(iommu_buses[i]);
>> +
>>       return 0;
> 
> Best regards,
> baolu

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-06 02:53, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>>   /*
>>    * Use a function instead of an array here because the domain-type is a
>>    * bit-field, so an array would waste memory.
>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>                   "(set via kernel command line)" : "");
>> +    /* If the system is so broken that this fails, it will WARN 
>> anyway */
> 
> Can you please elaborate a bit on this? iommu_bus_init() still return
> errors.

Indeed, it's commenting on the fact that we don't try to clean up or 
propagate an error value further even if it did ever manage to return 
one. I feared that if I strip the error handling out of iommu_bus_init() 
itself on the same reasoning, we'll just get constant patches from the 
static checker brigade trying to add it back, so it seemed like the 
neatest compromise to keep that decision where it's obviously in an 
early initcall, rather than in the helper function which can be viewed 
out of context. However, I'm happy to either expand this comment or go 
the whole way and make iommu_bus_init() return void if you think it's 
worthwhile.

Cheers,
Robin.

> 
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
>> +        iommu_bus_init(iommu_buses[i]);
>> +
>>       return 0;
> 
> Best regards,
> baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-07-06 03:35, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>> @@ -202,12 +210,32 @@ 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);
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +        struct bus_type *bus = iommu_buses[i];
>> +        int err;
>> +
>> +        if (bus->iommu_ops && bus->iommu_ops != ops) {
>> +            err = -EBUSY;
>> +        } else {
>> +            bus->iommu_ops = ops;
>> +            err = bus_iommu_probe(bus);
>> +        }
>> +        if (err) {
>> +            iommu_device_unregister(iommu);
>> +            return err;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_device_register);
> 
> With bus_set_iommu() retired, my understanding is that now we embrace
> the first-come-first-serve policy for bus->iommu_ops setting. This will
> lead to problem in different iommu_ops for different bus case. Did I
> overlook anything?

This is just formalising the de-facto situation that we don't actually 
have any combination of drivers that could load on the same system 
without already attempting to claim at least one bus in common. It's 
also only temporary until the bus ops are removed completely and we 
fully support multiple drivers coexisting, which only actually takes a 
handful more patches - I've realised I could even bring that change 
*ahead* of the big job of converting iommu_domain_alloc() (I'm not 
convinced that the tree-wide flag-day patch for that I currently have in 
the dev branch is really viable, nor that I've actually got the correct 
device at some of the callsites), although whether it's worth the 
potentially-surprising behaviour that might result I'm less sure.

If we already had systems where in-tree drivers successfully coexisted 
on different buses then I'd have split this up and done something a bit 
more involved to keep a vestigial bus_set_iommu() around until the final 
bus ops removal, but since we don't, it seemed neatest to do all the 
related work in one go.

Thanks,
Robin.

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-07-06 03:35, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>> @@ -202,12 +210,32 @@ 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);
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +        struct bus_type *bus = iommu_buses[i];
>> +        int err;
>> +
>> +        if (bus->iommu_ops && bus->iommu_ops != ops) {
>> +            err = -EBUSY;
>> +        } else {
>> +            bus->iommu_ops = ops;
>> +            err = bus_iommu_probe(bus);
>> +        }
>> +        if (err) {
>> +            iommu_device_unregister(iommu);
>> +            return err;
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_device_register);
> 
> With bus_set_iommu() retired, my understanding is that now we embrace
> the first-come-first-serve policy for bus->iommu_ops setting. This will
> lead to problem in different iommu_ops for different bus case. Did I
> overlook anything?

This is just formalising the de-facto situation that we don't actually 
have any combination of drivers that could load on the same system 
without already attempting to claim at least one bus in common. It's 
also only temporary until the bus ops are removed completely and we 
fully support multiple drivers coexisting, which only actually takes a 
handful more patches - I've realised I could even bring that change 
*ahead* of the big job of converting iommu_domain_alloc() (I'm not 
convinced that the tree-wide flag-day patch for that I currently have in 
the dev branch is really viable, nor that I've actually got the correct 
device at some of the callsites), although whether it's worth the 
potentially-surprising behaviour that might result I'm less sure.

If we already had systems where in-tree drivers successfully coexisted 
on different buses then I'd have split this up and done something a bit 
more involved to keep a vestigial bus_set_iommu() around until the final 
bus ops removal, but since we don't, it seemed neatest to do all the 
related work in one go.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Baolu Lu]

On 2022/7/6 21:43, Robin Murphy wrote:
> On 2022-07-06 02:53, Baolu Lu wrote:
>> On 2022/7/6 01:08, Robin Murphy wrote:
>>>   /*
>>>    * Use a function instead of an array here because the domain-type 
>>> is a
>>>    * bit-field, so an array would waste memory.
>>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>>                   "(set via kernel command line)" : "");
>>> +    /* If the system is so broken that this fails, it will WARN 
>>> anyway */
>>
>> Can you please elaborate a bit on this? iommu_bus_init() still return
>> errors.
> 
> Indeed, it's commenting on the fact that we don't try to clean up or 
> propagate an error value further even if it did ever manage to return 
> one. I feared that if I strip the error handling out of iommu_bus_init() 
> itself on the same reasoning, we'll just get constant patches from the 
> static checker brigade trying to add it back, so it seemed like the 
> neatest compromise to keep that decision where it's obviously in an 
> early initcall, rather than in the helper function which can be viewed 
> out of context. However, I'm happy to either expand this comment or go 
> the whole way and make iommu_bus_init() return void if you think it's 
> worthwhile.

Thanks for the explanation. It would be helpful if the comment could be
expanded. In this case, after a long time, people will not consider it
an oversight. :-)

Best regards,
baolu

> 
> Cheers,
> Robin.
> 
>>
>>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
>>> +        iommu_bus_init(iommu_buses[i]);
>>> +
>>>       return 0;
>>
>> Best regards,
>> baolu
> 

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Baolu Lu]

On 2022/7/6 21:43, Robin Murphy wrote:
> On 2022-07-06 02:53, Baolu Lu wrote:
>> On 2022/7/6 01:08, Robin Murphy wrote:
>>>   /*
>>>    * Use a function instead of an array here because the domain-type 
>>> is a
>>>    * bit-field, so an array would waste memory.
>>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>>                   "(set via kernel command line)" : "");
>>> +    /* If the system is so broken that this fails, it will WARN 
>>> anyway */
>>
>> Can you please elaborate a bit on this? iommu_bus_init() still return
>> errors.
> 
> Indeed, it's commenting on the fact that we don't try to clean up or 
> propagate an error value further even if it did ever manage to return 
> one. I feared that if I strip the error handling out of iommu_bus_init() 
> itself on the same reasoning, we'll just get constant patches from the 
> static checker brigade trying to add it back, so it seemed like the 
> neatest compromise to keep that decision where it's obviously in an 
> early initcall, rather than in the helper function which can be viewed 
> out of context. However, I'm happy to either expand this comment or go 
> the whole way and make iommu_bus_init() return void if you think it's 
> worthwhile.

Thanks for the explanation. It would be helpful if the comment could be
expanded. In this case, after a long time, people will not consider it
an oversight. :-)

Best regards,
baolu

> 
> Cheers,
> Robin.
> 
>>
>>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++)
>>> +        iommu_bus_init(iommu_buses[i]);
>>> +
>>>       return 0;
>>
>> Best regards,
>> baolu
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

On 2022/7/6 22:37, Robin Murphy wrote:
> On 2022-07-06 03:35, Baolu Lu wrote:
>> On 2022/7/6 01:08, Robin Murphy wrote:
>>> @@ -202,12 +210,32 @@ 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);
>>> +
>>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>>> +        struct bus_type *bus = iommu_buses[i];
>>> +        int err;
>>> +
>>> +        if (bus->iommu_ops && bus->iommu_ops != ops) {
>>> +            err = -EBUSY;
>>> +        } else {
>>> +            bus->iommu_ops = ops;
>>> +            err = bus_iommu_probe(bus);
>>> +        }
>>> +        if (err) {
>>> +            iommu_device_unregister(iommu);
>>> +            return err;
>>> +        }
>>> +    }
>>> +
>>>       return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(iommu_device_register);
>>
>> With bus_set_iommu() retired, my understanding is that now we embrace
>> the first-come-first-serve policy for bus->iommu_ops setting. This will
>> lead to problem in different iommu_ops for different bus case. Did I
>> overlook anything?
> 
> This is just formalising the de-facto situation that we don't actually 
> have any combination of drivers that could load on the same system 
> without already attempting to claim at least one bus in common. It's 
> also only temporary until the bus ops are removed completely and we 
> fully support multiple drivers coexisting, which only actually takes a 
> handful more patches - I've realised I could even bring that change 
> *ahead* of the big job of converting iommu_domain_alloc() (I'm not 
> convinced that the tree-wide flag-day patch for that I currently have in 
> the dev branch is really viable, nor that I've actually got the correct 
> device at some of the callsites), although whether it's worth the 
> potentially-surprising behaviour that might result I'm less sure.
> 
> If we already had systems where in-tree drivers successfully coexisted 
> on different buses then I'd have split this up and done something a bit 
> more involved to keep a vestigial bus_set_iommu() around until the final 
> bus ops removal, but since we don't, it seemed neatest to do all the 
> related work in one go.

Fair enough. I've never seen a mixed system as far. It's fine for us to
retire bus_set_iommu() for now and then formally support mixed IOMMU
drivers later.

Best regards,
baolu

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Baolu Lu]

On 2022/7/6 22:37, Robin Murphy wrote:
> On 2022-07-06 03:35, Baolu Lu wrote:
>> On 2022/7/6 01:08, Robin Murphy wrote:
>>> @@ -202,12 +210,32 @@ 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);
>>> +
>>> +    for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>>> +        struct bus_type *bus = iommu_buses[i];
>>> +        int err;
>>> +
>>> +        if (bus->iommu_ops && bus->iommu_ops != ops) {
>>> +            err = -EBUSY;
>>> +        } else {
>>> +            bus->iommu_ops = ops;
>>> +            err = bus_iommu_probe(bus);
>>> +        }
>>> +        if (err) {
>>> +            iommu_device_unregister(iommu);
>>> +            return err;
>>> +        }
>>> +    }
>>> +
>>>       return 0;
>>>   }
>>>   EXPORT_SYMBOL_GPL(iommu_device_register);
>>
>> With bus_set_iommu() retired, my understanding is that now we embrace
>> the first-come-first-serve policy for bus->iommu_ops setting. This will
>> lead to problem in different iommu_ops for different bus case. Did I
>> overlook anything?
> 
> This is just formalising the de-facto situation that we don't actually 
> have any combination of drivers that could load on the same system 
> without already attempting to claim at least one bus in common. It's 
> also only temporary until the bus ops are removed completely and we 
> fully support multiple drivers coexisting, which only actually takes a 
> handful more patches - I've realised I could even bring that change 
> *ahead* of the big job of converting iommu_domain_alloc() (I'm not 
> convinced that the tree-wide flag-day patch for that I currently have in 
> the dev branch is really viable, nor that I've actually got the correct 
> device at some of the callsites), although whether it's worth the 
> potentially-surprising behaviour that might result I'm less sure.
> 
> If we already had systems where in-tree drivers successfully coexisted 
> on different buses then I'd have split this up and done something a bit 
> more involved to keep a vestigial bus_set_iommu() around until the final 
> bus ops removal, but since we don't, it seemed neatest to do all the 
> related work in one go.

Fair enough. I've never seen a mixed system as far. It's fine for us to
retire bus_set_iommu() for now and then formally support mixed IOMMU
drivers later.

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 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.
> 

Suppose we miss a check on iommu ops in iommu_release_device():

	if (!dev->iommu) <<<<<<<
		return;

	iommu_device_unlink(dev->iommu->iommu_dev, dev);

	ops = dev_iommu_ops(dev);
	ops->release_device(dev);

following the rationale in patch01 a device could be removed when
it's associated with a known but not registered instance.

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 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.
> 

Suppose we miss a check on iommu ops in iommu_release_device():

	if (!dev->iommu) <<<<<<<
		return;

	iommu_device_unlink(dev->iommu->iommu_dev, dev);

	ops = dev_iommu_ops(dev);
	ops->release_device(dev);

following the rationale in patch01 a device could be removed when
it's associated with a known but not registered instance.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 7, 2022 8:21 AM
> 
> On 2022/7/6 21:43, Robin Murphy wrote:
> > On 2022-07-06 02:53, Baolu Lu wrote:
> >> On 2022/7/6 01:08, Robin Murphy wrote:
> >>>   /*
> >>>    * Use a function instead of an array here because the domain-type
> >>> is a
> >>>    * bit-field, so an array would waste memory.
> >>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
> >>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> >>>                   "(set via kernel command line)" : "");
> >>> +    /* If the system is so broken that this fails, it will WARN
> >>> anyway */
> >>
> >> Can you please elaborate a bit on this? iommu_bus_init() still return
> >> errors.
> >
> > Indeed, it's commenting on the fact that we don't try to clean up or
> > propagate an error value further even if it did ever manage to return
> > one. I feared that if I strip the error handling out of iommu_bus_init()
> > itself on the same reasoning, we'll just get constant patches from the
> > static checker brigade trying to add it back, so it seemed like the
> > neatest compromise to keep that decision where it's obviously in an
> > early initcall, rather than in the helper function which can be viewed
> > out of context. However, I'm happy to either expand this comment or go
> > the whole way and make iommu_bus_init() return void if you think it's
> > worthwhile.
> 
> Thanks for the explanation. It would be helpful if the comment could be
> expanded. In this case, after a long time, people will not consider it
> an oversight. :-)
> 

I'd prefer to making iommu_bus_init() return void plus expanding
the comment otherwise the question arises that if the only caller
is not interested in the return value then why bother returning it
in the first place. 😊

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 7, 2022 8:21 AM
> 
> On 2022/7/6 21:43, Robin Murphy wrote:
> > On 2022-07-06 02:53, Baolu Lu wrote:
> >> On 2022/7/6 01:08, Robin Murphy wrote:
> >>>   /*
> >>>    * Use a function instead of an array here because the domain-type
> >>> is a
> >>>    * bit-field, so an array would waste memory.
> >>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
> >>>               (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
> >>>                   "(set via kernel command line)" : "");
> >>> +    /* If the system is so broken that this fails, it will WARN
> >>> anyway */
> >>
> >> Can you please elaborate a bit on this? iommu_bus_init() still return
> >> errors.
> >
> > Indeed, it's commenting on the fact that we don't try to clean up or
> > propagate an error value further even if it did ever manage to return
> > one. I feared that if I strip the error handling out of iommu_bus_init()
> > itself on the same reasoning, we'll just get constant patches from the
> > static checker brigade trying to add it back, so it seemed like the
> > neatest compromise to keep that decision where it's obviously in an
> > early initcall, rather than in the helper function which can be viewed
> > out of context. However, I'm happy to either expand this comment or go
> > the whole way and make iommu_bus_init() return void if you think it's
> > worthwhile.
> 
> Thanks for the explanation. It would be helpful if the comment could be
> expanded. In this case, after a long time, people will not consider it
> an oversight. :-)
> 

I'd prefer to making iommu_bus_init() return void plus expanding
the comment otherwise the question arises that if the only caller
is not interested in the return value then why bother returning it
in the first place. 😊
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 AM
> 
> @@ -202,12 +210,32 @@ 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);
> +
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +		struct bus_type *bus = iommu_buses[i];
> +		int err;
> +
> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> +			err = -EBUSY;
> +		} else {
> +			bus->iommu_ops = ops;
> +			err = bus_iommu_probe(bus);
> +		}
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +

Probably move above into a new function bus_iommu_probe_all():

	/* probe all buses for devices associated with this iommu */
	err = bus_iommu_probe_all();
	if (err) {
		iommu_device_unregister(iommu);
		return err;
	}

Just  my personal preference on leaving logic in iommu_device_register()
more relevant to the iommu instance itself.

Apart from that:

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

RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 AM
> 
> @@ -202,12 +210,32 @@ 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);
> +
> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> +		struct bus_type *bus = iommu_buses[i];
> +		int err;
> +
> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> +			err = -EBUSY;
> +		} else {
> +			bus->iommu_ops = ops;
> +			err = bus_iommu_probe(bus);
> +		}
> +		if (err) {
> +			iommu_device_unregister(iommu);
> +			return err;
> +		}
> +	}
> +

Probably move above into a new function bus_iommu_probe_all():

	/* probe all buses for devices associated with this iommu */
	err = bus_iommu_probe_all();
	if (err) {
		iommu_device_unregister(iommu);
		return err;
	}

Just  my personal preference on leaving logic in iommu_device_register()
more relevant to the iommu instance itself.

Apart from that:

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 AM
> 
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

RE: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:08 AM
> 
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:09 AM
> 
> 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!
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

RE: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Wednesday, July 6, 2022 1:09 AM
> 
> 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!
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-07 07:34, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 7, 2022 8:21 AM
>>
>> On 2022/7/6 21:43, Robin Murphy wrote:
>>> On 2022-07-06 02:53, Baolu Lu wrote:
>>>> On 2022/7/6 01:08, Robin Murphy wrote:
>>>>>    /*
>>>>>     * Use a function instead of an array here because the domain-type
>>>>> is a
>>>>>     * bit-field, so an array would waste memory.
>>>>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>>>>                (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>>>>                    "(set via kernel command line)" : "");
>>>>> +    /* If the system is so broken that this fails, it will WARN
>>>>> anyway */
>>>>
>>>> Can you please elaborate a bit on this? iommu_bus_init() still return
>>>> errors.
>>>
>>> Indeed, it's commenting on the fact that we don't try to clean up or
>>> propagate an error value further even if it did ever manage to return
>>> one. I feared that if I strip the error handling out of iommu_bus_init()
>>> itself on the same reasoning, we'll just get constant patches from the
>>> static checker brigade trying to add it back, so it seemed like the
>>> neatest compromise to keep that decision where it's obviously in an
>>> early initcall, rather than in the helper function which can be viewed
>>> out of context. However, I'm happy to either expand this comment or go
>>> the whole way and make iommu_bus_init() return void if you think it's
>>> worthwhile.
>>
>> Thanks for the explanation. It would be helpful if the comment could be
>> expanded. In this case, after a long time, people will not consider it
>> an oversight. :-)
>>
> 
> I'd prefer to making iommu_bus_init() return void plus expanding
> the comment otherwise the question arises that if the only caller
> is not interested in the return value then why bother returning it
> in the first place. 😊

OK, that's fair enough, will do.

Thanks,
Robin.

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-07 07:34, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 7, 2022 8:21 AM
>>
>> On 2022/7/6 21:43, Robin Murphy wrote:
>>> On 2022-07-06 02:53, Baolu Lu wrote:
>>>> On 2022/7/6 01:08, Robin Murphy wrote:
>>>>>    /*
>>>>>     * Use a function instead of an array here because the domain-type
>>>>> is a
>>>>>     * bit-field, so an array would waste memory.
>>>>> @@ -152,6 +172,10 @@ static int __init iommu_subsys_init(void)
>>>>>                (iommu_cmd_line & IOMMU_CMD_LINE_STRICT) ?
>>>>>                    "(set via kernel command line)" : "");
>>>>> +    /* If the system is so broken that this fails, it will WARN
>>>>> anyway */
>>>>
>>>> Can you please elaborate a bit on this? iommu_bus_init() still return
>>>> errors.
>>>
>>> Indeed, it's commenting on the fact that we don't try to clean up or
>>> propagate an error value further even if it did ever manage to return
>>> one. I feared that if I strip the error handling out of iommu_bus_init()
>>> itself on the same reasoning, we'll just get constant patches from the
>>> static checker brigade trying to add it back, so it seemed like the
>>> neatest compromise to keep that decision where it's obviously in an
>>> early initcall, rather than in the helper function which can be viewed
>>> out of context. However, I'm happy to either expand this comment or go
>>> the whole way and make iommu_bus_init() return void if you think it's
>>> worthwhile.
>>
>> Thanks for the explanation. It would be helpful if the comment could be
>> expanded. In this case, after a long time, people will not consider it
>> an oversight. :-)
>>
> 
> I'd prefer to making iommu_bus_init() return void plus expanding
> the comment otherwise the question arises that if the only caller
> is not interested in the return value then why bother returning it
> in the first place. 😊

OK, that's fair enough, will do.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-07 07:31, Tian, Kevin wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Sent: Wednesday, July 6, 2022 1:08 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.
>>
> 
> Suppose we miss a check on iommu ops in iommu_release_device():
> 
> 	if (!dev->iommu) <<<<<<<
> 		return;
> 
> 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
> 
> 	ops = dev_iommu_ops(dev);
> 	ops->release_device(dev);
> 
> following the rationale in patch01 a device could be removed when
> it's associated with a known but not registered instance.

No, because at that point the instance is only known internally to the 
driver. As long as it isn't erroneously returned from 
->probe_device(dev), dev->iommu will remain NULL and the rest of the 
core code works as expected.

Thanks,
Robin.

Re: [PATCH v3 03/15] iommu: Always register bus notifiers

[Robin Murphy]

On 2022-07-07 07:31, Tian, Kevin wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Sent: Wednesday, July 6, 2022 1:08 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.
>>
> 
> Suppose we miss a check on iommu ops in iommu_release_device():
> 
> 	if (!dev->iommu) <<<<<<<
> 		return;
> 
> 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
> 
> 	ops = dev_iommu_ops(dev);
> 	ops->release_device(dev);
> 
> following the rationale in patch01 a device could be removed when
> it's associated with a known but not registered instance.

No, because at that point the instance is only known internally to the 
driver. As long as it isn't erroneously returned from 
->probe_device(dev), dev->iommu will remain NULL and the rest of the 
core code works as expected.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-07-07 07:51, Tian, Kevin wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Sent: Wednesday, July 6, 2022 1:08 AM
>>
>> @@ -202,12 +210,32 @@ 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);
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +		struct bus_type *bus = iommu_buses[i];
>> +		int err;
>> +
>> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
>> +			err = -EBUSY;
>> +		} else {
>> +			bus->iommu_ops = ops;
>> +			err = bus_iommu_probe(bus);
>> +		}
>> +		if (err) {
>> +			iommu_device_unregister(iommu);
>> +			return err;
>> +		}
>> +	}
>> +
> 
> Probably move above into a new function bus_iommu_probe_all():
> 
> 	/* probe all buses for devices associated with this iommu */
> 	err = bus_iommu_probe_all();
> 	if (err) {
> 		iommu_device_unregister(iommu);
> 		return err;
> 	}
> 
> Just  my personal preference on leaving logic in iommu_device_register()
> more relevant to the iommu instance itself.

On reflection I think it makes sense to pull the 
iommu_device_unregister() out of the loop anyway - I think that's really 
a left-over from between v1 and v2 when that error case briefly jumped 
to another cleanup loop, before I realised it was actually trivial for 
iommu_device_unregister() to clean up for itself.

However I now see I've also missed another opportunity, and the -EBUSY 
case should be hoisted out of the loop as well, since checking 
iommu_buses[0] is sufficient. Then it's hopefully much clearer that once 
the bus ops go away we'll be left with just a single extra line for the 
loop, as in iommu_device_unregister(). Does that sound reasonable?

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

Thanks! (and for the others as well)

Robin.

Re: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Robin Murphy]

On 2022-07-07 07:51, Tian, Kevin wrote:
>> From: Robin Murphy <robin.murphy@arm.com>
>> Sent: Wednesday, July 6, 2022 1:08 AM
>>
>> @@ -202,12 +210,32 @@ 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);
>> +
>> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
>> +		struct bus_type *bus = iommu_buses[i];
>> +		int err;
>> +
>> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
>> +			err = -EBUSY;
>> +		} else {
>> +			bus->iommu_ops = ops;
>> +			err = bus_iommu_probe(bus);
>> +		}
>> +		if (err) {
>> +			iommu_device_unregister(iommu);
>> +			return err;
>> +		}
>> +	}
>> +
> 
> Probably move above into a new function bus_iommu_probe_all():
> 
> 	/* probe all buses for devices associated with this iommu */
> 	err = bus_iommu_probe_all();
> 	if (err) {
> 		iommu_device_unregister(iommu);
> 		return err;
> 	}
> 
> Just  my personal preference on leaving logic in iommu_device_register()
> more relevant to the iommu instance itself.

On reflection I think it makes sense to pull the 
iommu_device_unregister() out of the loop anyway - I think that's really 
a left-over from between v1 and v2 when that error case briefly jumped 
to another cleanup loop, before I realised it was actually trivial for 
iommu_device_unregister() to clean up for itself.

However I now see I've also missed another opportunity, and the -EBUSY 
case should be hoisted out of the loop as well, since checking 
iommu_buses[0] is sufficient. Then it's hopefully much clearer that once 
the bus ops go away we'll be left with just a single extra line for the 
loop, as in iommu_device_unregister(). Does that sound reasonable?

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

Thanks! (and for the others as well)

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/5/22 1:08 PM, Robin Murphy wrote:
> 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!
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v3: Also catch Intel's cheeky open-coded assignment
> 

...

> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index c898bcbbce11..dd957145fb81 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -385,9 +385,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);

Previously s390_iommu_ops was only being set for pci_bus_type, but with 
this series it will now also be set for platform_bus_type.

To tolerate that, this series needs a change along the lines of:


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

Date: Thu, 7 Jul 2022 08:45:44 -0400 

Subject: [PATCH] iommu/s390: fail probe for non-pci device 

 

s390-iommu only supports pci_bus_type today 

 

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> 

--- 

  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 dd957145fb81..762f892b4ec3 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; 

  }  return &zdev->iommu_dev; 

  }

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/5/22 1:08 PM, Robin Murphy wrote:
> 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!
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v3: Also catch Intel's cheeky open-coded assignment
> 

...

> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index c898bcbbce11..dd957145fb81 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -385,9 +385,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);

Previously s390_iommu_ops was only being set for pci_bus_type, but with 
this series it will now also be set for platform_bus_type.

To tolerate that, this series needs a change along the lines of:


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

Date: Thu, 7 Jul 2022 08:45:44 -0400 

Subject: [PATCH] iommu/s390: fail probe for non-pci device 

 

s390-iommu only supports pci_bus_type today 

 

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> 

--- 

  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 dd957145fb81..762f892b4ec3 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; 

  }  return &zdev->iommu_dev; 

  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/7/22 8:49 AM, Matthew Rosato wrote:
> On 7/5/22 1:08 PM, Robin Murphy wrote:
>> 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!
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v3: Also catch Intel's cheeky open-coded assignment
>>
> 
> ...
> 
>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>> index c898bcbbce11..dd957145fb81 100644
>> --- a/drivers/iommu/s390-iommu.c
>> +++ b/drivers/iommu/s390-iommu.c
>> @@ -385,9 +385,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);
> 
> Previously s390_iommu_ops was only being set for pci_bus_type, but with 
> this series it will now also be set for platform_bus_type.
> 
> To tolerate that, this series needs a change along the lines of:
> 

...  Sorry, let's try that again without a mangled diff:

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

Date: Thu, 7 Jul 2022 08:45:44 -0400 

Subject: [PATCH] iommu/s390: fail probe for non-pci device 

 

s390-iommu only supports pci_bus_type today 

 

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> 

--- 

  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 dd957145fb81..762f892b4ec3 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; 

  }

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/7/22 8:49 AM, Matthew Rosato wrote:
> On 7/5/22 1:08 PM, Robin Murphy wrote:
>> 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!
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v3: Also catch Intel's cheeky open-coded assignment
>>
> 
> ...
> 
>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>> index c898bcbbce11..dd957145fb81 100644
>> --- a/drivers/iommu/s390-iommu.c
>> +++ b/drivers/iommu/s390-iommu.c
>> @@ -385,9 +385,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);
> 
> Previously s390_iommu_ops was only being set for pci_bus_type, but with 
> this series it will now also be set for platform_bus_type.
> 
> To tolerate that, this series needs a change along the lines of:
> 

...  Sorry, let's try that again without a mangled diff:

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

Date: Thu, 7 Jul 2022 08:45:44 -0400 

Subject: [PATCH] iommu/s390: fail probe for non-pci device 

 

s390-iommu only supports pci_bus_type today 

 

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> 

--- 

  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 dd957145fb81..762f892b4ec3 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; 

  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Robin Murphy]

On 2022-07-07 13:54, Matthew Rosato wrote:
> On 7/7/22 8:49 AM, Matthew Rosato wrote:
>> On 7/5/22 1:08 PM, Robin Murphy wrote:
>>> 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!
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>
>>> v3: Also catch Intel's cheeky open-coded assignment
>>>
>>
>> ...
>>
>>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>>> index c898bcbbce11..dd957145fb81 100644
>>> --- a/drivers/iommu/s390-iommu.c
>>> +++ b/drivers/iommu/s390-iommu.c
>>> @@ -385,9 +385,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);
>>
>> Previously s390_iommu_ops was only being set for pci_bus_type, but 
>> with this series it will now also be set for platform_bus_type.

Ah, indeed I hadn't got as far as fully appreciating that to_zpci_dev() 
isn't robust enough on its own. Thanks for the patch, I've pulled it in 
and will include it in v4. Do I take it that all else works OK with this 
fixed?

Cheers,
Robin.

>>
>> To tolerate that, this series needs a change along the lines of:
>>
> 
> ...  Sorry, let's try that again without a mangled diff:
> 
> From: Matthew Rosato <mjrosato@linux.ibm.com>
> Date: Thu, 7 Jul 2022 08:45:44 -0400
> Subject: [PATCH] iommu/s390: fail probe for non-pci device
> 
> 
> s390-iommu only supports pci_bus_type today
> 
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   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 dd957145fb81..762f892b4ec3 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;
>   }
> 

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Robin Murphy]

On 2022-07-07 13:54, Matthew Rosato wrote:
> On 7/7/22 8:49 AM, Matthew Rosato wrote:
>> On 7/5/22 1:08 PM, Robin Murphy wrote:
>>> 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!
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>> ---
>>>
>>> v3: Also catch Intel's cheeky open-coded assignment
>>>
>>
>> ...
>>
>>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>>> index c898bcbbce11..dd957145fb81 100644
>>> --- a/drivers/iommu/s390-iommu.c
>>> +++ b/drivers/iommu/s390-iommu.c
>>> @@ -385,9 +385,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);
>>
>> Previously s390_iommu_ops was only being set for pci_bus_type, but 
>> with this series it will now also be set for platform_bus_type.

Ah, indeed I hadn't got as far as fully appreciating that to_zpci_dev() 
isn't robust enough on its own. Thanks for the patch, I've pulled it in 
and will include it in v4. Do I take it that all else works OK with this 
fixed?

Cheers,
Robin.

>>
>> To tolerate that, this series needs a change along the lines of:
>>
> 
> ...  Sorry, let's try that again without a mangled diff:
> 
> From: Matthew Rosato <mjrosato@linux.ibm.com>
> Date: Thu, 7 Jul 2022 08:45:44 -0400
> Subject: [PATCH] iommu/s390: fail probe for non-pci device
> 
> 
> s390-iommu only supports pci_bus_type today
> 
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
> ---
>   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 dd957145fb81..762f892b4ec3 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;
>   }
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/7/22 10:58 AM, Robin Murphy wrote:
> On 2022-07-07 13:54, Matthew Rosato wrote:
>> On 7/7/22 8:49 AM, Matthew Rosato wrote:
>>> On 7/5/22 1:08 PM, Robin Murphy wrote:
>>>> 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!
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>
>>>> v3: Also catch Intel's cheeky open-coded assignment
>>>>
>>>
>>> ...
>>>
>>>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>>>> index c898bcbbce11..dd957145fb81 100644
>>>> --- a/drivers/iommu/s390-iommu.c
>>>> +++ b/drivers/iommu/s390-iommu.c
>>>> @@ -385,9 +385,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);
>>>
>>> Previously s390_iommu_ops was only being set for pci_bus_type, but 
>>> with this series it will now also be set for platform_bus_type.
> 
> Ah, indeed I hadn't got as far as fully appreciating that to_zpci_dev() 
> isn't robust enough on its own. Thanks for the patch, I've pulled it in 
> and will include it in v4. Do I take it that all else works OK with this 
> fixed?
> 

Yes, with that patch included this looks OK so for s390 you can also take a

Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v3 15/15] iommu: Clean up bus_set_iommu()

[Matthew Rosato]

On 7/7/22 10:58 AM, Robin Murphy wrote:
> On 2022-07-07 13:54, Matthew Rosato wrote:
>> On 7/7/22 8:49 AM, Matthew Rosato wrote:
>>> On 7/5/22 1:08 PM, Robin Murphy wrote:
>>>> 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!
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>
>>>> v3: Also catch Intel's cheeky open-coded assignment
>>>>
>>>
>>> ...
>>>
>>>> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
>>>> index c898bcbbce11..dd957145fb81 100644
>>>> --- a/drivers/iommu/s390-iommu.c
>>>> +++ b/drivers/iommu/s390-iommu.c
>>>> @@ -385,9 +385,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);
>>>
>>> Previously s390_iommu_ops was only being set for pci_bus_type, but 
>>> with this series it will now also be set for platform_bus_type.
> 
> Ah, indeed I hadn't got as far as fully appreciating that to_zpci_dev() 
> isn't robust enough on its own. Thanks for the patch, I've pulled it in 
> and will include it in v4. Do I take it that all else works OK with this 
> fixed?
> 

Yes, with that patch included this looks OK so for s390 you can also take a

Tested-by: Matthew Rosato <mjrosato@linux.ibm.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Thursday, July 7, 2022 5:59 PM
> 
> On 2022-07-07 07:31, Tian, Kevin wrote:
> >> From: Robin Murphy <robin.murphy@arm.com>
> >> Sent: Wednesday, July 6, 2022 1:08 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.
> >>
> >
> > Suppose we miss a check on iommu ops in iommu_release_device():
> >
> > 	if (!dev->iommu) <<<<<<<
> > 		return;
> >
> > 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
> >
> > 	ops = dev_iommu_ops(dev);
> > 	ops->release_device(dev);
> >
> > following the rationale in patch01 a device could be removed when
> > it's associated with a known but not registered instance.
> 
> No, because at that point the instance is only known internally to the
> driver. As long as it isn't erroneously returned from
> ->probe_device(dev), dev->iommu will remain NULL and the rest of the
> core code works as expected.
> 

You are correct. I overlooked dev->iommu as device_to_iommu() in
patch01. As long as the device hasn't been probed or ->probe_device
doesn't do bad thing then dev->iommu should be NULL in this case.

RE: [PATCH v3 03/15] iommu: Always register bus notifiers

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Thursday, July 7, 2022 5:59 PM
> 
> On 2022-07-07 07:31, Tian, Kevin wrote:
> >> From: Robin Murphy <robin.murphy@arm.com>
> >> Sent: Wednesday, July 6, 2022 1:08 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.
> >>
> >
> > Suppose we miss a check on iommu ops in iommu_release_device():
> >
> > 	if (!dev->iommu) <<<<<<<
> > 		return;
> >
> > 	iommu_device_unlink(dev->iommu->iommu_dev, dev);
> >
> > 	ops = dev_iommu_ops(dev);
> > 	ops->release_device(dev);
> >
> > following the rationale in patch01 a device could be removed when
> > it's associated with a known but not registered instance.
> 
> No, because at that point the instance is only known internally to the
> driver. As long as it isn't erroneously returned from
> ->probe_device(dev), dev->iommu will remain NULL and the rest of the
> core code works as expected.
> 

You are correct. I overlooked dev->iommu as device_to_iommu() in
patch01. As long as the device hasn't been probed or ->probe_device
doesn't do bad thing then dev->iommu should be NULL in this case.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Thursday, July 7, 2022 6:58 PM
> 
> On 2022-07-07 07:51, Tian, Kevin wrote:
> >> From: Robin Murphy <robin.murphy@arm.com>
> >> Sent: Wednesday, July 6, 2022 1:08 AM
> >>
> >> @@ -202,12 +210,32 @@ 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);
> >> +
> >> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> >> +		struct bus_type *bus = iommu_buses[i];
> >> +		int err;
> >> +
> >> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> >> +			err = -EBUSY;
> >> +		} else {
> >> +			bus->iommu_ops = ops;
> >> +			err = bus_iommu_probe(bus);
> >> +		}
> >> +		if (err) {
> >> +			iommu_device_unregister(iommu);
> >> +			return err;
> >> +		}
> >> +	}
> >> +
> >
> > Probably move above into a new function bus_iommu_probe_all():
> >
> > 	/* probe all buses for devices associated with this iommu */
> > 	err = bus_iommu_probe_all();
> > 	if (err) {
> > 		iommu_device_unregister(iommu);
> > 		return err;
> > 	}
> >
> > Just  my personal preference on leaving logic in iommu_device_register()
> > more relevant to the iommu instance itself.
> 
> On reflection I think it makes sense to pull the
> iommu_device_unregister() out of the loop anyway - I think that's really
> a left-over from between v1 and v2 when that error case briefly jumped
> to another cleanup loop, before I realised it was actually trivial for
> iommu_device_unregister() to clean up for itself.
> 
> However I now see I've also missed another opportunity, and the -EBUSY
> case should be hoisted out of the loop as well, since checking
> iommu_buses[0] is sufficient. Then it's hopefully much clearer that once
> the bus ops go away we'll be left with just a single extra line for the
> loop, as in iommu_device_unregister(). Does that sound reasonable?
> 

Yes, sounds good to me.

RE: [PATCH v3 04/15] iommu: Move bus setup to IOMMU device registration

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Thursday, July 7, 2022 6:58 PM
> 
> On 2022-07-07 07:51, Tian, Kevin wrote:
> >> From: Robin Murphy <robin.murphy@arm.com>
> >> Sent: Wednesday, July 6, 2022 1:08 AM
> >>
> >> @@ -202,12 +210,32 @@ 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);
> >> +
> >> +	for (int i = 0; i < ARRAY_SIZE(iommu_buses); i++) {
> >> +		struct bus_type *bus = iommu_buses[i];
> >> +		int err;
> >> +
> >> +		if (bus->iommu_ops && bus->iommu_ops != ops) {
> >> +			err = -EBUSY;
> >> +		} else {
> >> +			bus->iommu_ops = ops;
> >> +			err = bus_iommu_probe(bus);
> >> +		}
> >> +		if (err) {
> >> +			iommu_device_unregister(iommu);
> >> +			return err;
> >> +		}
> >> +	}
> >> +
> >
> > Probably move above into a new function bus_iommu_probe_all():
> >
> > 	/* probe all buses for devices associated with this iommu */
> > 	err = bus_iommu_probe_all();
> > 	if (err) {
> > 		iommu_device_unregister(iommu);
> > 		return err;
> > 	}
> >
> > Just  my personal preference on leaving logic in iommu_device_register()
> > more relevant to the iommu instance itself.
> 
> On reflection I think it makes sense to pull the
> iommu_device_unregister() out of the loop anyway - I think that's really
> a left-over from between v1 and v2 when that error case briefly jumped
> to another cleanup loop, before I realised it was actually trivial for
> iommu_device_unregister() to clean up for itself.
> 
> However I now see I've also missed another opportunity, and the -EBUSY
> case should be hoisted out of the loop as well, since checking
> iommu_buses[0] is sufficient. Then it's hopefully much clearer that once
> the bus ops go away we'll be left with just a single extra line for the
> loop, as in iommu_device_unregister(). Does that sound reasonable?
> 

Yes, sounds good to me.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.

I am trying to reproduce this problem. Strangely, even if I selected
CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)

In fact the rmrr list in the Intel IOMMU driver is always static after
parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
Hence we can safely remove below down/up_read().

4512 static void intel_iommu_get_resv_regions(struct device *device,
4513                                          struct list_head *head)
4514 {
4515         int prot = DMA_PTE_READ | DMA_PTE_WRITE;
4516         struct iommu_resv_region *reg;
4517         struct dmar_rmrr_unit *rmrr;
4518         struct device *i_dev;
4519         int i;
4520
4521         down_read(&dmar_global_lock);
4522         for_each_rmrr_units(rmrr) {
4523                 for_each_active_dev_scope(rmrr->devices, 
rmrr->devices_cnt,
4524                                           i, i_dev) {

Best regards,
baolu

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Baolu Lu]

On 2022/7/6 01:08, Robin Murphy wrote:
> 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, so I'm leaving it here for someone with
> more confidence to tackle a larger rework of the locking.

I am trying to reproduce this problem. Strangely, even if I selected
CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)

In fact the rmrr list in the Intel IOMMU driver is always static after
parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
Hence we can safely remove below down/up_read().

4512 static void intel_iommu_get_resv_regions(struct device *device,
4513                                          struct list_head *head)
4514 {
4515         int prot = DMA_PTE_READ | DMA_PTE_WRITE;
4516         struct iommu_resv_region *reg;
4517         struct dmar_rmrr_unit *rmrr;
4518         struct device *i_dev;
4519         int i;
4520
4521         down_read(&dmar_global_lock);
4522         for_each_rmrr_units(rmrr) {
4523                 for_each_active_dev_scope(rmrr->devices, 
rmrr->devices_cnt,
4524                                           i, i_dev) {

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] iommu/s390: fail probe for non-pci device

[Niklas Schnelle]

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

s390-iommu only supports pci_bus_type today

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 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 dd957145fb81..762f892b4ec3 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.34.1

[PATCH] iommu/s390: fail probe for non-pci device

[Niklas Schnelle]

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

s390-iommu only supports pci_bus_type today

Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
---
 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 dd957145fb81..762f892b4ec3 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.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Niklas Schnelle]

On Tue, 2022-07-05 at 18:08 +0100, Robin Murphy wrote:
> v2: https://lore.kernel.org/linux-iommu/cover.1650890638.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> Here's v3, now with working x86! Having finally made sense of how I
> broke Intel, I've given AMD the same fix by inspection. I'm still not
> 100% sure about s390, but it looks like it should probably be OK since
> it seems to register an IOMMU instance for each PCI device (?!) before
> disappearing into PCI hotplug code, wherein I assume we should never see
> a PCI device appear without its IOMMU already registered.

Yes, this is a bit unusual as our PCI architecture doesn't really have
a notion of an IOMMU device only of I/O translation tables. These are
then registered per PCI function. PCI functions may share I/O
translation tables and thus DMA address spaces but this is not done at
the moment. As Matt already mentioned we do need a small change for
this patch series. Since that was still mangled in his mail for me I
just replied with that using "git send-email". With Matt's patch
applied I can confirm that this works fine for us and does look like a
useful simplification. So feel free to add my

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>

Re: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Niklas Schnelle]

On Tue, 2022-07-05 at 18:08 +0100, Robin Murphy wrote:
> v2: https://lore.kernel.org/linux-iommu/cover.1650890638.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> Here's v3, now with working x86! Having finally made sense of how I
> broke Intel, I've given AMD the same fix by inspection. I'm still not
> 100% sure about s390, but it looks like it should probably be OK since
> it seems to register an IOMMU instance for each PCI device (?!) before
> disappearing into PCI hotplug code, wherein I assume we should never see
> a PCI device appear without its IOMMU already registered.

Yes, this is a bit unusual as our PCI architecture doesn't really have
a notion of an IOMMU device only of I/O translation tables. These are
then registered per PCI function. PCI functions may share I/O
translation tables and thus DMA address spaces but this is not done at
the moment. As Matt already mentioned we do need a small change for
this patch series. Since that was still mangled in his mail for me I
just replied with that using "git send-email". With Matt's patch
applied I can confirm that this works fine for us and does look like a
useful simplification. So feel free to add my

Tested-by: Niklas Schnelle <schnelle@linux.ibm.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Robin Murphy]

On 2022-07-08 08:52, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>> 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, so I'm leaving it here for someone with
>> more confidence to tackle a larger rework of the locking.
> 
> I am trying to reproduce this problem. Strangely, even if I selected
> CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)

FWIW, see below for the full report I get with this series applied (my 
machine doesn't have any ANDD entries to trigger the existing case).

> In fact the rmrr list in the Intel IOMMU driver is always static after
> parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
> Hence we can safely remove below down/up_read().

IIRC that leads to RCU warnings via for_each_dev_scope(), though. I did 
try replacing this down_read() with rcu_read_lock(), but then it doesn't 
like the GFP_KERNEL allocation in iommu_alloc_resv_region(), and that's 
where I gave up :)

I'm mostly left wondering whether the dmar_drhd_units list really needs 
to be RCU protected at all, as that seems to be the root of most of the 
problems here.

Cheers,
Robin.

> 4512 static void intel_iommu_get_resv_regions(struct device *device,
> 4513                                          struct list_head *head)
> 4514 {
> 4515         int prot = DMA_PTE_READ | DMA_PTE_WRITE;
> 4516         struct iommu_resv_region *reg;
> 4517         struct dmar_rmrr_unit *rmrr;
> 4518         struct device *i_dev;
> 4519         int i;
> 4520
> 4521         down_read(&dmar_global_lock);
> 4522         for_each_rmrr_units(rmrr) {
> 4523                 for_each_active_dev_scope(rmrr->devices, 
> rmrr->devices_cnt,
> 4524                                           i, i_dev) {
> 
> Best regards,
> baolu


---->8-----

[   11.421712] pci 0000:00:1b.0: Adding to iommu group 0
[   11.421977]
[   11.421978] ============================================
[   11.421979] WARNING: possible recursive locking detected
[   11.421981] 5.19.0-rc3-00015-gdc44a2269276 #32 Not tainted
[   11.421984] --------------------------------------------
[   11.421985] swapper/0/1 is trying to acquire lock:
[   11.421986] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422000]
[   11.422000] but task is already holding lock:
[   11.422001] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_init+0x1638/0x1a08
[   11.422011]
[   11.422011] other info that might help us debug this:
[   11.422013]  Possible unsafe locking scenario:
[   11.422013]
[   11.422013]        CPU0
[   11.422014]        ----
[   11.422015]   lock(dmar_global_lock);
[   11.422018]   lock(dmar_global_lock);
[   11.422020]
[   11.422020]  *** DEADLOCK ***
[   11.422020]
[   11.422021]  May be due to missing lock nesting notation
[   11.422021]
[   11.422022] 2 locks held by swapper/0/1:
[   11.422024]  #0: ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_init+0x1638/0x1a08
[   11.422033]  #1: ffff8881077a10c0 (&group->mutex){+.+.}-{3:3}, at: 
bus_iommu_probe+0x139/0x4c0
[   11.422044]
[   11.422044] stack backtrace:
[   11.422046] CPU: 8 PID: 1 Comm: swapper/0 Not tainted 
5.19.0-rc3-00015-gdc44a2269276 #32
[   11.422050] Hardware name: LENOVO 30B6S08J03/1030, BIOS S01KT29A 
06/20/2016
[   11.422052] Call Trace:
[   11.422054]  <TASK>
[   11.422056]  dump_stack_lvl+0x45/0x59
[   11.422064]  __lock_acquire.cold+0x131/0x305
[   11.422075]  ? lockdep_hardirqs_on_prepare+0x220/0x220
[   11.422082]  ? lock_is_held_type+0xd7/0x130
[   11.422090]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422098]  lock_acquire+0x165/0x410
[   11.422102]  ? intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422108]  ? lock_release+0x410/0x410
[   11.422112]  ? __iommu_domain_alloc+0xc5/0x130
[   11.422116]  ? iommu_group_alloc_default_domain+0x16/0x90
[   11.422121]  ? bus_iommu_probe+0x26d/0x4c0
[   11.422126]  ? iommu_device_register+0x11e/0x160
[   11.422130]  ? intel_iommu_init+0x16e0/0x1a08
[   11.422135]  ? do_one_initcall+0xb6/0x3c0
[   11.422140]  ? lock_is_held_type+0xd7/0x130
[   11.422147]  down_read+0x97/0x2f0
[   11.422152]  ? intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422156]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422161]  ? rwsem_down_read_slowpath+0xa10/0xa10
[   11.422166]  ? find_held_lock+0x85/0xa0
[   11.422173]  intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422178]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422185]  iommu_create_device_direct_mappings.isra.0+0x11a/0x330
[   11.422193]  ? iommu_map+0x50/0x50
[   11.422198]  ? __iommu_domain_alloc+0xc5/0x130
[   11.422205]  bus_iommu_probe+0x2bc/0x4c0
[   11.422210]  ? iommu_device_register+0xba/0x160
[   11.422216]  ? iommu_group_default_domain+0x20/0x20
[   11.422221]  ? do_raw_spin_lock+0x114/0x1d0
[   11.422226]  ? rwlock_bug.part.0+0x50/0x50
[   11.422231]  ? rwsem_down_read_slowpath+0xa10/0xa10
[   11.422239]  iommu_device_register+0x11e/0x160
[   11.422244]  intel_iommu_init+0x16e0/0x1a08
[   11.422249]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422254]  ? _raw_spin_unlock_irqrestore+0x28/0x50
[   11.422261]  ? lock_release+0x240/0x410
[   11.422265]  ? populate_rootfs+0x26/0x37
[   11.422272]  ? lock_downgrade+0x3a0/0x3a0
[   11.422277]  ? dmar_parse_one_rmrr+0x203/0x203
[   11.422281]  ? lock_is_held_type+0xd7/0x130
[   11.422286]  ? iommu_setup+0x282/0x282
[   11.422291]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422296]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422301]  ? up_write+0xd3/0x260
[   11.422305]  ? iommu_setup+0x282/0x282
[   11.422309]  pci_iommu_init+0x15/0x39
[   11.422313]  do_one_initcall+0xb6/0x3c0
[   11.422317]  ? initcall_blacklisted+0x120/0x120
[   11.422322]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422327]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422331]  ? kasan_unpoison+0x23/0x50
[   11.422337]  ? __kasan_slab_alloc+0x2c/0x80
[   11.422344]  kernel_init_freeable+0x330/0x389
[   11.422349]  ? rest_init+0x1b0/0x1b0
[   11.422354]  kernel_init+0x14/0x130
[   11.422359]  ret_from_fork+0x22/0x30
[   11.422367]  </TASK>

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Robin Murphy]

On 2022-07-08 08:52, Baolu Lu wrote:
> On 2022/7/6 01:08, Robin Murphy wrote:
>> 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, so I'm leaving it here for someone with
>> more confidence to tackle a larger rework of the locking.
> 
> I am trying to reproduce this problem. Strangely, even if I selected
> CONFIG_LOCKDEP=y, the kernel didn't complain anything. :-)

FWIW, see below for the full report I get with this series applied (my 
machine doesn't have any ANDD entries to trigger the existing case).

> In fact the rmrr list in the Intel IOMMU driver is always static after
> parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
> Hence we can safely remove below down/up_read().

IIRC that leads to RCU warnings via for_each_dev_scope(), though. I did 
try replacing this down_read() with rcu_read_lock(), but then it doesn't 
like the GFP_KERNEL allocation in iommu_alloc_resv_region(), and that's 
where I gave up :)

I'm mostly left wondering whether the dmar_drhd_units list really needs 
to be RCU protected at all, as that seems to be the root of most of the 
problems here.

Cheers,
Robin.

> 4512 static void intel_iommu_get_resv_regions(struct device *device,
> 4513                                          struct list_head *head)
> 4514 {
> 4515         int prot = DMA_PTE_READ | DMA_PTE_WRITE;
> 4516         struct iommu_resv_region *reg;
> 4517         struct dmar_rmrr_unit *rmrr;
> 4518         struct device *i_dev;
> 4519         int i;
> 4520
> 4521         down_read(&dmar_global_lock);
> 4522         for_each_rmrr_units(rmrr) {
> 4523                 for_each_active_dev_scope(rmrr->devices, 
> rmrr->devices_cnt,
> 4524                                           i, i_dev) {
> 
> Best regards,
> baolu


---->8-----

[   11.421712] pci 0000:00:1b.0: Adding to iommu group 0
[   11.421977]
[   11.421978] ============================================
[   11.421979] WARNING: possible recursive locking detected
[   11.421981] 5.19.0-rc3-00015-gdc44a2269276 #32 Not tainted
[   11.421984] --------------------------------------------
[   11.421985] swapper/0/1 is trying to acquire lock:
[   11.421986] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422000]
[   11.422000] but task is already holding lock:
[   11.422001] ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_init+0x1638/0x1a08
[   11.422011]
[   11.422011] other info that might help us debug this:
[   11.422013]  Possible unsafe locking scenario:
[   11.422013]
[   11.422013]        CPU0
[   11.422014]        ----
[   11.422015]   lock(dmar_global_lock);
[   11.422018]   lock(dmar_global_lock);
[   11.422020]
[   11.422020]  *** DEADLOCK ***
[   11.422020]
[   11.422021]  May be due to missing lock nesting notation
[   11.422021]
[   11.422022] 2 locks held by swapper/0/1:
[   11.422024]  #0: ffffffffb987b770 (dmar_global_lock){++++}-{3:3}, at: 
intel_iommu_init+0x1638/0x1a08
[   11.422033]  #1: ffff8881077a10c0 (&group->mutex){+.+.}-{3:3}, at: 
bus_iommu_probe+0x139/0x4c0
[   11.422044]
[   11.422044] stack backtrace:
[   11.422046] CPU: 8 PID: 1 Comm: swapper/0 Not tainted 
5.19.0-rc3-00015-gdc44a2269276 #32
[   11.422050] Hardware name: LENOVO 30B6S08J03/1030, BIOS S01KT29A 
06/20/2016
[   11.422052] Call Trace:
[   11.422054]  <TASK>
[   11.422056]  dump_stack_lvl+0x45/0x59
[   11.422064]  __lock_acquire.cold+0x131/0x305
[   11.422075]  ? lockdep_hardirqs_on_prepare+0x220/0x220
[   11.422082]  ? lock_is_held_type+0xd7/0x130
[   11.422090]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422098]  lock_acquire+0x165/0x410
[   11.422102]  ? intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422108]  ? lock_release+0x410/0x410
[   11.422112]  ? __iommu_domain_alloc+0xc5/0x130
[   11.422116]  ? iommu_group_alloc_default_domain+0x16/0x90
[   11.422121]  ? bus_iommu_probe+0x26d/0x4c0
[   11.422126]  ? iommu_device_register+0x11e/0x160
[   11.422130]  ? intel_iommu_init+0x16e0/0x1a08
[   11.422135]  ? do_one_initcall+0xb6/0x3c0
[   11.422140]  ? lock_is_held_type+0xd7/0x130
[   11.422147]  down_read+0x97/0x2f0
[   11.422152]  ? intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422156]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422161]  ? rwsem_down_read_slowpath+0xa10/0xa10
[   11.422166]  ? find_held_lock+0x85/0xa0
[   11.422173]  intel_iommu_get_resv_regions+0x28/0x3a0
[   11.422178]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422185]  iommu_create_device_direct_mappings.isra.0+0x11a/0x330
[   11.422193]  ? iommu_map+0x50/0x50
[   11.422198]  ? __iommu_domain_alloc+0xc5/0x130
[   11.422205]  bus_iommu_probe+0x2bc/0x4c0
[   11.422210]  ? iommu_device_register+0xba/0x160
[   11.422216]  ? iommu_group_default_domain+0x20/0x20
[   11.422221]  ? do_raw_spin_lock+0x114/0x1d0
[   11.422226]  ? rwlock_bug.part.0+0x50/0x50
[   11.422231]  ? rwsem_down_read_slowpath+0xa10/0xa10
[   11.422239]  iommu_device_register+0x11e/0x160
[   11.422244]  intel_iommu_init+0x16e0/0x1a08
[   11.422249]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422254]  ? _raw_spin_unlock_irqrestore+0x28/0x50
[   11.422261]  ? lock_release+0x240/0x410
[   11.422265]  ? populate_rootfs+0x26/0x37
[   11.422272]  ? lock_downgrade+0x3a0/0x3a0
[   11.422277]  ? dmar_parse_one_rmrr+0x203/0x203
[   11.422281]  ? lock_is_held_type+0xd7/0x130
[   11.422286]  ? iommu_setup+0x282/0x282
[   11.422291]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422296]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422301]  ? up_write+0xd3/0x260
[   11.422305]  ? iommu_setup+0x282/0x282
[   11.422309]  pci_iommu_init+0x15/0x39
[   11.422313]  do_one_initcall+0xb6/0x3c0
[   11.422317]  ? initcall_blacklisted+0x120/0x120
[   11.422322]  ? rcu_read_lock_sched_held+0x9c/0xd0
[   11.422327]  ? rcu_read_lock_bh_held+0xb0/0xb0
[   11.422331]  ? kasan_unpoison+0x23/0x50
[   11.422337]  ? __kasan_slab_alloc+0x2c/0x80
[   11.422344]  kernel_init_freeable+0x330/0x389
[   11.422349]  ? rest_init+0x1b0/0x1b0
[   11.422354]  kernel_init+0x14/0x130
[   11.422359]  ret_from_fork+0x22/0x30
[   11.422367]  </TASK>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Robin Murphy]

On 2022-07-05 18:08, Robin Murphy wrote:
> v2: https://lore.kernel.org/linux-iommu/cover.1650890638.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> Here's v3, now with working x86! Having finally made sense of how I
> broke Intel, I've given AMD the same fix by inspection. I'm still not
> 100% sure about s390, but it looks like it should probably be OK since
> it seems to register an IOMMU instance for each PCI device (?!) before
> disappearing into PCI hotplug code, wherein I assume we should never see
> a PCI device appear without its IOMMU already registered.
> 
> Otherwise, the only other updates are hooking up the new host1x context
> bus (noting that it now takes all of 4 lines to support a whole new bus,
> yay!), and a slight tweak to make sure we keep rejecting registration of
> conflicting iommu_ops rather than needlessly change that just yet.

FWIW I've prepared v4, including Matt's s390 patch and some nice extra 
cleanups thanks to Kevin's suggestions, but have now decided to wait to 
rebase and send it after the upcoming merge window. If anyone's 
interested in the meantime, there's a preliminary branch here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4

(temporarily including the host1x patch from -next to avoid breakage on 
arm64 as well)

Thanks,
Robin.

Re: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Robin Murphy]

On 2022-07-05 18:08, Robin Murphy wrote:
> v2: https://lore.kernel.org/linux-iommu/cover.1650890638.git.robin.murphy@arm.com/
> 
> Hi all,
> 
> Here's v3, now with working x86! Having finally made sense of how I
> broke Intel, I've given AMD the same fix by inspection. I'm still not
> 100% sure about s390, but it looks like it should probably be OK since
> it seems to register an IOMMU instance for each PCI device (?!) before
> disappearing into PCI hotplug code, wherein I assume we should never see
> a PCI device appear without its IOMMU already registered.
> 
> Otherwise, the only other updates are hooking up the new host1x context
> bus (noting that it now takes all of 4 lines to support a whole new bus,
> yay!), and a slight tweak to make sure we keep rejecting registration of
> conflicting iommu_ops rather than needlessly change that just yet.

FWIW I've prepared v4, including Matt's s390 patch and some nice extra 
cleanups thanks to Kevin's suggestions, but have now decided to wait to 
rebase and send it after the upcoming merge window. If anyone's 
interested in the meantime, there's a preliminary branch here:

https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4

(temporarily including the host1x patch from -next to avoid breakage on 
arm64 as well)

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Lu Baolu]

Hi Robin,

On 2022/7/15 20:37, Robin Murphy wrote:
>> In fact the rmrr list in the Intel IOMMU driver is always static after
>> parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
>> Hence we can safely remove below down/up_read().
> 
> IIRC that leads to RCU warnings via for_each_dev_scope(), though. I did 
> try replacing this down_read() with rcu_read_lock(), but then it doesn't 
> like the GFP_KERNEL allocation in iommu_alloc_resv_region(), and that's 
> where I gave up :)
> 
> I'm mostly left wondering whether the dmar_drhd_units list really needs 
> to be RCU protected at all, as that seems to be the root of most of the 
> problems here.

I just posted a fix patch here:

https://lore.kernel.org/linux-iommu/20220718235325.3952426-1-baolu.lu@linux.intel.com/

It can remove the recursive locking and RCU warnings. Can you please
take a look at it?

Best regards,
baolu

Re: [PATCH v3 01/15] iommu/vt-d: Handle race between registration and device probe

[Lu Baolu]

Hi Robin,

On 2022/7/15 20:37, Robin Murphy wrote:
>> In fact the rmrr list in the Intel IOMMU driver is always static after
>> parsing the ACPI/DMAR tables. There's no need to protect it with a lock.
>> Hence we can safely remove below down/up_read().
> 
> IIRC that leads to RCU warnings via for_each_dev_scope(), though. I did 
> try replacing this down_read() with rcu_read_lock(), but then it doesn't 
> like the GFP_KERNEL allocation in iommu_alloc_resv_region(), and that's 
> where I gave up :)
> 
> I'm mostly left wondering whether the dmar_drhd_units list really needs 
> to be RCU protected at all, as that seems to be the root of most of the 
> problems here.

I just posted a fix patch here:

https://lore.kernel.org/linux-iommu/20220718235325.3952426-1-baolu.lu@linux.intel.com/

It can remove the recursive locking and RCU warnings. Can you please
take a look at it?

Best regards,
baolu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, July 15, 2022 9:13 PM
> 
> On 2022-07-05 18:08, Robin Murphy wrote:
> > v2: https://lore.kernel.org/linux-
> iommu/cover.1650890638.git.robin.murphy@arm.com/
> >
> > Hi all,
> >
> > Here's v3, now with working x86! Having finally made sense of how I
> > broke Intel, I've given AMD the same fix by inspection. I'm still not
> > 100% sure about s390, but it looks like it should probably be OK since
> > it seems to register an IOMMU instance for each PCI device (?!) before
> > disappearing into PCI hotplug code, wherein I assume we should never see
> > a PCI device appear without its IOMMU already registered.
> >
> > Otherwise, the only other updates are hooking up the new host1x context
> > bus (noting that it now takes all of 4 lines to support a whole new bus,
> > yay!), and a slight tweak to make sure we keep rejecting registration of
> > conflicting iommu_ops rather than needlessly change that just yet.
> 
> FWIW I've prepared v4, including Matt's s390 patch and some nice extra
> cleanups thanks to Kevin's suggestions, but have now decided to wait to
> rebase and send it after the upcoming merge window. If anyone's
> interested in the meantime, there's a preliminary branch here:
> 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4
> 
> (temporarily including the host1x patch from -next to avoid breakage on
> arm64 as well)
> 

This looks good to me.

RE: [PATCH v3 00/15] iommu: Retire bus_set_iommu()

[Tian, Kevin]

> From: Robin Murphy <robin.murphy@arm.com>
> Sent: Friday, July 15, 2022 9:13 PM
> 
> On 2022-07-05 18:08, Robin Murphy wrote:
> > v2: https://lore.kernel.org/linux-
> iommu/cover.1650890638.git.robin.murphy@arm.com/
> >
> > Hi all,
> >
> > Here's v3, now with working x86! Having finally made sense of how I
> > broke Intel, I've given AMD the same fix by inspection. I'm still not
> > 100% sure about s390, but it looks like it should probably be OK since
> > it seems to register an IOMMU instance for each PCI device (?!) before
> > disappearing into PCI hotplug code, wherein I assume we should never see
> > a PCI device appear without its IOMMU already registered.
> >
> > Otherwise, the only other updates are hooking up the new host1x context
> > bus (noting that it now takes all of 4 lines to support a whole new bus,
> > yay!), and a slight tweak to make sure we keep rejecting registration of
> > conflicting iommu_ops rather than needlessly change that just yet.
> 
> FWIW I've prepared v4, including Matt's s390 patch and some nice extra
> cleanups thanks to Kevin's suggestions, but have now decided to wait to
> rebase and send it after the upcoming merge window. If anyone's
> interested in the meantime, there's a preliminary branch here:
> 
> https://gitlab.arm.com/linux-arm/linux-rm/-/commits/bus-set-iommu-v4
> 
> (temporarily including the host1x patch from -next to avoid breakage on
> arm64 as well)
> 

This looks good to me.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel