Re: [PATCH V7 01/11] iommu/of: Refactor of_iommu_configure() for error handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Robin,

On 25.01.2017 18:35, Robin Murphy wrote:
Hi Tomasz,

On 25/01/17 17:17, Tomasz Nowicki wrote:
Hi Sricharan,

On 23.01.2017 17:18, Sricharan R wrote:
From: Robin Murphy <robin.murphy@xxxxxxx>

In preparation for some upcoming cleverness, rework the control flow in
of_iommu_configure() to minimise duplication and improve the propogation
of errors. It's also as good a time as any to switch over from the
now-just-a-compatibility-wrapper of_iommu_get_ops() to using the generic
IOMMU instance interface directly.

Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
---
 drivers/iommu/of_iommu.c | 83
+++++++++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index 0f57ddc..ee49081 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -96,6 +96,28 @@ int of_get_dma_window(struct device_node *dn, const
char *prefix, int index,
 }
 EXPORT_SYMBOL_GPL(of_get_dma_window);

+static const struct iommu_ops
+*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 err;
+
+    ops = iommu_get_instance(fwnode);
+    if (!ops || !ops->of_xlate)
+        return NULL;
+
+    err = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops);
+    if (err)
+        return ERR_PTR(err);
+
+    err = ops->of_xlate(dev, iommu_spec);
+    if (err)
+        return ERR_PTR(err);
+
+    return ops;
+}
+
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
     struct of_phandle_args *iommu_spec = data;
@@ -105,10 +127,11 @@ static int __get_pci_rid(struct pci_dev *pdev,
u16 alias, void *data)
 }

 static const struct iommu_ops
-*of_pci_iommu_configure(struct pci_dev *pdev, struct device_node
*bridge_np)
+*of_pci_iommu_init(struct pci_dev *pdev, struct device_node *bridge_np)
 {
     const struct iommu_ops *ops;
     struct of_phandle_args iommu_spec;
+    int err;

     /*
      * Start by tracing the RID alias down the PCI topology as
@@ -123,56 +146,56 @@ static int __get_pci_rid(struct pci_dev *pdev,
u16 alias, void *data)
      * bus into the system beyond, and which IOMMU it ends up at.
      */
     iommu_spec.np = NULL;
-    if (of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
-               "iommu-map-mask", &iommu_spec.np, iommu_spec.args))
-        return NULL;
+    err = of_pci_map_rid(bridge_np, iommu_spec.args[0], "iommu-map",
+                 "iommu-map-mask", &iommu_spec.np,
+                 iommu_spec.args);
+    if (err)
+        return ERR_PTR(err);

-    ops = of_iommu_get_ops(iommu_spec.np);
-    if (!ops || !ops->of_xlate ||
-        iommu_fwspec_init(&pdev->dev, &iommu_spec.np->fwnode, ops) ||
-        ops->of_xlate(&pdev->dev, &iommu_spec))
-        ops = NULL;
+    ops = of_iommu_xlate(&pdev->dev, &iommu_spec);

     of_node_put(iommu_spec.np);
     return ops;
 }

-const struct iommu_ops *of_iommu_configure(struct device *dev,
-                       struct device_node *master_np)
+static const struct iommu_ops
+*of_platform_iommu_init(struct device *dev, struct device_node *np)
 {
     struct of_phandle_args iommu_spec;
-    struct device_node *np;
     const struct iommu_ops *ops = NULL;
     int idx = 0;

-    if (dev_is_pci(dev))
-        return of_pci_iommu_configure(to_pci_dev(dev), master_np);
-
     /*
      * We don't currently walk up the tree looking for a parent IOMMU.
      * See the `Notes:' section of
      * Documentation/devicetree/bindings/iommu/iommu.txt
      */
-    while (!of_parse_phandle_with_args(master_np, "iommus",
-                       "#iommu-cells", idx,
-                       &iommu_spec)) {
-        np = iommu_spec.np;
-        ops = of_iommu_get_ops(np);
-
-        if (!ops || !ops->of_xlate ||
-            iommu_fwspec_init(dev, &np->fwnode, ops) ||
-            ops->of_xlate(dev, &iommu_spec))
-            goto err_put_node;
-
-        of_node_put(np);
+    while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells",
+                       idx, &iommu_spec)) {
+        ops = of_iommu_xlate(dev, &iommu_spec);
+        of_node_put(iommu_spec.np);
         idx++;
+        if (IS_ERR_OR_NULL(ops))
+            break;
     }

     return ops;
+}
+
+const struct iommu_ops *of_iommu_configure(struct device *dev,
+                       struct device_node *master_np)
+{
+    const struct iommu_ops *ops;
+
+    if (!master_np)
+        return NULL;
+
+    if (dev_is_pci(dev))
+        ops = of_pci_iommu_init(to_pci_dev(dev), master_np);
I gave the whole patch set a try on ThunderX. really_probe() is failing
on dma_configure()->of_pci_iommu_init() for each PCI device.
When you say "failing", do you mean cleanly, or with a crash? I've
managed to hit __of_match_node() dereferencing NULL from
of_iommu_xlate() in a horribly complicated chain of events, which I'm
trying to figure out now, and I wonder if the two might be related.


Cleanly, of_pci_iommu_init()->of_pci_map_rid() can't find "iommu-map" property for my case. Probably not related to your crash.

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux