On Mon, 29 Jan 2024, Jonathan Cameron wrote: > On Mon, 29 Jan 2024 15:02:19 +0100 (CET) > Julia Lawall <julia.lawall@xxxxxxxx> wrote: > > > On Mon, 29 Jan 2024, Jonathan Cameron wrote: > > > > > On Sun, 28 Jan 2024 19:06:53 +0100 (CET) > > > Julia Lawall <julia.lawall@xxxxxxxx> wrote: > > > > > > > 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. > > > > > > Absolutely. I think I spotted 2 bugs whilst just looking for places this pattern > > > doesn't apply. Will circle back to those once this discussion is resolved. > > > I think I've taken dozen's of fixes for cases where these were missed over the years > > > so hoping this means I'll never see another one! > > > > > > > > > > > I guess that > > > > > > > > for_each_child_of_node_scoped(parent, struct device_node *, child) > > > > > > > > would seem too verbose? > > > > > > Intent just to make the allocated internal type clear? Sure we can do that if > > > it helps with making it clear something is being allocated. > > > I can't think of a way this could be used with anything other than > > > a struct device_node * as the second parameter but I guess it still helps > > > 'hint' at what is going on.. > > To touch back on this, I'm still not sure what your intent was in suggesting > having the explicit struct device_node * Well, I wanted struct device_node * child, but then there wouldn't be the macro argument that would give the variable name. So perhaps even with the comma it would look a little more like a declaration. It does seem problematic to have eg two freestanding occurrences of child that are not in the same scope, if there is a child in the iterator argument list and a use elsewhere in the function. > > > > > > > > > > > > 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. > > > > > > The non loop cases should be handled via the __free(device_node) as provided > > > by patch 1. We'll need to keep the declaration local and initial assignment > > > together but that is easy enough to do and similar to the many other cleanup.h > > > usecases that are surfacing. > > > > I tried the following semantic patch: > > > > @@ > > identifier x,f; > > attribute name __free; > > expression list es; > > expression e; > > statement S; > > @@ > > > > { > > ... when != S > > struct device_node *x > > + __free(device_node) > > ; > > ... when strict > > x = f(es); > > <... when any > > when != x = e > > -of_node_put(x); > > ...> > > -of_node_put(x); > > } > > > Nice. > > It is written defensively in various ways: > > > > when != S means tha tthe device_node declaration has t be at the top of > > the block, perhaps with other declarations before it. > > > > when strict means that all paths must lead from the declaration to the > > initialization. So there is no need to intiialize the variable to NULL, > > as far as I understand. > > > > when != x = e means that the declared variable is not reinitialized, which > > would require keeping the previous of_node_put. > > > > There is an of_node_put at the end of the block, so the use of __free > > doesn't change the lifetime. > > > > An unfortunate aspect of the last constraint is that some functions may > > declare multiple device_node variables, and only one of the of_not_puts > > can come at the very end of the block. This can be solved by just running > > the semantic patch again. > > > > An alternative would be to move the initialization up to the declaration, > > but the result was often a bit ugly, due to the various initialization > > function calls having long names and argument lists. > > You have to do that in order to ensure there is no window for someone to > easily insert code that leaves them uninitialized. > > Linus had some rather specific comments on that being the only right way to > do it. OK, the semantic patch can be changed to do that. It would have to be a bit more complex or a bit more defensive, to be sure that no variables are used between the declaration and the current call site. > > > > > The output is below. I have looked quickly through all of the changes and > > they all look reasonable, but have not tried compiling anything (which I > > guess wouldn't currently work anyway). > > > > julia > > > > I picked a couple from the end. They show the sort of things that > would want cleaning up. The script is great for finding low hanging > fruit but as you've identified there is often some stuff that isn't > easy to automate. > > > diff -u -p a/drivers/clk/clk-nomadik.c b/drivers/clk/clk-nomadik.c > > --- a/drivers/clk/clk-nomadik.c > > +++ b/drivers/clk/clk-nomadik.c > > @@ -87,7 +87,7 @@ static const struct of_device_id nomadik > > > > static void __init nomadik_src_init(void) > > { > > - struct device_node *np; > > + struct device_node *np __free(device_node); > > u32 val; > > > > np = of_find_matching_node(NULL, nomadik_src_match); > > @@ -134,7 +134,6 @@ static void __init nomadik_src_init(void > > register_reboot_notifier(&nomadik_clk_reboot_notifier); > > > > out_put: > Can avoid the label given nothing to do any more. Indeed. > > - of_node_put(np); > > } > > > > /** > > diff -u -p a/arch/powerpc/platforms/chrp/setup.c b/arch/powerpc/platforms/chrp/setup.c > > --- a/arch/powerpc/platforms/chrp/setup.c > > +++ b/arch/powerpc/platforms/chrp/setup.c > > @@ -99,7 +99,7 @@ static void chrp_show_cpuinfo(struct seq > > { > > int i, sdramen; > > unsigned int t; > > - struct device_node *root; > > + struct device_node *root __free(device_node); > > Same as next 2. > > > const char *model = ""; > > > > root = of_find_node_by_path("/"); > > @@ -152,7 +152,6 @@ static void chrp_show_cpuinfo(struct seq > > gg2_cachetypes[(t>>2) & 3], > > gg2_cachemodes[t & 3]); > > } > > - of_node_put(root); > > } > > > > /* > > @@ -195,7 +194,7 @@ static void __init sio_fixup_irq(const c > > > > static void __init sio_init(void) > > { > > - struct device_node *root; > > + struct device_node *root __free(device_node); > > As below needs to be > struct device_node *root __free(device_node) = of_find_node_by_path("/"); > so there is no space for code to be added inbetween that might return before > this is set. OK > > const char *model; > > > > root = of_find_node_by_path("/"); > > @@ -209,8 +208,6 @@ static void __init sio_init(void) > > /* select logical device 1 (KBC/Mouse) */ > > sio_fixup_irq("mouse", 1, 12, 2); > > } > > - > > - of_node_put(root); > > } > > > > > > @@ -364,7 +361,7 @@ static void chrp_8259_cascade(struct irq > > */ > > static void __init chrp_find_openpic(void) > > { > > - struct device_node *np, *root; > > + struct device_node *np __free(device_node), *root; > > This one looks dangerous because of the chance of other code > getting added between here and the point where it is set. > > Better to move that down so we have > > struct device_node *np __free(device_node) = of_find_node_by_type(NULL, "open-pic"); > > Also there is a nicer use in: > https://elixir.bootlin.com/linux/v6.8-rc2/source/arch/powerpc/platforms/chrp/setup.c#L217 So the point is that we now allow declarations at random places, and not only at the top of a function? > > > int len, i, j; > > int isu_size; > > const unsigned int *iranges, *opprop = NULL; > > @@ -438,7 +435,6 @@ static void __init chrp_find_openpic(voi > > ppc_md.get_irq = mpic_get_irq; > > bail: > > of_node_put(root); > With root covered this can return early. Yes, this could be addressed by running the rule more than once. Thanks for the feedback! julia > > > - of_node_put(np); > > } > > > >
