| Message ID | 20240415161409.8375-1-jain.abhinav177@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | pstore/ram: Replace of_node_put with __free() for automatic cleanup | expand |
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 >
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
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/
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; }
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(-)