Hello,
On 03/02/17 15:48, 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.
>
> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> ---
> [*] Resolved a conflict while rebasing on top linux-next as the patch
> is not there in mainline master.
>
> "iommu: Drop the of_iommu_{set/get}_ops() interface"
> https://lkml.org/lkml/2017/1/3/489
>
> 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 d7f480a..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);
This change doesn't work with of_pci_map_rid when the PCI RC isn't behind
an IOMMU:
map = of_get_property(np, map_name, &map_len);
if (!map) {
if (target)
return -ENODEV;
/* Otherwise, no map implies no translation */
*id_out = rid;
return 0;
}
Previously with no iommu-map, we returned -ENODEV but it was discarded by
of_pci_iommu_configure. Now it is propagated and the whole device probing
fails. Instead, maybe of_pci_map_rid should always return 0 if no
iommu-map, and the caller should check if *target is still NULL?
Thanks,
Jean-Philippe
>
> - ops = iommu_get_instance(&iommu_spec.np->fwnode);
> - 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 = iommu_get_instance(&np->fwnode);
> -
> - 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);
> + else
> + ops = of_platform_iommu_init(dev, master_np);
>
> -err_put_node:
> - of_node_put(np);
> - return NULL;
> + return IS_ERR(ops) ? NULL : ops;
> }
>
> static int __init of_iommu_init(void)
>
