| Message ID | 20240114165358.119916-3-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | of: Automate handling of of_node_put() | expand |
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? Rob [1] https://lore.kernel.org/all/289c4af00bcc46e83555dacbc76f56477126d645.camel@pengutronix.de
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. 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
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 >
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. > 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. OTOH, defining a name which has no type defined anywhere in the user function isn't great for readability either. 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? > 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. :) > 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? Rob
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 >
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; int count = 0, rc; for_each_child_of_node(np, child) { if (child->parent != np) { pr_err("Child node %pOFn links to wrong parent %pOFn\n", child, np); - rc = -EINVAL; - goto put_child; + return -EINVAL; } rc = of_unittest_check_node_linkage(child); if (rc < 0) - goto put_child; + return rc; count += rc; } return count + 1; -put_child: - of_node_put(child); - return rc; } static void __init of_unittest_check_tree_linkage(void)