Skip to contentSkip to conversation

iommu: Remove iommu_fwspec ops

PATCH Superseded Robin Murphy 5 participants 9 messages Apr 19, 2024 – Apr 23, 2024

Subsystems (11)
ACPI Arm MediaTek Tegra architecture support IOMMU +7 more subsystems

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Robin Murphy

author reviewer
#0 lore raw

cover iommu: Remove iommu_fwspec ops

Hi All,

Building on top of the arch_setup_dma_ops() cleanup[1], the next step down the chain is {acpi,of}_dma_configure()... There's plenty to do here, but it may as well start with this fairly self-contained little cleanup, pruning yet more redundancy and exposed API surface.

Thanks,
Robin.

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

Robin Murphy (4):
iommu: Resolve fwspec ops automatically
ACPI: Retire acpi_iommu_fwspec_ops()
OF: Simplify of_iommu_configure()
iommu: Remove iommu_fwspec ops

 drivers/acpi/arm64/iort.c             | 19 +++-------
 drivers/acpi/scan.c                   | 38 +++++---------------
 drivers/acpi/viot.c                   | 11 ++----
 drivers/iommu/arm/arm-smmu/arm-smmu.c |  3 +-
 drivers/iommu/iommu-priv.h            |  7 ++++
 drivers/iommu/iommu.c                 | 20 +++++------
 drivers/iommu/mtk_iommu_v1.c          |  2 +-
 drivers/iommu/of_iommu.c              | 50 ++++++++++-----------------
 drivers/iommu/tegra-smmu.c            |  2 +-
 drivers/of/device.c                   | 30 ++++++----------
 include/acpi/acpi_bus.h               |  3 +-
 include/linux/iommu.h                 | 15 ++------
 12 files changed, 66 insertions(+), 134 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#1 lore raw

PATCH 1/4 iommu: Resolve fwspec ops automatically

There's no real need for callers to resolve ops from a fwnode in order to then pass both to iommu_fwspec_init() - it's simpler and more sensible for that to resolve the ops itself. This in turn means we can centralise the notion of checking for a present driver, and enforce that fwspecs aren't allocated unless and until we know they will be usable.

Also we've grown a generic fwnode_handle_get() since this code was first written, so may as well clear up that ugly mismatch while we're in here.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/acpi/arm64/iort.c             | 19 +++++--------------
 drivers/acpi/scan.c                   |  8 +++-----
 drivers/acpi/viot.c                   | 11 ++---------
 drivers/iommu/arm/arm-smmu/arm-smmu.c |  3 +--
 drivers/iommu/iommu-priv.h            |  2 ++
 drivers/iommu/iommu.c                 |  9 ++++++---
 drivers/iommu/mtk_iommu_v1.c          |  2 +-
 drivers/iommu/of_iommu.c              | 19 ++++++-------------
 drivers/iommu/tegra-smmu.c            |  2 +-
 include/acpi/acpi_bus.h               |  3 +--
 include/linux/iommu.h                 | 13 ++-----------
 11 files changed, 30 insertions(+), 61 deletions(-)
drivers/acpi/arm64/iort.c +5 -14
index c0b1c2c19444..1b39e9ae7ac1 100644
@@ -1221,10 +1221,10 @@ static bool iort_pci_rc_supports_ats(struct acpi_iort_node *node)
 static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
 			    u32 streamid)
 {
-	const struct iommu_ops *ops;
 	struct fwnode_handle *iort_fwnode;
 
-	if (!node)
+	/* If there's no SMMU driver at all, give up now */
+	if (!node || !iort_iommu_driver_enabled(node->type))
 		return -ENODEV;
 
 	iort_fwnode = iort_get_fwnode(node);

@@ -1232,19 +1232,10 @@ static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
 		return -ENODEV;
 
 	/*
-	 * If the ops look-up fails, this means that either
-	 * the SMMU drivers have not been probed yet or that
-	 * the SMMU drivers are not built in the kernel;
-	 * Depending on whether the SMMU drivers are built-in
-	 * in the kernel or not, defer the IOMMU configuration
-	 * or just abort it.
+	 * If the SMMU drivers are enabled but not loaded/probed
+	 * yet, this will defer.
 	 */
-	ops = iommu_ops_from_fwnode(iort_fwnode);
-	if (!ops)
-		return iort_iommu_driver_enabled(node->type) ?
-		       -EPROBE_DEFER : -ENODEV;
-
-	return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode, ops);
+	return acpi_iommu_fwspec_init(dev, streamid, iort_fwnode);
 }
 
 struct iort_pci_alias_info {
drivers/acpi/scan.c +3 -5
index b1a88992c1a9..9d36fc3dc5ac 100644
@@ -1578,10 +1578,9 @@ int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map)
 
 #ifdef CONFIG_IOMMU_API
 int acpi_iommu_fwspec_init(struct device *dev, u32 id,
-			   struct fwnode_handle *fwnode,
-			   const struct iommu_ops *ops)
+			   struct fwnode_handle *fwnode)
 {
-	int ret = iommu_fwspec_init(dev, fwnode, ops);
+	int ret = iommu_fwspec_init(dev, fwnode);
 
 	if (!ret)
 		ret = iommu_fwspec_add_ids(dev, &id, 1);

@@ -1640,8 +1639,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 #else /* !CONFIG_IOMMU_API */
 
 int acpi_iommu_fwspec_init(struct device *dev, u32 id,
-			   struct fwnode_handle *fwnode,
-			   const struct iommu_ops *ops)
+			   struct fwnode_handle *fwnode)
 {
 	return -ENODEV;
 }
