Re: [PATCH RFC 02/17] of: Do not return struct iommu_ops from of_iommu_configure()

[Jason Gunthorpe <jgg@nvidia.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