iommu: Fix the longstanding probe issues

Since iommu_init_device() was factored out, it is in fact the only consumer of the ops which __iommu_probe_device() is resolving, so let it do that itself rather than passing them in. This also puts the ops lookup at a more logical point relative to the rest of the flow through __iommu_probe_device().

Signed-off-by Robin Murphy <[email protected]>

---

v2: New

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

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++
 drivers/acpi/scan.c             |  7 -------
 drivers/amba/bus.c              |  3 ++-
 drivers/base/platform.c         |  3 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
 drivers/cdx/cdx.c               |  3 ++-
 drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
 drivers/iommu/of_iommu.c        |  7 ++++++-
 drivers/of/device.c             |  7 ++++++-
 drivers/pci/pci-driver.c        |  3 ++-
 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +0 -0

index 52b2abf88689..f30f138352b7 100644

drivers/acpi/arm64/dma.c +0 -0

 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
 		end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -7

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 		err = viot_iommu_configure(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-
 	return err;
 }
 

drivers/amba/bus.c +2 -1

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device *dev)
 		ret = acpi_dma_configure(dev, attr);
 	}
 
-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/base/platform.c +2 -1

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct device *dev)
 		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
 		ret = acpi_dma_configure(dev, attr);
 	}
-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (ret || !dev->driver || drv->driver_managed_dma)
 		return ret;
 
 	ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl-mc/fsl-mc-bus.c +2 -1

index d1f3d327ddd1..a8be8cf246fb 100644
@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device *dev)
 	else
 		ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 
-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +2 -1

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device *dev)
 		return ret;
 	}
 
-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +21 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;
 	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is
+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a fwspec which will
+	 * match ops registered with a non-NULL fwnode, or we can reasonably
 	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
 	 * be present, and that any of their registered instances has suitable
 	 * ops for probing, and thus cheekily co-opt the same mechanism.

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
 		ret = -ENODEV;
 		goto err_free;
 	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;

drivers/iommu/of_iommu.c +6 -1

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
 		dev_iommu_free(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */
+	if (!err && dev->bus && !dev_iommu_present)
 		err = iommu_probe_device(dev);
 
 	if (err && err != -EPROBE_DEFER)

drivers/of/device.c +6 -1

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	bool coherent, set_map = false;
 	int ret;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+
 	if (np == dev->of_node)
 		bus_np = __of_get_dma_parent(np);
 	else

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		end = dma_range_map_max(map);
 		set_map = true;
 	}
-
+skip_map:
 	/*
 	 * If @dev is expected to be DMA-capable then the bus code that created
 	 * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +2 -1

index f57ea36d125d..4468b7327cab 100644
@@ -1653,7 +1653,8 @@ static int pci_dma_configure(struct device *dev)
 
 	pci_put_host_bridge_device(bridge);
 
-	if (!ret && !driver->driver_managed_dma) {
+	/* @driver may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !driver->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

drivers/acpi/arm64/dma.c void acpi_arch_dma_setup(struct device *dev) · line 33

Quoted 13 lines

drivers/acpi/arm64/dma.c +5 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
  	else
  		end = (1ULL << 32) - 1;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+

Why are we checking that dev->dma_range_map exists here rather than at function entry ? Is that because we want to run the previous code for some reasons even if dev->dma_range_map is already set ?

Just noticed, the OF counterpart does not seem to take the same approach, so I am just flagging this up if it matters at all.

Other than that, for acpi/arm64:

Reviewed-by Lorenzo Pieralisi <[email protected]>

drivers/iommu/iommu.c static int iommu_init_device(struct device *dev) · line 446

Quoted 6 lines · Robin Murphy

drivers/iommu/iommu.c +6 -0

+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");

This warning triggers on my workstation (with an AMD IOMMU), any ideas?

  ------------[ cut here ]------------
  reg-dummy reg-dummy: late IOMMU probe at driver bind, something fishy here!
  WARNING: CPU: 0 PID: 1 at drivers/iommu/iommu.c:449 __iommu_probe_device+0x10b/0x510
  Modules linked in:

  CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc6-iommu-next+ #1 1d691d7aebf343bde741cf4c8610d78a2ea2d2d9
  Hardware name: System manufacturer System Product Name/PRIME X470-PRO, BIOS 5406 11/13/2019
  RIP: 0010:__iommu_probe_device+0x10b/0x510
  Code: 68 00 74 28 48 8b 6b 50 48 85 ed 75 03 48 8b 2b 48 89 df e8 87 71 06 00 48 89 ea 48 c7 c7 90 dd 2c 8b 48 89 c6 e8 35 83 77 ff <0f> 0b 49 8b bd a8 00 00 00 e8 77 ab 85 ff 84 c0 0f 84 ad 01 00 00
  RSP: 0018:ffffafba00047c58 EFLAGS: 00010282
  RAX: 0000000000000000 RBX: ffffa00481301c10 RCX: 0000000000000003
  RDX: 0000000000000000 RSI: 0000000000000003 RDI: 0000000000000001
  RBP: ffffa00480ffaee0 R08: 0000000000000000 R09: ffffafba00047ae8
  R10: ffffa0135e93ffa8 R11: 0000000000000003 R12: ffffafba00047d18
  R13: ffffffff8adac1a0 R14: 0000000000000000 R15: ffffa004802c5800
  FS:  0000000000000000(0000) GS:ffffa0135ea00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: ffffa00baca01000 CR3: 000000082b838000 CR4: 00000000003506f0
  Call Trace:
   <TASK>
   ? __iommu_probe_device+0x10b/0x510
   ? __warn.cold+0x93/0xf7
   ? __iommu_probe_device+0x10b/0x510
   ? report_bug+0xff/0x140
   ? prb_read_valid+0x1b/0x30
   ? handle_bug+0x58/0x90
   ? exc_invalid_op+0x17/0x70
   ? asm_exc_invalid_op+0x1a/0x20
   ? __iommu_probe_device+0x10b/0x510
   ? __iommu_probe_device+0x10b/0x510
   ? __pfx_probe_iommu_group+0x10/0x10
   probe_iommu_group+0x28/0x50
   bus_for_each_dev+0x7e/0xd0
   iommu_device_register+0xee/0x260
   iommu_go_to_state+0xa2a/0x1970
   ? srso_return_thunk+0x5/0x5f
   ? blake2s_update+0x68/0x160
   ? __pfx_pci_iommu_init+0x10/0x10
   amd_iommu_init+0x14/0x50
   ? __pfx_pci_iommu_init+0x10/0x10
   pci_iommu_init+0x12/0x40
   do_one_initcall+0x46/0x300

   kernel_init_freeable+0x23d/0x2a0
   ? __pfx_kernel_init+0x10/0x10
   kernel_init+0x1a/0x140
   ret_from_fork+0x34/0x50
   ? __pfx_kernel_init+0x10/0x10
   ret_from_fork_asm+0x1a/0x30
   </TASK>
  ---[ end trace 0000000000000000 ]---

Hi Robin,

Review

Quoted 6 lines · Robin Murphy

(quoted diff) +6 -0

+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");

This warning triggers on my workstation (with an AMD IOMMU), any ideas?

Quoted 12 lines · Joerg Roedel

Hi Robin,

Quoted 8 lines · Robin Murphy

+    /*
+     * And if we do now see any replay calls, they would indicate someone
+     * misusing the dma_configure path outside bus code. +     */
+    if (dev->driver)
+        dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");

Quoted 1 line

This warning triggers on my workstation (with an AMD IOMMU), any ideas?

Argh! When I moved the dma_configure call into iommu_init_device() for v2 I moved the warning with it, but of course that needs to stay where it was, *after* the point that ops->probe_device has had a chance to filter out irrelevant devices. Does this make it behave?

Argh! When I moved the dma_configure call into iommu_init_device() for v2 I moved the warning with it, but of course that needs to stay where it was, *after* the point that ops->probe_device has had a chance to filter out irrelevant devices. Does this make it behave?

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

  drivers/acpi/arm64/dma.c        |  5 +++++
  drivers/acpi/scan.c             |  7 -------
  drivers/amba/bus.c              |  3 ++-
  drivers/base/platform.c         |  3 ++-
  drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
  drivers/cdx/cdx.c               |  3 ++-
  drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
  drivers/iommu/of_iommu.c        |  7 ++++++-
  drivers/of/device.c             |  7 ++++++-
  drivers/pci/pci-driver.c        |  3 ++-
  10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +5 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
  	else
  		end = (1ULL << 32) - 1;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+
  	ret = acpi_dma_get_range(dev, &map);
  	if (!ret && map) {
  		end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -7

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
  		err = viot_iommu_configure(dev);
  	mutex_unlock(&iommu_probe_device_lock);
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-
  	return err;
  }
 

drivers/amba/bus.c +2 -1

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device *dev)
  		ret = acpi_dma_configure(dev, attr);
  	}
 
-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !drv->driver_managed_dma) {
  		ret = iommu_device_use_default_domain(dev);
  		if (ret)
  			arch_teardown_dma_ops(dev);

drivers/base/platform.c +2 -1

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct device *dev)
  		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
  		ret = acpi_dma_configure(dev, attr);
  	}
-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (ret || !dev->driver || drv->driver_managed_dma)
  		return ret;
 
  	ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl-mc/fsl-mc-bus.c +2 -1

index d1f3d327ddd1..a8be8cf246fb 100644
@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device *dev)
  	else
  		ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 
-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {
  		ret = iommu_device_use_default_domain(dev);
  		if (ret)
  			arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +2 -1

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device *dev)
  		return ret;
  	}
 
