Skip to contentSkip to conversation

iommu/arm-smmu: Minor probe_device related improvements

PATCH Applied Robin Murphy 2 participants 6 messages Dec 5, 2024 – Dec 10, 2024

Subsystems (2)
lore
Share & tools
lore

Robin Murphy

author reviewer
#0 lore raw

v2 cover iommu/arm-smmu: Minor probe_device related improvements

v1: https://lore.kernel.org/linux-iommu/[email protected]

Hi all,

Here's what I probably should have sent instead of rushing v1 out earlier, now rebased and polished :)

Cheers,
Robin.

Robin Murphy (4):
iommu/arm-smmu: Make instance lookup robust iommu/arm-smmu: Retire probe deferral workaround iommu/arm-smmu-v3: Clean up more on probe failure
iommu: Manage driver probe deferral better

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 21 +++++++----
 drivers/iommu/arm/arm-smmu/arm-smmu.c       | 42 +++++++--------------
 drivers/iommu/iommu.c                       |  2 +-
 drivers/iommu/of_iommu.c                    |  2 -
 4 files changed, 29 insertions(+), 38 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#1 lore raw

PATCH v2 1/4 iommu/arm-smmu: Make instance lookup robust

Relying on the driver list was a cute idea for minimising the scope of our SMMU device lookups, however it turns out to have a subtle flaw. The SMMU device only gets added to that list after arm_smmu_device_probe() returns success, so there's actually no way the iommu_device_register() call from there could ever work as intended, even if it wasn't already hampered by the fwspec setup not happening early enough.

Switch both arm_smmu_get_by_fwnode() implementations to use a platform bus lookup instead, which *will* reliably work. Also make sure that we don't register SMMUv2 instances until we've fully initialised them, to avoid similar consequences of the lookup now finding a device with no drvdata. Moving the error returns is also a perfect excuse to streamline them with dev_err_probe() in the process.

---

v2: Tweak commit message

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  4 +--
 drivers/iommu/arm/arm-smmu/arm-smmu.c       | 31 ++++++++++-----------
 2 files changed, 16 insertions(+), 19 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +2 -2
index a5c7002ff75b..e1eae4ecfa02 100644
@@ -3237,8 +3237,8 @@ static struct platform_driver arm_smmu_driver;
 static
 struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
 {
-	struct device *dev = driver_find_device_by_fwnode(&arm_smmu_driver.driver,
-							  fwnode);
+	struct device *dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
+
 	put_device(dev);
 	return dev ? dev_get_drvdata(dev) : NULL;
 }
drivers/iommu/arm/arm-smmu/arm-smmu.c +14 -17
index 650664e0f6e3..0949f2734e5d 100644
@@ -1411,8 +1411,8 @@ static bool arm_smmu_capable(struct device *dev, enum iommu_cap cap)
 static
 struct arm_smmu_device *arm_smmu_get_by_fwnode(struct fwnode_handle *fwnode)
 {
-	struct device *dev = driver_find_device_by_fwnode(&arm_smmu_driver.driver,
-							  fwnode);
+	struct device *dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
+
 	put_device(dev);
 	return dev ? dev_get_drvdata(dev) : NULL;
 }
@@ -2227,21 +2227,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 					i, irq);
 	}
 
-	err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL,
-				     "smmu.%pa", &smmu->ioaddr);
-	if (err) {
-		dev_err(dev, "Failed to register iommu in sysfs\n");
-		return err;
-	}
-
-	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
-				    using_legacy_binding ? NULL : dev);
-	if (err) {
-		dev_err(dev, "Failed to register iommu\n");
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return err;
-	}
-
 	platform_set_drvdata(pdev, smmu);
 
 	/* Check for RMRs and install bypass SMRs if any */
@@ -2250,6 +2235,18 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	arm_smmu_device_reset(smmu);
 	arm_smmu_test_smr_masks(smmu);
 
+	err = iommu_device_sysfs_add(&smmu->iommu, smmu->dev, NULL,
+				     "smmu.%pa", &smmu->ioaddr);
+	if (err)
+		return dev_err_probe(dev, err, "Failed to register iommu in sysfs\n");
+
+	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops,
+				    using_legacy_binding ? NULL : dev);
+	if (err) {
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return dev_err_probe(dev, err, "Failed to register iommu\n");
+	}
+
 	/*
 	 * We want to avoid touching dev->power.lock in fastpaths unless
 	 * it's really going to do something useful - pm_runtime_enabled()
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#2 lore raw

PATCH v2 2/4 iommu/arm-smmu: Retire probe deferral workaround

In reply to Robin Murphy

This reverts commit 229e6ee43d2a160a1592b83aad620d6027084aad.

Now that the fundamental ordering issue between arm_smmu_get_by_fwnode() and iommu_device_register() is resolved, the race condition for client probe no longer exists either, so retire the specific workaround.

---

v2: New

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 11 -----------
 1 file changed, 11 deletions(-)
drivers/iommu/arm/arm-smmu/arm-smmu.c +0 -11
index 0949f2734e5d..79afc92e1d8b 100644
@@ -1437,17 +1437,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 			goto out_free;
 	} else {
 		smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
-
-		/*
-		 * Defer probe if the relevant SMMU instance hasn't finished
-		 * probing yet. This is a fragile hack and we'd ideally
-		 * avoid this race in the core code. Until that's ironed
-		 * out, however, this is the most pragmatic option on the
-		 * table.
-		 */
-		if (!smmu)
-			return ERR_PTR(dev_err_probe(dev, -EPROBE_DEFER,
-						"smmu dev has not bound yet\n"));
 	}
 
 	ret = -EINVAL;
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#3 lore raw

