Skip to contentSkip to conversation

iommu: Remove iommu_fwspec ops

PATCH Applied Robin Murphy 3 participants 15 messages Jul 2, 2024 – Jul 12, 2024

Subsystems (11)
ACPI Arm MediaTek Tegra architecture support IOMMU +7 more subsystems
lore
Share & tools
lore

Robin Murphy

author reviewer
#0 lore raw

v3 cover iommu: Remove iommu_fwspec ops

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

Hi all,

Just a quick update with an extra patch to avoid the mediatek-v1 driver breaking (and I have now build-tested ARCH=arm to make sure, apologies for being lazy before...)

Thanks,
Robin.

Robin Murphy (5):
iommu/mediatek-v1: Clean up redundant fwspec checks
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                   | 36 +++++--------------
 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          | 12 ++-----
 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, 67 insertions(+), 141 deletions(-)
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#1 lore raw

PATCH v3 1/5 iommu/mediatek-v1: Clean up redundant fwspec checks

The driver explicitly clears any existing fwspec before calling mtk_iommu_v1_create_mapping(), but even if it didn't, the checks it's doing there duplicate what iommu_fwspec_init() would do anyway. Clean them up.

Reported-by kernel test robot <[email protected]>
Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here
Signed-off-by Robin Murphy <[email protected]>
---
 drivers/iommu/mtk_iommu_v1.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)
drivers/iommu/mtk_iommu_v1.c +3 -9
index d6e4002200bd..2b64ea46318f 100644
@@ -401,7 +401,6 @@ static const struct iommu_ops mtk_iommu_v1_ops;
 static int mtk_iommu_v1_create_mapping(struct device *dev,
 				       const struct of_phandle_args *args)
 {
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct mtk_iommu_v1_data *data;
 	struct platform_device *m4updev;
 	struct dma_iommu_mapping *mtk_mapping;

@@ -413,14 +412,9 @@ static int mtk_iommu_v1_create_mapping(struct device *dev,
 		return -EINVAL;
 	}
 
-	if (!fwspec) {
-		ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
-		if (ret)
-			return ret;
-		fwspec = dev_iommu_fwspec_get(dev);
-	} else if (dev_iommu_fwspec_get(dev)->ops != &mtk_iommu_v1_ops) {
-		return -EINVAL;
-	}
+	ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+	if (ret)
+		return ret;
 
 	if (!dev_iommu_priv_get(dev)) {
 		/* Get the m4u device */
Signature · 1 line
2.39.2.101.g768bb238c484.dirty
author reviewer
#2 lore raw

PATCH v3 2/5 iommu: Resolve fwspec ops automatically

In reply to Robin Murphy

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 use this opportunity to modernise with some "new" helpers that arrived shortly after this code was first written; the generic fwnode_handle_get() clears up that ugly get/put mismatch, while of_fwnode_handle() can now abstract those open-coded dereferences.

Tested-by Jean-Philippe Brucker <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---
v2: Add of_fwnode_handle() cleanup as well

---
 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 503773707e01..8d5a589db141 100644
@@ -1577,12 +1577,11 @@ 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;
 
-	ret = iommu_fwspec_init(dev, fwnode, ops);
+	ret = iommu_fwspec_init(dev, fwnode);
 	if (ret)
 		return ret;
 

@@ -1639,8 +1638,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 87c81f75cf84..c200e6d3aed5 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 9df7cc75c1bc..7618c4285cf9 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 2b64ea46318f..c6ea5b4baff3 100644
@@ -412,7 +412,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev,
 		return -EINVAL;
 	}
 
-	ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+	ret = iommu_fwspec_init(dev, of_fwnode_handle(args->np));
 	if (ret)
 		return ret;
drivers/iommu/of_iommu.c +6 -13
index 3afe0b48a48d..08c523ad55ad 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, of_fwnode_handle(iommu_spec->np));
+	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 f86c7ae91814..4365d9936e68 100644
@@ -837,7 +837,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, of_fwnode_handle(dev->of_node));
 	if (err < 0) {
 		dev_err(dev, "failed to initialize fwspec: %d\n", err);
 		return err;
include/acpi/acpi_bus.h +1 -2
index 1a4dfd7a1c4a..9d815837e297 100644
@@ -736,8 +736,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 17b3f36ad843..81893aad9ee4 100644
@@ -1005,11 +1005,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)
 {

@@ -1315,8 +1313,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;
 }