-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {
  		ret = iommu_device_use_default_domain(dev);
  		if (ret)
  			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +21 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
  	if (!dev_iommu_get(dev))
  		return -ENOMEM;
  	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is
+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a fwspec which will
+	 * match ops registered with a non-NULL fwnode, or we can reasonably
  	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
  	 * be present, and that any of their registered instances has suitable
  	 * ops for probing, and thus cheekily co-opt the same mechanism.

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
  		ret = -ENODEV;
  		goto err_free;
  	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
  	if (!try_module_get(ops->owner)) {
  		ret = -EINVAL;

drivers/iommu/of_iommu.c +6 -1

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
  		dev_iommu_free(dev);
  	mutex_unlock(&iommu_probe_device_lock);
 
-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */
+	if (!err && dev->bus && !dev_iommu_present)
  		err = iommu_probe_device(dev);
 
  	if (err && err != -EPROBE_DEFER)

drivers/of/device.c +6 -1

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
  	bool coherent, set_map = false;
  	int ret;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+
  	if (np == dev->of_node)
  		bus_np = __of_get_dma_parent(np);
  	else

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
  		end = dma_range_map_max(map);
  		set_map = true;
  	}
-
+skip_map:
  	/*
  	 * If @dev is expected to be DMA-capable then the bus code that created
  	 * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +0 -0

index f57ea36d125d..4468b7327cab 100644

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
  ESR = 0x0000000096000004
  EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
  EA = 0, S1PTW = 0
  FSC = 0x04: level 0 translation fault
Data abort info:
  ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0
  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[00000000000003e8] user address but active_mm is swapper

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
   devm_kmalloc+0x2c/0x114 (P)
   rk_iommu_of_xlate+0x30/0x90

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 5 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
    ESR = 0x0000000096000004
    EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x04: level 0 translation fault
Data abort info:
    ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0
    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
   devm_kmalloc+0x2c/0x114 (P)
   rk_iommu_of_xlate+0x30/0x90

Yeah, looks like this is doing something a bit questionable which can't work properly. TBH the whole dma_dev thing could probably be cleaned up now that we have proper instances, but for now does this work?

(annoyingly none of my Rockchip boards are set up for testing right now, but I might have time to dig one out later)

Thanks,
Robin.

----->8-----

Subject: [PATCH] iommu/rockchip: Allocate per-device data sensibly

Now that DT-based probing is finally happening in the right order again, it reveals an issue in Rockchip's of_xlate, which can now be called during registration, but is using the global dma_dev which is only assigned later. However, this makes little sense when we're already looking up the correct IOMMU device, who should logically be the owner of the devm allocation anyway.

Fixes bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe

path")

Signed-off-by Robin Murphy <[email protected]>

---
 drivers/iommu/rockchip-iommu.c | 6 +++---

 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/iommu/rockchip-iommu.c +1 -1

index 323cc665c357..48826d1ccfd8 100644
@@ -1148,12 +1148,12 @@ static int rk_iommu_of_xlate(struct device *dev,
      struct platform_device *iommu_dev;
      struct rk_iommudata *data;
 
-    data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
+    iommu_dev = of_find_device_by_node(args->np);

Quoted 35 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 5 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
    ESR = 0x0000000096000004
    EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
    EA = 0, S1PTW = 0
    FSC = 0x04: level 0 translation fault
Data abort info:
    ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
    devm_kmalloc+0x2c/0x114 (P)
    rk_iommu_of_xlate+0x30/0x90

Quoted 3 lines

Yeah, looks like this is doing something a bit questionable which can't work properly. TBH the whole dma_dev thing could probably be cleaned up now that we have proper instances, but for now does this work?

Yes, this patch fixes the problem I've observed.

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and I doubt it can be cleaned up.

Quoted 42 lines · Marek Szyprowski

Quoted 35 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 5 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
     ESR = 0x0000000096000004
     EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
     EA = 0, S1PTW = 0
     FSC = 0x04: level 0 translation fault
Data abort info:
     ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
    devm_kmalloc+0x2c/0x114 (P)
    rk_iommu_of_xlate+0x30/0x90

Quoted 3 lines

Yeah, looks like this is doing something a bit questionable which can't work properly. TBH the whole dma_dev thing could probably be cleaned up now that we have proper instances, but for now does this work?

Yes, this patch fixes the problem I've observed.

Quoted 5 lines

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and I doubt it can be cleaned up.

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Quoted 53 lines · Robin Murphy

Quoted 43 lines · Marek Szyprowski

Quoted 36 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 6 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I
found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
     ESR = 0x0000000096000004
     EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
     EA = 0, S1PTW = 0
     FSC = 0x04: level 0 translation fault
Data abort info:
     ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
    devm_kmalloc+0x2c/0x114 (P)
    rk_iommu_of_xlate+0x30/0x90

Quoted 5 lines

Yeah, looks like this is doing something a bit questionable which can't
work properly. TBH the whole dma_dev thing could probably be cleaned up
now that we have proper instances, but for now does this work?

Quoted 8 lines

Yes, this patch fixes the problem I've observed.

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and
I doubt it can be cleaned up.

Quoted 9 lines

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Bah, in fact I think I am going to have to do that now, since although it doesn't crash, rk_domain_alloc_paging() will also be failing for the same reason. Time to find a PSU for the RK3399 board, I guess...

(Or maybe just move the dma_dev assignment earlier to match Exynos?)

Quoted 68 lines · Robin Murphy

Quoted 53 lines · Robin Murphy

Quoted 43 lines · Marek Szyprowski

Quoted 36 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 6 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I
found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
     ESR = 0x0000000096000004
     EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
     EA = 0, S1PTW = 0
     FSC = 0x04: level 0 translation fault
Data abort info:
     ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
     devm_kmalloc+0x2c/0x114 (P)
     rk_iommu_of_xlate+0x30/0x90

Quoted 5 lines

Yeah, looks like this is doing something a bit questionable which can't
work properly. TBH the whole dma_dev thing could probably be cleaned up
now that we have proper instances, but for now does this work?

Quoted 8 lines

Yes, this patch fixes the problem I've observed.

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and
I doubt it can be cleaned up.

Quoted 9 lines

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Bah, in fact I think I am going to have to do that now, since although it doesn't crash, rk_domain_alloc_paging() will also be failing for the same reason. Time to find a PSU for the RK3399 board, I guess...

Quoted 1 line

(Or maybe just move the dma_dev assignment earlier to match Exynos?)

Well I just found that Exynos IOMMU is also broken on some on my test boards. It looks that the runtime pm links are somehow not correctly established. I will try to analyze this later in the afternoon.

Quoted 69 lines · Marek Szyprowski

Quoted 68 lines · Robin Murphy

Quoted 53 lines · Robin Murphy

Quoted 43 lines · Marek Szyprowski

Quoted 36 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 6 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I
found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
      ESR = 0x0000000096000004
      EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
      EA = 0, S1PTW = 0
      FSC = 0x04: level 0 translation fault Data abort info:
      ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
     devm_kmalloc+0x2c/0x114 (P)
     rk_iommu_of_xlate+0x30/0x90

Quoted 5 lines

Yeah, looks like this is doing something a bit questionable which can't
work properly. TBH the whole dma_dev thing could probably be cleaned up
now that we have proper instances, but for now does this work?

Quoted 8 lines

Yes, this patch fixes the problem I've observed.

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and
I doubt it can be cleaned up.

Quoted 9 lines

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Bah, in fact I think I am going to have to do that now, since although it doesn't crash, rk_domain_alloc_paging() will also be failing for the same reason. Time to find a PSU for the RK3399 board, I guess...

Quoted 5 lines

Quoted 1 line

(Or maybe just move the dma_dev assignment earlier to match Exynos?)

Well I just found that Exynos IOMMU is also broken on some on my test boards. It looks that the runtime pm links are somehow not correctly established. I will try to analyze this later in the afternoon.

Hmm, I tried to get an Odroid-XU3 up and running, but it seems unable to boot my original 6.14-rc3-based branch - even with the IOMMU driver disabled, it's consistently dying somewhere near (or just after) init with what looks like some catastrophic memory corruption issue - very occasionally it's managed to print the first line of various different panics.

Before that point though, with the IOMMU driver enabled it does appear to show signs of working OK:

[    0.649703] exynos-sysmmu 14650000.sysmmu: hardware version: 3.3 [    0.654220] platform 14450000.mixer: Adding to iommu group 1
...
[    2.680920] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42924000
...
[    5.196674] exynos-mixer 14450000.mixer:
exynos_iommu_identity_attach: Restored IOMMU to IDENTITY from pgtable 0x42924000
[    5.207091] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42884000

The multi-instance stuff in probe/release does look a bit suspect, however - seems like the second instance probe would overwrite the first instance's links, and then there would be a double-del() if the device were ever actually released again? I may have made that much more likely to happen, but I suspect it was already possible with async driver probe...

Quoted 99 lines · Robin Murphy

Quoted 69 lines · Marek Szyprowski

Quoted 68 lines · Robin Murphy

Quoted 53 lines · Robin Murphy

Quoted 43 lines · Marek Szyprowski

Quoted 36 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 6 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I
found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 13 lines

Unable to handle kernel NULL pointer dereference at virtual address

00000000000003e8
Mem abort info:
      ESR = 0x0000000096000004
      EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
      EA = 0, S1PTW = 0
      FSC = 0x04: level 0 translation fault
Data abort info:
      ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[00000000000003e8] user address but active_mm is swapper

Quoted 11 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ #15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
      devm_kmalloc+0x2c/0x114 (P)
      rk_iommu_of_xlate+0x30/0x90

Quoted 5 lines

Yeah, looks like this is doing something a bit questionable which can't
work properly. TBH the whole dma_dev thing could probably be cleaned up
now that we have proper instances, but for now does this work?

Quoted 8 lines

Yes, this patch fixes the problem I've observed.

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver and
I doubt it can be cleaned up.

Quoted 9 lines

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Bah, in fact I think I am going to have to do that now, since although it doesn't crash, rk_domain_alloc_paging() will also be failing for the same reason. Time to find a PSU for the RK3399 board, I guess...

Quoted 5 lines

Quoted 1 line

(Or maybe just move the dma_dev assignment earlier to match Exynos?)

Well I just found that Exynos IOMMU is also broken on some on my test boards. It looks that the runtime pm links are somehow not correctly established. I will try to analyze this later in the afternoon.

Hmm, I tried to get an Odroid-XU3 up and running, but it seems unable to boot my original 6.14-rc3-based branch - even with the IOMMU driver disabled, it's consistently dying somewhere near (or just after) init with what looks like some catastrophic memory corruption issue - very occasionally it's managed to print the first line of various different panics.

Before that point though, with the IOMMU driver enabled it does appear to show signs of working OK:

[    0.649703] exynos-sysmmu 14650000.sysmmu: hardware version: 3.3 [    0.654220] platform 14450000.mixer: Adding to iommu group 1
...
[    2.680920] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42924000
...
[    5.196674] exynos-mixer 14450000.mixer:
exynos_iommu_identity_attach: Restored IOMMU to IDENTITY from pgtable 0x42924000
[    5.207091] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42884000

Quoted 6 lines

The multi-instance stuff in probe/release does look a bit suspect, however - seems like the second instance probe would overwrite the first instance's links, and then there would be a double-del() if the device were ever actually released again? I may have made that much more likely to happen, but I suspect it was already possible with async driver probe...

That is really strange. My Odroid XU3 boots fine from commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"), although the IOMMU seems not to be working correctly. I've tested this with 14450000.mixer device (one need to attach HDMI cable to get it activated) and it looks that the video data are not being read from memory at all (the lack of VSYNC is reported, no IOMMU fault). However, from time to time, everything initializes and works properly.

It looks that this is somehow related to the different IOMMU/DMA-mapping glue code, as the other boards (ARM64 based) with exactly the same Exynos IOMMU driver always work fine. I've tried to figure out what actually happens, but so far I didn't get anything for sure. Disabling the call to dev->bus->dma_configure(dev) from iommu_init_device() seems to be fixing this, but this is almost equal to the revert of the $subject patch. I don't get why calling it in iommu_init_device() causes problems. It also doesn't look that this is anyhow related to the multi-instance stuff, as the same happens if I only leave a single exynos-sysmmu instance and its client (only 14450000.mixer device in the system).

Quoted 113 lines · Marek Szyprowski

Quoted 105 lines · Robin Murphy

Quoted 75 lines · Marek Szyprowski

Quoted 74 lines · Robin Murphy

Quoted 57 lines · Robin Murphy

Quoted 48 lines · Marek Szyprowski

Quoted 39 lines · Robin Murphy

On 2025-03-13 9:56 am, Marek Szyprowski wrote:
[...]

Quoted 7 lines

This patch landed in yesterday's linux-next as commit bcb81ac6ae3c
("iommu: Get DT/ACPI parsing into the proper probe path"). In my tests I
found it breaks booting of ARM64 RK3568-based Odroid-M1 board (arch/arm64/boot/dts/rockchip/rk3568-odroid-m1.dts). Here is the relevant kernel log:

...and the bug-flushing-out begins!

Quoted 14 lines

Unable to handle kernel NULL pointer dereference at virtual 

address
00000000000003e8
Mem abort info:
       ESR = 0x0000000096000004
       EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0
       EA = 0, S1PTW = 0
       FSC = 0x04: level 0 translation fault Data abort info:
       ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 CM = 0, WnR = 0, TnD = 0, TagAccess = 0 GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [00000000000003e8] user address but active_mm is swapper

Quoted 12 lines

Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.14.0-rc3+ 
#15533
Hardware name: Hardkernel ODROID-M1 (DT)
pstate: 00400009 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : devm_kmalloc+0x2c/0x114
lr : rk_iommu_of_xlate+0x30/0x90
...
Call trace:
      devm_kmalloc+0x2c/0x114 (P)
      rk_iommu_of_xlate+0x30/0x90

Quoted 5 lines

Yeah, looks like this is doing something a bit questionable which can't
work properly. TBH the whole dma_dev thing could probably be cleaned up
now that we have proper instances, but for now does this work?

Yes, this patch fixes the problem I've observed.

Quoted 7 lines

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

BTW, this dma_dev idea has been borrowed from my exynos_iommu driver
and
I doubt it can be cleaned up.

Quoted 10 lines

On the contrary I suspect they both can - it all dates back to when we had the single global platform bus iommu_ops and the SoC drivers were forced to bodge their own notion of multiple instances, but with
the modern core code, ops are always called via a valid IOMMU instance or domain, so in principle it should always be possible to get at an appropriate IOMMU device now. IIRC it was mostly about allocating and DMA-mapping the pagetables in domain_alloc, where the private notion of instances didn't have enough information, but domain_alloc_paging solves that.

Bah, in fact I think I am going to have to do that now, since although
it doesn't crash, rk_domain_alloc_paging() will also be failing for the same reason. Time to find a PSU for the RK3399 board, I guess...

Quoted 5 lines

Quoted 1 line

(Or maybe just move the dma_dev assignment earlier to match Exynos?)

Well I just found that Exynos IOMMU is also broken on some on my test boards. It looks that the runtime pm links are somehow not correctly established. I will try to analyze this later in the afternoon.

Hmm, I tried to get an Odroid-XU3 up and running, but it seems unable to boot my original 6.14-rc3-based branch - even with the IOMMU driver disabled, it's consistently dying somewhere near (or just after) init with what looks like some catastrophic memory corruption issue - very occasionally it's managed to print the first line of various different panics.

Before that point though, with the IOMMU driver enabled it does appear to show signs of working OK:

[    0.649703] exynos-sysmmu 14650000.sysmmu: hardware version: 3.3 [    0.654220] platform 14450000.mixer: Adding to iommu group 1
...
[    2.680920] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42924000
...
[    5.196674] exynos-mixer 14450000.mixer:
exynos_iommu_identity_attach: Restored IOMMU to IDENTITY from pgtable 0x42924000
[    5.207091] exynos-mixer 14450000.mixer:
exynos_iommu_attach_device: Attached IOMMU with pgtable 0x42884000

Quoted 6 lines

The multi-instance stuff in probe/release does look a bit suspect, however - seems like the second instance probe would overwrite the first instance's links, and then there would be a double-del() if the device were ever actually released again? I may have made that much more likely to happen, but I suspect it was already possible with async driver probe...

Quoted 7 lines

That is really strange. My Odroid XU3 boots fine from commit bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path"), although the IOMMU seems not to be working correctly. I've tested this with 14450000.mixer device (one need to attach HDMI cable to get it activated) and it looks that the video data are not being read from memory at all (the lack of VSYNC is reported, no IOMMU fault). However, from time to time, everything initializes and works properly.

Urgh, seems my mistake was assuming exynos_defconfig was the right thing to begin from - bcb81ac6ae3c with that still dies in the same way (this time I saw a hint of spin_bug() being hit...), however a multi_v7_defconfig build does get to userspace OK again with no obvious signs of distress:

[root@alarm ~]# grep -Hr . /sys/kernel/iommu_groups/*/type /sys/kernel/iommu_groups/0/type:identity /sys/kernel/iommu_groups/1/type:identity /sys/kernel/iommu_groups/10/type:identity /sys/kernel/iommu_groups/2/type:identity /sys/kernel/iommu_groups/3/type:identity /sys/kernel/iommu_groups/4/type:identity /sys/kernel/iommu_groups/5/type:identity /sys/kernel/iommu_groups/6/type:identity /sys/kernel/iommu_groups/7/type:identity /sys/kernel/iommu_groups/8/type:identity /sys/kernel/iommu_groups/9/type:identity

Annoyingly I do have an adapter for the fiddly micro-HDMI, but it's at home :(

Quoted 11 lines

It looks that this is somehow related to the different IOMMU/DMA-mapping glue code, as the other boards (ARM64 based) with exactly the same Exynos IOMMU driver always work fine. I've tried to figure out what actually happens, but so far I didn't get anything for sure. Disabling the call to dev->bus->dma_configure(dev) from iommu_init_device() seems to be fixing this, but this is almost equal to the revert of the $subject patch. I don't get why calling it in iommu_init_device() causes problems. It also doesn't look that this is anyhow related to the multi-instance stuff, as the same happens if I only leave a single exynos-sysmmu instance and its client (only 14450000.mixer device in the system).

On a hunch I stuck a print in exynos_iommu_probe_device(), and it looks like in fact device_link_add() isn't getting called at all, and indeed your symptoms do sound like they could be explained by the IOMMU not being reliably resumed... lemme stare at exynos_iommu_of_xlate() a bit longer...

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

Thanks for your patch, which is now commit bcb81ac6ae3c2ef9 ("iommu:
Get DT/ACPI parsing into the proper probe path") in iommu/next.

This patch triggers two issues on R-Car Gen3 platforms:

1. I am seeing a warning on Renesas Salvator-XS with R-Car M3N (but not on the similar board with R-Car H3), and only for SATA[1]. Unfortunately commit 73d2f10957f517e5 ("iommu: Don't warn prematurely about dodgy probes") does not help:

     Call trace:
      __iommu_probe_device+0x208/0x38c (P)
      iommu_probe_device+0x34/0x74
      of_iommu_configure+0x128/0x200
      of_dma_configure_id+0xdc/0x1d4
      platform_dma_configure+0x48/0x6c
      really_probe+0xf0/0x260
      __driver_probe_device+0xec/0x104
      driver_probe_device+0x3c/0xc0

2. The IOMMU driver's iommu_ops.of_xlate() callback is called about three times as much as before:

On 18/03/2025 4:37 pm, Geert Uytterhoeven wrote:
[...]

Quoted 9 lines

Thanks for your patch, which is now commit bcb81ac6ae3c2ef9 ("iommu:
Get DT/ACPI parsing into the proper probe path") in iommu/next.

This patch triggers two issues on R-Car Gen3 platforms:

1. I am seeing a warning on Renesas Salvator-XS with R-Car M3N (but not on the similar board with R-Car H3), and only for SATA[1]. Unfortunately commit 73d2f10957f517e5 ("iommu: Don't warn prematurely about dodgy probes") does not help:

[...]

Quoted 9 lines

     Call trace:
      __iommu_probe_device+0x208/0x38c (P)
      iommu_probe_device+0x34/0x74
      of_iommu_configure+0x128/0x200
      of_dma_configure_id+0xdc/0x1d4
      platform_dma_configure+0x48/0x6c
      really_probe+0xf0/0x260
      __driver_probe_device+0xec/0x104
      driver_probe_device+0x3c/0xc0

Hurrah, this is the warning doing the correct job - something *is* off if we're now getting here without the IOMMU configuration being done already (for a normal device with no other funny business going on).

Quoted 2 lines

2. The IOMMU driver's iommu_ops.of_xlate() callback is called about three times as much as before:

That would suggest that the fwspec gets set up OK, then something later in the __iommu_probe_device() path fails and tears it down again, so the next attempt starts from scratch. Do you see the "Cannot attach to IPMMU" message firing?

And similarly to the Rockchip case, does the
below help?

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++
 drivers/acpi/scan.c             |  7 -------
 drivers/amba/bus.c              |  3 ++-
 drivers/base/platform.c         |  3 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
 drivers/cdx/cdx.c               |  3 ++-
 drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
 drivers/iommu/of_iommu.c        |  7 ++++++-
 drivers/of/device.c             |  7 ++++++-
 drivers/pci/pci-driver.c        |  3 ++-
 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +5 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
 	else
 		end = (1ULL << 32) - 1;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
 		end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -7

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 		err = viot_iommu_configure(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-
 	return err;
 }
 

drivers/amba/bus.c +2 -1

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device *dev)
 		ret = acpi_dma_configure(dev, attr);
 	}
 
-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/base/platform.c +2 -1

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct device *dev)
 		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
 		ret = acpi_dma_configure(dev, attr);
 	}
