[v2,3/4] OF: Simplify of_iommu_configure()

Commit Message

Robin Murphy June 21, 2024, 6:46 p.m. UTC

We no longer have a notion of partially-initialised fwspecs existing,
and we also no longer need to use an iommu_ops pointer to return status
to of_dma_configure(). Clean up the remains of those, which lends itself
to clarifying the logic around the dma_range_map allocation as well.

Acked-by: Rob Herring (Arm) <[e-mail]>
Signed-off-by: Robin Murphy <[e-mail]>
---
 drivers/iommu/of_iommu.c | 29 ++++++++++-------------------
 drivers/of/device.c      | 30 +++++++++++-------------------
 2 files changed, 21 insertions(+), 38 deletions(-)

Comments

Andy Shevchenko June 22, 2024, 10:23 p.m. UTC | #1

On Fri, Jun 21, 2024 at 8:47 PM Robin Murphy <[e-mail]> wrote:
>
> We no longer have a notion of partially-initialised fwspecs existing,
> and we also no longer need to use an iommu_ops pointer to return status
> to of_dma_configure(). Clean up the remains of those, which lends itself
> to clarifying the logic around the dma_range_map allocation as well.

...

> +       if (!err && dev->bus)
> +               err = iommu_probe_device(dev);
>
> +       if (err && err != -EPROBE_DEFER)
> +               dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);

Hmm... I'm wondering if dev_err_probe() can be used here.

>         return err;

...

> +       dev_dbg(dev, "device is%sbehind an iommu\n",
> +               !ret ? " " : " not ");

Why not a positive test?

Robin Murphy June 25, 2024, 6:44 p.m. UTC | #2

On 2024-06-22 11:23 pm, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 8:47 PM Robin Murphy <[e-mail]> wrote:
>>
>> We no longer have a notion of partially-initialised fwspecs existing,
>> and we also no longer need to use an iommu_ops pointer to return status
>> to of_dma_configure(). Clean up the remains of those, which lends itself
>> to clarifying the logic around the dma_range_map allocation as well.
> 
> ...
> 
>> +       if (!err && dev->bus)
>> +               err = iommu_probe_device(dev);
>>
>> +       if (err && err != -EPROBE_DEFER)
>> +               dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> 
> Hmm... I'm wondering if dev_err_probe() can be used here.

It's still possible to have other errors here benignly [1] (however 
questionable the underlying reason), and this has always been a 
dev_dbg(), it's just getting shuffled around again. The aim here is to 
carry on removing cruft to work towards getting rid of this 
iommu_probe_device() call altogether since it's fundamentally wrong, so 
I'm not inclined to add anything new or spend too much effort polishing 
code I still want to delete.

>>          return err;
> 
> ...
> 
>> +       dev_dbg(dev, "device is%sbehind an iommu\n",
>> +               !ret ? " " : " not ");
> 
> Why not a positive test?

Again, mostly because that's how it was written in 2014, same reason I'm 
not deduplicating the redundant space despite it still being the tiniest 
bit irritating. If you make me think about it, though, I suppose when 
both outcomes are otherwise equally weighted it does seems natural to 
consider "success" before "failure", thus the condition tests for success.

Thanks,
Robin.

[1] 
https://lore.kernel.org/linux-iommu/bbmhcoghrprmbdibnjum6lefix2eoquxrde7wyqeulm4xabmlm@b6jy32saugqh/