@@ -1331,12 +1328,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
maintainer
#3 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

Hi Robin,

Quoted 290 lines · Robin Murphy

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 use this opportunity to modernise with some "new" helpers that arrived shortly after this code was first written; the generic fwnode_handle_get() clears up that ugly get/put mismatch, while of_fwnode_handle() can now abstract those open-coded dereferences.

Tested-by Jean-Philippe Brucker <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---
v2: Add of_fwnode_handle() cleanup as well

---
  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 503773707e01..8d5a589db141 100644
@@ -1577,12 +1577,11 @@ 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;
 
-	ret = iommu_fwspec_init(dev, fwnode, ops);
+	ret = iommu_fwspec_init(dev, fwnode);
  	if (ret)
  		return ret;
 

@@ -1639,8 +1638,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 87c81f75cf84..c200e6d3aed5 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 9df7cc75c1bc..7618c4285cf9 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 2b64ea46318f..c6ea5b4baff3 100644
@@ -412,7 +412,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev,
  		return -EINVAL;
  	}
 
-	ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+	ret = iommu_fwspec_init(dev, of_fwnode_handle(args->np));
  	if (ret)
  		return ret;
drivers/iommu/of_iommu.c +6 -13
index 3afe0b48a48d..08c523ad55ad 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, of_fwnode_handle(iommu_spec->np));
+	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 f86c7ae91814..4365d9936e68 100644
@@ -837,7 +837,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, of_fwnode_handle(dev->of_node));
  	if (err < 0) {
  		dev_err(dev, "failed to initialize fwspec: %d\n", err);
  		return err;
include/acpi/acpi_bus.h +1 -2
index 1a4dfd7a1c4a..9d815837e297 100644
@@ -736,8 +736,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 -5
index 17b3f36ad843..81893aad9ee4 100644
@@ -1005,11 +1005,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)
  {

@@ -1315,8 +1313,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;
  }

@@ -1331,12 +1328,6 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
  	return -ENODEV;
  }

include/linux/iommu.h static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids, · line 1333

Quoted 10 lines
include/linux/iommu.h +0 -6
 
-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)
  {

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Cheers
Jon

Signature · 1 line
nvpublic
author reviewer
#4 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

Quoted 298 lines · Jon Hunter

Hi Robin,

Quoted 288 lines · Robin Murphy

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 use this opportunity to modernise with some "new" helpers that arrived shortly after this code was first written; the generic fwnode_handle_get() clears up that ugly get/put mismatch, while of_fwnode_handle() can now abstract those open-coded dereferences.

Tested-by Jean-Philippe Brucker <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---
v2: Add of_fwnode_handle() cleanup as well
---
  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 503773707e01..8d5a589db141 100644
@@ -1577,12 +1577,11 @@ 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;
-    ret = iommu_fwspec_init(dev, fwnode, ops);
+    ret = iommu_fwspec_init(dev, fwnode);
       if (ret)
           return ret;

@@ -1639,8 +1638,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 87c81f75cf84..c200e6d3aed5 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 9df7cc75c1bc..7618c4285cf9 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 2b64ea46318f..c6ea5b4baff3 100644
@@ -412,7 +412,7 @@ static int mtk_iommu_v1_create_mapping(struct 
 device *dev,
           return -EINVAL;
       }
-    ret = iommu_fwspec_init(dev, &args->np->fwnode, &mtk_iommu_v1_ops);
+    ret = iommu_fwspec_init(dev, of_fwnode_handle(args->np));
       if (ret)
           return ret;
drivers/iommu/of_iommu.c +4 -7
index 3afe0b48a48d..08c523ad55ad 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, of_fwnode_handle(iommu_spec->np));
+    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 f86c7ae91814..4365d9936e68 100644
@@ -837,7 +837,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, of_fwnode_handle(dev->of_node));
       if (err < 0) {
           dev_err(dev, "failed to initialize fwspec: %d\n", err);
           return err;
include/acpi/acpi_bus.h +1 -2
index 1a4dfd7a1c4a..9d815837e297 100644
@@ -736,8 +736,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 +1 -3
index 17b3f36ad843..81893aad9ee4 100644
@@ -1005,11 +1005,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)

  {

include/linux/iommu.h +1 -8
@@ -1315,8 +1313,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;
   }

@@ -1331,12 +1328,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;
-}
-
Quoted 3 lines

  static inline int
  iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat) {

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

Quoted 4 lines

 tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

I *think* it should probably just be a case of:

- err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node));
+ err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Thanks,
Robin.