-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (ret || !dev->driver || drv->driver_managed_dma)
 		return ret;
 
 	ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl-mc/fsl-mc-bus.c +2 -1

index d1f3d327ddd1..a8be8cf246fb 100644
@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device *dev)
 	else
 		ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 
-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +2 -1

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device *dev)
 		return ret;
 	}
 
-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +21 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;
 	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is
+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a fwspec which will
+	 * match ops registered with a non-NULL fwnode, or we can reasonably
 	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
 	 * be present, and that any of their registered instances has suitable
 	 * ops for probing, and thus cheekily co-opt the same mechanism.

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
 		ret = -ENODEV;
 		goto err_free;
 	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;

drivers/iommu/of_iommu.c +6 -1

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
 		dev_iommu_free(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */
+	if (!err && dev->bus && !dev_iommu_present)
 		err = iommu_probe_device(dev);
 
 	if (err && err != -EPROBE_DEFER)

drivers/of/device.c +6 -1

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	bool coherent, set_map = false;
 	int ret;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+
 	if (np == dev->of_node)
 		bus_np = __of_get_dma_parent(np);
 	else

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		end = dma_range_map_max(map);
 		set_map = true;
 	}
-
+skip_map:
 	/*
 	 * If @dev is expected to be DMA-capable then the bus code that created
 	 * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +2 -1

index f57ea36d125d..4468b7327cab 100644
@@ -1653,7 +1653,8 @@ static int pci_dma_configure(struct device *dev)
 
 	pci_put_host_bridge_device(bridge);
 
-	if (!ret && !driver->driver_managed_dma) {
+	/* @driver may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !driver->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

Hi,

Quoted 73 lines · Robin Murphy

In hindsight, there were some crucial subtleties overlooked when moving
{of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental
assumptions that iommu_probe_device() is called for every device added
to the system, in the order in which they are added. Calling it in a
random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions.
We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come
to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two
distinct purposes; one is identifying the IOMMU instance associated with
a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the
device. However the latter also depends on the former, and at the time
there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in
the core API itself, and each one gets a chance to probe its own devices
upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to
be able to run the iommu_fwspec machinery currently buried deep in the
wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty
much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really
should *not* have a driver already, so we can use that as a condition to
disambiguate the two cases, and avoid recursing back into the IOMMU core
at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many
more awkward cross-subsystem patches. There are some minor side- effects
like dma_range_map potentially being created earlier, and some debug
prints being repeated, but these aren't significantly detrimental. Let's
make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is
probably turning the bus->dma_configure paths inside-out, since all we
really need from bus code is its notion of which device and input ID(s)
to parse the common firmware properties with...

Quoted 3 lines

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

This change causes the MediaTek IOMMU driver to panic on probe, resulting in multiple MediaTek platforms not being able to boot. This was observed on Linus's tree at commit 1a9239bb4253 ("Merge tag 'net-next-6.15' of
git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next")
which just received the IOMMU updates a couple merge commits prior.

The regression was bisected down to this patch on MT8183 Juniper Chromebook. The error is a NULL pointer dereference. Here's the decoded backtrace:

    Unable to handle kernel read from unreadable memory at virtual

address 0000000000000000
    Mem abort info:
      ESR = 0x0000000096000005
    usb 1-1.1.2: new high-speed USB device number 6 using xhci-mtk EC = 0x25: DABT (current EL), IL = 32 bits
      SET = 0, FnV = 0
      EA = 0, S1PTW = 0
      FSC = 0x05: level 1 translation fault
    Data abort info:
      ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
      CM = 0, WnR = 0, TnD = 0, TagAccess = 0
      GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [0000000000000000] user address but active_mm is swapper

    Internal error: Oops: 0000000096000005 [#1]  SMP
    Modules linked in:
    CPU: 4 UID: 0 PID: 68 Comm: kworker/u34:1 Tainted: G   
B              6.14.0-05877-g1a9239bb4253 #621 PREEMPT 
a6631d3f04612a5c23866dc67cf38316c6b023e0
    Tainted: [B]=BAD_PAGE
    Hardware name: Google juniper sku16 board (DT)
    Workqueue: events_unbound deferred_probe_work_func
    pstate: 40400005 (nZcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
    pc : mtk_iommu_device_group (drivers/iommu/mtk_iommu.c:368
drivers/iommu/mtk_iommu.c:940)
    lr : mtk_iommu_device_group (drivers/iommu/mtk_iommu.c:368

drivers/iommu/mtk_iommu.c:940)
    sp : ffffffc0809674a0
    x29: ffffffc0809674a0 x28: ffffff80c1f0f078 x27: ffffff80c1f0f460
    x26: ffffff80c1f0f458 x25: ffffffdb1b738788 x24: ffffffc0809676d0
    x23: ffffff80c8fe4130 x22: ffffffffffffffed x21: 0000000000000000
    x20: ffffff80c1f0f010 x19: ffffff80c8fd7d00 x18: 0000000000000000
    x17: 000000040044ffff x16: 00500072b5593510 x15: 0000000000000000
    x14: ffffff80c09fbb80 x13: ffffffa5bf03f000 x12: ffffffbb6399cdf1
    x11: 1ffffffb6399cdf0 x10: ffffffbb6399cdf0 x9 : dfffffc000000000 x8 : 000000449c663210 x7 : ffffffdb1cce6f87 x6 : 0000000000000001 x5 : ffffffdb1cce6f80 x4 : ffffffbb6399cdf1 x3 : 0000000000000000 x2 : 0000000000000020 x1 : ffffff80c22ed940 x0 : 0000000000000001

    Call trace:
    mtk_iommu_device_group (drivers/iommu/mtk_iommu.c:368

drivers/iommu/mtk_iommu.c:940) (P) __iommu_probe_device (drivers/iommu/iommu.c:461 drivers/iommu/iommu.c:563)
    probe_iommu_group (drivers/iommu/iommu.c:1722) bus_for_each_dev (drivers/base/bus.c:370) iommu_device_register (drivers/iommu/iommu.c:1875 drivers/iommu/iommu.c:276)
    mtk_iommu_probe (drivers/iommu/mtk_iommu.c:1380) platform_probe (drivers/base/platform.c:1404) really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658) __driver_probe_device (drivers/base/dd.c:800) driver_probe_device (drivers/base/dd.c:830) __device_attach_driver (drivers/base/dd.c:959) bus_for_each_drv (drivers/base/bus.c:462) __device_attach (drivers/base/dd.c:1032) device_initial_probe (drivers/base/dd.c:1080) bus_probe_device (drivers/base/bus.c:537) deferred_probe_work_func (drivers/base/dd.c:124) process_one_work (./arch/arm64/include/asm/jump_label.h:36 ./include/trace/events/workqueue.h:110 kernel/workqueue.c:3243) worker_thread (kernel/workqueue.c:3313 (discriminator 2) kernel/workqueue.c:3400 (discriminator 2))
    kthread (kernel/kthread.c:464)
    ret_from_fork (arch/arm64/kernel/entry.S:863)

    Code: 92800256 f940d2b5 aa1503e0 97e8dbd7 (f94002b5)

    All code
    ========
       0: 92800256  mov x22,
#0xffffffffffffffed     // #-19
       4: f940d2b5  ldr x21, [x21, #416]
       8: aa1503e0  mov x0, x21
       c: 97e8dbd7  bl 0xffffffffffa36f68
      10:* f94002b5  ldr x21, [x21] <--
trapping instruction

    Code starting with the faulting instruction
    ===========================================
       0: f94002b5  ldr x21, [x21]

    ---[ end trace 0000000000000000 ]---

ChenYu

Quoted 255 lines

---

v2:
 - Comment bus driver changes for clarity
 - Use dev->iommu as the now-robust replay condition
 - Drop the device_iommu_mapped() checks in the firmware paths as they
   weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++ drivers/acpi/scan.c             |  7 ------- drivers/amba/bus.c              |  3 ++- drivers/base/platform.c         |  3 ++- drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++- drivers/cdx/cdx.c               |  3 ++- drivers/iommu/iommu.c           | 24 +++++++++++++++++++++--- drivers/iommu/of_iommu.c        |  7 ++++++- drivers/of/device.c             |  7 ++++++- drivers/pci/pci-driver.c        |  3 ++-

 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +0 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)

  else
  end = (1ULL << 32) - 1;

drivers/acpi/arm64/dma.c +5 -0

+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+

  ret = acpi_dma_get_range(dev, &map);
  if (!ret && map) {
  end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -0

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct
 device *dev, const u32 *id_in)
  		err = viot_iommu_configure(dev);
  	mutex_unlock(&iommu_probe_device_lock);

drivers/acpi/scan.c +0 -2

-	/*
-	 * If we have reason to believe the IOMMU driver missed

the initial

drivers/acpi/scan.c +0 -1

-	 * iommu_probe_device() call for dev, replay it to get

things in order.

drivers/acpi/scan.c +0 -4

-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-

  return err;
 }

drivers/amba/bus.c +0 -0

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device
 *dev)
  		ret = acpi_dma_configure(dev, attr);
  	}

drivers/amba/bus.c +1 -1

-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU

layer */

