Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()

[Vincenzo Mezzela]

On 24/04/24 12:37, Sudeep Holla wrote:
> On Mon, Apr 22, 2024 at 03:09:31PM +0200, Vincenzo Mezzela wrote:
> > Introduce the __free attribute for scope-based resource management.
> > Resources allocated with __free are automatically released at the end of
> > the scope. This enhancement aims to mitigate memory management issues
> > associated with forgetting to release resources by utilizing __free
> > instead of of_node_put().
> >
> > The declaration of the device_node used within the do-while loops is
> > moved directly within the loop so that the resource is automatically
> > freed at the end of each iteration.
> >
> > Suggested-by: Julia Lawall <julia.lawall@xxxxxxxx>
> > Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@xxxxxxxxx>
> > ---
> > changes in v2:
> >      - check loop exit condition within the loop
> >      - add cleanup.h header
> >
> >   drivers/base/arch_topology.c | 150 +++++++++++++++++------------------
> >   1 file changed, 73 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> > index 024b78a0cfc1..c9c4af55953e 100644
> > --- a/drivers/base/arch_topology.c
> > +++ b/drivers/base/arch_topology.c
> > @@ -20,6 +20,7 @@
> >   #include <linux/rcupdate.h>
> >   #include <linux/sched.h>
> >   #include <linux/units.h>
> > +#include <linux/cleanup.h>
> Keep it alphabetical. Also since <linux/of.h> does define kfree for
> of_node_get(), may not be needed strictly. Sorry for not noticing those
> details earlier. I am fine either way, it is good to keep it IMO.
>
> >   #define CREATE_TRACE_POINTS
> >   #include <trace/events/thermal_pressure.h>
> > @@ -513,10 +514,10 @@ core_initcall(free_raw_capacity);
> >    */
> >   static int __init get_cpu_for_node(struct device_node *node)
> >   {
> > -	struct device_node *cpu_node;
> >   	int cpu;
> >
> > -	cpu_node = of_parse_phandle(node, "cpu", 0);
> > +	struct device_node *cpu_node __free(device_node) =
> > +		of_parse_phandle(node, "cpu", 0);
> >   	if (!cpu_node)
> >   		return -1;
> >
> > @@ -527,7 +528,6 @@ static int __init get_cpu_for_node(struct device_node *node)
> >   		pr_info("CPU node for %pOF exist but the possible cpu range is :%*pbl\n",
> >   			cpu_node, cpumask_pr_args(cpu_possible_mask));
> >
> > -	of_node_put(cpu_node);
> >   	return cpu;
> >   }
> >
> > @@ -538,28 +538,27 @@ static int __init parse_core(struct device_node *core, int package_id,
> >   	bool leaf = true;
> >   	int i = 0;
> >   	int cpu;
> > -	struct device_node *t;
> >
> > -	do {
> > +	for(;;) {
> Did you run checkpatch.pl on this ? It should have complained here and 3 other
> places below.
It does indeed, I'll fix this.
> > -			if (leaf) {
> > -				ret = parse_core(c, package_id, cluster_id,
> > -						 core_id++);
> > -			} else {
> > -				pr_err("%pOF: Non-leaf cluster with core %s\n",
> > -				       cluster, name);
> > -				ret = -EINVAL;
> > -			}
> > +		has_cores = true;
> >
> > -			of_node_put(c);
> > -			if (ret != 0)
> > -				return ret;
> > +		if (depth == 0) {
> > +			pr_err("%pOF: cpu-map children should be clusters\n", c);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (leaf) {
> > +			ret = parse_core(c, package_id, cluster_id, core_id++);
> > +		} else {
> > +			pr_err("%pOF: Non-leaf cluster with core %s\n",
> > +					cluster, name);
> Missing alignment here.
>
> --
> Regards,
> Sudeep

I'll fix the misalignment and the checkpatch.pl warnings and send an 
updated version.

Furthermore, would you like to see this patch split in two patches where:

- patch 1 reorganizes the content of the loop using "if(!t) break;" 
instead of having the "if(t) { all for body }";

- patch 2 gets rid of of_node_put;

This might be better than having both the reorganizations in the same patch.

Please let me know what would you prefer.

Thanks,

Vincenzo