maintainer
#5 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

On 12/07/2024 12:48, Robin Murphy wrote:

...

Quoted 9 lines
Quoted 6 lines

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

  tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

Quoted 8 lines

I *think* it should probably just be a case of:

-    err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node)); +    err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Yes making that change in the tegra-smmu driver does fix it.

Thanks!
Jon

Signature · 1 line
nvpublic
maintainer
#6 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

Quoted 32 lines · Jon Hunter

On 12/07/2024 12:48, Robin Murphy wrote:

...

Quoted 27 lines
Quoted 4 lines

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

Quoted 8 lines

  tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

I *think* it should probably just be a case of:

-    err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node)); +    err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

Quoted 4 lines
Quoted 1 line

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Yes making that change in the tegra-smmu driver does fix it.

Fantastic! I'll pick it up right now.

Cheers,

Will

author reviewer
#7 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

In reply to Jon Hunter
Quoted 19 lines · Jon Hunter

On 12/07/2024 12:48, Robin Murphy wrote:

...

Quoted 14 lines
Quoted 6 lines

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

  tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

I *think* it should probably just be a case of:

-    err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node)); +    err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

Quoted 6 lines
Quoted 3 lines

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Yes making that change in the tegra-smmu driver does fix it.

Ace, thanks for confirming! I was just writing a follow-up to say that I've pretty much convinced myself that this (proper diff below) should in fact be the right thing to do in general as well :)

Will, Joerg, would you prefer to have a standalone fix patch for the nvidia/tegra branch to then re-merge fwspec-ops-removal and fix up the conflict, or just a patch on top of fwspec-ops-removal as below?

Thanks,
Robin.

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

drivers/iommu/tegra-smmu.c +1 -1
index 4365d9936e68..7f633bb5efef 100644
@@ -837,7 +837,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, of_fwnode_handle(dev->of_node));
+	err = iommu_fwspec_init(dev, dev_fwnode(smmu->dev));
 	if (err < 0) {
 		dev_err(dev, "failed to initialize fwspec: %d\n", err);
 		return err;
maintainer
#8 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

Quoted 38 lines · Robin Murphy
Quoted 32 lines · Jon Hunter

On 12/07/2024 12:48, Robin Murphy wrote:

...

Quoted 27 lines
Quoted 4 lines

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

Quoted 8 lines

  tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

I *think* it should probably just be a case of:

-    err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node)); +    err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

Quoted 4 lines
Quoted 1 line

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Yes making that change in the tegra-smmu driver does fix it.

Quoted 7 lines

Ace, thanks for confirming! I was just writing a follow-up to say that I've pretty much convinced myself that this (proper diff below) should in fact be the right thing to do in general as well :)

Will, Joerg, would you prefer to have a standalone fix patch for the nvidia/tegra branch to then re-merge fwspec-ops-removal and fix up the conflict, or just a patch on top of fwspec-ops-removal as below?

I've just fixed it locally on the tegra branch, so I'll then just resolve the conflict with fwspec-ops-removal the right way. That way, we can backport the thing if we need to.

Cheers,

Will

author reviewer
#9 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

Quoted 43 lines · Will Deacon
Quoted 42 lines · Robin Murphy
Quoted 32 lines · Jon Hunter

On 12/07/2024 12:48, Robin Murphy wrote:

...

Quoted 27 lines
Quoted 4 lines

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

Quoted 8 lines

��tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 ��nouveau 57000000.gpu: failed to initialize fwspec: -517

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

I *think* it should probably just be a case of:

-��� err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node)); +��� err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

Quoted 4 lines
Quoted 1 line

(Also now I'm wondering why I didn't just use dev_fwnode() there...)

Yes making that change in the tegra-smmu driver does fix it.

Ace, thanks for confirming! I was just writing a follow-up to say that I've pretty much convinced myself that this (proper diff below) should in fact be the right thing to do in general as well :)

Quoted 7 lines
Quoted 3 lines

Will, Joerg, would you prefer to have a standalone fix patch for the nvidia/tegra branch to then re-merge fwspec-ops-removal and fix up the conflict, or just a patch on top of fwspec-ops-removal as below?

I've just fixed it locally on the tegra branch, so I'll then just resolve the conflict with fwspec-ops-removal the right way. That way, we can backport the thing if we need to.