drivers/amba/bus.c +1 -0

+	if (!ret && dev->driver && !drv->driver_managed_dma) {

  ret = iommu_device_use_default_domain(dev); if (ret) arch_teardown_dma_ops(dev);

drivers/base/platform.c +0 -0

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct
 device *dev)

  attr = acpi_get_dma_attr(to_acpi_device_node(fwnode)); ret = acpi_dma_configure(dev, attr); }

drivers/base/platform.c +1 -1

-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU

layer */

drivers/base/platform.c +1 -0

+	if (ret || !dev->driver || drv->driver_managed_dma)

  return ret;

  ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl- renamed from drivers/bus/fsl-mc/fsl-mc-bus.c +0 -0

mc/fsl-mc-bus.c
index d1f3d327ddd1..a8be8cf246fb 100644

drivers/bus/fsl-mc/fsl-mc-bus.c +0 -0

@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device
 *dev)

  else
  ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);

drivers/bus/fsl-mc/fsl-mc-bus.c +1 -1

-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the

IOMMU layer */

drivers/bus/fsl-mc/fsl-mc-bus.c +1 -0

+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {

  ret = iommu_device_use_default_domain(dev); if (ret) arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +0 -0

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device
 *dev)
  		return ret;
  	}

drivers/cdx/cdx.c +1 -1

