[v1] drivers: use __free attribute instead of of_node_put()

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

Posted by Vincenzo Mezzela 4 weeks, 1 day ago

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@inria.fr>
Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
 drivers/base/arch_topology.c | 41 ++++++++++++++----------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 024b78a0cfc1..58eeb8183747 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -513,10 +513,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 +527,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,11 +537,11 @@ 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 {
 		snprintf(name, sizeof(name), "thread%d", i);
-		t = of_get_child_by_name(core, name);
+		struct device_node *t __free(device_node) =
+			of_get_child_by_name(core, name);
 		if (t) {
 			leaf = false;
 			cpu = get_cpu_for_node(t);
@@ -553,10 +552,8 @@ static int __init parse_core(struct device_node *core, int package_id,
 				cpu_topology[cpu].thread_id = i;
 			} else if (cpu != -ENODEV) {
 				pr_err("%pOF: Can't get CPU for thread\n", t);
-				of_node_put(t);
 				return -EINVAL;
 			}
-			of_node_put(t);
 		}
 		i++;
 	} while (t);
@@ -586,7 +583,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	char name[20];
 	bool leaf = true;
 	bool has_cores = false;
-	struct device_node *c;
 	int core_id = 0;
 	int i, ret;
 
@@ -598,13 +594,13 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "cluster%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
 		if (c) {
 			leaf = false;
 			ret = parse_cluster(c, package_id, i, depth + 1);
 			if (depth > 0)
 				pr_warn("Topology for clusters of clusters not yet supported\n");
-			of_node_put(c);
 			if (ret != 0)
 				return ret;
 		}
@@ -615,14 +611,14 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "core%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
 		if (c) {
 			has_cores = true;
 
 			if (depth == 0) {
 				pr_err("%pOF: cpu-map children should be clusters\n",
 				       c);
-				of_node_put(c);
 				return -EINVAL;
 			}
 
@@ -635,7 +631,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 				ret = -EINVAL;
 			}
 
-			of_node_put(c);
 			if (ret != 0)
 				return ret;
 		}
