On Wed, 17 Jan 2024 17:01:44 +0000
Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> 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.
>
> 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@xxxxxxxxxxxxxx
>