drivers/acpi/viot.c +2 -9
index c8025921c129..2aa69a2fba73 100644
@@ -307,21 +307,14 @@ void __init acpi_viot_init(void)
 static int viot_dev_iommu_init(struct device *dev, struct viot_iommu *viommu,
 			       u32 epid)
 {
-	const struct iommu_ops *ops;
-
-	if (!viommu)
+	if (!viommu || !IS_ENABLED(CONFIG_VIRTIO_IOMMU))
 		return -ENODEV;
 
 	/* We're not translating ourself */
 	if (device_match_fwnode(dev, viommu->fwnode))
 		return -EINVAL;
 
-	ops = iommu_ops_from_fwnode(viommu->fwnode);
-	if (!ops)
-		return IS_ENABLED(CONFIG_VIRTIO_IOMMU) ?
-			-EPROBE_DEFER : -ENODEV;
-
-	return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode, ops);
+	return acpi_iommu_fwspec_init(dev, epid, viommu->fwnode);
 }
 
 static int viot_pci_dev_iommu_init(struct pci_dev *pdev, u16 dev_id, void *data)
drivers/iommu/arm/arm-smmu/arm-smmu.c +1 -2
index c572d877b0e1..21c77dbb5ada 100644
@@ -178,8 +178,7 @@ static int arm_smmu_register_legacy_master(struct device *dev,
 		it.cur_count = 1;
 	}
 
-	err = iommu_fwspec_init(dev, &smmu_dev->of_node->fwnode,
-				&arm_smmu_ops);
+	err = iommu_fwspec_init(dev, NULL);
 	if (err)
 		return err;
drivers/iommu/iommu-priv.h +2 -0
index 5f731d994803..078cafcf49b4 100644
@@ -17,6 +17,8 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 	return dev->iommu->iommu_dev->ops;
 }
 
+const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode);
+
 int iommu_group_replace_domain(struct iommu_group *group,
 			       struct iommu_domain *new_domain);
drivers/iommu/iommu.c +6 -3
index f01133b906e2..07a647e34c72 100644
@@ -2822,11 +2822,14 @@ const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode
 	return ops;
 }
 