@@ -651,17 +646,16 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 static int __init parse_socket(struct device_node *socket)
 {
 	char name[20];
-	struct device_node *c;
 	bool has_socket = false;
 	int package_id = 0, ret;
 
 	do {
 		snprintf(name, sizeof(name), "socket%d", package_id);
-		c = of_get_child_by_name(socket, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(socket, name);
 		if (c) {
 			has_socket = true;
 			ret = parse_cluster(c, package_id, -1, 0);
-			of_node_put(c);
 			if (ret != 0)
 				return ret;
 		}
@@ -676,11 +670,11 @@ static int __init parse_socket(struct device_node *socket)
 
 static int __init parse_dt_topology(void)
 {
-	struct device_node *cn, *map;
 	int ret = 0;
 	int cpu;
 
-	cn = of_find_node_by_path("/cpus");
+	struct device_node *cn __free(device_node) =
+		of_find_node_by_path("/cpus");
 	if (!cn) {
 		pr_err("No CPU information found in DT\n");
 		return 0;
@@ -690,13 +684,14 @@ static int __init parse_dt_topology(void)
 	 * When topology is provided cpu-map is essentially a root
 	 * cluster with restricted subnodes.
 	 */
-	map = of_get_child_by_name(cn, "cpu-map");
+	struct device_node *map __free(devide_node) =
+		of_get_child_by_name(cn, "cpu-map");
 	if (!map)
-		goto out;
+		return ret;
 
 	ret = parse_socket(map);
 	if (ret != 0)
-		goto out_map;
+		return ret;
 
 	topology_normalize_cpu_scale();
 
@@ -710,10 +705,6 @@ static int __init parse_dt_topology(void)
 			break;
 		}
 
-out_map:
-	of_node_put(map);
-out:
-	of_node_put(cn);
 	return ret;
 }
 #endif
-- 
2.34.1

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

Posted by Sudeep Holla 4 weeks, 1 day ago

On Fri, Apr 19, 2024 at 03:19:56PM +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@inria.fr>
> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
> ---
>  drivers/base/arch_topology.c | 41 ++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 024b78a0cfc1..58eeb8183747 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -513,10 +513,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) =

Missing include <linux/cleanup.h> for this ?

> +		of_parse_phandle(node, "cpu", 0);
>  	if (!cpu_node)
>  		return -1;
>
> @@ -527,7 +527,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,11 +537,11 @@ 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 {
>  		snprintf(name, sizeof(name), "thread%d", i);
> -		t = of_get_child_by_name(core, name);
> +		struct device_node *t __free(device_node) =
> +			of_get_child_by_name(core, name);
>  		if (t) {
>  			leaf = false;
>  			cpu = get_cpu_for_node(t);
> @@ -553,10 +552,8 @@ static int __init parse_core(struct device_node *core, int package_id,
>  				cpu_topology[cpu].thread_id = i;
>  			} else if (cpu != -ENODEV) {
>  				pr_err("%pOF: Can't get CPU for thread\n", t);
> -				of_node_put(t);
>  				return -EINVAL;
>  			}
> -			of_node_put(t);

OK you moved 't' inside the loop and this must be taken care, but...

>  		}
>  		i++;
>  	} while (t);

....now, will it even compile if 't' is not in scope ? I think you might get
compilation here. If not, I still don't understand what is the value of
't' being checked there.

> @@ -586,7 +583,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  	char name[20];
>  	bool leaf = true;
>  	bool has_cores = false;
> -	struct device_node *c;
>  	int core_id = 0;
>  	int i, ret;
>
> @@ -598,13 +594,13 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  	i = 0;
>  	do {
>  		snprintf(name, sizeof(name), "cluster%d", i);
> -		c = of_get_child_by_name(cluster, name);
> +		struct device_node *c __free(device_node) =
> +			of_get_child_by_name(cluster, name);
>  		if (c) {
>  			leaf = false;
>  			ret = parse_cluster(c, package_id, i, depth + 1);
>  			if (depth > 0)
>  				pr_warn("Topology for clusters of clusters not yet supported\n");
> -			of_node_put(c);
>  			if (ret != 0)
>  				return ret;
>  		}
> @@ -615,14 +611,14 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  	i = 0;
>  	do {
>  		snprintf(name, sizeof(name), "core%d", i);
> -		c = of_get_child_by_name(cluster, name);
> +		struct device_node *c __free(device_node) =
> +			of_get_child_by_name(cluster, name);
>  		if (c) {
>  			has_cores = true;
>
>  			if (depth == 0) {
>  				pr_err("%pOF: cpu-map children should be clusters\n",
>  				       c);
> -				of_node_put(c);
>  				return -EINVAL;
>  			}
>
> @@ -635,7 +631,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  				ret = -EINVAL;
>  			}
>
> -			of_node_put(c);
>  			if (ret != 0)
>  				return ret;
>  		}
> @@ -651,17 +646,16 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  static int __init parse_socket(struct device_node *socket)
>  {
>  	char name[20];
> -	struct device_node *c;
>  	bool has_socket = false;
>  	int package_id = 0, ret;
>
>  	do {
>  		snprintf(name, sizeof(name), "socket%d", package_id);
> -		c = of_get_child_by_name(socket, name);
> +		struct device_node *c __free(device_node) =
> +			of_get_child_by_name(socket, name);
>  		if (c) {
>  			has_socket = true;
>  			ret = parse_cluster(c, package_id, -1, 0);
> -			of_node_put(c);
>  			if (ret != 0)
>  				return ret;
>  		}

Same thing applies to these while(c) loop. I don't understand how this
could work even if it is compiling fine which I doubt.

> @@ -676,11 +670,11 @@ static int __init parse_socket(struct device_node *socket)
>
>  static int __init parse_dt_topology(void)
>  {
> -	struct device_node *cn, *map;
>  	int ret = 0;
>  	int cpu;
>
> -	cn = of_find_node_by_path("/cpus");
> +	struct device_node *cn __free(device_node) =
> +		of_find_node_by_path("/cpus");
>  	if (!cn) {
>  		pr_err("No CPU information found in DT\n");
>  		return 0;
> @@ -690,13 +684,14 @@ static int __init parse_dt_topology(void)
>  	 * When topology is provided cpu-map is essentially a root
>  	 * cluster with restricted subnodes.
>  	 */
> -	map = of_get_child_by_name(cn, "cpu-map");
> +	struct device_node *map __free(devide_node) =

If not above ones, this must fail to compile. Perhaps s/devide_node/device_node/ ?
I now doubt if this patch is compile tested ?

--
Regards,
Sudeep

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

Posted by Vincenzo Mezzela 3 weeks, 5 days ago

On 19/04/24 16:01, Sudeep Holla wrote:
> On Fri, Apr 19, 2024 at 03:19:56PM +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@inria.fr>
>> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
>> ---
>>   drivers/base/arch_topology.c | 41 ++++++++++++++----------------------
>>   1 file changed, 16 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index 024b78a0cfc1..58eeb8183747 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -513,10 +513,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) =
> Missing include <linux/cleanup.h> for this ?
>
>> +		of_parse_phandle(node, "cpu", 0);
>>   	if (!cpu_node)
>>   		return -1;
>>
>> @@ -527,7 +527,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,11 +537,11 @@ 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 {
>>   		snprintf(name, sizeof(name), "thread%d", i);
>> -		t = of_get_child_by_name(core, name);
>> +		struct device_node *t __free(device_node) =
>> +			of_get_child_by_name(core, name);
>>   		if (t) {
>>   			leaf = false;
>>   			cpu = get_cpu_for_node(t);
>> @@ -553,10 +552,8 @@ static int __init parse_core(struct device_node *core, int package_id,
>>   				cpu_topology[cpu].thread_id = i;
>>   			} else if (cpu != -ENODEV) {
>>   				pr_err("%pOF: Can't get CPU for thread\n", t);
>> -				of_node_put(t);
>>   				return -EINVAL;
>>   			}
>> -			of_node_put(t);
> OK you moved 't' inside the loop and this must be taken care, but...
>
>>   		}
>>   		i++;
>>   	} while (t);
> ....now, will it even compile if 't' is not in scope ? I think you might get
> compilation here. If not, I still don't understand what is the value of
> 't' being checked there.
>
>> @@ -586,7 +583,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>>   	char name[20];
>>   	bool leaf = true;
>>   	bool has_cores = false;
>> -	struct device_node *c;
>>   	int core_id = 0;
>>   	int i, ret;
>>
>> @@ -598,13 +594,13 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>>   	i = 0;
>>   	do {
>>   		snprintf(name, sizeof(name), "cluster%d", i);
>> -		c = of_get_child_by_name(cluster, name);
>> +		struct device_node *c __free(device_node) =
>> +			of_get_child_by_name(cluster, name);
>>   		if (c) {
>>   			leaf = false;
>>   			ret = parse_cluster(c, package_id, i, depth + 1);
>>   			if (depth > 0)
>>   				pr_warn("Topology for clusters of clusters not yet supported\n");
>> -			of_node_put(c);
>>   			if (ret != 0)
>>   				return ret;
>>   		}
>> @@ -615,14 +611,14 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>>   	i = 0;
>>   	do {
>>   		snprintf(name, sizeof(name), "core%d", i);
>> -		c = of_get_child_by_name(cluster, name);
>> +		struct device_node *c __free(device_node) =
>> +			of_get_child_by_name(cluster, name);
>>   		if (c) {
>>   			has_cores = true;
>>
>>   			if (depth == 0) {
>>   				pr_err("%pOF: cpu-map children should be clusters\n",
>>   				       c);
>> -				of_node_put(c);
>>   				return -EINVAL;
>>   			}
>>
>> @@ -635,7 +631,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>>   				ret = -EINVAL;
>>   			}
>>
>> -			of_node_put(c);
>>   			if (ret != 0)
>>   				return ret;
>>   		}
>> @@ -651,17 +646,16 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>>   static int __init parse_socket(struct device_node *socket)
>>   {
>>   	char name[20];
>> -	struct device_node *c;
>>   	bool has_socket = false;
>>   	int package_id = 0, ret;
>>
>>   	do {
>>   		snprintf(name, sizeof(name), "socket%d", package_id);
>> -		c = of_get_child_by_name(socket, name);
>> +		struct device_node *c __free(device_node) =
>> +			of_get_child_by_name(socket, name);
>>   		if (c) {
>>   			has_socket = true;
>>   			ret = parse_cluster(c, package_id, -1, 0);
>> -			of_node_put(c);
>>   			if (ret != 0)
>>   				return ret;
>>   		}
> Same thing applies to these while(c) loop. I don't understand how this
> could work even if it is compiling fine which I doubt.
>
>> @@ -676,11 +670,11 @@ static int __init parse_socket(struct device_node *socket)
>>
>>   static int __init parse_dt_topology(void)
>>   {
>> -	struct device_node *cn, *map;
>>   	int ret = 0;
>>   	int cpu;
>>
>> -	cn = of_find_node_by_path("/cpus");
>> +	struct device_node *cn __free(device_node) =
>> +		of_find_node_by_path("/cpus");
>>   	if (!cn) {
>>   		pr_err("No CPU information found in DT\n");
>>   		return 0;
>> @@ -690,13 +684,14 @@ static int __init parse_dt_topology(void)
>>   	 * When topology is provided cpu-map is essentially a root
>>   	 * cluster with restricted subnodes.
>>   	 */
>> -	map = of_get_child_by_name(cn, "cpu-map");
>> +	struct device_node *map __free(devide_node) =
> If not above ones, this must fail to compile. Perhaps s/devide_node/device_node/ ?
> I now doubt if this patch is compile tested ?
>
> --
> Regards,
> Sudeep

Hi,

As you rightly pointed out, I inadvertently omitted to compile this file 
during the kernel build process. Consequently, certain errors remained 
undetected. I apologize for the oversight.

I'll send an updated version of this patch soon.

Regards,

Vincenzo

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

Posted by Vincenzo Mezzela 3 weeks, 5 days ago

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@inria.fr>
Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
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>
 
 #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(;;) {
 		snprintf(name, sizeof(name), "thread%d", i);
-		t = of_get_child_by_name(core, name);
-		if (t) {
-			leaf = false;
-			cpu = get_cpu_for_node(t);
-			if (cpu >= 0) {
-				cpu_topology[cpu].package_id = package_id;
-				cpu_topology[cpu].cluster_id = cluster_id;
-				cpu_topology[cpu].core_id = core_id;
-				cpu_topology[cpu].thread_id = i;
-			} else if (cpu != -ENODEV) {
-				pr_err("%pOF: Can't get CPU for thread\n", t);
-				of_node_put(t);
-				return -EINVAL;
-			}
-			of_node_put(t);
+		struct device_node *t __free(device_node) =
+			of_get_child_by_name(core, name);
+		if (!t)
+			break;
+
+		leaf = false;
+		cpu = get_cpu_for_node(t);
+		if (cpu >= 0) {
+			cpu_topology[cpu].package_id = package_id;
+			cpu_topology[cpu].cluster_id = cluster_id;
+			cpu_topology[cpu].core_id = core_id;
+			cpu_topology[cpu].thread_id = i;
+		} else if (cpu != -ENODEV) {
+			pr_err("%pOF: Can't get CPU for thread\n", t);
+			return -EINVAL;
 		}
 		i++;
-	} while (t);
+	}
 
 	cpu = get_cpu_for_node(core);
 	if (cpu >= 0) {
@@ -586,7 +585,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	char name[20];
 	bool leaf = true;
 	bool has_cores = false;
-	struct device_node *c;
 	int core_id = 0;
 	int i, ret;
 
@@ -596,51 +594,51 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	 * scheduler with a flat list of them.
 	 */
 	i = 0;
-	do {
+	for(;;) {
 		snprintf(name, sizeof(name), "cluster%d", i);
-		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			leaf = false;
-			ret = parse_cluster(c, package_id, i, depth + 1);
-			if (depth > 0)
-				pr_warn("Topology for clusters of clusters not yet supported\n");
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
+		if (!c)
+			break;
+
+		leaf = false;
+		ret = parse_cluster(c, package_id, i, depth + 1);
+		if (depth > 0)
+			pr_warn("Topology for clusters of clusters not yet supported\n");
+		if (ret != 0)
+			return ret;
 		i++;
-	} while (c);
+	}
 
 	/* Now check for cores */
 	i = 0;
-	do {
+	for(;;) {
 		snprintf(name, sizeof(name), "core%d", i);
-		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			has_cores = true;
-
-			if (depth == 0) {
-				pr_err("%pOF: cpu-map children should be clusters\n",
-				       c);
-				of_node_put(c);
-				return -EINVAL;
-			}
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
+		if (!c)
+			break;
 
-			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);
+			ret = -EINVAL;
 		}
+
+		if (ret != 0)
+			return ret;
+
 		i++;
-	} while (c);
+	}
 
 	if (leaf && !has_cores)
 		pr_warn("%pOF: empty cluster\n", cluster);
@@ -651,22 +649,23 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 static int __init parse_socket(struct device_node *socket)
 {
 	char name[20];
-	struct device_node *c;
 	bool has_socket = false;
 	int package_id = 0, ret;
 
-	do {
+	for(;;) {
 		snprintf(name, sizeof(name), "socket%d", package_id);
-		c = of_get_child_by_name(socket, name);
-		if (c) {
-			has_socket = true;
-			ret = parse_cluster(c, package_id, -1, 0);
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(socket, name);
+		if (!c)
+			break;
+
+		has_socket = true;
+		ret = parse_cluster(c, package_id, -1, 0);
+		if (ret != 0)
+			return ret;
+
 		package_id++;
-	} while (c);
+	}
 
 	if (!has_socket)
 		ret = parse_cluster(socket, 0, -1, 0);
@@ -676,11 +675,11 @@ static int __init parse_socket(struct device_node *socket)
 
 static int __init parse_dt_topology(void)
 {
-	struct device_node *cn, *map;
 	int ret = 0;
 	int cpu;
 
-	cn = of_find_node_by_path("/cpus");
+	struct device_node *cn __free(device_node) =
+		of_find_node_by_path("/cpus");
 	if (!cn) {
 		pr_err("No CPU information found in DT\n");
 		return 0;
@@ -690,13 +689,14 @@ static int __init parse_dt_topology(void)
 	 * When topology is provided cpu-map is essentially a root
 	 * cluster with restricted subnodes.
 	 */
-	map = of_get_child_by_name(cn, "cpu-map");
+	struct device_node *map __free(device_node) =
+		of_get_child_by_name(cn, "cpu-map");
 	if (!map)
-		goto out;
+		return ret;
 
 	ret = parse_socket(map);
 	if (ret != 0)
-		goto out_map;
+		return ret;
 
 	topology_normalize_cpu_scale();
 
@@ -710,10 +710,6 @@ static int __init parse_dt_topology(void)
 			break;
 		}
 
-out_map:
-	of_node_put(map);
-out:
-	of_node_put(cn);
 	return ret;
 }
 #endif
-- 
2.34.1

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

Posted by Sudeep Holla 3 weeks, 3 days ago

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@inria.fr>
> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
> ---
> 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

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

Posted by Vincenzo Mezzela 3 weeks, 3 days ago

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@inria.fr>
>> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
>> ---
>> 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

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

Posted by Sudeep Holla 3 weeks, 3 days ago

On Wed, Apr 24, 2024 at 02:42:16PM +0200, Vincenzo Mezzela wrote:
>
> 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.

I am fine either way. Splitting might help in the review for others.

--
Regards,
Sudeep

[PATCH 0/2 v3] drivers: introduce automatic cleanup feature

Posted by Vincenzo Mezzela 2 weeks, 3 days ago

This patch series introduces the automatic cleanup feature using the __free
attribute. With this modification, resources allocated with __free are 
automatically released at the end of the scope.

In some cases, modifying the structure of loops is necessary to utilize the
__free attribute effectively. For example:

```
struct device_node *t;

do {
    t = of_get_child_by_name(..);
    if (t) {
        
        // some code here

        of_node_put(t);
    }
    i++;

} while (t);

//       ^
//       |
//  device_node here
```

To use the __free attribute here, we need to move the declaration of the device_node
within the loop, otherwise the automatic cleanup is called only at the end of the
function, and not at end of each iteration of the loop, being it scope-based. 

However, moving the declaration of the device_node within the loop, we can no
longer check the exit condition in the loop statement, being it outside
the loop's scope.

Therefore, this work is split into two patches. The first patch moves the exit
condition of the loop directly within the loop's scope with an explicit break
statement:

```
struct device_node *t;

do {
    t = of_get_child_by_name(..);
    if (!t)
        break;

    // some code here

    of_node_put(t);
    i++;

} while (1);
```
The second patch eliminates all of_node_put() calls, introducing the __free 
attribute to the device_node.


changes in v2:
    - check loop exit condition within the loop
    - add cleanup.h header

changes in v3:
    - split patch in two
    - fix misalignment
    - fix checkpatch warnings
    - replace break with return statement where possible


Vincenzo Mezzela (2):
  drivers: reorganize do-while loops
  drivers: use __free attribute instead of of_node_put()

 drivers/base/arch_topology.c | 140 +++++++++++++++++------------------
 1 file changed, 67 insertions(+), 73 deletions(-)

-- 
2.34.1

[PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature

Posted by Vincenzo Mezzela 5 days, 16 hours ago

This patch series introduces the automatic cleanup feature using the __free
attribute. With this modification, resources allocated with __free are 
automatically released at the end of the scope.

In some cases, modifying the structure of loops is necessary to utilize the
__free attribute effectively. For example:

```
struct device_node *t;

do {
    t = of_get_child_by_name(..);
    if (t) {
        
        // some code here

        of_node_put(t);
    }
    i++;

} while (t);

//       ^
//       |
//  device_node here
```

To use the __free attribute here, we need to move the declaration of the device_node
within the loop, otherwise the automatic cleanup is called only at the end of the
function, and not at end of each iteration of the loop, being it scope-based. 

However, moving the declaration of the device_node within the loop, we can no
longer check the exit condition in the loop statement, being it outside
the loop's scope.

Therefore, this work is split into two patches. The first patch moves the exit
condition of the loop directly within the loop's scope with an explicit break
statement:

```
struct device_node *t;

do {
    t = of_get_child_by_name(..);
    if (!t)
        break;

    // some code here

    of_node_put(t);
    i++;

} while (1);
```
The second patch eliminates all of_node_put() calls, introducing the __free 
attribute to the device_node.


changes in v2:
    - check loop exit condition within the loop
    - add cleanup.h header

changes in v3:
    - split patch in two
    - fix misalignment
    - fix checkpatch warnings
    - replace break with return statement where possible

changes in v4:
    - fix commit subject
    - fix coding style 

Vincenzo Mezzela (2):
  drivers: arch_topology: Refactor do-while loops
  drivers: arch_topology: use __free attribute instead of of_node_put()

 drivers/base/arch_topology.c | 145 +++++++++++++++++------------------
 1 file changed, 72 insertions(+), 73 deletions(-)

-- 
2.34.1

Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature

Posted by Sudeep Holla 5 days, 14 hours ago

On Mon, May 13, 2024 at 10:13:02AM +0200, Vincenzo Mezzela wrote:
> This patch series introduces the automatic cleanup feature using the __free
> attribute. With this modification, resources allocated with __free are
> automatically released at the end of the scope.
>
> In some cases, modifying the structure of loops is necessary to utilize the
> __free attribute effectively. For example:
>
> ```
> struct device_node *t;
>
> do {
>     t = of_get_child_by_name(..);
>     if (t) {
>
>         // some code here
>
>         of_node_put(t);
>     }
>     i++;
>
> } while (t);
>
> //       ^
> //       |
> //  device_node here
> ```
>
> To use the __free attribute here, we need to move the declaration of the device_node
> within the loop, otherwise the automatic cleanup is called only at the end of the
> function, and not at end of each iteration of the loop, being it scope-based.
>
> However, moving the declaration of the device_node within the loop, we can no
> longer check the exit condition in the loop statement, being it outside
> the loop's scope.
>
> Therefore, this work is split into two patches. The first patch moves the exit
> condition of the loop directly within the loop's scope with an explicit break
> statement:
>
> ```
> struct device_node *t;
>
> do {
>     t = of_get_child_by_name(..);
>     if (!t)
>         break;
>
>     // some code here
>
>     of_node_put(t);
>     i++;
>
> } while (1);
> ```
> The second patch eliminates all of_node_put() calls, introducing the __free
> attribute to the device_node.
>
>
> changes in v2:
>     - check loop exit condition within the loop
>     - add cleanup.h header
>
> changes in v3:
>     - split patch in two
>     - fix misalignment
>     - fix checkpatch warnings
>     - replace break with return statement where possible
>
> changes in v4:
>     - fix commit subject
>     - fix coding style
>

Looks good now to me.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

It is merge window now, so there is a chance that it may get lost. You
may have to post it again at -rc1. Greg can then pick it up for v6.11

--
Regards,
Sudeep

Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature

Posted by Vincenzo Mezzela 4 days, 17 hours ago

On 13/05/24 12:02, Sudeep Holla wrote:
> On Mon, May 13, 2024 at 10:13:02AM +0200, Vincenzo Mezzela wrote:
>> This patch series introduces the automatic cleanup feature using the __free
>> attribute. With this modification, resources allocated with __free are
>> automatically released at the end of the scope.
>>
>> In some cases, modifying the structure of loops is necessary to utilize the
>> __free attribute effectively. For example:
>>
>> ```
>> struct device_node *t;
>>
>> do {
>>      t = of_get_child_by_name(..);
>>      if (t) {
>>
>>          // some code here
>>
>>          of_node_put(t);
>>      }
>>      i++;
>>
>> } while (t);
>>
>> //       ^
>> //       |
>> //  device_node here
>> ```
>>
>> To use the __free attribute here, we need to move the declaration of the device_node
>> within the loop, otherwise the automatic cleanup is called only at the end of the
>> function, and not at end of each iteration of the loop, being it scope-based.
>>
>> However, moving the declaration of the device_node within the loop, we can no
>> longer check the exit condition in the loop statement, being it outside
>> the loop's scope.
>>
>> Therefore, this work is split into two patches. The first patch moves the exit
>> condition of the loop directly within the loop's scope with an explicit break
>> statement:
>>
>> ```
>> struct device_node *t;
>>
>> do {
>>      t = of_get_child_by_name(..);
>>      if (!t)
>>          break;
>>
>>      // some code here
>>
>>      of_node_put(t);
>>      i++;
>>
>> } while (1);
>> ```
>> The second patch eliminates all of_node_put() calls, introducing the __free
>> attribute to the device_node.
>>
>>
>> changes in v2:
>>      - check loop exit condition within the loop
>>      - add cleanup.h header
>>
>> changes in v3:
>>      - split patch in two
>>      - fix misalignment
>>      - fix checkpatch warnings
>>      - replace break with return statement where possible
>>
>> changes in v4:
>>      - fix commit subject
>>      - fix coding style
>>
> Looks good now to me.
>
> Acked-by: Sudeep Holla <sudeep.holla@arm.com>
>
> It is merge window now, so there is a chance that it may get lost. You
> may have to post it again at -rc1. Greg can then pick it up for v6.11
>
> --
> Regards,
> Sudeep

Ok, perfect.

Thanks!


Vincenzo

[PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops

Posted by Vincenzo Mezzela 5 days, 16 hours ago

Refactor do-while loops to move break condition within the loop's scope.
This modification is in preparation to move the declaration of the
device_node directly within the loop and take advantage of the automatic
cleanup feature provided by the __free(device_node) attribute.

Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
 drivers/base/arch_topology.c | 107 ++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 52 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 024b78a0cfc1..0115011b7a99 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -543,23 +543,24 @@ static int __init parse_core(struct device_node *core, int package_id,
 	do {
 		snprintf(name, sizeof(name), "thread%d", i);
 		t = of_get_child_by_name(core, name);
-		if (t) {
-			leaf = false;
-			cpu = get_cpu_for_node(t);
-			if (cpu >= 0) {
-				cpu_topology[cpu].package_id = package_id;
-				cpu_topology[cpu].cluster_id = cluster_id;
-				cpu_topology[cpu].core_id = core_id;
-				cpu_topology[cpu].thread_id = i;
-			} else if (cpu != -ENODEV) {
-				pr_err("%pOF: Can't get CPU for thread\n", t);
-				of_node_put(t);
-				return -EINVAL;
-			}
+		if (!t)
+			break;
+
+		leaf = false;
+		cpu = get_cpu_for_node(t);
+		if (cpu >= 0) {
+			cpu_topology[cpu].package_id = package_id;
+			cpu_topology[cpu].cluster_id = cluster_id;
+			cpu_topology[cpu].core_id = core_id;
+			cpu_topology[cpu].thread_id = i;
+		} else if (cpu != -ENODEV) {
+			pr_err("%pOF: Can't get CPU for thread\n", t);
 			of_node_put(t);
+			return -EINVAL;
 		}
+		of_node_put(t);
 		i++;
-	} while (t);
+	} while (1);
 
 	cpu = get_cpu_for_node(core);
 	if (cpu >= 0) {
@@ -599,48 +600,48 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	do {
 		snprintf(name, sizeof(name), "cluster%d", i);
 		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			leaf = false;
-			ret = parse_cluster(c, package_id, i, depth + 1);
-			if (depth > 0)
-				pr_warn("Topology for clusters of clusters not yet supported\n");
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		if (!c)
+			break;
+
+		leaf = false;
+		ret = parse_cluster(c, package_id, i, depth + 1);
+		if (depth > 0)
+			pr_warn("Topology for clusters of clusters not yet supported\n");
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
 		i++;
-	} while (c);
+	} while (1);
 
 	/* Now check for cores */
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "core%d", i);
 		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			has_cores = true;
-
-			if (depth == 0) {
-				pr_err("%pOF: cpu-map children should be clusters\n",
-				       c);
-				of_node_put(c);
-				return -EINVAL;
-			}
+		if (!c)
+			break;
 
-			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;
 
+		if (depth == 0) {
+			pr_err("%pOF: cpu-map children should be clusters\n", c);
 			of_node_put(c);
-			if (ret != 0)
-				return ret;
+			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);
+			ret = -EINVAL;
+		}
+
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
 		i++;
-	} while (c);
+	} while (1);
 
 	if (leaf && !has_cores)
 		pr_warn("%pOF: empty cluster\n", cluster);
@@ -658,15 +659,17 @@ static int __init parse_socket(struct device_node *socket)
 	do {
 		snprintf(name, sizeof(name), "socket%d", package_id);
 		c = of_get_child_by_name(socket, name);
-		if (c) {
-			has_socket = true;
-			ret = parse_cluster(c, package_id, -1, 0);
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		if (!c)
+			break;
+
+		has_socket = true;
+		ret = parse_cluster(c, package_id, -1, 0);
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
+
 		package_id++;
-	} while (c);
+	} while (1);
 
 	if (!has_socket)
 		ret = parse_cluster(socket, 0, -1, 0);
-- 
2.34.1

[PATCH 2/2 v4] drivers: arch_topology: use __free attribute instead of of_node_put()

Posted by Vincenzo Mezzela 5 days, 16 hours ago

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@inria.fr>
Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
 drivers/base/arch_topology.c | 56 +++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 0115011b7a99..93c9f0499694 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -8,6 +8,7 @@
 
 #include <linux/acpi.h>
 #include <linux/cacheinfo.h>
+#include <linux/cleanup.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/device.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;
+	struct device_node *cpu_node __free(device_node) =
+		of_parse_phandle(node, "cpu", 0);
 
-	cpu_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,11 +538,12 @@ 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 {
 		snprintf(name, sizeof(name), "thread%d", i);
-		t = of_get_child_by_name(core, name);
+		struct device_node *t __free(device_node) =
+			of_get_child_by_name(core, name);
+
 		if (!t)
 			break;
 
@@ -555,10 +556,8 @@ static int __init parse_core(struct device_node *core, int package_id,
 			cpu_topology[cpu].thread_id = i;
 		} else if (cpu != -ENODEV) {
 			pr_err("%pOF: Can't get CPU for thread\n", t);
-			of_node_put(t);
 			return -EINVAL;
 		}
-		of_node_put(t);
 		i++;
 	} while (1);
 
@@ -587,7 +586,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	char name[20];
 	bool leaf = true;
 	bool has_cores = false;
-	struct device_node *c;
 	int core_id = 0;
 	int i, ret;
 
@@ -599,7 +597,9 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "cluster%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
+
 		if (!c)
 			break;
 
@@ -607,7 +607,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 		ret = parse_cluster(c, package_id, i, depth + 1);
 		if (depth > 0)
 			pr_warn("Topology for clusters of clusters not yet supported\n");
-		of_node_put(c);
 		if (ret != 0)
 			return ret;
 		i++;
@@ -617,7 +616,9 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "core%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
+
 		if (!c)
 			break;
 
@@ -625,21 +626,19 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 
 		if (depth == 0) {
 			pr_err("%pOF: cpu-map children should be clusters\n", c);
-			of_node_put(c);
 			return -EINVAL;
 		}
 
 		if (leaf) {
 			ret = parse_core(c, package_id, cluster_id, core_id++);
+			if (ret != 0)
+				return ret;
 		} else {
 			pr_err("%pOF: Non-leaf cluster with core %s\n",
 			       cluster, name);
-			ret = -EINVAL;
+			return -EINVAL;
 		}
 
-		of_node_put(c);
-		if (ret != 0)
-			return ret;
 		i++;
 	} while (1);
 
@@ -652,19 +651,19 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 static int __init parse_socket(struct device_node *socket)
 {
 	char name[20];
-	struct device_node *c;
 	bool has_socket = false;
 	int package_id = 0, ret;
 
 	do {
 		snprintf(name, sizeof(name), "socket%d", package_id);
-		c = of_get_child_by_name(socket, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(socket, name);
+
 		if (!c)
 			break;
 
 		has_socket = true;
 		ret = parse_cluster(c, package_id, -1, 0);
-		of_node_put(c);
 		if (ret != 0)
 			return ret;
 
@@ -679,11 +678,11 @@ static int __init parse_socket(struct device_node *socket)
 
 static int __init parse_dt_topology(void)
 {
-	struct device_node *cn, *map;
 	int ret = 0;
 	int cpu;
+	struct device_node *cn __free(device_node) =
+		of_find_node_by_path("/cpus");
 
-	cn = of_find_node_by_path("/cpus");
 	if (!cn) {
 		pr_err("No CPU information found in DT\n");
 		return 0;
@@ -693,13 +692,15 @@ static int __init parse_dt_topology(void)
 	 * When topology is provided cpu-map is essentially a root
 	 * cluster with restricted subnodes.
 	 */
-	map = of_get_child_by_name(cn, "cpu-map");
+	struct device_node *map __free(device_node) =
+		of_get_child_by_name(cn, "cpu-map");
+
 	if (!map)
-		goto out;
+		return ret;
 
 	ret = parse_socket(map);
 	if (ret != 0)
-		goto out_map;
+		return ret;
 
 	topology_normalize_cpu_scale();
 
@@ -709,14 +710,9 @@ static int __init parse_dt_topology(void)
 	 */
 	for_each_possible_cpu(cpu)
 		if (cpu_topology[cpu].package_id < 0) {
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
-out_map:
-	of_node_put(map);
-out:
-	of_node_put(cn);
 	return ret;
 }
 #endif
-- 
2.34.1

Re: [PATCH 0/2 v3] drivers: introduce automatic cleanup feature

Posted by Conor Dooley 2 weeks, 3 days ago

On Wed, May 01, 2024 at 11:43:11AM +0200, Vincenzo Mezzela wrote:
> This patch series introduces the automatic cleanup feature using the __free
> attribute. With this modification, resources allocated with __free are 
> automatically released at the end of the scope.

FWIW, I did run this on a system that uses the generic topology code.
Nothing blew up, and the topology seemed to be reported correctly still. I
won't give you any hints as to how since Greg wants you to figure it out
for yourself ;)

b4 handles it fine, but usually new revisions of patchsets are not sent
as replies to earlier ones, so that might be something to change if you
resend to fix the subject lines.

Cheers,
Conor.

[PATCH 1/2 v3] drivers: reorganize do-while loops

Posted by Vincenzo Mezzela 2 weeks, 3 days ago

Test c = of_get_child_by_name() failures using "if(!c) break;" instead of
having the body of the loop all within the "if(c){ }" body.

This modification is required to move the declaration of the device_node
directly within the loop and take advantage of the automatic cleanup
feature provided by the __free(device_node) attribute.

Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
 drivers/base/arch_topology.c | 107 ++++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 52 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index 024b78a0cfc1..ea8836f0bb4b 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -543,23 +543,24 @@ static int __init parse_core(struct device_node *core, int package_id,
 	do {
 		snprintf(name, sizeof(name), "thread%d", i);
 		t = of_get_child_by_name(core, name);
-		if (t) {
-			leaf = false;
-			cpu = get_cpu_for_node(t);
-			if (cpu >= 0) {
-				cpu_topology[cpu].package_id = package_id;
-				cpu_topology[cpu].cluster_id = cluster_id;
-				cpu_topology[cpu].core_id = core_id;
-				cpu_topology[cpu].thread_id = i;
-			} else if (cpu != -ENODEV) {
-				pr_err("%pOF: Can't get CPU for thread\n", t);
-				of_node_put(t);
-				return -EINVAL;
-			}
+		if (!t)
+			break;
+
+		leaf = false;
+		cpu = get_cpu_for_node(t);
+		if (cpu >= 0) {
+			cpu_topology[cpu].package_id = package_id;
+			cpu_topology[cpu].cluster_id = cluster_id;
+			cpu_topology[cpu].core_id = core_id;
+			cpu_topology[cpu].thread_id = i;
+		} else if (cpu != -ENODEV) {
+			pr_err("%pOF: Can't get CPU for thread\n", t);
 			of_node_put(t);
+			return -EINVAL;
 		}
+		of_node_put(t);
 		i++;
-	} while (t);
+	} while (1);
 
 	cpu = get_cpu_for_node(core);
 	if (cpu >= 0) {
@@ -599,48 +600,48 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	do {
 		snprintf(name, sizeof(name), "cluster%d", i);
 		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			leaf = false;
-			ret = parse_cluster(c, package_id, i, depth + 1);
-			if (depth > 0)
-				pr_warn("Topology for clusters of clusters not yet supported\n");
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		if (!c)
+			break;
+
+		leaf = false;
+		ret = parse_cluster(c, package_id, i, depth + 1);
+		if (depth > 0)
+			pr_warn("Topology for clusters of clusters not yet supported\n");
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
 		i++;
-	} while (c);
+	} while (1);
 
 	/* Now check for cores */
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "core%d", i);
 		c = of_get_child_by_name(cluster, name);
-		if (c) {
-			has_cores = true;
-
-			if (depth == 0) {
-				pr_err("%pOF: cpu-map children should be clusters\n",
-				       c);
-				of_node_put(c);
-				return -EINVAL;
-			}
+		if (!c)
+			break;
 
-			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;
 
+		if (depth == 0) {
+			pr_err("%pOF: cpu-map children should be clusters\n", c);
 			of_node_put(c);
-			if (ret != 0)
-				return ret;
+			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);
+			ret = -EINVAL;
+		}
+
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
 		i++;
-	} while (c);
+	} while (1);
 
 	if (leaf && !has_cores)
 		pr_warn("%pOF: empty cluster\n", cluster);
@@ -658,15 +659,17 @@ static int __init parse_socket(struct device_node *socket)
 	do {
 		snprintf(name, sizeof(name), "socket%d", package_id);
 		c = of_get_child_by_name(socket, name);
-		if (c) {
-			has_socket = true;
-			ret = parse_cluster(c, package_id, -1, 0);
-			of_node_put(c);
-			if (ret != 0)
-				return ret;
-		}
+		if (!c)
+			break;
+
+		has_socket = true;
+		ret = parse_cluster(c, package_id, -1, 0);
+		of_node_put(c);
+		if (ret != 0)
+			return ret;
+
 		package_id++;
-	} while (c);
+	} while (1);
 
 	if (!has_socket)
 		ret = parse_cluster(socket, 0, -1, 0);
-- 
2.34.1

Re: [PATCH 1/2 v3] drivers: reorganize do-while loops

Posted by Sudeep Holla 2 weeks, 3 days ago

Hi,

$subject seems to be too generic. Please change it to something like
drivers: arch_topology: Refactor do-while loops

On Wed, May 01, 2024 at 11:43:12AM +0200, Vincenzo Mezzela wrote:
> Test c = of_get_child_by_name() failures using "if(!c) break;" instead of
> having the body of the loop all within the "if(c){ }" body.
>

Drop the above description which is clear from the code.

Just mention it as refactor do-while look to move the break condition
inside the loop.

> This modification is required

s/required/in preparation/

> to move the declaration of the device_node
> directly within the loop and take advantage of the automatic cleanup
> feature provided by the __free(device_node) attribute.
> 
> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
> ---
>  drivers/base/arch_topology.c | 107 ++++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index 024b78a0cfc1..ea8836f0bb4b 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c

[...]

> @@ -599,48 +600,48 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
>  	do {
>  		snprintf(name, sizeof(name), "cluster%d", i);
>  		c = of_get_child_by_name(cluster, name);
> -		if (c) {
> -			leaf = false;
> -			ret = parse_cluster(c, package_id, i, depth + 1);
> -			if (depth > 0)
> -				pr_warn("Topology for clusters of clusters not yet supported\n");
> -			of_node_put(c);
> -			if (ret != 0)
> -				return ret;
> -		}
> +		if (!c)
> +			break;
> +
> +		leaf = false;
> +		ret = parse_cluster(c, package_id, i, depth + 1);
> +		if (depth > 0)
> +			pr_warn("Topology for clusters of clusters not yet supported\n");
> +		of_node_put(c);
> +		if (ret != 0)
> +			return ret;
>  		i++;
> -	} while (c);
> +	} while (1);
>  
>  	/* Now check for cores */
>  	i = 0;
>  	do {
>  		snprintf(name, sizeof(name), "core%d", i);
>  		c = of_get_child_by_name(cluster, name);
> -		if (c) {
> -			has_cores = true;
> -
> -			if (depth == 0) {
> -				pr_err("%pOF: cpu-map children should be clusters\n",
> -				       c);
> -				of_node_put(c);
> -				return -EINVAL;
> -			}
> +		if (!c)
> +			break;
>  
> -			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;
>  
> +		if (depth == 0) {
> +			pr_err("%pOF: cpu-map children should be clusters\n", c);
>  			of_node_put(c);
> -			if (ret != 0)
> -				return ret;
> +			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);

Extra space before 'cluster' ? checkpatch must have complain if I am not
reading this correctly.

--
Regards,
Sudeep

[PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()

Posted by Vincenzo Mezzela 2 weeks, 3 days ago

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@inria.fr>
Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
---
 drivers/base/arch_topology.c | 51 +++++++++++++++---------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index ea8836f0bb4b..eef26e304018 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -8,6 +8,7 @@
 
 #include <linux/acpi.h>
 #include <linux/cacheinfo.h>
+#include <linux/cleanup.h>
 #include <linux/cpu.h>
 #include <linux/cpufreq.h>
 #include <linux/device.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,11 +538,11 @@ 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 {
 		snprintf(name, sizeof(name), "thread%d", i);
-		t = of_get_child_by_name(core, name);
+		struct device_node *t __free(device_node) =
+			of_get_child_by_name(core, name);
 		if (!t)
 			break;
 
@@ -555,10 +555,8 @@ static int __init parse_core(struct device_node *core, int package_id,
 			cpu_topology[cpu].thread_id = i;
 		} else if (cpu != -ENODEV) {
 			pr_err("%pOF: Can't get CPU for thread\n", t);
-			of_node_put(t);
 			return -EINVAL;
 		}
-		of_node_put(t);
 		i++;
 	} while (1);
 
@@ -587,7 +585,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	char name[20];
 	bool leaf = true;
 	bool has_cores = false;
-	struct device_node *c;
 	int core_id = 0;
 	int i, ret;
 
@@ -599,7 +596,8 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "cluster%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
 		if (!c)
 			break;
 
@@ -607,7 +605,6 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 		ret = parse_cluster(c, package_id, i, depth + 1);
 		if (depth > 0)
 			pr_warn("Topology for clusters of clusters not yet supported\n");
-		of_node_put(c);
 		if (ret != 0)
 			return ret;
 		i++;
@@ -617,7 +614,8 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 	i = 0;
 	do {
 		snprintf(name, sizeof(name), "core%d", i);
-		c = of_get_child_by_name(cluster, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(cluster, name);
 		if (!c)
 			break;
 
@@ -625,21 +623,19 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 
 		if (depth == 0) {
 			pr_err("%pOF: cpu-map children should be clusters\n", c);
-			of_node_put(c);
 			return -EINVAL;
 		}
 
 		if (leaf) {
 			ret = parse_core(c, package_id, cluster_id, core_id++);
+			if (ret != 0)
+				return ret;
 		} else {
 			pr_err("%pOF: Non-leaf cluster with core %s\n",
 				cluster, name);
-			ret = -EINVAL;
+			return -EINVAL;
 		}
 
-		of_node_put(c);
-		if (ret != 0)
-			return ret;
 		i++;
 	} while (1);
 
@@ -652,19 +648,18 @@ static int __init parse_cluster(struct device_node *cluster, int package_id,
 static int __init parse_socket(struct device_node *socket)
 {
 	char name[20];
-	struct device_node *c;
 	bool has_socket = false;
 	int package_id = 0, ret;
 
 	do {
 		snprintf(name, sizeof(name), "socket%d", package_id);
-		c = of_get_child_by_name(socket, name);
+		struct device_node *c __free(device_node) =
+			of_get_child_by_name(socket, name);
 		if (!c)
 			break;
 
 		has_socket = true;
 		ret = parse_cluster(c, package_id, -1, 0);
-		of_node_put(c);
 		if (ret != 0)
 			return ret;
 
@@ -679,11 +674,11 @@ static int __init parse_socket(struct device_node *socket)
 
 static int __init parse_dt_topology(void)
 {
-	struct device_node *cn, *map;
 	int ret = 0;
 	int cpu;
 
-	cn = of_find_node_by_path("/cpus");
+	struct device_node *cn __free(device_node) =
+		of_find_node_by_path("/cpus");
 	if (!cn) {
 		pr_err("No CPU information found in DT\n");
 		return 0;
@@ -693,13 +688,14 @@ static int __init parse_dt_topology(void)
 	 * When topology is provided cpu-map is essentially a root
 	 * cluster with restricted subnodes.
 	 */
-	map = of_get_child_by_name(cn, "cpu-map");
+	struct device_node *map __free(device_node) =
+		of_get_child_by_name(cn, "cpu-map");
 	if (!map)
-		goto out;
+		return ret;
 
 	ret = parse_socket(map);
 	if (ret != 0)
-		goto out_map;
+		return ret;
 
 	topology_normalize_cpu_scale();
 
@@ -709,14 +705,9 @@ static int __init parse_dt_topology(void)
 	 */
 	for_each_possible_cpu(cpu)
 		if (cpu_topology[cpu].package_id < 0) {
-			ret = -EINVAL;
-			break;
+			return -EINVAL;
 		}
 
-out_map:
-	of_node_put(map);
-out:
-	of_node_put(cn);
 	return ret;
 }
 #endif
-- 
2.34.1