-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the

IOMMU layer */

drivers/cdx/cdx.c +1 -0

+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {

  ret = iommu_device_use_default_domain(dev); if (ret) arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +0 -1

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device
 *dev)
  	if (!dev_iommu_get(dev))
  		return -ENOMEM;
  	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers

register IOMMU

drivers/iommu/iommu.c +0 -1

-	 * instances with non-NULL fwnodes, and client devices

should have been

drivers/iommu/iommu.c +0 -1

-	 * identified with a fwspec by this point. Otherwise, we

can currently

drivers/iommu/iommu.c +1 -0

+	 * For FDT-based systems and ACPI IORT/VIOT, the common

firmware parsing

drivers/iommu/iommu.c +1 -0

+	 * is buried in the bus dma_configure path. Properly

unpicking that is

drivers/iommu/iommu.c +1 -0

+	 * still a big job, so for now just invoke the whole

thing. The device

drivers/iommu/iommu.c +1 -0

+	 * already having a driver bound means dma_configure has

already run and

drivers/iommu/iommu.c +1 -0

+	 * either found no IOMMU to wait for, or we're in its

replay call right

drivers/iommu/iommu.c +9 -0

+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a

fwspec which will

drivers/iommu/iommu.c +1 -0

+	 * match ops registered with a non-NULL fwnode, or we can

reasonably
  * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
  * be present, and that any of their registered instances has suitable
  * ops for probing, and thus cheekily co-opt the same mechanism.

drivers/iommu/iommu.c +2 -0

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device
 *dev)
  		ret = -ENODEV;
  		goto err_free;
  	}
+	/*
+	 * And if we do now see any replay calls, they would

indicate someone

drivers/iommu/iommu.c +4 -0

+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind,

something fishy here!\n");

  if (!try_module_get(ops->owner)) {
  ret = -EINVAL;

drivers/iommu/of_iommu.c +0 -0

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev,
 struct device_node *master_np,
  		dev_iommu_free(dev);
  	mutex_unlock(&iommu_probe_device_lock);

drivers/iommu/of_iommu.c +2 -1

-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as

indicated by the

drivers/iommu/of_iommu.c +1 -0

+	 * initial dev->iommu) then try to simulate it. This

should no longer

drivers/iommu/of_iommu.c +1 -0

+	 * happen unless of_dma_configure() is being misused

outside bus code.

drivers/iommu/of_iommu.c +2 -0

+	 */
+	if (!err && dev->bus && !dev_iommu_present)

  err = iommu_probe_device(dev);

  if (err && err != -EPROBE_DEFER)

drivers/of/device.c +0 -0

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev,
 struct device_node *np,
  	bool coherent, set_map = false;
  	int ret;

drivers/of/device.c +5 -0

+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+

  if (np == dev->of_node)
  bus_np = __of_get_dma_parent(np);
  else

drivers/of/device.c +1 -1

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev,
 struct device_node *np,
  		end = dma_range_map_max(map);
  		set_map = true;
  	}
-
+skip_map:
  	/*
  	 * If @dev is expected to be DMA-capable then the bus code

that created
  * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +0 -0

index f57ea36d125d..4468b7327cab 100644
@@ -1653,7 +1653,8 @@ static int pci_dma_configure(struct device
 *dev)

  pci_put_host_bridge_device(bridge);

drivers/pci/pci-driver.c +1 -1

-	if (!ret && !driver->driver_managed_dma) {
+	/* @driver may not be valid when we're called from the

IOMMU layer */

drivers/pci/pci-driver.c +1 -0

