| Message ID | 3-v1-5f734af130a3+34f-iommu_fwspec_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Solve iommu probe races around iommu_fwspec | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Fri, Nov 03, 2023 at 01:44:48PM -0300, Jason Gunthorpe wrote: > Instead of returning 1 and trying to handle positive error codes just > stick to the convention of returning -ENODEV. Remove references to ops > from of_iommu_configure(), a NULL ops will already generate an error code. > > There is no reason to check dev->bus, if err=0 at this point then the > called configure functions thought there was an iommu and we should try to > probe it. Remove it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/of_iommu.c | 42 +++++++++++++--------------------------- > 1 file changed, 13 insertions(+), 29 deletions(-) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index e2fa29c16dd758..4f77495a2543ea 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -17,7 +17,7 @@ > #include <linux/slab.h> > #include <linux/fsl/mc.h> > > -#define NO_IOMMU 1 > +#define NO_IOMMU -ENODEV > With this the following can be simplified in of_iommu_configure_dev_id: diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 4f77495a2543..b9b995712029 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -61,7 +61,7 @@ static int of_iommu_configure_dev_id(struct device_node *master_np, "iommu-map-mask", &iommu_spec.np, iommu_spec.args); if (err) - return err == -ENODEV ? NO_IOMMU : err; + return err; err = of_iommu_xlate(dev, &iommu_spec); of_node_put(iommu_spec.np);
On Fri, Nov 03, 2023 at 03:03:53PM -0700, Jerry Snitselaar wrote: > With this the following can be simplified in of_iommu_configure_dev_id: > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 4f77495a2543..b9b995712029 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -61,7 +61,7 @@ static int of_iommu_configure_dev_id(struct device_node *master_np, > "iommu-map-mask", &iommu_spec.np, > iommu_spec.args); > if (err) > - return err == -ENODEV ? NO_IOMMU : err; > + return err; Yeah, at this point it just makes sense to erase the whole thing: -#define NO_IOMMU -ENODEV - static int of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec) { @@ -29,7 +27,7 @@ static int of_iommu_xlate(struct device *dev, ops = iommu_ops_from_fwnode(fwnode); if ((ops && !ops->of_xlate) || !of_device_is_available(iommu_spec->np)) - return NO_IOMMU; + return -ENODEV; ret = iommu_fwspec_init(dev, &iommu_spec->np->fwnode, ops); if (ret) @@ -61,7 +59,7 @@ static int of_iommu_configure_dev_id(struct device_node *master_np, "iommu-map-mask", &iommu_spec.np, iommu_spec.args); if (err) - return err == -ENODEV ? NO_IOMMU : err; + return err; err = of_iommu_xlate(dev, &iommu_spec); of_node_put(iommu_spec.np); @@ -72,7 +70,7 @@ static int of_iommu_configure_dev(struct device_node *master_np, struct device *dev) { struct of_phandle_args iommu_spec; - int err = NO_IOMMU, idx = 0; + int err = -ENODEV, idx = 0; while (!of_parse_phandle_with_args(master_np, "iommus", "#iommu-cells", Thanks, Jason
On Fri, Nov 03, 2023 at 01:44:48PM -0300, Jason Gunthorpe wrote: > Instead of returning 1 and trying to handle positive error codes just > stick to the convention of returning -ENODEV. Remove references to ops > from of_iommu_configure(), a NULL ops will already generate an error code. nit: "iommu: of: ..." or "iommu/of: " for the subject. "of: ..." is generally drivers/of/. > > There is no reason to check dev->bus, if err=0 at this point then the > called configure functions thought there was an iommu and we should try to > probe it. Remove it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/of_iommu.c | 42 +++++++++++++--------------------------- > 1 file changed, 13 insertions(+), 29 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index e2fa29c16dd758..4f77495a2543ea 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -17,7 +17,7 @@ #include <linux/slab.h> #include <linux/fsl/mc.h> -#define NO_IOMMU 1 +#define NO_IOMMU -ENODEV static int of_iommu_xlate(struct device *dev, struct of_phandle_args *iommu_spec) @@ -117,9 +117,8 @@ 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) { - const struct iommu_ops *ops = NULL; struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev); - int err = NO_IOMMU; + int err; if (!master_np) return -ENODEV; @@ -150,34 +149,19 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np, err = of_iommu_configure_device(master_np, dev, id); } - /* - * Two success conditions can be represented by non-negative err here: - * >0 : there is no IOMMU, or one was unavailable for non-fatal reasons - * 0 : we found an IOMMU, and dev->fwspec is initialised appropriately - * <0 : any actual error - */ - if (!err) { - /* The fwspec pointer changed, read it again */ - fwspec = dev_iommu_fwspec_get(dev); - ops = fwspec->ops; - } - /* - * If we have reason to believe the IOMMU driver missed the initial - * probe for dev, replay it to get things in order. - */ - if (!err && dev->bus) - err = iommu_probe_device(dev); + if (err == -ENODEV || err == -EPROBE_DEFER) + return err; + if (err) + goto err_log; - /* Ignore all other errors apart from EPROBE_DEFER */ - if (err < 0) { - if (err == -EPROBE_DEFER) - return err; - dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); - return -ENODEV; - } - if (!ops) - return -ENODEV; + err = iommu_probe_device(dev); + if (err) + goto err_log; return 0; + +err_log: + dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); + return err; } static enum iommu_resv_type __maybe_unused
Instead of returning 1 and trying to handle positive error codes just stick to the convention of returning -ENODEV. Remove references to ops from of_iommu_configure(), a NULL ops will already generate an error code. There is no reason to check dev->bus, if err=0 at this point then the called configure functions thought there was an iommu and we should try to probe it. Remove it. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/of_iommu.c | 42 +++++++++++++--------------------------- 1 file changed, 13 insertions(+), 29 deletions(-)