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

[Sudeep Holla]

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.

> -			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