On Wed, 17 Jan 2024 13:47:43 -0600
Rob Herring <robh@xxxxxxxxxx> wrote:
> On Wed, Jan 17, 2024 at 05:01:44PM +0000, Jonathan Cameron wrote:
> > On Tue, 16 Jan 2024 12:26:49 -0600
> > Rob Herring <robh@xxxxxxxxxx> wrote:
> >
> > > On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > >
> > > > A simple example of the utility of this autocleanup approach to
> > > > handling of_node_put()
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > > > ---
> > > > drivers/of/unittest.c | 10 +++-------
> > > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > > > index e9e90e96600e..b6d9edb831f0 100644
> > > > --- a/drivers/of/unittest.c
> > > > +++ b/drivers/of/unittest.c
> > > > @@ -233,27 +233,23 @@ static void __init of_unittest_dynamic(void)
> > > >
> > > > static int __init of_unittest_check_node_linkage(struct device_node *np)
> > > > {
> > > > - struct device_node *child;
> > > > + struct device_node *child __free(device_node) = NULL;
> > >
> > > In another thread[1], it seems that initializing to NULL is bad form
> > > according to the chief penguin. But as this is a refcounted pointer
> > > rather than an allocation, IDK?
> >
> > I'm not sure the argument applies here. My understanding is it's not
> > really about the = NULL, but more about where the __free(device_node) is.
> > The ordering of that cleanup wrt to other similar clean up is to do it
> > in reverse order of declaration and in some cases that might cause trouble.
>
> Rereading Linus' reply again, I think it is more that you see how
> something is freed right where it is allocated, and the way to do that
> is the allocation and declaration are together.
Ok. Maybe both issues surfaced in different threads. Both are valid points
I've seen made about this stuff.
>
> > Here, the only way we could ensure the allocation was done at the right
> > point and we didn't have that __free before it was set, would be to add
> > variants of for_each_child_of_node() etc that did something like
> >
> > #define for_each_child_of_node_scoped(parent, child) \
>
> Note that you don't need child here except to define the name of child.
> Otherwise, the variable name you need for the loop is implicit.
Agreed.
> OTOH,
> defining a name which has no type defined anywhere in the user function
> isn't great for readability either.
Agreed it's not great to have the type missing, but I couldn't
think of a better option. So I think if we want these toys and to not
have it as a 2 part statement that's what we get.
>
> WRT the whole renaming, it might be better to keep 'scoped' in the name
> so that it is obvious how child needs to be handled. Or is the compiler
> smart enough to catch any case of doing it wrong?
I'm not sure we have variable name shadowing detection turned on.
If that was on we should see a warning on someone not realizing there
is a local scoped version. I'm fine with keeping the name.
>
> > for (struct device_node *child __free(device_node) = \
> > of_get_next_child(parent, NULL); \
> > child != NULL; \
> > child = of_get_next_child(parent, child))
> >
> > So that the child variable doesn't exist at all outside of the scope
> > of the loop.
> >
> > I thought about proposing that style of solution but it felt more invasive
> > than a simple __free() annotation. I don't mind going that way though
> > if you prefer it.
>
> My only preference currently is to not get yelled at. :)
Always nice ;)
>
> > Alternative is just to make sure the struct device_node * is always
> > declared just before the for loop and not bother setting it to NULL
> > (which is pointless anyway - it just felt fragile to not do so!)
>
> Would the compiler know to avoid invoking __free if exiting before the
> variable is set?
If it's declared just before the loop, there wouldn't be such a path
as the init condition of the loop sets it to a valid handle or NULL.
I looked at about the first half (alphabetical order) of the 127 files
with for_each_child_of_node().
Vast majority are easily converted. I think I counted 4 possible bugs
that need a closer look and a bunch were care would be needed (typically
steal the pointer for a return).
Happy to do a small set of them and see what it looks like if you think
that is a promising direction to try?
+CC Peter Zijlstra as person most likely to have seen similar discussions
or provide input based on his own cleanup.h conversions.
Jonathan
>
> Rob
>
