Re: [PATCH v2 12/17] iommu: Make iommu_ops_from_fwnode() static
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 00/17] Solve iommu probe races around iommu_fwspec
Did patch 12 v2 get sent? I'm not seeing it locally, nor in lore, and b4 doesn't find it when pulling then thread. Regards, Jerry ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 04/17] acpi: Do not return struct iommu_ops from acpi_iommu_configure_id()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v2 03/17] iommu/of: Use -ENODEV consistently in of_iommu_configure()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 17/17] iommu: Mark dev_iommu_priv_set() with a lockdep
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 16/17] iommu: Mark dev_iommu_get() with lockdep
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 15/17] iommu: Add ops->of_xlate_fwspec()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 14/17] iommu: Remove pointless iommu_fwspec_free()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 11/17] iommu: Hold iommu_probe_device_lock while calling ops->of_xlate
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 10/17] acpi: Do not use dev->iommu within acpi_iommu_configure()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 09/17] iommu: Add iommu_fwspec_append_ids()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 08/17] of: Do not use dev->iommu within of_iommu_configure()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 07/17] iommu: Add iommu_probe_device_fwspec()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 06/17] iommu: Add iommu_fwspec_alloc/dealloc()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 05/17] iommu: Make iommu_fwspec->ids a distinct allocation
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 13/17] iommu: Remove dev_iommu_fwspec_set()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 12/17] iommu: Make iommu_ops_from_fwnode() static
On Fri, Nov 03, 2023 at 01:44:57PM -0300, Jason Gunthorpe wrote:
...
> @@ -1044,11 +1043,6 @@ static inline int iommu_fwspec_add_ids(struct device
> *dev, u32 *ids,
> }
>
> static inline
^ was missed in the deletion below
> -const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
> -{
> - return NULL;
> -}
> -
> static inline int
> iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
> {
> --
> 2.42.0
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 04/17] acpi: Do not return struct iommu_ops from acpi_iommu_configure_id()
On Sun, Nov 05, 2023 at 09:24:09AM -0400, Jason Gunthorpe wrote:
> On Fri, Nov 03, 2023 at 05:48:01PM -0700, Jerry Snitselaar wrote:
> > > @@ -1632,10 +1633,15 @@ int acpi_dma_configure_id(struct device *dev,
> > > enum dev_dma_attr attr,
> > >
> > > acpi_arch_dma_setup(dev);
> > >
> > > - iommu = acpi_iommu_configure_id(dev, input_id);
> > > - if (PTR_ERR(iommu) == -EPROBE_DEFER)
> > > + ret = acpi_iommu_configure_id(dev, input_id);
> > > + if (ret == -EPROBE_DEFER)
> > > return -EPROBE_DEFER;
> > >
> > return ret; ?
>
> Maybe? Like this seemed to be a pattern in this code so I left it
Yeah, it is fine. I think it just caught my eye, because of this earlier
bit in the patch:
if (err == -EPROBE_DEFER) {
- return ERR_PTR(err);
+ return err;
which needed to get rid of the ERR_PTR.
Regards,
Jerry
>
> > > + /*
> > > + * Historically this routine doesn't fail driver probing due to errors
> > > + * in acpi_iommu_configure()
> >
> > acpi_iommu_configure_id()
>
> Thanks
>
> Jason
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 04/17] acpi: Do not return struct iommu_ops from acpi_iommu_configure_id()
On Fri, Nov 03, 2023 at 01:44:49PM -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
> ---
> drivers/acpi/scan.c | 24 +++-
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
...
> #else /* !CONFIG_IOMMU_API */
> @@ -1623,7 +1624,7 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
> int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
> const u32 *input_id)
> {
> - const struct iommu_ops *iommu;
> + int ret;
>
> if (attr == DEV_DMA_NOT_SUPPORTED) {
> set_dma_ops(dev, &dma_dummy_ops);
> @@ -1632,10 +1633,15 @@ int acpi_dma_configure_id(struct device *dev, enum
> dev_dma_attr attr,
>
> acpi_arch_dma_setup(dev);
>
> - iommu = acpi_iommu_configure_id(dev, input_id);
> - if (PTR_ERR(iommu) == -EPROBE_DEFER)
> + ret = acpi_iommu_configure_id(dev, input_id);
> + if (ret == -EPROBE_DEFER)
> return -EPROBE_DEFER;
>
return ret; ?
> + /*
> + * Historically this routine doesn't fail driver probing due to errors
> + * in acpi_iommu_configure()
acpi_iommu_configure_id()
> + */
> +
> arch_setup_dma_ops(dev, 0, U64_MAX, attr == DEV_DMA_COHERENT);
>
> return 0;
> --
> 2.42.0
>
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 03/17] of: Use -ENODEV consistently in of_iommu_configure()
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 > --- > 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 > #include > > -#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); ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 02/17] of: Do not return struct iommu_ops from of_iommu_configure()
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 > > --- > > 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH RFC 02/17] of: Do not return struct iommu_ops from of_iommu_configure()
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
> ---
> 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,1
Re: [PATCH RFC 01/17] iommu: Remove struct iommu_ops *iommu from arch_setup_dma_ops()
Reviewed-by: Jerry Snitselaar ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
