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

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

On Wed, 17 Jan 2024 17:01:44 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> 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.
> 
> 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) \
> 	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.

Note that if we did this I'd expect us to convert all current use case
and then we can probably do a global name change back to
for_each_child_of_node() as I can't see why we'd retain the other variant.
The rare (I think) case of breaking out of the loop whilst holding the
handle can be covered by pointer stealing anyway.

Jonathan

> 
> 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!)
> 
> Jonathan
> 
> > 
> > Rob
> > 
> > [1] https://lore.kernel.org/all/289c4af00bcc46e83555dacbc76f56477126d645.camel@pengutronix.de  
>