On Sun, Jan 28, 2024 at 04:05:37PM +0000, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > +CC includes peopleinterested in property.h equivalents to minimize > duplication of discussion. Outcome of this discussion will affect: > https://lore.kernel.org/all/20240114172009.179893-1-jic23@xxxxxxxxxx/ > [PATCH 00/13] device property / IIO: Use cleanup.h magic for fwnode_handle_put() handling. > > In discussion of previous approach with Rob Herring we talked about various > ways to avoid a disconnect between the declaration of the __free(device_node) > and the first non NULL assignment. Making this connection clear is useful for 2 > reasons: > 1) Avoids out of order cleanup with respect to other cleanup.h usage. > 2) Avoids disconnect between how cleanup is to be done and how the reference > was acquired in the first place. > > https://lore.kernel.org/all/20240117194743.GA2888190-robh@xxxxxxxxxx/ > > The options we discussed are: > > 1) Ignore this issue and merge original set. > > 2) Always put the declaration just before the for loop and don't set it NULL. > > { > int ret; > > ret = ... and other fun code. > > struct device_node *child __free(device_node); > for_each_child_of_node(np, child) { > } > } > > This works but careful review is needed to ensure that this unusual pattern is > followed. We don't set it to NULL as the loop will do that anyway if there are > no child nodes, or the loop finishes without an early break or return. > > 3) Introduced the pointer to auto put device_node only within the > for loop scope. > > +#define for_each_child_of_node_scoped(parent, child) \ > + for (struct device_node *child __free(device_node) = \ > + of_get_next_child(parent, NULL); \ > + child != NULL; \ Just child; \ > + child = of_get_next_available_child(parent, child)) > + > > This series is presenting option 3. I only implemented this loop out of > all the similar ones and it is only compile tested. > > Disadvantage Rob raised is that it isn't obvious this macro will instantiate > a struct device_node *child. I can't see a way around that other than option 2 > above, but all suggestions welcome. Note that if a conversion leaves an > 'external' struct device_node *child variable, in many cases the compiler > will catch that as an unused variable. We don't currently run shaddow > variable detection in normal kernel builds, but that could also be used > to catch such bugs. -- With Best Regards, Andy Shevchenko
