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@xxxxxxxxxx>
> ---
> 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
>
