Re: [PATCH v10 5/7] iio: add the IIO backend framework

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Feb 10, 2024 at 6:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Fri, 9 Feb 2024 18:30:53 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@analog.com> wrote:

...

> > > +struct iio_backend *devm_iio_backend_get(struct device *dev, const char *name)
> > > +{
> > > +       struct fwnode_handle *fwnode;
> > > +       struct iio_backend *back;
> > > +       unsigned int index;
> > > +       int ret;
> > > +
> > > +       if (name) {
> > > +               ret = device_property_match_string(dev, "io-backend-names",
> > > +                                                  name);
> > > +               if (ret < 0)
> > > +                       return ERR_PTR(ret);
> > > +               index = ret;
> > > +       } else {
> > > +               index = 0;
> > > +       }
> > > +
> > > +       fwnode = fwnode_find_reference(dev_fwnode(dev), "io-backends", index);
> > > +       if (IS_ERR(fwnode)) {
> > > +               dev_err_probe(dev, PTR_ERR(fwnode),
> > > +                             "Cannot get Firmware reference\n");
> > > +               return ERR_CAST(fwnode);
> > > +       }
> > > +
> > > +       guard(mutex)(&iio_back_lock);
> > > +       list_for_each_entry(back, &iio_back_list, entry) {
> > > +               if (!device_match_fwnode(back->dev, fwnode))
> > > +                       continue;
> >
> > > +               fwnode_handle_put(fwnode);
> > > +               ret = __devm_iio_backend_get(dev, back);
> >
> > This order makes me think about the reference counting. So, fwnode is
> > the one of the backend devices to which the property points to.
> > Another piece is the local (to this framework) list that keeps backend
> > devices. So, fwnode reference can be  dropped earlier, while the usual
> > pattern to interleave gets and puts in a chain. Dunno if above needs a
> > comment, reordering or nothing.
> >
> I'm lost. Why don't we need to hold fwnode reference for the
> device_match_fwnode() just before here?

> Or do you mean that we are safe here with the fwnode_handle_put() being
> before the __devm_iio_backend_get()?

This one.

> I think you are correct that the
> lifetimes are fine as we switched from the fwnode to the
> iio_backend from the list at this point.
>
> > > +               if (ret)
> > > +                       return ERR_PTR(ret);
> > > +
> > > +               return back;
> > > +       }
> > > +
> > > +       fwnode_handle_put(fwnode);
> > > +       return ERR_PTR(-EPROBE_DEFER);
> >
> > While thinking about the above, I noticed the room to refactor.
> >
> >   list_for_each_entry(...) {
> >     if (...)
> >       break;
> >   }
> >   fwnode_handle_put(...);
> >   // Yes, we may use the below macro as the (global) pointers are
> > protected by a mutex.
> >   if (list_entry_is_head(...))
>
> Knowing that means we failed to match is a bit obscure.
>
> >     return ERR_PTR(...);
> >
> >   ret = __devm_iio_backend_get(...);
> >   ...
>
> Maybe - it's a little ugly either way.  I don't think we care about
> potentially holding the fwnode handle too long, so flipping over to
> the cleanup.h handling (I need to get back to that sometime this week)
> will make this all simpler.

Yes, I agree with your point of view. That's why I'm not insisting on
this change.

> > > +}


-- 
With Best Regards,
Andy Shevchenko