On Sun, 28 Jan 2024, 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; \ > + 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. > > All comments welcome. It looks promising to get rid of a lot of clunky and error-prone error-handling code. I guess that for_each_child_of_node_scoped(parent, struct device_node *, child) would seem too verbose? There are a lot of opportunities for device_node loops, but also for some more obscure loops over other types. And there are a lot of of_node_puts that could be eliminated independent of loops. julia > > Jonathan Cameron (5): > of: Add cleanup.h based auto release via __free(device_node) markings. > of: Introduce for_each_child_of_node_scoped() to automate > of_node_put() handling > of: unittest: Use __free(device_node) > iio: adc: fsl-imx25-gcq: Use for_each_child_node_scoped() > iio: adc: rcar-gyroadc: use for_each_child_node_scoped() > > drivers/iio/adc/fsl-imx25-gcq.c | 13 +++---------- > drivers/iio/adc/rcar-gyroadc.c | 21 ++++++--------------- > drivers/of/unittest.c | 11 +++-------- > include/linux/of.h | 8 ++++++++ > 4 files changed, 20 insertions(+), 33 deletions(-) > > -- > 2.43.0 > >