+	if (!ret && dev->driver && !driver->driver_managed_dma) {

  ret = iommu_device_use_default_domain(dev); if (ret) arch_teardown_dma_ops(dev);

Signature · 1 line

2.39.2.101.g768bb238c484.dirty

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

drivers/iommu/iommu.c +6 -0

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
 		ret = -ENODEV;
 		goto err_free;
 	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;

drivers/iommu/of_iommu.c +0 -0

index e10a68b5ffde..6b989a62def2 100644

Hi Robin,

Quoted 34 lines · Robin Murphy

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

drivers/iommu/iommu.c +6 -0

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
  		ret = -ENODEV;
  		goto err_free;
  	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
  	if (!try_module_get(ops->owner)) {
  		ret = -EINVAL;

drivers/iommu/of_iommu.c +0 -0

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
  		dev_iommu_free(dev);
  	mutex_unlock(&iommu_probe_device_lock);
 

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 162

Quoted 6 lines

drivers/iommu/of_iommu.c +5 -1

-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */

This assumption does not hold as there is nothing preventing iommu driver probe from racing with a client driver probe.

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 166

Quoted 4 lines

drivers/iommu/of_iommu.c +1 -0

+	if (!err && dev->bus && !dev_iommu_present)
  		err = iommu_probe_device(dev);
 
  	if (err && err != -EPROBE_DEFER)

I hit the (now moved) dev_WARN() on the ThinkPad T14s where the GPU SMMU is probed late due to a clock dependency and can end up probing in parallel with the GPU driver.

[    3.805282] arm-smmu 3da0000.iommu: probing hardware configuration...
[    3.806007] arm-smmu 3da0000.iommu: SMMUv2 with:
[    3.806843] arm-smmu 3da0000.iommu:  stage 1 translation
[    3.807562] arm-smmu 3da0000.iommu:  coherent table walk
[    3.808253] arm-smmu 3da0000.iommu:  stream matching with 24 register groups
[    3.808957] arm-smmu 3da0000.iommu:  22 context banks (0 stage-2 only)
[    3.809651] arm-smmu 3da0000.iommu:  Supported page sizes: 0x61311000
[    3.810339] arm-smmu 3da0000.iommu:  Stage-1: 48-bit VA -> 40-bit IPA
[    3.811130] arm-smmu 3da0000.iommu:  preserved 0 boot mappings

[    3.829042] platform 3d6a000.gmu: Adding to iommu group 8

[    3.992050] ------------[ cut here ]------------
[    3.993045] adreno 3d00000.gpu: late IOMMU probe at driver bind, something fishy here!
[    3.994058] WARNING: CPU: 9 PID: 343 at drivers/iommu/iommu.c:579 __iommu_probe_device+0x2b0/0x4ac

[    4.003272] CPU: 9 UID: 0 PID: 343 Comm: kworker/u50:2 Not tainted 6.15.0-rc1 #109 PREEMPT
[    4.003276] Hardware name: LENOVO 21N2ZC5PUS/21N2ZC5PUS, BIOS N42ET83W (2.13 ) 10/04/2024

[    4.025943] Call trace:
[    4.025945]  __iommu_probe_device+0x2b0/0x4ac (P)
[    4.030453]  iommu_probe_device+0x38/0x7c
[    4.030455]  of_iommu_configure+0x188/0x26c
[    4.030457]  of_dma_configure_id+0xcc/0x300
[    4.030460]  platform_dma_configure+0x74/0xac
[    4.030462]  really_probe+0x74/0x38c

[    4.030464]  __driver_probe_device+0x7c/0x160
[    4.030465]  driver_probe_device+0x40/0x110
[    4.030467]  __device_attach_driver+0xbc/0x158
[    4.030468]  bus_for_each_drv+0x84/0xe0
[    4.030470]  __device_attach+0xa8/0x1d4
[    4.030472]  device_initial_probe+0x14/0x20
[    4.030473]  bus_probe_device+0xb0/0xb4
[    4.030476]  deferred_probe_work_func+0xa0/0xf4

[    4.030501] ---[ end trace 0000000000000000 ]---
[    4.031269] adreno 3d00000.gpu: Adding to iommu group 9

Johan

Quoted 6 lines · Johan Hovold

Quoted 4 lines · Robin Murphy

drivers/iommu/of_iommu.c +0 -0

@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
  		dev_iommu_free(dev);
  	mutex_unlock(&iommu_probe_device_lock);
 

Quoted 9 lines

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 162

Quoted 6 lines

drivers/iommu/of_iommu.c +5 -1

-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */

This assumption does not hold as there is nothing preventing iommu driver probe from racing with a client driver probe.

Not sure I follow - *this* assumption is that if we arrived here with dev->iommu already allocated then __iommu_probe_device() is already in progress for this device, either in the current callchain or on another thread, and so we can (and should) skip calling into it again. There's no ambiguity about that.

Quoted 8 lines

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 166

Quoted 4 lines

drivers/iommu/of_iommu.c +1 -0

+	if (!err && dev->bus && !dev_iommu_present)
  		err = iommu_probe_device(dev);
 
  	if (err && err != -EPROBE_DEFER)

I hit the (now moved) dev_WARN() on the ThinkPad T14s where the GPU SMMU is probed late due to a clock dependency and can end up probing in parallel with the GPU driver.

And what *should* happen is that the GPU driver probe waits for the IOMMU driver probe to finish. Do you have fw_devlink enabled?

Quoted 11 lines

[    3.805282] arm-smmu 3da0000.iommu: probing hardware configuration...

[    3.829042] platform 3d6a000.gmu: Adding to iommu group 8

[    3.992050] ------------[ cut here ]------------
[    3.993045] adreno 3d00000.gpu: late IOMMU probe at driver bind, something fishy here!
[    3.994058] WARNING: CPU: 9 PID: 343 at drivers/iommu/iommu.c:579 __iommu_probe_device+0x2b0/0x4ac

[    4.003272] CPU: 9 UID: 0 PID: 343 Comm: kworker/u50:2 Not tainted 6.15.0-rc1 #109 PREEMPT
[    4.003276] Hardware name: LENOVO 21N2ZC5PUS/21N2ZC5PUS, BIOS N42ET83W (2.13 ) 10/04/2024

Quoted 7 lines

[    4.025943] Call trace:
[    4.025945]  __iommu_probe_device+0x2b0/0x4ac (P)
[    4.030453]  iommu_probe_device+0x38/0x7c
[    4.030455]  of_iommu_configure+0x188/0x26c
[    4.030457]  of_dma_configure_id+0xcc/0x300
[    4.030460]  platform_dma_configure+0x74/0xac
[    4.030462]  really_probe+0x74/0x38c

Indeed this is exactly what is *not* supposed to be happening - does this patch help at all?

https://lore.kernel.org/linux-iommu/09d901ad11b3a410fbb6e27f7d04ad4609c3fe4a.1741706365.git.robin.murphy@arm.com/

Quoted 17 lines · Robin Murphy

Quoted 6 lines · Johan Hovold

Quoted 4 lines · Robin Murphy

drivers/iommu/of_iommu.c +0 -0

@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
   		dev_iommu_free(dev);
   	mutex_unlock(&iommu_probe_device_lock);
 

Quoted 9 lines

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 162

Quoted 6 lines

drivers/iommu/of_iommu.c +5 -1

-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */

This assumption does not hold as there is nothing preventing iommu driver probe from racing with a client driver probe.

Quoted 5 lines

Not sure I follow - *this* assumption is that if we arrived here with dev->iommu already allocated then __iommu_probe_device() is already in progress for this device, either in the current callchain or on another thread, and so we can (and should) skip calling into it again. There's no ambiguity about that.

I was referring to the this "should no longer happen unless of_dma_configure() is being misused outside bus code" claim, which appears to be false given the splat below.

Quoted 11 lines

Quoted 8 lines

drivers/iommu/of_iommu.c int of_iommu_configure(struct device *dev, struct device_node *master_np, · line 166

Quoted 4 lines

drivers/iommu/of_iommu.c +1 -0

+	if (!err && dev->bus && !dev_iommu_present)
   		err = iommu_probe_device(dev);
 
   	if (err && err != -EPROBE_DEFER)

I hit the (now moved) dev_WARN() on the ThinkPad T14s where the GPU SMMU is probed late due to a clock dependency and can end up probing in parallel with the GPU driver.

And what *should* happen is that the GPU driver probe waits for the IOMMU driver probe to finish. Do you have fw_devlink enabled?

Yes, but you shouldn't rely on devlinks for correctness.

That said it does seem like something is not working as you think it is here, and indeed the iommu supplier link is not created until SMMUv2 probe_device() (see arm_smmu_probe_device()).

So client devices can start to be probed (bus dma_configure() is called) before their iommu is ready also with devlinks enabled (and I do see this happen on every boot).

Quoted 22 lines

Quoted 11 lines

[    3.805282] arm-smmu 3da0000.iommu: probing hardware configuration...

[    3.829042] platform 3d6a000.gmu: Adding to iommu group 8

[    3.992050] ------------[ cut here ]------------
[    3.993045] adreno 3d00000.gpu: late IOMMU probe at driver bind, something fishy here!
[    3.994058] WARNING: CPU: 9 PID: 343 at drivers/iommu/iommu.c:579 __iommu_probe_device+0x2b0/0x4ac

[    4.003272] CPU: 9 UID: 0 PID: 343 Comm: kworker/u50:2 Not tainted 6.15.0-rc1 #109 PREEMPT
[    4.003276] Hardware name: LENOVO 21N2ZC5PUS/21N2ZC5PUS, BIOS N42ET83W (2.13 ) 10/04/2024

Quoted 7 lines

[    4.025943] Call trace:
[    4.025945]  __iommu_probe_device+0x2b0/0x4ac (P)
[    4.030453]  iommu_probe_device+0x38/0x7c
[    4.030455]  of_iommu_configure+0x188/0x26c
[    4.030457]  of_dma_configure_id+0xcc/0x300
[    4.030460]  platform_dma_configure+0x74/0xac
[    4.030462]  really_probe+0x74/0x38c

Indeed this is exactly what is *not* supposed to be happening - does this patch help at all?

Quoted 1 line

https://lore.kernel.org/linux-iommu/09d901ad11b3a410fbb6e27f7d04ad4609c3fe4a.1741706365.git.robin.murphy@arm.com/

I've only seen that splat once so far so I don't have a reliable reproducer.

But AFAICS that patch won't help help here where we appear to have iommu probe racing with bus dma_configure() called from really_probe() for the client device.

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++
 drivers/acpi/scan.c             |  7 -------
 drivers/amba/bus.c              |  3 ++-
 drivers/base/platform.c         |  3 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
 drivers/cdx/cdx.c               |  3 ++-
 drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
 drivers/iommu/of_iommu.c        |  7 ++++++-
 drivers/of/device.c             |  7 ++++++-
 drivers/pci/pci-driver.c        |  3 ++-
 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/base/platform.c +0 -0

index 6f2a33722c52..1813cfd0c4bd 100644

drivers/base/platform.c +1 -2

index 1813cfd0c4bd..6d124447545c 100644
@@ -1440,8 +1440,8 @@ static void platform_shutdown(struct device *_dev)
 
 static int platform_dma_configure(struct device *dev)
 {
-       struct platform_driver *drv = to_platform_driver(dev->driver);
        struct fwnode_handle *fwnode = dev_fwnode(dev);
+       struct platform_driver *drv;
        enum dev_dma_attr attr;
        int ret = 0;
 

@@ -1451,8 +1451,12 @@ static int platform_dma_configure(struct device *dev)
                attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
                ret = acpi_dma_configure(dev, attr);
        }
-       /* @drv may not be valid when we're called from the IOMMU layer */

Quoted 19 lines · William McVicker

drivers/base/platform.c +1 -2

index 1813cfd0c4bd..6d124447545c 100644
@@ -1440,8 +1440,8 @@ static void platform_shutdown(struct device *_dev)
 
 static int platform_dma_configure(struct device *dev)
 {
-       struct platform_driver *drv = to_platform_driver(dev->driver);
        struct fwnode_handle *fwnode = dev_fwnode(dev);
+       struct platform_driver *drv;
        enum dev_dma_attr attr;
        int ret = 0;
 

@@ -1451,8 +1451,12 @@ static int platform_dma_configure(struct device *dev)
                attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
                ret = acpi_dma_configure(dev, attr);
        }
-       /* @drv may not be valid when we're called from the IOMMU layer */

drivers/base/platform.c static int platform_dma_configure(struct device *dev) · line 1462

Quoted 10 lines

drivers/base/platform.c +6 -1

-       if (ret || !dev->driver || drv->driver_managed_dma)
+       /* @dev->driver may not be valid when we're called from the IOMMU layer */
+       if (ret || !dev->driver)
+               return ret;
+
+       drv = to_platform_driver(dev->driver);
+       if (drv->driver_managed_dma)
                return ret;
 
        ret = iommu_device_use_default_domain(dev);

The diagnosis looks right to me, but pedantically I think it should have a READ_ONCE():

struct driver *drv = READ_ONCE(dev->driver);

And then never touch dev->driver again in the function.

Send a proper patch?

Jason

On this note, I was looking through of_dma_configure_id() and am also wondering if we may hit other race conditions if the device is still being probed and the dma properties (like the coherent dma mask) haven't been fully populated? Just checking if the driver is bound, doesn't seem like enough to start configuring the DMA when async probing can happen.

Quoted 5 lines · William McVicker

On this note, I was looking through of_dma_configure_id() and am also wondering if we may hit other race conditions if the device is still being probed and the dma properties (like the coherent dma mask) haven't been fully populated? Just checking if the driver is bound, doesn't seem like enough to start configuring the DMA when async probing can happen.

I think the reasoning at work here is that the plugin path for a struct device should synchronously setup the iommu.

There is enough locking there that the iommu code won't allow the device plugin to continue until the iommu is fully setup under the global lock.

The trick of using dev->driver is only a way to tell if this function is being called from the driver plugin path just before starting the driver, or from the iommu code just before configuring the iommu.

Given that explanation can you see issues with of_dma_configure_id() ?

Jason

Quoted 19 lines · Jason Gunthorpe

Quoted 5 lines · William McVicker

On this note, I was looking through of_dma_configure_id() and am also wondering if we may hit other race conditions if the device is still being probed and the dma properties (like the coherent dma mask) haven't been fully populated? Just checking if the driver is bound, doesn't seem like enough to start configuring the DMA when async probing can happen.

I think the reasoning at work here is that the plugin path for a struct device should synchronously setup the iommu.

There is enough locking there that the iommu code won't allow the device plugin to continue until the iommu is fully setup under the global lock.

The trick of using dev->driver is only a way to tell if this function is being called from the driver plugin path just before starting the driver, or from the iommu code just before configuring the iommu.

Quoted 3 lines

Given that explanation can you see issues with of_dma_configure_id() ?

Jason

I think the only concern is when a driver calls dma_set_mask_and_coherent() in it's probe function. If we can handle that case in an asynchrounous manner, then I think we are good.

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++
 drivers/acpi/scan.c             |  7 -------
 drivers/amba/bus.c              |  3 ++-
 drivers/base/platform.c         |  3 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
 drivers/cdx/cdx.c               |  3 ++-
 drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
 drivers/iommu/of_iommu.c        |  7 ++++++-
 drivers/of/device.c             |  7 ++++++-
 drivers/pci/pci-driver.c        |  3 ++-
 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +5 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
 	else
 		end = (1ULL << 32) - 1;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
 		end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -7

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 		err = viot_iommu_configure(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-
 	return err;
 }
 

drivers/amba/bus.c +2 -1

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device *dev)
 		ret = acpi_dma_configure(dev, attr);
 	}
 
-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/base/platform.c +2 -1

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct device *dev)
 		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
 		ret = acpi_dma_configure(dev, attr);
 	}