-int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
-		      const struct iommu_ops *ops)
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
 {
+	const struct iommu_ops *ops = iommu_ops_from_fwnode(iommu_fwnode);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 
+	if (!ops)
+		return -EPROBE_DEFER;
+
 	if (fwspec)
 		return ops == fwspec->ops ? 0 : -EINVAL;
 

@@ -2838,7 +2841,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 	if (!fwspec)
 		return -ENOMEM;
 
-	of_node_get(to_of_node(iommu_fwnode));
+	fwnode_handle_get(iommu_fwnode);
 	fwspec->iommu_fwnode = iommu_fwnode;
 	fwspec->ops = ops;
 	dev_iommu_fwspec_set(dev, fwspec);
drivers/iommu/mtk_iommu_v1.c +1 -1
index a9fa2a54dc9b..59b1f8701e7d 100644
@@ -414,7 +414,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev,
 	}
 
 	if (!fwspec) {
-		ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+		ret = iommu_fwspec_init(dev, &args->np->fwnode);
 		if (ret)
 			return ret;
 		fwspec = dev_iommu_fwspec_get(dev);
drivers/iommu/of_iommu.c +6 -13
index 3afe0b48a48d..bbfe960cdc13 100644
@@ -21,26 +21,19 @@ static int of_iommu_xlate(struct device *dev,
 			  struct of_phandle_args *iommu_spec)
 {
 	const struct iommu_ops *ops;
-	struct fwnode_handle *fwnode = &iommu_spec->np->fwnode;
 	int ret;
 
-	ops = iommu_ops_from_fwnode(fwnode);
-	if ((ops && !ops->of_xlate) ||
-	    !of_device_is_available(iommu_spec->np))
+	if (!of_device_is_available(iommu_spec->np))
 		return -ENODEV;
 
-	ret = iommu_fwspec_init(dev, fwnode, ops);
+	ret = iommu_fwspec_init(dev, &iommu_spec->np->fwnode);
+	if (ret == -EPROBE_DEFER)
+		return driver_deferred_probe_check_state(dev);
 	if (ret)
 		return ret;
-	/*
-	 * The otherwise-empty fwspec handily serves to indicate the specific
-	 * IOMMU device we're waiting for, which will be useful if we ever get
-	 * a proper probe-ordering dependency mechanism in future.
-	 */
-	if (!ops)
-		return driver_deferred_probe_check_state(dev);
 
-	if (!try_module_get(ops->owner))
+	ops = dev_iommu_fwspec_get(dev)->ops;
+	if (!ops->of_xlate || !try_module_get(ops->owner))
 		return -ENODEV;
 
 	ret = ops->of_xlate(dev, iommu_spec);
drivers/iommu/tegra-smmu.c +1 -1
index 14e525bd0d9b..bd8143b82eac 100644
@@ -835,7 +835,7 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
 	const struct iommu_ops *ops = smmu->iommu.ops;
 	int err;
 
-	err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
+	err = iommu_fwspec_init(dev, &dev->of_node->fwnode);
 	if (err < 0) {
 		dev_err(dev, "failed to initialize fwspec: %d\n", err);
 		return err;
include/acpi/acpi_bus.h +1 -2
index 5de954e2b18a..589cd697738f 100644
@@ -724,8 +724,7 @@ struct iommu_ops;
 bool acpi_dma_supported(const struct acpi_device *adev);
 enum dev_dma_attr acpi_get_dma_attr(struct acpi_device *adev);
 int acpi_iommu_fwspec_init(struct device *dev, u32 id,
-			   struct fwnode_handle *fwnode,
-			   const struct iommu_ops *ops);
+			   struct fwnode_handle *fwnode);
 int acpi_dma_get_range(struct device *dev, const struct bus_dma_region **map);
 int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 			   const u32 *input_id);
include/linux/iommu.h +2 -11
index ae6e5adebbd1..0614b2736d66 100644
@@ -1002,11 +1002,9 @@ struct iommu_mm_data {
 	struct list_head	sva_handles;
 };
 
-int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
-		      const struct iommu_ops *ops);
+int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode);
 void iommu_fwspec_free(struct device *dev);
 int iommu_fwspec_add_ids(struct device *dev, const u32 *ids, int num_ids);
-const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode);
 
 static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
 {

@@ -1312,8 +1310,7 @@ static inline void iommu_device_unlink(struct device *dev, struct device *link)
 }
 
 static inline int iommu_fwspec_init(struct device *dev,
-				    struct fwnode_handle *iommu_fwnode,
-				    const struct iommu_ops *ops)
+				    struct fwnode_handle *iommu_fwnode)
 {
 	return -ENODEV;
 }

@@ -1328,12 +1325,6 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
 	return -ENODEV;
 }
 