Cool, thanks for taking care of it!

Robin.

maintainer
#10 lore raw

Re: [PATCH v3 2/5] iommu: Resolve fwspec ops automatically

In reply to Robin Murphy
Quoted 8 lines · Robin Murphy
Quoted 7 lines · Jon Hunter

I am seeing some failures on -next with some of our devices. Bisect is pointing to this commit. Looks like the host1x device is no longer probing successfully. I see the following ...

 tegra-host1x 50000000.host1x: failed to initialize fwspec: -517 nouveau 57000000.gpu: failed to initialize fwspec: -517

Quoted 8 lines
Quoted 4 lines

The probe seems to be deferred forever. The above is seen on Tegra210 but Tegra30 and Tegra194 are also having the same problem. Interestingly it is not all devices and so make me wonder if we are missing something on these devices? Let me know if you have any thoughts.

Ugh, tegra-smmu has been doing a complete nonsense this whole time - on closer inspection, it's passing the fwnode of the *client device* where it should be that of the IOMMU device :(

Ha, so it is!

Quoted 9 lines

I *think* it should probably just be a case of:

(quoted diff) +1 -1
-    err = iommu_fwspec_init(dev, of_fwnode_handle(dev->of_node));
+    err = iommu_fwspec_init(dev, of_fwnode_handle(smmu->dev->of_node));

since smmu->dev appears to be the same one initially passed to iommu_device_register(), so it at least ought to match and work, but the SMMU device vs. MC device thing leaves me mildly wary of how correct it might be overall.

Jon -- any chance you could give this fix a go, please? I'm hesitant to drop the whole branch just for this, but we've basically run out of time for 6.11 and so knowing we have a working fix would be really helpful.

Cheers,

Will

author reviewer
#11 lore raw

PATCH v3 3/5 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.

Acked-by Rafael J. Wysocki <[email protected]>
Tested-by Jean-Philippe Brucker <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
 drivers/acpi/scan.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)
drivers/acpi/scan.c +5 -23
index 8d5a589db141..2cfbb365c4ab 100644
@@ -1588,26 +1588,14 @@ int acpi_iommu_fwspec_init(struct device *dev, u32 id,
 	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
-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,15 +1612,7 @@ static int acpi_iommu_configure_id(struct device *dev, const u32 *id_in)
 	if (!err && dev->bus)
 		err = iommu_probe_device(dev);
 
-	if (err == -EPROBE_DEFER)
-		return err;
-	if (err) {
-		dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
-		return err;
-	}
-	if (!acpi_iommu_fwspec_ops(dev))
-		return -ENODEV;
-	return 0;
+	return err;
 }
 
 #else /* !CONFIG_IOMMU_API */

@@ -1672,6 +1652,8 @@ 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;
+	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
author reviewer
#12 lore raw

PATCH v3 4/5 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.

Acked-by Rob Herring (Arm) <[email protected]>
Tested-by Jean-Philippe Brucker <[email protected]>
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 08c523ad55ad..c946521a5906 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
author reviewer
#13 lore raw

PATCH v3 5/5 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.

Tested-by Jean-Philippe Brucker <[email protected]>
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 7618c4285cf9..e15ae1dd494b 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 c946521a5906..559c5db78edb 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 81893aad9ee4..11ae1750cb1d 100644
@@ -968,7 +968,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

@@ -979,7 +978,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
maintainer Applied
#14 lore raw
In reply to Robin Murphy
Quoted 9 lines · Robin Murphy

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

Hi all,

Just a quick update with an extra patch to avoid the mediatek-v1 driver breaking (and I have now build-tested ARCH=arm to make sure, apologies for being lazy before...)

[...]

Applied to arm64 (fwspec-ops-removal), thanks!

[1/5] iommu/mediatek-v1: Clean up redundant fwspec checks

      https://git.kernel.org/iommu/c/e7acc36f26b0
[2/5] iommu: Resolve fwspec ops automatically
      https://git.kernel.org/iommu/c/3f7c32091628
[3/5] ACPI: Retire acpi_iommu_fwspec_ops()
      https://git.kernel.org/iommu/c/78596b5c321c
[4/5] OF: Simplify of_iommu_configure()
      https://git.kernel.org/iommu/c/5f937bc48a6a
[5/5] iommu: Remove iommu_fwspec ops
      https://git.kernel.org/iommu/c/3e36c15fc1cc

Cheers,

Signature · 5 lines
Will

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