| Message ID | 1486136933-20328-2-git-send-email-sricharan@codeaurora.org |
|---|---|
| State | Not Applicable |
| Headers | show |
Hello, On 03/02/17 15:48, Sricharan R wrote: > From: Robin Murphy <robin.murphy@arm.com> > > 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@samsung.com> > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > [*] 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) >
On 08/03/17 18:58, Jean-Philippe Brucker wrote: [...] >> 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? Ah yes, Tomasz had found breakages with the "mmu-masters" binding before, and I'd already pushed out a fixup for this one[1], but I forgot that that discussion was all off-list (out of diplomatic concern that the breakage might have been intentional - it wasn't, honest!) Now that rc1 is out I should re-do that branch with v8 of this series plus the fixups folded in, unless Sricharan beats me to it. Thanks for the reminder, Robin. [1]:http://www.linux-arm.org/git?p=linux-rm.git;a=commitdiff;h=0049a34e523506813995c05766f5e2c16d616354 > > Thanks, > Jean-Philippe
Hi Robin, >On 08/03/17 18:58, Jean-Philippe Brucker wrote: >[...] >>> 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? > >Ah yes, Tomasz had found breakages with the "mmu-masters" binding >before, and I'd already pushed out a fixup for this one[1], but I forgot that >that discussion was all off-list (out of diplomatic concern that the breakage >might have been intentional - it wasn't, honest!) > >Now that rc1 is out I should re-do that branch with v8 of this series plus the >fixups folded in, unless Sricharan beats me to it. > Right, I had this one [1] as V9 which had all your fixes that we discussed offline as well. I was about to post a V9 today on top of -rc1 today as well. [1] https://github.com/sricharanaz/iommu/tree/pd_v9 Regards, Sricharan >Thanks for the reminder, >Robin. > >[1]:http://www.linux-arm.org/git?p=linux- >rm.git;a=commitdiff;h=0049a34e523506813995c05766f5e2c16d616354 > >> >> Thanks, >> Jean-Philippe > >-- >To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >the body of a message to majordomo@vger.kernel.org More majordomo info >at http://vger.kernel.org/majordomo-info.html
On 09/03/17 09:52, sricharan wrote: > Hi Robin, > >> On 08/03/17 18:58, Jean-Philippe Brucker wrote: >> [...] >>>> 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? >> >> Ah yes, Tomasz had found breakages with the "mmu-masters" binding >> before, and I'd already pushed out a fixup for this one[1], but I forgot > that >> that discussion was all off-list (out of diplomatic concern that the > breakage >> might have been intentional - it wasn't, honest!) >> >> Now that rc1 is out I should re-do that branch with v8 of this series plus > the >> fixups folded in, unless Sricharan beats me to it. >> > > Right, I had this one [1] as V9 which had all your fixes that we discussed > offline as > well. I was about to post a V9 today on top of -rc1 today as well. > > [1] https://github.com/sricharanaz/iommu/tree/pd_v9 Awesome, I'll try to get on with testing that ASAP. Thanks! Robin. > > Regards, > Sricharan > > >> Thanks for the reminder, >> Robin. >> >> [1]:http://www.linux-arm.org/git?p=linux- >> rm.git;a=commitdiff;h=0049a34e523506813995c05766f5e2c16d616354 >> >>> >>> Thanks, >>> Jean-Philippe >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in >> the body of a message to majordomo@vger.kernel.org More majordomo info >> at http://vger.kernel.org/majordomo-info.html >
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); - 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)