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

[Jason Gunthorpe]

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 <j...@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