| Message ID | 2-v1-5f734af130a3+34f-iommu_fwspec_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Solve iommu probe races around iommu_fwspec | expand |
On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote: > Nothing needs this pointer. Return a normal error code with the usual > IOMMU semantic that ENODEV means 'there is no IOMMU driver'. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/of_iommu.c | 29 ++++++++++++++++++----------- > drivers/of/device.c | 22 +++++++++++++++------- > include/linux/of_iommu.h | 13 ++++++------- > 3 files changed, 39 insertions(+), 25 deletions(-) > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > index 157b286e36bf3a..e2fa29c16dd758 100644 > --- a/drivers/iommu/of_iommu.c > +++ b/drivers/iommu/of_iommu.c > @@ -107,20 +107,26 @@ static int of_iommu_configure_device(struct device_node *master_np, > of_iommu_configure_dev(master_np, dev); > } > > -const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np, > - const u32 *id) > +/* > + * Returns: > + * 0 on success, an iommu was configured > + * -ENODEV if the device does not have any IOMMU > + * -EPROBEDEFER if probing should be tried again > + * -errno fatal errors It looks to me like it will only return 0, -ENODEV, or -EPROBEDEFER with other -errno getting boiled down to -ENODEV. > + */ > +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; > > if (!master_np) > - return NULL; > + return -ENODEV; > > if (fwspec) { > if (fwspec->ops) > - return fwspec->ops; > + return 0; > > /* In the deferred case, start again from scratch */ > iommu_fwspec_free(dev); > @@ -163,14 +169,15 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > err = iommu_probe_device(dev); > > /* Ignore all other errors apart from EPROBE_DEFER */ > - if (err == -EPROBE_DEFER) { > - ops = ERR_PTR(err); > - } else if (err < 0) { > + if (err < 0) { > + if (err == -EPROBE_DEFER) > + return err; > dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); minor thing, but should this use %pe and ERR_PTR(err) like is done in of_dma_configure_id? > - ops = NULL; > + return -ENODEV; > } > - > - return ops; > + if (!ops) > + return -ENODEV; > + return 0; > } > > static enum iommu_resv_type __maybe_unused > diff --git a/drivers/of/device.c b/drivers/of/device.c > index 65c71be71a8d45..873d933e8e6d1d 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -93,12 +93,12 @@ of_dma_set_restricted_buffer(struct device *dev, struct device_node *np) > int of_dma_configure_id(struct device *dev, struct device_node *np, > bool force_dma, const u32 *id) > { > - const struct iommu_ops *iommu; > const struct bus_dma_region *map = NULL; > struct device_node *bus_np; > u64 dma_start = 0; > u64 mask, end, size = 0; > bool coherent; > + int iommu_ret; > int ret; > > if (np == dev->of_node) > @@ -181,21 +181,29 @@ int of_dma_configure_id(struct device *dev, struct device_node *np, > dev_dbg(dev, "device is%sdma coherent\n", > coherent ? " " : " not "); > > - iommu = of_iommu_configure(dev, np, id); > - if (PTR_ERR(iommu) == -EPROBE_DEFER) { > + iommu_ret = of_iommu_configure(dev, np, id); > + if (iommu_ret == -EPROBE_DEFER) { > /* Don't touch range map if it wasn't set from a valid dma-ranges */ > if (!ret) > 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)); > > - dev_dbg(dev, "device is%sbehind an iommu\n", > - iommu ? " " : " not "); > + /* > + * 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"); > > arch_setup_dma_ops(dev, dma_start, size, coherent); > > - if (!iommu) > + if (iommu_ret) > of_dma_set_restricted_buffer(dev, np); > > return 0; > diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h > index 9a5e6b410dd2fb..e61cbbe12dac6f 100644 > --- a/include/linux/of_iommu.h > +++ b/include/linux/of_iommu.h > @@ -8,20 +8,19 @@ struct iommu_ops; > > #ifdef CONFIG_OF_IOMMU > > -extern const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np, > - const u32 *id); > +extern int of_iommu_configure(struct device *dev, struct device_node *master_np, > + const u32 *id); > > extern void of_iommu_get_resv_regions(struct device *dev, > struct list_head *list); > > #else > > -static inline const struct iommu_ops *of_iommu_configure(struct device *dev, > - struct device_node *master_np, > - const u32 *id) > +static inline int of_iommu_configure(struct device *dev, > + struct device_node *master_np, > + const u32 *id) > { > - return NULL; > + return -ENODEV; > } > > static inline void of_iommu_get_resv_regions(struct device *dev, > -- > 2.42.0 >
On Fri, Nov 03, 2023 at 02:42:01PM -0700, Jerry Snitselaar wrote: > On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote: > > Nothing needs this pointer. Return a normal error code with the usual > > IOMMU semantic that ENODEV means 'there is no IOMMU driver'. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/of_iommu.c | 29 ++++++++++++++++++----------- > > drivers/of/device.c | 22 +++++++++++++++------- > > include/linux/of_iommu.h | 13 ++++++------- > > 3 files changed, 39 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > index 157b286e36bf3a..e2fa29c16dd758 100644 > > --- a/drivers/iommu/of_iommu.c > > +++ b/drivers/iommu/of_iommu.c > > @@ -107,20 +107,26 @@ static int of_iommu_configure_device(struct device_node *master_np, > > of_iommu_configure_dev(master_np, dev); > > } > > > > -const struct iommu_ops *of_iommu_configure(struct device *dev, > > - struct device_node *master_np, > > - const u32 *id) > > +/* > > + * Returns: > > + * 0 on success, an iommu was configured > > + * -ENODEV if the device does not have any IOMMU > > + * -EPROBEDEFER if probing should be tried again > > + * -errno fatal errors > > It looks to me like it will only return 0, -ENODEV, or -EPROBEDEFER > with other -errno getting boiled down to -ENODEV. > Ah, I should've looked at the next patch first. So, never mind on this and the question about the dev_dbg. Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Fri, Nov 03, 2023 at 02:42:01PM -0700, Jerry Snitselaar wrote: > On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote: > > Nothing needs this pointer. Return a normal error code with the usual > > IOMMU semantic that ENODEV means 'there is no IOMMU driver'. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/of_iommu.c | 29 ++++++++++++++++++----------- > > drivers/of/device.c | 22 +++++++++++++++------- > > include/linux/of_iommu.h | 13 ++++++------- > > 3 files changed, 39 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c > > index 157b286e36bf3a..e2fa29c16dd758 100644 > > --- a/drivers/iommu/of_iommu.c > > +++ b/drivers/iommu/of_iommu.c > > @@ -107,20 +107,26 @@ static int of_iommu_configure_device(struct device_node *master_np, > > of_iommu_configure_dev(master_np, dev); > > } > > > > -const struct iommu_ops *of_iommu_configure(struct device *dev, > > - struct device_node *master_np, > > - const u32 *id) > > +/* > > + * Returns: > > + * 0 on success, an iommu was configured > > + * -ENODEV if the device does not have any IOMMU > > + * -EPROBEDEFER if probing should be tried again > > + * -errno fatal errors > > It looks to me like it will only return 0, -ENODEV, or -EPROBEDEFER > with other -errno getting boiled down to -ENODEV. Yeah, that next patch sorts it out, it is sort of a typo here: @@ -173,7 +173,7 @@ int of_iommu_configure(struct device *dev, struct device_node *master_np, if (err == -EPROBE_DEFER) return err; dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); - return -ENODEV; + return err; } if (!ops) return -ENODEV; > > @@ -163,14 +169,15 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, > > err = iommu_probe_device(dev); > > > > /* Ignore all other errors apart from EPROBE_DEFER */ > > - if (err == -EPROBE_DEFER) { > > - ops = ERR_PTR(err); > > - } else if (err < 0) { > > + if (err < 0) { > > + if (err == -EPROBE_DEFER) > > + return err; > > dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); > > minor thing, but should this use %pe and ERR_PTR(err) like is done > in of_dma_configure_id? Sure Thanks, Jason
On Fri, Nov 03, 2023 at 01:44:47PM -0300, Jason Gunthorpe wrote: > Nothing needs this pointer. Return a normal error code with the usual > IOMMU semantic that ENODEV means 'there is no IOMMU driver'. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/of_iommu.c | 29 ++++++++++++++++++----------- > drivers/of/device.c | 22 +++++++++++++++------- Acked-by: Rob Herring <robh@kernel.org> > include/linux/of_iommu.h | 13 ++++++------- > 3 files changed, 39 insertions(+), 25 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c index 157b286e36bf3a..e2fa29c16dd758 100644 --- a/drivers/iommu/of_iommu.c +++ b/drivers/iommu/of_iommu.c @@ -107,20 +107,26 @@ static int of_iommu_configure_device(struct device_node *master_np, of_iommu_configure_dev(master_np, dev); } -const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np, - const u32 *id) +/* + * Returns: + * 0 on success, an iommu was configured + * -ENODEV if the device does not have any IOMMU + * -EPROBEDEFER if probing should be tried again + * -errno fatal errors + */ +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; if (!master_np) - return NULL; + return -ENODEV; if (fwspec) { if (fwspec->ops) - return fwspec->ops; + return 0; /* In the deferred case, start again from scratch */ iommu_fwspec_free(dev); @@ -163,14 +169,15 @@ const struct iommu_ops *of_iommu_configure(struct device *dev, err = iommu_probe_device(dev); /* Ignore all other errors apart from EPROBE_DEFER */ - if (err == -EPROBE_DEFER) { - ops = ERR_PTR(err); - } else if (err < 0) { + if (err < 0) { + if (err == -EPROBE_DEFER) + return err; dev_dbg(dev, "Adding to IOMMU failed: %d\n", err); - ops = NULL; + return -ENODEV; } - - return ops; + if (!ops) + return -ENODEV; + return 0; } static enum iommu_resv_type __maybe_unused diff --git a/drivers/of/device.c b/drivers/of/device.c index 65c71be71a8d45..873d933e8e6d1d 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -93,12 +93,12 @@ of_dma_set_restricted_buffer(struct device *dev, struct device_node *np) int of_dma_configure_id(struct device *dev, struct device_node *np, bool force_dma, const u32 *id) { - const struct iommu_ops *iommu; const struct bus_dma_region *map = NULL; struct device_node *bus_np; u64 dma_start = 0; u64 mask, end, size = 0; bool coherent; + int iommu_ret; int ret; if (np == dev->of_node) @@ -181,21 +181,29 @@ int of_dma_configure_id(struct device *dev, struct device_node *np, dev_dbg(dev, "device is%sdma coherent\n", coherent ? " " : " not "); - iommu = of_iommu_configure(dev, np, id); - if (PTR_ERR(iommu) == -EPROBE_DEFER) { + iommu_ret = of_iommu_configure(dev, np, id); + if (iommu_ret == -EPROBE_DEFER) { /* Don't touch range map if it wasn't set from a valid dma-ranges */ if (!ret) 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)); - dev_dbg(dev, "device is%sbehind an iommu\n", - iommu ? " " : " not "); + /* + * 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"); arch_setup_dma_ops(dev, dma_start, size, coherent); - if (!iommu) + if (iommu_ret) of_dma_set_restricted_buffer(dev, np); return 0; diff --git a/include/linux/of_iommu.h b/include/linux/of_iommu.h index 9a5e6b410dd2fb..e61cbbe12dac6f 100644 --- a/include/linux/of_iommu.h +++ b/include/linux/of_iommu.h @@ -8,20 +8,19 @@ struct iommu_ops; #ifdef CONFIG_OF_IOMMU -extern const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np, - const u32 *id); +extern int of_iommu_configure(struct device *dev, struct device_node *master_np, + const u32 *id); extern void of_iommu_get_resv_regions(struct device *dev, struct list_head *list); #else -static inline const struct iommu_ops *of_iommu_configure(struct device *dev, - struct device_node *master_np, - const u32 *id) +static inline int of_iommu_configure(struct device *dev, + struct device_node *master_np, + const u32 *id) { - return NULL; + return -ENODEV; } static inline void of_iommu_get_resv_regions(struct device *dev,
Nothing needs this pointer. Return a normal error code with the usual IOMMU semantic that ENODEV means 'there is no IOMMU driver'. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/of_iommu.c | 29 ++++++++++++++++++----------- drivers/of/device.c | 22 +++++++++++++++------- include/linux/of_iommu.h | 13 ++++++------- 3 files changed, 39 insertions(+), 25 deletions(-)