Patch

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 08c523ad55ad..c946521a5906 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -108,7 +108,6 @@  static int of_iommu_configure_device(struct device_node *master_np,
 int of_iommu_configure(struct device *dev, struct device_node *master_np,
 		       const u32 *id)
 {
-	struct iommu_fwspec *fwspec;
 	int err;
 
 	if (!master_np)
@@ -116,14 +115,9 @@  int of_iommu_configure(struct device *dev, struct device_node *master_np,
 
 	/* Serialise to make dev->iommu stable under our potential fwspec */
 	mutex_lock(&iommu_probe_device_lock);
-	fwspec = dev_iommu_fwspec_get(dev);
-	if (fwspec) {
-		if (fwspec->ops) {
-			mutex_unlock(&iommu_probe_device_lock);
-			return 0;
-		}
-		/* In the deferred case, start again from scratch */
-		iommu_fwspec_free(dev);
+	if (dev_iommu_fwspec_get(dev)) {
+		mutex_unlock(&iommu_probe_device_lock);
+		return 0;
 	}
 
 	/*
@@ -143,20 +137,17 @@  int of_iommu_configure(struct device *dev, struct device_node *master_np,
 	} else {
 		err = of_iommu_configure_device(master_np, dev, id);
 	}
+
+	if (err)
+		iommu_fwspec_free(dev);
 	mutex_unlock(&iommu_probe_device_lock);
 
-	if (err == -ENODEV || err == -EPROBE_DEFER)
-		return err;
-	if (err)
-		goto err_log;
+	if (!err && dev->bus)
+		err = iommu_probe_device(dev);
 
-	err = iommu_probe_device(dev);
-	if (err)
-		goto err_log;
-	return 0;
+	if (err && err != -EPROBE_DEFER)
+		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
 
-err_log:
-	dev_dbg(dev, "Adding to IOMMU failed: %pe\n", ERR_PTR(err));
 	return err;
 }
 
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 312c63361211..edf3be197265 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -96,8 +96,7 @@  int of_dma_configure_id(struct device *dev, struct device_node *np,
 	const struct bus_dma_region *map = NULL;
 	struct device_node *bus_np;
 	u64 mask, end = 0;
-	bool coherent;
-	int iommu_ret;
+	bool coherent, set_map = false;
 	int ret;
 
 	if (np == dev->of_node)
@@ -118,6 +117,7 @@  int of_dma_configure_id(struct device *dev, struct device_node *np,
 	} else {
 		/* Determine the overall bounds of all DMA regions */
 		end = dma_range_map_max(map);
+		set_map = true;
 	}
 
 	/*
@@ -144,7 +144,7 @@  int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev->coherent_dma_mask &= mask;
 	*dev->dma_mask &= mask;
 	/* ...but only set bus limit and range map if we found valid dma-ranges earlier */
-	if (!ret) {
+	if (set_map) {
 		dev->bus_dma_limit = end;
 		dev->dma_range_map = map;
 	}
@@ -153,29 +153,21 @@  int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev_dbg(dev, "device is%sdma coherent\n",
 		coherent ? " " : " not ");
 
-	iommu_ret = of_iommu_configure(dev, np, id);
-	if (iommu_ret == -EPROBE_DEFER) {
+	ret = of_iommu_configure(dev, np, id);
+	if (ret == -EPROBE_DEFER) {
 		/* Don't touch range map if it wasn't set from a valid dma-ranges */
-		if (!ret)
+		if (set_map)
 			dev->dma_range_map = NULL;
 		kfree(map);
 		return -EPROBE_DEFER;
-	} else if (iommu_ret == -ENODEV) {
-		dev_dbg(dev, "device is not behind an iommu\n");
-	} else if (iommu_ret) {
-		dev_err(dev, "iommu configuration for device failed with %pe\n",
-			ERR_PTR(iommu_ret));
-
-		/*
-		 * Historically this routine doesn't fail driver probing
-		 * due to errors in of_iommu_configure()
-		 */
-	} else
-		dev_dbg(dev, "device is behind an iommu\n");
+	}
+	/* Take all other IOMMU errors to mean we'll just carry on without it */
+	dev_dbg(dev, "device is%sbehind an iommu\n",
+		!ret ? " " : " not ");
 
 	arch_setup_dma_ops(dev, coherent);
 
-	if (iommu_ret)
+	if (ret)
 		of_dma_set_restricted_buffer(dev, np);
 
 	return 0;