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

[Jonathan Cameron <jic23@kernel.org>]

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()? 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.

> 
> > +}  
>