On Sat, Feb 10, 2024 at 6:42 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Fri, 9 Feb 2024 18:30:53 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Fri, Feb 9, 2024 at 5:26 PM Nuno Sa <nuno.sa@xxxxxxxxxx> 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
