Re: [PATCH 2/4] of: unittest: Use __free(device_node)

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Wed, 17 Jan 2024 13:47:43 -0600
Rob Herring <robh@kernel.org> 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@kernel.org> wrote:
> >   
> > > On Sun, Jan 14, 2024 at 10:54 AM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > A simple example of the utility of this autocleanup approach to
> > > > handling of_node_put()
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >  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
>