Add a cleanup function attribute '__free(device_node)' to the device node
pointer initialization statement and remove the pairing cleanup function
call of 'of_node_put' at the end of the function.
The '_free()' attrubute is introduced by scope-based resource management
in-kernel framework implemented in 'cleanup.h'. A pointer marked with
'__free()' attribute makes a compiler insert a cleanup function call
to the places where the pointer goes out of the scope. This feature
allows to get rid of manual cleanup function calls.
Suggested-by: Julia.Lawall <[email protected]>
Signed-off-by: Roman Storozhenko <[email protected]>
---
This patch targets the next tree:
tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
tag: next-20240411
---
drivers/tty/sysrq.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 02217e3c916b..1d1261f618c0 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
static void sysrq_of_get_keyreset_config(void)
{
u32 key;
- struct device_node *np;
struct property *prop;
const __be32 *p;
- np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
+ struct device_node *np __free(device_node) =
+ of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
+
if (!np) {
pr_debug("No sysrq node found");
return;
@@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
/* Get reset timeout if any. */
of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
-
- of_node_put(np);
}
#else
static void sysrq_of_get_keyreset_config(void)
--
2.34.1
This change allows us to put this pointer under automatic scope
management and get rid of node_put. Besides, if a new code path is
introduced we won't need to add a new of_node_put.
Thanks,
Roman
On Thu, Apr 11, 2024 at 8:11 PM Greg KH <[email protected]> wrote:
>
> On Thu, Apr 11, 2024 at 08:02:56PM +0200, Roman Storozhenko wrote:
> > Add a cleanup function attribute '__free(device_node)' to the device node
> > pointer initialization statement and remove the pairing cleanup function
> > call of 'of_node_put' at the end of the function.
> > The '_free()' attrubute is introduced by scope-based resource management
> > in-kernel framework implemented in 'cleanup.h'. A pointer marked with
> > '__free()' attribute makes a compiler insert a cleanup function call
> > to the places where the pointer goes out of the scope. This feature
> > allows to get rid of manual cleanup function calls.
> >
> > Suggested-by: Julia.Lawall <[email protected]>
> > Signed-off-by: Roman Storozhenko <[email protected]>
> > ---
> > This patch targets the next tree:
> > tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > tag: next-20240411
> > ---
> > drivers/tty/sysrq.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 02217e3c916b..1d1261f618c0 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> > static void sysrq_of_get_keyreset_config(void)
> > {
> > u32 key;
> > - struct device_node *np;
> > struct property *prop;
> > const __be32 *p;
> >
> > - np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > + struct device_node *np __free(device_node) =
> > + of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > +
> > if (!np) {
> > pr_debug("No sysrq node found");
> > return;
> > @@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
> >
> > /* Get reset timeout if any. */
> > of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> > -
> > - of_node_put(np);
> > }
> > #else
> > static void sysrq_of_get_keyreset_config(void)
>
> Also, this change really makes no sense at all, the pointer never goes
> out of scope except when the function is over, at the bottom. So why
> make this complex change at all for no benefit?
>
> In other words, properly understand the change you are making and only
> make it if it actually makes sense. It does not make any sense here,
> right?
>
> thanks,
>
> greg k-h
--
Kind regards,
Roman Storozhenko
On Thu, 11 Apr 2024, Greg KH wrote:
> On Thu, Apr 11, 2024 at 08:02:56PM +0200, Roman Storozhenko wrote:
> > Add a cleanup function attribute '__free(device_node)' to the device node
> > pointer initialization statement and remove the pairing cleanup function
> > call of 'of_node_put' at the end of the function.
> > The '_free()' attrubute is introduced by scope-based resource management
> > in-kernel framework implemented in 'cleanup.h'. A pointer marked with
> > '__free()' attribute makes a compiler insert a cleanup function call
> > to the places where the pointer goes out of the scope. This feature
> > allows to get rid of manual cleanup function calls.
> >
> > Suggested-by: Julia.Lawall <[email protected]>
> > Signed-off-by: Roman Storozhenko <[email protected]>
> > ---
> > This patch targets the next tree:
> > tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > tag: next-20240411
> > ---
> > drivers/tty/sysrq.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 02217e3c916b..1d1261f618c0 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> > static void sysrq_of_get_keyreset_config(void)
> > {
> > u32 key;
> > - struct device_node *np;
> > struct property *prop;
> > const __be32 *p;
> >
> > - np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > + struct device_node *np __free(device_node) =
> > + of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > +
> > if (!np) {
> > pr_debug("No sysrq node found");
> > return;
> > @@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
> >
> > /* Get reset timeout if any. */
> > of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> > -
> > - of_node_put(np);
> > }
> > #else
> > static void sysrq_of_get_keyreset_config(void)
>
> Also, this change really makes no sense at all, the pointer never goes
> out of scope except when the function is over, at the bottom. So why
> make this complex change at all for no benefit?
>
> In other words, properly understand the change you are making and only
> make it if it actually makes sense. It does not make any sense here,
> right?
Maybe it would be nice to get rid of of_node_puts in the general case?
Even though this one is not very annoying, there are some other functions
where there are many of_node_puts, and convoluted error handling code to
incorporate them on both the success and failure path. So maybe it would
be better to avoid the situation of having them sometimes and not having
them other times? But this is an opinion, and if the general consensus is
to only get rid of the cases that currently add complexity, then that is
possible too.
julia
On Thu, Apr 11, 2024 at 08:02:56PM +0200, Roman Storozhenko wrote:
> Add a cleanup function attribute '__free(device_node)' to the device node
> pointer initialization statement and remove the pairing cleanup function
> call of 'of_node_put' at the end of the function.
> The '_free()' attrubute is introduced by scope-based resource management
> in-kernel framework implemented in 'cleanup.h'. A pointer marked with
> '__free()' attribute makes a compiler insert a cleanup function call
> to the places where the pointer goes out of the scope. This feature
> allows to get rid of manual cleanup function calls.
>
> Suggested-by: Julia.Lawall <[email protected]>
> Signed-off-by: Roman Storozhenko <[email protected]>
> ---
> This patch targets the next tree:
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> tag: next-20240411
> ---
> drivers/tty/sysrq.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> index 02217e3c916b..1d1261f618c0 100644
> --- a/drivers/tty/sysrq.c
> +++ b/drivers/tty/sysrq.c
> @@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> static void sysrq_of_get_keyreset_config(void)
> {
> u32 key;
> - struct device_node *np;
> struct property *prop;
> const __be32 *p;
>
> - np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> + struct device_node *np __free(device_node) =
> + of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> +
> if (!np) {
> pr_debug("No sysrq node found");
> return;
> @@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
>
> /* Get reset timeout if any. */
> of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> -
> - of_node_put(np);
> }
> #else
> static void sysrq_of_get_keyreset_config(void)
Also, this change really makes no sense at all, the pointer never goes
out of scope except when the function is over, at the bottom. So why
make this complex change at all for no benefit?
In other words, properly understand the change you are making and only
make it if it actually makes sense. It does not make any sense here,
right?
thanks,
greg k-h
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Thu, Apr 11, 2024 at 08:28:17PM +0200, Roman Storozhenko wrote:
> This change allows us to put this pointer under automatic scope
> management and get rid of node_put. Besides, if a new code path is
> introduced we won't need to add a new of_node_put.
We worry about future stuff then, in the future. So no need for
changing code today for things that are not present at all, otherwise
you would never be finished with anything, right?
Don't make things more complex when it is not needed. Only add
complexity when it is needed.
thanks,
greg k-h
On Thu, Apr 11, 2024 at 08:17:07PM +0200, Julia Lawall wrote:
>
>
> On Thu, 11 Apr 2024, Greg KH wrote:
>
> > On Thu, Apr 11, 2024 at 08:02:56PM +0200, Roman Storozhenko wrote:
> > > Add a cleanup function attribute '__free(device_node)' to the device node
> > > pointer initialization statement and remove the pairing cleanup function
> > > call of 'of_node_put' at the end of the function.
> > > The '_free()' attrubute is introduced by scope-based resource management
> > > in-kernel framework implemented in 'cleanup.h'. A pointer marked with
> > > '__free()' attribute makes a compiler insert a cleanup function call
> > > to the places where the pointer goes out of the scope. This feature
> > > allows to get rid of manual cleanup function calls.
> > >
> > > Suggested-by: Julia.Lawall <[email protected]>
> > > Signed-off-by: Roman Storozhenko <[email protected]>
> > > ---
> > > This patch targets the next tree:
> > > tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > > tag: next-20240411
> > > ---
> > > drivers/tty/sysrq.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > > index 02217e3c916b..1d1261f618c0 100644
> > > --- a/drivers/tty/sysrq.c
> > > +++ b/drivers/tty/sysrq.c
> > > @@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> > > static void sysrq_of_get_keyreset_config(void)
> > > {
> > > u32 key;
> > > - struct device_node *np;
> > > struct property *prop;
> > > const __be32 *p;
> > >
> > > - np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > > + struct device_node *np __free(device_node) =
> > > + of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > > +
> > > if (!np) {
> > > pr_debug("No sysrq node found");
> > > return;
> > > @@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
> > >
> > > /* Get reset timeout if any. */
> > > of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> > > -
> > > - of_node_put(np);
> > > }
> > > #else
> > > static void sysrq_of_get_keyreset_config(void)
> >
> > Also, this change really makes no sense at all, the pointer never goes
> > out of scope except when the function is over, at the bottom. So why
> > make this complex change at all for no benefit?
> >
> > In other words, properly understand the change you are making and only
> > make it if it actually makes sense. It does not make any sense here,
> > right?
>
> Maybe it would be nice to get rid of of_node_puts in the general case?
That's a call for the of maintainer to make, and then if so, to do it
across the whole tree, right?
> Even though this one is not very annoying, there are some other functions
> where there are many of_node_puts, and convoluted error handling code to
> incorporate them on both the success and failure path. So maybe it would
> be better to avoid the situation of having them sometimes and not having
> them other times? But this is an opinion, and if the general consensus is
> to only get rid of the cases that currently add complexity, then that is
> possible too.
Let's keep things simple until it has to be complex please.
thanks,
greg k-h
[Adding the OF maintainers and device tree mailing list]
> > Maybe it would be nice to get rid of of_node_puts in the general case?
>
> That's a call for the of maintainer to make, and then if so, to do it
> across the whole tree, right?
Sure.
Rob and Saravana, what do you think about the following:
* Is it ok to use __free(device_tree) directly in a declaration, or is
there some macro that should be used instead?
* If is ok to use __free(device_tree) even in simple cases where a
variable is just declared at the start of the scope and freed at the end,
and there is no internediate error handling code?
Please see for example:
https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2401291455430.8649@hadrien/
But I don't think we can do the whole thing at once, in one patch. There
are a lot of things that need to be checked, and it don't break anything
to do them one at a time.
>
> > Even though this one is not very annoying, there are some other functions
> > where there are many of_node_puts, and convoluted error handling code to
> > incorporate them on both the success and failure path. So maybe it would
> > be better to avoid the situation of having them sometimes and not having
> > them other times? But this is an opinion, and if the general consensus is
> > to only get rid of the cases that currently add complexity, then that is
> > possible too.
>
> Let's keep things simple until it has to be complex please.
I meant that the current code is complex and error prone, and using __free
eliminates code that is ugly and has led to memory leaks in the past (and
a lot of effort to find and fix them in the past). Even if some instances
don't have that property, they may become more complex in the future, if
some error condition is detected.
julia
On Fri, 12 Apr 2024, Greg KH wrote:
> On Thu, Apr 11, 2024 at 08:17:07PM +0200, Julia Lawall wrote:
> >
> >
> > On Thu, 11 Apr 2024, Greg KH wrote:
> >
> > > On Thu, Apr 11, 2024 at 08:02:56PM +0200, Roman Storozhenko wrote:
> > > > Add a cleanup function attribute '__free(device_node)' to the device node
> > > > pointer initialization statement and remove the pairing cleanup function
> > > > call of 'of_node_put' at the end of the function.
> > > > The '_free()' attrubute is introduced by scope-based resource management
> > > > in-kernel framework implemented in 'cleanup.h'. A pointer marked with
> > > > '__free()' attribute makes a compiler insert a cleanup function call
> > > > to the places where the pointer goes out of the scope. This feature
> > > > allows to get rid of manual cleanup function calls.
> > > >
> > > > Suggested-by: Julia.Lawall <[email protected]>
> > > > Signed-off-by: Roman Storozhenko <[email protected]>
> > > > ---
> > > > This patch targets the next tree:
> > > > tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > > > tag: next-20240411
> > > > ---
> > > > drivers/tty/sysrq.c | 7 +++----
> > > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > > > index 02217e3c916b..1d1261f618c0 100644
> > > > --- a/drivers/tty/sysrq.c
> > > > +++ b/drivers/tty/sysrq.c
> > > > @@ -758,11 +758,12 @@ static void sysrq_detect_reset_sequence(struct sysrq_state *state,
> > > > static void sysrq_of_get_keyreset_config(void)
> > > > {
> > > > u32 key;
> > > > - struct device_node *np;
> > > > struct property *prop;
> > > > const __be32 *p;
> > > >
> > > > - np = of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > > > + struct device_node *np __free(device_node) =
> > > > + of_find_node_by_path("/chosen/linux,sysrq-reset-seq");
> > > > +
> > > > if (!np) {
> > > > pr_debug("No sysrq node found");
> > > > return;
> > > > @@ -781,8 +782,6 @@ static void sysrq_of_get_keyreset_config(void)
> > > >
> > > > /* Get reset timeout if any. */
> > > > of_property_read_u32(np, "timeout-ms", &sysrq_reset_downtime_ms);
> > > > -
> > > > - of_node_put(np);
> > > > }
> > > > #else
> > > > static void sysrq_of_get_keyreset_config(void)
> > >
> > > Also, this change really makes no sense at all, the pointer never goes
> > > out of scope except when the function is over, at the bottom. So why
> > > make this complex change at all for no benefit?
> > >
> > > In other words, properly understand the change you are making and only
> > > make it if it actually makes sense. It does not make any sense here,
> > > right?
> >
> > Maybe it would be nice to get rid of of_node_puts in the general case?
>
> That's a call for the of maintainer to make, and then if so, to do it
> across the whole tree, right?
>
> > Even though this one is not very annoying, there are some other functions
> > where there are many of_node_puts, and convoluted error handling code to
> > incorporate them on both the success and failure path. So maybe it would
> > be better to avoid the situation of having them sometimes and not having
> > them other times? But this is an opinion, and if the general consensus is
> > to only get rid of the cases that currently add complexity, then that is
> > possible too.
>
> Let's keep things simple until it has to be complex please.
Jonathan Cameron pointed me to the following series from Rob Herring:
https://lore.kernel.org/linux-devicetree/[email protected]/
The patch for of_node_put is:
https://lore.kernel.org/linux-devicetree/[email protected]/
It uses __free directy. The cases in the file drivers/of/property.c have
quite simple structure, with for each get just one put at the end of the
scope in most cases.
julia
>
> thanks,
>
> greg k-h
>