-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (ret || !dev->driver || drv->driver_managed_dma)
 		return ret;
 
 	ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl-mc/fsl-mc-bus.c +2 -1

index d1f3d327ddd1..a8be8cf246fb 100644
@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device *dev)
 	else
 		ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 
-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +2 -1

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device *dev)
 		return ret;
 	}
 
-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +21 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;
 	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is
+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a fwspec which will
+	 * match ops registered with a non-NULL fwnode, or we can reasonably
 	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
 	 * be present, and that any of their registered instances has suitable
 	 * ops for probing, and thus cheekily co-opt the same mechanism.

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
 		ret = -ENODEV;
 		goto err_free;
 	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;

drivers/iommu/of_iommu.c +6 -1

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
 		dev_iommu_free(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */
+	if (!err && dev->bus && !dev_iommu_present)
 		err = iommu_probe_device(dev);
 
 	if (err && err != -EPROBE_DEFER)

drivers/of/device.c +6 -1

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	bool coherent, set_map = false;
 	int ret;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+
 	if (np == dev->of_node)
 		bus_np = __of_get_dma_parent(np);
 	else

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		end = dma_range_map_max(map);
 		set_map = true;
 	}
-
+skip_map:
 	/*
 	 * If @dev is expected to be DMA-capable then the bus code that created
 	 * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +2 -1

index f57ea36d125d..4468b7327cab 100644
@@ -1653,7 +1653,8 @@ static int pci_dma_configure(struct device *dev)
 
 	pci_put_host_bridge_device(bridge);
 
-	if (!ret && !driver->driver_managed_dma) {
+	/* @driver may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !driver->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

Quoted 42 lines · Robin Murphy

In hindsight, there were some crucial subtleties overlooked when moving {of,acpi}_dma_configure() to driver probe time to allow waiting for IOMMU drivers with -EPROBE_DEFER, and these have become an ever-increasing source of problems. The IOMMU API has some fundamental assumptions that iommu_probe_device() is called for every device added to the system, in the order in which they are added. Calling it in a random order or not at all dependent on driver binding leads to malformed groups, a potential lack of isolation for devices with no driver, and all manner of unexpected concurrency and race conditions. We've attempted to mitigate the latter with point-fix bodges like iommu_probe_device_lock, but it's a losing battle and the time has come to bite the bullet and address the true source of the problem instead.

The crux of the matter is that the firmware parsing actually serves two distinct purposes; one is identifying the IOMMU instance associated with a device so we can check its availability, the second is actually telling that instance about the relevant firmware-provided data for the device. However the latter also depends on the former, and at the time there was no good place to defer and retry that separately from the availability check we also wanted for client driver probe.

Nowadays, though, we have a proper notion of multiple IOMMU instances in the core API itself, and each one gets a chance to probe its own devices upon registration, so we can finally make that work as intended for DT/IORT/VIOT platforms too. All we need is for iommu_probe_device() to be able to run the iommu_fwspec machinery currently buried deep in the wrong end of {of,acpi}_dma_configure(). Luckily it turns out to be surprisingly straightforward to bootstrap this transformation by pretty much just calling the same path twice. At client driver probe time, dev->driver is obviously set; conversely at device_add(), or a subsequent bus_iommu_probe(), any device waiting for an IOMMU really should *not* have a driver already, so we can use that as a condition to disambiguate the two cases, and avoid recursing back into the IOMMU core at the wrong times.

Obviously this isn't the nicest thing, but for now it gives us a functional baseline to then unpick the layers in between without many more awkward cross-subsystem patches. There are some minor side-effects like dma_range_map potentially being created earlier, and some debug prints being repeated, but these aren't significantly detrimental. Let's make things work first, then deal with making them nice.

Quoted 8 lines

With the basic flow finally in the right order again, the next step is probably turning the bus->dma_configure paths inside-out, since all we really need from bus code is its notion of which device and input ID(s) to parse the common firmware properties with...

Acked-by Bjorn Helgaas <[email protected]> # pci-driver.c

Acked-by Rob Herring (Arm) <[email protected]> # of/device.c

Signed-off-by Robin Murphy <[email protected]>

This patch seems to break virtio-iommu along with qemu. After applying it we cannot see any iommu group. I don't have any specific warning in dmesg though. Reverting bcb81ac6ae3c ("iommu: Get DT/ACPI parsing into the proper probe path") fixes the issue for me. Added Jerry and jean-Philippe in the loop.

Quoted 207 lines

v2:
- Comment bus driver changes for clarity
- Use dev->iommu as the now-robust replay condition
- Drop the device_iommu_mapped() checks in the firmware paths as they weren't doing much - we can't replace probe_device_lock just yet...

 drivers/acpi/arm64/dma.c        |  5 +++++
 drivers/acpi/scan.c             |  7 -------
 drivers/amba/bus.c              |  3 ++-
 drivers/base/platform.c         |  3 ++-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 ++-
 drivers/cdx/cdx.c               |  3 ++-
 drivers/iommu/iommu.c           | 24 +++++++++++++++++++++---
 drivers/iommu/of_iommu.c        |  7 ++++++-
 drivers/of/device.c             |  7 ++++++-
 drivers/pci/pci-driver.c        |  3 ++-
 10 files changed, 48 insertions(+), 17 deletions(-)

drivers/acpi/arm64/dma.c +5 -0

index 52b2abf88689..f30f138352b7 100644
@@ -26,6 +26,11 @@ void acpi_arch_dma_setup(struct device *dev)
 	else
 		end = (1ULL << 32) - 1;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		return;
+	}
+
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
 		end = dma_range_map_max(map);

drivers/acpi/scan.c +0 -7

index 9f4efa8f75a6..fb1fe9f3b1a3 100644
@@ -1632,13 +1632,6 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 		err = viot_iommu_configure(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	/*
-	 * If we have reason to believe the IOMMU driver missed the initial
-	 * iommu_probe_device() call for dev, replay it to get things in order.
-	 */
-	if (!err && dev->bus)
-		err = iommu_probe_device(dev);
-
 	return err;
 }
 

drivers/amba/bus.c +2 -1

index 8ef259b4d037..71482d639a6d 100644
@@ -364,7 +364,8 @@ static int amba_dma_configure(struct device *dev)
 		ret = acpi_dma_configure(dev, attr);
 	}
 
-	if (!ret && !drv->driver_managed_dma) {
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/base/platform.c +2 -1

index 6f2a33722c52..1813cfd0c4bd 100644
@@ -1451,7 +1451,8 @@ static int platform_dma_configure(struct device *dev)
 		attr = acpi_get_dma_attr(to_acpi_device_node(fwnode));
 		ret = acpi_dma_configure(dev, attr);
 	}
-	if (ret || drv->driver_managed_dma)
+	/* @drv may not be valid when we're called from the IOMMU layer */
+	if (ret || !dev->driver || drv->driver_managed_dma)
 		return ret;
 
 	ret = iommu_device_use_default_domain(dev);

drivers/bus/fsl-mc/fsl-mc-bus.c +2 -1

index d1f3d327ddd1..a8be8cf246fb 100644
@@ -153,7 +153,8 @@ static int fsl_mc_dma_configure(struct device *dev)
 	else
 		ret = acpi_dma_configure_id(dev, DEV_DMA_COHERENT, &input_id);
 