-static inline
-const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode)
-{
-	return NULL;
-}
-
 static inline int
 iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
 {
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
#2 lore raw

Re: [PATCH 1/4] iommu: Resolve fwspec ops automatically

Fri, Apr 19, 2024 at 05:55:59PM +0100, Robin Murphy kirjoitti:

Quoted 8 lines

There's no real need for callers to resolve ops from a fwnode in order to then pass both to iommu_fwspec_init() - it's simpler and more sensible for that to resolve the ops itself. This in turn means we can centralise the notion of checking for a present driver, and enforce that fwspecs aren't allocated unless and until we know they will be usable.

Also we've grown a generic fwnode_handle_get() since this code was first written, so may as well clear up that ugly mismatch while we're in here.

...

drivers/iommu/mtk_iommu_v1.c static int mtk_iommu_v1_create_mapping(struct device *dev, · line 417

Quoted 5 lines

+++ b/drivers/iommu/mtk_iommu_v1.c

drivers/iommu/mtk_iommu_v1.c +1 -1
 	if (!fwspec) {
-		ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+		ret = iommu_fwspec_init(dev, &args->np->fwnode);

I'm wondering, while at it, if can avoid direct dereference of fwnode by using of_fwnode_handle().

Quoted 2 lines
(quoted diff) +0 -0
 		if (ret)
 			return ret;

...

drivers/iommu/of_iommu.c static int of_iommu_xlate(struct device *dev, · line 29

Quoted 4 lines

+++ b/drivers/iommu/of_iommu.c

drivers/iommu/of_iommu.c +1 -1
-	ret = iommu_fwspec_init(dev, fwnode, ops);
+	ret = iommu_fwspec_init(dev, &iommu_spec->np->fwnode);

Ditto.

Quoted 4 lines
drivers/iommu/of_iommu.c +2 -0
+	if (ret == -EPROBE_DEFER)
+		return driver_deferred_probe_check_state(dev);
 	if (ret)
 		return ret;

...

drivers/iommu/tegra-smmu.c static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev, · line 838

Quoted 5 lines
drivers/iommu/tegra-smmu.c +1 -1
 
-	err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
+	err = iommu_fwspec_init(dev, &dev->of_node->fwnode);

Ditto.

Quoted 3 lines
drivers/iommu/tegra-smmu.c +0 -0
 	if (err < 0) {
 		dev_err(dev, "failed to initialize fwspec: %d\n", err);
 		return err;
Signature · 9 lines
With Best Regards,
Andy Shevchenko



_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
author reviewer
#3 lore raw

PATCH 2/4 ACPI: Retire acpi_iommu_fwspec_ops()

In reply to Robin Murphy

Now that iommu_fwspec_init() can signal for probe deferral directly, acpi_iommu_fwspec_ops() is unneeded and can be cleaned up.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/acpi/scan.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)
drivers/acpi/scan.c +6 -24
index 9d36fc3dc5ac..d6b64dcbf9a6 100644
@@ -1588,26 +1588,14 @@ int acpi_iommu_fwspec_init(struct device *dev, u32 id,
 	return ret;
 }
 
-static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
-{
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-	return fwspec ? fwspec->ops : NULL;
-}
-
 static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 {
 	int err;
-	const struct iommu_ops *ops;
 
 	/* Serialise to make dev->iommu stable under our potential fwspec */
 	mutex_lock(&iommu_probe_device_lock);
-	/*
-	 * If we already translated the fwspec there is nothing left to do,
-	 * return the iommu_ops.
-	 */
-	ops = acpi_iommu_fwspec_ops(dev);
-	if (ops) {
+	/* If we already translated the fwspec there is nothing left to do */
+	if (dev_iommu_fwspec_get(dev)) {
 		mutex_unlock(&iommu_probe_device_lock);
 		return 0;
 	}

@@ -1624,16 +1612,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 	if (!err && dev->bus)
 		err = iommu_probe_device(dev);
 
-	/* Ignore all other errors apart from EPROBE_DEFER */
-	if (err == -EPROBE_DEFER) {
-		return err;
-	} else if (err) {
-		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
-		return -ENODEV;
-	}
-	if (!acpi_iommu_fwspec_ops(dev))
-		return -ENODEV;
-	return 0;
+	return err;
 }
 
 #else /* !CONFIG_IOMMU_API */

@@ -1672,6 +1651,9 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 	ret = acpi_iommu_configure_id(dev, input_id);
 	if (ret == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
+	/* Ignore all other errors apart from EPROBE_DEFER */
+	if (ret)
+		dev_dbg(dev, "Adding to IOMMU failed: %d\n", ret);
 
 	arch_setup_dma_ops(dev, attr == DEV_DMA_COHERENT);
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
maintainer Acked-by on v1 2/4
#4 lore raw

Re: [PATCH 2/4] ACPI: Retire acpi_iommu_fwspec_ops()

Quoted 5 lines · Robin Murphy

Now that iommu_fwspec_init() can signal for probe deferral directly, acpi_iommu_fwspec_ops() is unneeded and can be cleaned up.

Signed-off-by Robin Murphy <[email protected]>
Acked-by Rafael J. Wysocki <[email protected]>
Quoted 68 lines
---
 drivers/acpi/scan.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)
drivers/acpi/scan.c +6 -25
index 9d36fc3dc5ac..d6b64dcbf9a6 100644
@@ -1588,26 +1588,14 @@ int acpi_iommu_fwspec_init(struct device *dev, u32 id,
        return ret;
 }
 
-static inline const struct iommu_ops *acpi_iommu_fwspec_ops(struct device *dev)
-{
-       struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-       return fwspec ? fwspec->ops : NULL;
-}
-
 static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 {
        int err;
-       const struct iommu_ops *ops;
 
        /* Serialise to make dev->iommu stable under our potential fwspec */
        mutex_lock(&iommu_probe_device_lock);
-       /*
-        * If we already translated the fwspec there is nothing left to do,
-        * return the iommu_ops.
-        */
-       ops = acpi_iommu_fwspec_ops(dev);
-       if (ops) {
+       /* If we already translated the fwspec there is nothing left to do */
+       if (dev_iommu_fwspec_get(dev)) {
                mutex_unlock(&iommu_probe_device_lock);
                return 0;
        }

@@ -1624,16 +1612,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
        if (!err && dev->bus)
                err = iommu_probe_device(dev);
 
-       /* Ignore all other errors apart from EPROBE_DEFER */
-       if (err == -EPROBE_DEFER) {
-               return err;
-       } else if (err) {
-               dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
-               return -ENODEV;
-       }
-       if (!acpi_iommu_fwspec_ops(dev))
-               return -ENODEV;
-       return 0;
+       return err;
 }
 
 #else /* !CONFIG_IOMMU_API */

@@ -1672,6 +1651,9 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
        ret = acpi_iommu_configure_id(dev, input_id);
        if (ret == -EPROBE_DEFER)
                return -EPROBE_DEFER;
+       /* Ignore all other errors apart from EPROBE_DEFER */
+       if (ret)
+               dev_dbg(dev, "Adding to IOMMU failed: %d\n", ret);
 
        arch_setup_dma_ops(dev, attr == DEV_DMA_COHERENT);
 
--

2.39.2.101.g768bb238c484.dirty

author reviewer
#5 lore raw

PATCH 3/4 OF: Simplify of_iommu_configure()

In reply to Robin Murphy

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.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/of_iommu.c | 29 ++++++++++-------------------
 drivers/of/device.c      | 30 +++++++++++-------------------
 2 files changed, 21 insertions(+), 38 deletions(-)
drivers/iommu/of_iommu.c +10 -19
index bbfe960cdc13..a5d6ff8e4e72 100644
@@ -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;
 }
drivers/of/device.c +11 -19
index 312c63361211..edf3be197265 100644
@@ -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;
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
maintainer Acked-by on v1 3/4
#6 lore raw

Re: [PATCH 3/4] OF: Simplify of_iommu_configure()

Quoted 11 lines · Robin Murphy

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.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/of_iommu.c | 29 ++++++++++-------------------
 drivers/of/device.c      | 30 +++++++++++-------------------
 2 files changed, 21 insertions(+), 38 deletions(-)
Acked-by Rob Herring (Arm) <[email protected]>
author reviewer
#7 lore raw

PATCH 4/4 iommu: Remove iommu_fwspec ops

In reply to Robin Murphy

The ops in iommu_fwspec are only needed for the early configuration and probe process, and by now are easy enough to derive on-demand in those couple of places which need them, so remove the redundant stored copy.

Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/iommu-priv.h |  5 +++++
 drivers/iommu/iommu.c      | 11 ++---------
 drivers/iommu/of_iommu.c   |  4 +++-
 include/linux/iommu.h      |  2 --
 4 files changed, 10 insertions(+), 12 deletions(-)
drivers/iommu/iommu-priv.h +5 -0
index 078cafcf49b4..a34efed2884b 100644
@@ -19,6 +19,11 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
 
 const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode);
 
+static inline const struct iommu_ops *iommu_fwspec_ops(struct iommu_fwspec *fwspec)
+{
+	return iommu_ops_from_fwnode(fwspec ? fwspec->iommu_fwnode : NULL);
+}
+
 int iommu_group_replace_domain(struct iommu_group *group,
 			       struct iommu_domain *new_domain);
drivers/iommu/iommu.c +2 -9
index 07a647e34c72..996e79dc582d 100644
@@ -510,7 +510,6 @@ DEFINE_MUTEX(iommu_probe_device_lock);
 static int __iommu_probe_device(struct device *dev, struct list_head *group_list)
 {
 	const struct iommu_ops *ops;
-	struct iommu_fwspec *fwspec;
 	struct iommu_group *group;
 	struct group_device *gdev;
 	int ret;

@@ -523,12 +522,7 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 * be present, and that any of their registered instances has suitable
 	 * ops for probing, and thus cheekily co-opt the same mechanism.
 	 */
-	fwspec = dev_iommu_fwspec_get(dev);
-	if (fwspec && fwspec->ops)
-		ops = fwspec->ops;
-	else
-		ops = iommu_ops_from_fwnode(NULL);
-
+	ops = iommu_fwspec_ops(dev_iommu_fwspec_get(dev));
 	if (!ops)
 		return -ENODEV;
 	/*

@@ -2831,7 +2825,7 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
 		return -EPROBE_DEFER;
 
 	if (fwspec)
-		return ops == fwspec->ops ? 0 : -EINVAL;
+		return ops == iommu_fwspec_ops(fwspec) ? 0 : -EINVAL;
 
 	if (!dev_iommu_get(dev))
 		return -ENOMEM;

@@ -2843,7 +2837,6 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode)
 
 	fwnode_handle_get(iommu_fwnode);
 	fwspec->iommu_fwnode = iommu_fwnode;
-	fwspec->ops = ops;
 	dev_iommu_fwspec_set(dev, fwspec);
 	return 0;
 }
drivers/iommu/of_iommu.c +3 -1
index a5d6ff8e4e72..fde3053706c0 100644
@@ -17,6 +17,8 @@
 #include <linux/slab.h>
 #include <linux/fsl/mc.h>
 
+#include "iommu-priv.h"
+
 static int of_iommu_xlate(struct device *dev,
 			  struct of_phandle_args *iommu_spec)
 {

@@ -32,7 +34,7 @@ static int of_iommu_xlate(struct device *dev,
 	if (ret)
 		return ret;
 
-	ops = dev_iommu_fwspec_get(dev)->ops;
+	ops = iommu_ops_from_fwnode(&iommu_spec->np->fwnode);
 	if (!ops->of_xlate || !try_module_get(ops->owner))
 		return -ENODEV;
include/linux/iommu.h +0 -2
index 0614b2736d66..ddfc2c9cdda4 100644
@@ -965,7 +965,6 @@ extern struct iommu_group *generic_single_device_group(struct device *dev);
 
 /**
  * struct iommu_fwspec - per-device IOMMU instance data
- * @ops: ops for this device's IOMMU
  * @iommu_fwnode: firmware handle for this device's IOMMU
  * @flags: IOMMU_FWSPEC_* flags
  * @num_ids: number of associated device IDs

@@ -976,7 +975,6 @@ extern struct iommu_group *generic_single_device_group(struct device *dev);
  * consumers.
  */
 struct iommu_fwspec {
-	const struct iommu_ops	*ops;
 	struct fwnode_handle	*iommu_fwnode;
 	u32			flags;
 	unsigned int		num_ids;
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
Tested-by on the v1 series
#8 lore raw
In reply to Robin Murphy
Quoted 6 lines · Robin Murphy

Hi All,

Building on top of the arch_setup_dma_ops() cleanup[1], the next step down the chain is {acpi,of}_dma_configure()... There's plenty to do here, but it may as well start with this fairly self-contained little cleanup, pruning yet more redundancy and exposed API surface.

Tested with QEMU: SMMUv3 DT/IORT, virtio-iommu builtin/module DT/VIOT arm64/x86

Tested-by Jean-Philippe Brucker <[email protected]>
Quoted 30 lines

Thanks,
Robin.

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

Robin Murphy (4):
iommu: Resolve fwspec ops automatically
ACPI: Retire acpi_iommu_fwspec_ops()
OF: Simplify of_iommu_configure()
iommu: Remove iommu_fwspec ops

 drivers/acpi/arm64/iort.c             | 19 +++-------
 drivers/acpi/scan.c                   | 38 +++++---------------
 drivers/acpi/viot.c                   | 11 ++----
 drivers/iommu/arm/arm-smmu/arm-smmu.c |  3 +-
 drivers/iommu/iommu-priv.h            |  7 ++++
 drivers/iommu/iommu.c                 | 20 +++++------
 drivers/iommu/mtk_iommu_v1.c          |  2 +-
 drivers/iommu/of_iommu.c              | 50 ++++++++++-----------------
 drivers/iommu/tegra-smmu.c            |  2 +-
 drivers/of/device.c                   | 30 ++++++----------
 include/acpi/acpi_bus.h               |  3 +-
 include/linux/iommu.h                 | 15 ++------
 12 files changed, 66 insertions(+), 134 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty