Re: [PATCH 03/10] iommu/of: Use -ENODEV consistently in of_iommu_configure()

[Moritz Fischer <moritzf@xxxxxxxxxx>]

On Tue, Nov 28, 2023 at 08:47:59PM -0400, 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.

> Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
> Tested-by: Hector Martin <marcan@xxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
>   drivers/iommu/of_iommu.c | 49 ++++++++++++----------------------------
>   1 file changed, 15 insertions(+), 34 deletions(-)

> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c6510d7e7b241b..164317bfb8a81f 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -17,8 +17,6 @@
>   #include <linux/slab.h>
>   #include <linux/fsl/mc.h>

> -#define NO_IOMMU	1
> -
>   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",
> @@ -117,9 +115,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;
> -	int err = NO_IOMMU;
> +	int err;

>   	if (!master_np)
>   		return -ENODEV;
> @@ -153,37 +150,21 @@ int of_iommu_configure(struct device *dev, struct  
> device_node *master_np,
>   	} else {
>   		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;
> -	}
>   	mutex_unlock(&iommu_probe_device_lock);

> -	/*
> -	 * 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);
> -
> -	/* Ignore all other errors apart from EPROBE_DEFER */
> -	if (err < 0) {
> -		if (err == -EPROBE_DEFER)
> -			return err;
> -		dev_dbg(dev, "Adding to IOMMU failed: %pe\n", ERR_PTR(err));
> +	if (err == -ENODEV || err == -EPROBE_DEFER)
>   		return err;
> -	}
> -	if (!ops)
> -		return -ENODEV;
> +	if (err)
> +		goto err_log;
> +
> +	err = iommu_probe_device(dev);
> +	if (err)
> +		goto err_log;
>   	return 0;
> +
> +err_log:
> +	dev_dbg(dev, "Adding to IOMMU failed: %pe\n", ERR_PTR(err));
> +	return err;
>   }

>   static enum iommu_resv_type __maybe_unused
> --
> 2.42.0


Reviewed-by: Moritz Fischer <moritzf@xxxxxxxxxx>