-	if (!ret && !mc_drv->driver_managed_dma) {
+	/* @mc_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !mc_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/cdx/cdx.c +2 -1

index c573ed2ee71a..780fb0c4adba 100644
@@ -360,7 +360,8 @@ static int cdx_dma_configure(struct device *dev)
 		return ret;
 	}
 
-	if (!ret && !cdx_drv->driver_managed_dma) {
+	/* @cdx_drv may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !cdx_drv->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +21 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;
 	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is
+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}
+	/*
+	 * At this point, relevant devices either now have a fwspec which will
+	 * match ops registered with a non-NULL fwnode, or we can reasonably
 	 * assume that only one of Intel, AMD, s390, PAMU or legacy SMMUv2 can
 	 * be present, and that any of their registered instances has suitable
 	 * ops for probing, and thus cheekily co-opt the same mechanism.

@@ -426,6 +438,12 @@ static int iommu_init_device(struct device *dev)
 		ret = -ENODEV;
 		goto err_free;
 	}
+	/*
+	 * And if we do now see any replay calls, they would indicate someone
+	 * misusing the dma_configure path outside bus code.
+	 */
+	if (dev->driver)
+		dev_WARN(dev, "late IOMMU probe at driver bind, something fishy here!\n");
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;

drivers/iommu/of_iommu.c +6 -1

index e10a68b5ffde..6b989a62def2 100644
@@ -155,7 +155,12 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np,
 		dev_iommu_free(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	if (!err && dev->bus)
+	/*
+	 * If we're not on the iommu_probe_device() path (as indicated by the
+	 * initial dev->iommu) then try to simulate it. This should no longer
+	 * happen unless of_dma_configure() is being misused outside bus code.
+	 */
+	if (!err && dev->bus && !dev_iommu_present)
 		err = iommu_probe_device(dev);
 
 	if (err && err != -EPROBE_DEFER)

drivers/of/device.c +6 -1

index edf3be197265..5053e5d532cc 100644
@@ -99,6 +99,11 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	bool coherent, set_map = false;
 	int ret;
 
+	if (dev->dma_range_map) {
+		dev_dbg(dev, "dma_range_map already set\n");
+		goto skip_map;
+	}
+
 	if (np == dev->of_node)
 		bus_np = __of_get_dma_parent(np);
 	else

@@ -119,7 +124,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		end = dma_range_map_max(map);
 		set_map = true;
 	}
-
+skip_map:
 	/*
 	 * If @dev is expected to be DMA-capable then the bus code that created
 	 * it should have initialised its dma_mask pointer by this point. For

drivers/pci/pci-driver.c +2 -1

index f57ea36d125d..4468b7327cab 100644
@@ -1653,7 +1653,8 @@ static int pci_dma_configure(struct device *dev)
 
 	pci_put_host_bridge_device(bridge);
 
-	if (!ret && !driver->driver_managed_dma) {
+	/* @driver may not be valid when we're called from the IOMMU layer */
+	if (!ret && dev->driver && !driver->driver_managed_dma) {
 		ret = iommu_device_use_default_domain(dev);
 		if (ret)
 			arch_teardown_dma_ops(dev);

drivers/iommu/iommu.c +2 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;
 	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is

Hi, Robin,

Quoted 13 lines · Robin Murphy

drivers/iommu/iommu.c +2 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
  	if (!dev_iommu_get(dev))
  		return -ENOMEM;
  	/*
-	 * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-	 * instances with non-NULL fwnodes, and client devices should have been
-	 * identified with a fwspec by this point. Otherwise, we can currently
+	 * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+	 * is buried in the bus dma_configure path. Properly unpicking that is

drivers/iommu/iommu.c static int iommu_init_device(struct device *dev) · line 428

Quoted 10 lines

drivers/iommu/iommu.c +10 -0

+	 * still a big job, so for now just invoke the whole thing. The device
+	 * already having a driver bound means dma_configure has already run and
+	 * either found no IOMMU to wait for, or we're in its replay call right
+	 * now, so either way there's no point calling it again.
+	 */
+	if (!dev->driver && dev->bus->dma_configure) {
+		mutex_unlock(&iommu_probe_device_lock);
+		dev->bus->dma_configure(dev);
+		mutex_lock(&iommu_probe_device_lock);
+	}

I was chasing the "something fishy" dev_WARN on a 6.19+ downstream android kernel and while looking at the IOMMU code I couldn't help myself and ask whether we shall prevent concurrent execution of dma_configure().

It seems to me that while the IOMMU subsystem is executing dma_configure(), the deferred probe workqueue can concurrently pick up the same device, enter really_probe(), set dev->driver, and execute dma_configure(). Is it worth protecting against this?

I can try to prove it if needed, using a downstream iommu driver (sigh).

Thanks!
ta

drivers/iommu/iommu.c +2 -1

index e61927b4d41f..5f0c1a8064b5 100644
@@ -461,9 +461,19 @@ static int iommu_init_device(struct device *dev)
          * already having a driver bound means dma_configure has already run and
          * found no IOMMU to wait for, so there's no point calling it again.
          */
-       if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
+       if (!dev->iommu->fwspec && !READ_ONCE(dev->driver) &&
+           dev->bus->dma_configure) {
                 mutex_unlock(&iommu_probe_device_lock);

drivers/iommu/iommu.c +1 -0

+
                 mutex_lock(&iommu_probe_device_lock);
                 /* If another instance finished the job for us, skip it */
                 if (!dev->iommu || dev->iommu_group)

(END)

Quoted 32 lines · Tudor Ambarus

Hi, Robin,

Quoted 13 lines · Robin Murphy

drivers/iommu/iommu.c +2 -3

index a3b45b84f42b..1cec7074367a 100644
@@ -414,9 +414,21 @@ static int iommu_init_device(struct device *dev)
       if (!dev_iommu_get(dev))
           return -ENOMEM;
       /*
-     * For FDT-based systems and ACPI IORT/VIOT, drivers register IOMMU
-     * instances with non-NULL fwnodes, and client devices should have been
-     * identified with a fwspec by this point. Otherwise, we can currently
+     * For FDT-based systems and ACPI IORT/VIOT, the common firmware parsing
+     * is buried in the bus dma_configure path. Properly unpicking that is

drivers/iommu/iommu.c static int iommu_init_device(struct device *dev) · line 428

Quoted 10 lines

drivers/iommu/iommu.c +10 -0

+     * still a big job, so for now just invoke the whole thing. The device
+     * already having a driver bound means dma_configure has already run and
+     * either found no IOMMU to wait for, or we're in its replay call right
+     * now, so either way there's no point calling it again.
+     */
+    if (!dev->driver && dev->bus->dma_configure) {
+        mutex_unlock(&iommu_probe_device_lock);
+        dev->bus->dma_configure(dev);
+        mutex_lock(&iommu_probe_device_lock);
+    }

I was chasing the "something fishy" dev_WARN on a 6.19+ downstream android kernel and while looking at the IOMMU code I couldn't help myself and ask whether we shall prevent concurrent execution of dma_configure().

Quoted 4 lines

It seems to me that while the IOMMU subsystem is executing dma_configure(), the deferred probe workqueue can concurrently pick up the same device, enter really_probe(), set dev->driver, and execute dma_configure(). Is it worth protecting against this?

Yes, it's certainly still possible to hit a false-positive if thread A in iommu_device_register()->bus_iommu_probe() races against thread B attempting to bind, simply because thread B can set dev->driver long before it gets to any point where ends up serialising on iommu_probe_device_lock again, so thread A can observe that even while it is doing the IOMMU probe in the "correct" context. Other than the warning though, it's still functionally OK even if the "wrong" thread does end up finishing the probe, at least after 0c8e9c148e29 ("iommu: Avoid introducing more races").

Quoted 17 lines

drivers/iommu/iommu.c +5 -2

index e61927b4d41f..5f0c1a8064b5 100644
@@ -461,9 +461,19 @@ static int iommu_init_device(struct device *dev)
           * already having a driver bound means dma_configure has already run and
           * found no IOMMU to wait for, so there's no point calling it again.
           */
-       if (!dev->iommu->fwspec && !dev->driver && dev->bus->dma_configure) {
+       if (!dev->iommu->fwspec && !READ_ONCE(dev->driver) &&
+           dev->bus->dma_configure) {
                  mutex_unlock(&iommu_probe_device_lock);
-               dev->bus->dma_configure(dev);
+
+               /*
+                * Serialize with really_probe(). Recheck dev->driver in case a

drivers/iommu/iommu.c static int iommu_init_device(struct device *dev) · line 475

Quoted 6 lines

drivers/iommu/iommu.c +6 -0

+                * driver bound while we were waiting for the lock.
+                */
+               device_lock(dev);
+               if (!dev->driver)
+                       dev->bus->dma_configure(dev);
+               device_unlock(dev);

Much as I can't wait to get rid of iommu_probe_device_lock, the main reason we still can't rely on device_lock() at the moment is not actually the remaining sketchy replay-dependers per the comment in __iommu_probe_device(), but more fundamentally that for most IOMMU drivers this will deadlock in that same iommu_device_register()->bus_iommu_probe() path, when the bus walk happens to stumble across the IOMMU device itself, which of course is already locked as it's still in the middle of its own driver bind. I couldn't see an easy, clean and reliable way to get around that, so that can got kicked down the road in order to get the "call of_xlate in the right order and make iommu_device_register() actually work" basics landed (and start shaking out all these other problems...)

This spin irons out a couple of issues which v1 had. Firstly there should now be no change in behaviour for the weird of_dma_configure() calls, other than possibly getting the warning if they deserve it. Secondly I think there was still a possibility for probe to run via the replay path while its "real" probe was waiting to reacquire the lock; this is now solved by making dev->iommu a reliable indicator of the probe lifecycle, with a couple more prep patches.

Thanks,
Robin.

Robin Murphy (4):
iommu: Handle race with default domain setup
iommu: Resolve ops in iommu_init_device()
iommu: Keep dev->iommu state consistent
iommu: Get DT/ACPI parsing into the proper probe path

 drivers/acpi/arm64/dma.c        |  5 +++
 drivers/acpi/scan.c             |  7 -----
 drivers/amba/bus.c              |  3 +-
 drivers/base/platform.c         |  3 +-
 drivers/bus/fsl-mc/fsl-mc-bus.c |  3 +-
 drivers/cdx/cdx.c               |  3 +-
 drivers/iommu/iommu-priv.h      |  2 ++
 drivers/iommu/iommu.c           | 55 ++++++++++++++++++++++++---------
 drivers/iommu/of_iommu.c        | 13 ++++++--
 drivers/of/device.c             |  7 ++++-
 drivers/pci/pci-driver.c        |  3 +-
 11 files changed, 74 insertions(+), 30 deletions(-)