PATCH v2 3/4 iommu/arm-smmu-v3: Clean up more on probe failure

In reply to Robin Murphy

kmemleak noticed that the iopf queue allocated deep down within arm_smmu_init_structures() can be leaked by a subsequent error return from arm_smmu_device_probe(). Furthermore, after arm_smmu_device_reset() we will also leave the SMMU enabled with an empty Stream Table, silently blocking all DMA. This proves rather annoying for debugging said probe failure, so let's handle it a bit better by putting the SMMU back into (more or less) the same state as if it hadn't probed at all.

---

v2: Squash disable fix, get labels right

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +12 -5
index e1eae4ecfa02..264a686d30cb 100644
@@ -4663,7 +4663,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	/* Initialise in-memory data structures */
 	ret = arm_smmu_init_structures(smmu);
 	if (ret)
-		return ret;
+		goto err_free_iopf;
 
 	/* Record our private device structure */
 	platform_set_drvdata(pdev, smmu);
@@ -4674,22 +4674,29 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	/* Reset the device */
 	ret = arm_smmu_device_reset(smmu);
 	if (ret)
-		return ret;
+		goto err_disable;
 
 	/* And we're up. Go go go! */
 	ret = iommu_device_sysfs_add(&smmu->iommu, dev, NULL,
 				     "smmu3.%pa", &ioaddr);
 	if (ret)
-		return ret;
+		goto err_disable;
 
 	ret = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (ret) {
 		dev_err(dev, "Failed to register iommu\n");
-		iommu_device_sysfs_remove(&smmu->iommu);
-		return ret;
+		goto err_free_sysfs;
 	}
 
 	return 0;
+
+err_free_sysfs:
+	iommu_device_sysfs_remove(&smmu->iommu);
+err_disable:
+	arm_smmu_device_disable(smmu);
+err_free_iopf:
+	iopf_queue_free(smmu->evtq.iopf);
+	return ret;
 }
 
 static void arm_smmu_device_remove(struct platform_device *pdev)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#4 lore raw

PATCH v2 4/4 iommu: Manage driver probe deferral better

In reply to Robin Murphy

Since iommu_fwspec_init() absorbed the basic driver probe deferral check to wait for an IOMMU to register, we may as well handle the probe deferral timeout there as well. The current inconsistency of callers results in client devices deferring forever on an arm64 ACPI system where an SMMU has failed its own driver probe.

---

v2: No change

 drivers/iommu/iommu.c    | 2 +-
 drivers/iommu/of_iommu.c | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)
drivers/iommu/iommu.c +1 -1
index 9bc0c74cca3c..7cd433803bd6 100644
@@ -2819,7 +2819,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 
 	if (!ops)
-		return -EPROBE_DEFER;
+		return driver_deferred_probe_check_state(dev);
 
 	if (fwspec)
 		return ops == iommu_fwspec_ops(fwspec) ? 0 : -EINVAL;
drivers/iommu/of_iommu.c +0 -2
index e7a6a1611d19..97987cd78da9 100644
@@ -29,8 +29,6 @@ static int of_iommu_xlate(struct device *dev,
 		return -ENODEV;
 
 	ret = iommu_fwspec_init(dev, of_fwnode_handle(iommu_spec->np));
-	if (ret == -EPROBE_DEFER)
-		return driver_deferred_probe_check_state(dev);
 	if (ret)
 		return ret;
 
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
maintainer Applied
#5 lore raw
In reply to Robin Murphy
Quoted 8 lines · Robin Murphy

v1: https://lore.kernel.org/linux-iommu/[email protected]

Hi all,

Here's what I probably should have sent instead of rushing v1 out earlier, now rebased and polished :)

[...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/4] iommu/arm-smmu: Make instance lookup robust

      https://git.kernel.org/will/c/7d835134d4e1
[2/4] iommu/arm-smmu: Retire probe deferral workaround
      https://git.kernel.org/will/c/97cb1fa02726
[3/4] iommu/arm-smmu-v3: Clean up more on probe failure
      https://git.kernel.org/will/c/fcbd62156742
[4/4] iommu: Manage driver probe deferral better
      https://git.kernel.org/will/c/46b3df8eb9bd

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev