[PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup

[PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup

[Abhinav Jain]

Add __free(device_node) to the parent_node struct declaration.
Move declaration to initialization for ensuring scope sanity.
Remove of_node_put from parent_node struct.

Suggested-by: Julia Lawall <julia.lawall@inria.fr>
Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
---
 fs/pstore/ram.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b1a455f42e93..14f2f4864e48 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -644,7 +644,6 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 			    struct ramoops_platform_data *pdata)
 {
 	struct device_node *of_node = pdev->dev.of_node;
-	struct device_node *parent_node;
 	struct resource *res;
 	u32 value;
 	int ret;
@@ -704,14 +703,13 @@ static int ramoops_parse_dt(struct platform_device *pdev,
 	 * we're not a child of "reserved-memory" and mimicking the
 	 * expected behavior.
 	 */
-	parent_node = of_get_parent(of_node);
+	struct device_node *parent_node __free(device_node) = of_node_parent(of_node);
 	if (!of_node_name_eq(parent_node, "reserved-memory") &&
 	    !pdata->console_size && !pdata->ftrace_size &&
 	    !pdata->pmsg_size && !pdata->ecc_info.ecc_size) {
 		pdata->console_size = pdata->record_size;
 		pdata->pmsg_size = pdata->record_size;
 	}
-	of_node_put(parent_node);
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup

[Kees Cook]

On Mon, Apr 15, 2024 at 04:14:09PM +0000, Abhinav Jain wrote:
> Add __free(device_node) to the parent_node struct declaration.
> Move declaration to initialization for ensuring scope sanity.
> Remove of_node_put from parent_node struct.
> 
> Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
> ---
>  fs/pstore/ram.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index b1a455f42e93..14f2f4864e48 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -644,7 +644,6 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  			    struct ramoops_platform_data *pdata)
>  {
>  	struct device_node *of_node = pdev->dev.of_node;
> -	struct device_node *parent_node;
>  	struct resource *res;
>  	u32 value;
>  	int ret;
> @@ -704,14 +703,13 @@ static int ramoops_parse_dt(struct platform_device *pdev,
>  	 * we're not a child of "reserved-memory" and mimicking the
>  	 * expected behavior.
>  	 */
> -	parent_node = of_get_parent(of_node);
> +	struct device_node *parent_node __free(device_node) = of_node_parent(of_node);

Please don't move variable definitions into the middle of the function
body. :)

>  	if (!of_node_name_eq(parent_node, "reserved-memory") &&
>  	    !pdata->console_size && !pdata->ftrace_size &&
>  	    !pdata->pmsg_size && !pdata->ecc_info.ecc_size) {
>  		pdata->console_size = pdata->record_size;
>  		pdata->pmsg_size = pdata->record_size;
>  	}
> -	of_node_put(parent_node);

So this change is functionally fine, but there's really no good reason
to do this -- there is no fancy error handling here, so there's no
benefit to making this change. It doesn't really help readability.

-Kees

>  
>  	return 0;
>  }
> -- 
> 2.34.1
> 

-- 
Kees Cook

Re: [PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup

[Julia Lawall]

On Wed, 24 Apr 2024, Kees Cook wrote:

> On Mon, Apr 15, 2024 at 04:14:09PM +0000, Abhinav Jain wrote:
> > Add __free(device_node) to the parent_node struct declaration.
> > Move declaration to initialization for ensuring scope sanity.
> > Remove of_node_put from parent_node struct.
> >
> > Suggested-by: Julia Lawall <julia.lawall@inria.fr>
> > Signed-off-by: Abhinav Jain <jain.abhinav177@gmail.com>
> > ---
> >  fs/pstore/ram.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> > index b1a455f42e93..14f2f4864e48 100644
> > --- a/fs/pstore/ram.c
> > +++ b/fs/pstore/ram.c
> > @@ -644,7 +644,6 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  			    struct ramoops_platform_data *pdata)
> >  {
> >  	struct device_node *of_node = pdev->dev.of_node;
> > -	struct device_node *parent_node;
> >  	struct resource *res;
> >  	u32 value;
> >  	int ret;
> > @@ -704,14 +703,13 @@ static int ramoops_parse_dt(struct platform_device *pdev,
> >  	 * we're not a child of "reserved-memory" and mimicking the
> >  	 * expected behavior.
> >  	 */
> > -	parent_node = of_get_parent(of_node);
> > +	struct device_node *parent_node __free(device_node) = of_node_parent(of_node);
>
> Please don't move variable definitions into the middle of the function
> body. :)

This is done in other cases where it makes more sense to put the
initialization later in the function.  The point is that the variable has
to be initialized, and puttng the declaration lower, which is now allowed
when needed, is better than first uselessly initializing the variable to
NULL.

>
> >  	if (!of_node_name_eq(parent_node, "reserved-memory") &&
> >  	    !pdata->console_size && !pdata->ftrace_size &&
> >  	    !pdata->pmsg_size && !pdata->ecc_info.ecc_size) {
> >  		pdata->console_size = pdata->record_size;
> >  		pdata->pmsg_size = pdata->record_size;
> >  	}
> > -	of_node_put(parent_node);
>
> So this change is functionally fine, but there's really no good reason
> to do this -- there is no fancy error handling here, so there's no
> benefit to making this change. It doesn't really help readability.

The benefit is general consistency across the code base.  If we could just
get rid of all local-scoped of_node_puts, then we could more easily check
that device nodes are safely used, without having to study the rest of the
code.

Just my opinion.  You decide.

julia

Re: [PATCH] pstore/ram: Replace of_node_put with __free() for automatic cleanup

[Abhinav Jain]

On Wed, 24 Apr 2024, Kees Cook wrote:
> Please don't move variable definitions into the middle of the function
> body. :)

I have moved the parent_node declaration back to the top of the function body.

Patch v2: 
https://lore.kernel.org/all/20240605214944.22113-1-jain.abhinav177@gmail.com/