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