All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drivers: use __free attribute instead of of_node_put()
@ 2024-04-19 13:19 Vincenzo Mezzela
  2024-04-19 14:01 ` Sudeep Holla
  0 siblings, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-04-19 13:19 UTC (permalink / raw)
  To: sudeep.holla, gregkh, rafael
  Cc: linux-kernel, julia.lawall, javier.carrasco.cruz, skhan,
	Vincenzo Mezzela

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH] drivers: use __free attribute instead of of_node_put()
  2024-04-19 13:19 [PATCH] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
@ 2024-04-19 14:01 ` Sudeep Holla
  2024-04-22  8:27   ` Vincenzo Mezzela
  2024-04-22 13:09   ` [PATCH v2] " Vincenzo Mezzela
  0 siblings, 2 replies; 29+ messages in thread
From: Sudeep Holla @ 2024-04-19 14:01 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, rafael, Sudeep Holla, linux-kernel, julia.lawall,
	javier.carrasco.cruz, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] drivers: use __free attribute instead of of_node_put()
  2024-04-19 14:01 ` Sudeep Holla
@ 2024-04-22  8:27   ` Vincenzo Mezzela
  2024-04-22 13:09   ` [PATCH v2] " Vincenzo Mezzela
  1 sibling, 0 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-04-22  8:27 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: gregkh, rafael, linux-kernel, julia.lawall, javier.carrasco.cruz, skhan

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v2] drivers: use __free attribute instead of of_node_put()
  2024-04-19 14:01 ` Sudeep Holla
  2024-04-22  8:27   ` Vincenzo Mezzela
@ 2024-04-22 13:09   ` Vincenzo Mezzela
  2024-04-24 10:37     ` Sudeep Holla
  1 sibling, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-04-22 13:09 UTC (permalink / raw)
  To: sudeep.holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael,
	skhan, vincenzo.mezzela

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()
  2024-04-22 13:09   ` [PATCH v2] " Vincenzo Mezzela
@ 2024-04-24 10:37     ` Sudeep Holla
  2024-04-24 12:42       ` Vincenzo Mezzela
  0 siblings, 1 reply; 29+ messages in thread
From: Sudeep Holla @ 2024-04-24 10:37 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, Sudeep Holla, julia.lawall,
	linux-kernel, rafael, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()
  2024-04-24 10:37     ` Sudeep Holla
@ 2024-04-24 12:42       ` Vincenzo Mezzela
  2024-04-24 12:54         ` Sudeep Holla
  0 siblings, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-04-24 12:42 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael, skhan

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()
  2024-04-24 12:42       ` Vincenzo Mezzela
@ 2024-04-24 12:54         ` Sudeep Holla
  2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
  0 siblings, 1 reply; 29+ messages in thread
From: Sudeep Holla @ 2024-04-24 12:54 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, Sudeep Holla, julia.lawall,
	linux-kernel, rafael, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 0/2 v3] drivers: introduce automatic cleanup feature
  2024-04-24 12:54         ` Sudeep Holla
@ 2024-05-01  9:43           ` Vincenzo Mezzela
  2024-05-01  9:43             ` [PATCH 1/2 v3] drivers: reorganize do-while loops Vincenzo Mezzela
                               ` (3 more replies)
  0 siblings, 4 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-01  9:43 UTC (permalink / raw)
  To: sudeep.holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael,
	skhan, vincenzo.mezzela

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 1/2 v3] drivers: reorganize do-while loops
  2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
@ 2024-05-01  9:43             ` Vincenzo Mezzela
  2024-05-01 11:04               ` Sudeep Holla
  2024-05-01  9:43             ` [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-01  9:43 UTC (permalink / raw)
  To: sudeep.holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael,
	skhan, vincenzo.mezzela

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
  2024-05-01  9:43             ` [PATCH 1/2 v3] drivers: reorganize do-while loops Vincenzo Mezzela
@ 2024-05-01  9:43             ` Vincenzo Mezzela
  2024-05-01 10:48               ` Greg KH
  2024-05-01 11:08               ` Sudeep Holla
  2024-05-01 13:19             ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Conor Dooley
  2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
  3 siblings, 2 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-01  9:43 UTC (permalink / raw)
  To: sudeep.holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael,
	skhan, vincenzo.mezzela

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01  9:43             ` [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
@ 2024-05-01 10:48               ` Greg KH
  2024-05-01 12:33                 ` Vincenzo Mezzela
  2024-05-01 11:08               ` Sudeep Holla
  1 sibling, 1 reply; 29+ messages in thread
From: Greg KH @ 2024-05-01 10:48 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: sudeep.holla, javier.carrasco.cruz, julia.lawall, linux-kernel,
	rafael, skhan

On Wed, May 01, 2024 at 11:43:13AM +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 | 51 +++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 30 deletions(-)

How was all of this tested?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 1/2 v3] drivers: reorganize do-while loops
  2024-05-01  9:43             ` [PATCH 1/2 v3] drivers: reorganize do-while loops Vincenzo Mezzela
@ 2024-05-01 11:04               ` Sudeep Holla
  0 siblings, 0 replies; 29+ messages in thread
From: Sudeep Holla @ 2024-05-01 11:04 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, Sudeep Holla, julia.lawall,
	linux-kernel, rafael, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01  9:43             ` [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
  2024-05-01 10:48               ` Greg KH
@ 2024-05-01 11:08               ` Sudeep Holla
  2024-05-01 11:56                 ` Julia Lawall
  1 sibling, 1 reply; 29+ messages in thread
From: Sudeep Holla @ 2024-05-01 11:08 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael, skhan

As mentioned in 1/2, please fix the subject to be more precise.

On Wed, May 01, 2024 at 11:43:13AM +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 | 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);

Prefer a blank line after this, applies to all the place where you are
introducing this style.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01 11:08               ` Sudeep Holla
@ 2024-05-01 11:56                 ` Julia Lawall
  0 siblings, 0 replies; 29+ messages in thread
From: Julia Lawall @ 2024-05-01 11:56 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Vincenzo Mezzela, gregkh, javier.carrasco.cruz, julia.lawall,
	linux-kernel, rafael, skhan



On Wed, 1 May 2024, Sudeep Holla wrote:

> As mentioned in 1/2, please fix the subject to be more precise.
>
> On Wed, May 01, 2024 at 11:43:13AM +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 | 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);
>
> Prefer a blank line after this, applies to all the place where you are
> introducing this style.

There should also be no blank line before it.

julia

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01 10:48               ` Greg KH
@ 2024-05-01 12:33                 ` Vincenzo Mezzela
  2024-05-01 13:06                   ` Greg KH
  0 siblings, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-01 12:33 UTC (permalink / raw)
  To: Greg KH
  Cc: sudeep.holla, javier.carrasco.cruz, julia.lawall, linux-kernel,
	rafael, skhan

On 01/05/24 12:48, Greg KH wrote:
> On Wed, May 01, 2024 at 11:43:13AM +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 | 51 +++++++++++++++---------------------
>>   1 file changed, 21 insertions(+), 30 deletions(-)
> How was all of this tested?
>
> thanks,
>
> greg k-h

Hi,

I just cross-compiled it for RISC-V to enable the config 
GENERIC_ARCH_TOPOLOGY
and include arch_topology.c as well.

Do you have any suggestion to trigger the affected code and perform some 
testing?

Thanks,

Vincenzo


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01 12:33                 ` Vincenzo Mezzela
@ 2024-05-01 13:06                   ` Greg KH
  2024-05-06 15:30                     ` Vincenzo Mezzela
  0 siblings, 1 reply; 29+ messages in thread
From: Greg KH @ 2024-05-01 13:06 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: sudeep.holla, javier.carrasco.cruz, julia.lawall, linux-kernel,
	rafael, skhan

On Wed, May 01, 2024 at 02:33:39PM +0200, Vincenzo Mezzela wrote:
> On 01/05/24 12:48, Greg KH wrote:
> > On Wed, May 01, 2024 at 11:43:13AM +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 | 51 +++++++++++++++---------------------
> > >   1 file changed, 21 insertions(+), 30 deletions(-)
> > How was all of this tested?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Hi,
> 
> I just cross-compiled it for RISC-V to enable the config
> GENERIC_ARCH_TOPOLOGY
> and include arch_topology.c as well.

Cross-compile is nice, how about running it?

> Do you have any suggestion to trigger the affected code and perform some
> testing?

That is up to you to determine if you wish to modify it :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2 v3] drivers: introduce automatic cleanup feature
  2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
  2024-05-01  9:43             ` [PATCH 1/2 v3] drivers: reorganize do-while loops Vincenzo Mezzela
  2024-05-01  9:43             ` [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
@ 2024-05-01 13:19             ` Conor Dooley
  2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
  3 siblings, 0 replies; 29+ messages in thread
From: Conor Dooley @ 2024-05-01 13:19 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: sudeep.holla, gregkh, javier.carrasco.cruz, julia.lawall,
	linux-kernel, rafael, skhan

[-- Attachment #1: Type: text/plain, Size: 715 bytes --]

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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-01 13:06                   ` Greg KH
@ 2024-05-06 15:30                     ` Vincenzo Mezzela
  2024-05-07 10:32                       ` Sudeep Holla
  0 siblings, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-06 15:30 UTC (permalink / raw)
  To: Greg KH
  Cc: sudeep.holla, javier.carrasco.cruz, julia.lawall, linux-kernel,
	rafael, skhan, conor


On 01/05/24 15:06, Greg KH wrote:
> On Wed, May 01, 2024 at 02:33:39PM +0200, Vincenzo Mezzela wrote:
>> On 01/05/24 12:48, Greg KH wrote:
>>> On Wed, May 01, 2024 at 11:43:13AM +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 | 51 +++++++++++++++---------------------
>>>>    1 file changed, 21 insertions(+), 30 deletions(-)
>>> How was all of this tested?
>>>
>>> thanks,
>>>
>>> greg k-h
>> Hi,
>>
>> I just cross-compiled it for RISC-V to enable the config
>> GENERIC_ARCH_TOPOLOGY
>> and include arch_topology.c as well.
> Cross-compile is nice, how about running it?
>
>> Do you have any suggestion to trigger the affected code and perform some
>> testing?
> That is up to you to determine if you wish to modify it :)
>
> thanks,
>
> greg k-h
Hi,

I've successfully run it on QEMU. There are no differences in the dmesg 
after applying the patches.

Furthermore, I've tracked the execution of the parse_dt_topology() which 
is calling all the functions that I've modified with the patches and I 
checked that of_node_put was correctly called at the end of each scope.

Is there anything else that can be done to further testing this changes?

Thanks,

Vincenzo


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put()
  2024-05-06 15:30                     ` Vincenzo Mezzela
@ 2024-05-07 10:32                       ` Sudeep Holla
  0 siblings, 0 replies; 29+ messages in thread
From: Sudeep Holla @ 2024-05-07 10:32 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: Greg KH, javier.carrasco.cruz, Sudeep Holla, julia.lawall,
	linux-kernel, rafael, skhan, conor

On Mon, May 06, 2024 at 05:30:49PM +0200, Vincenzo Mezzela wrote:
> 
> On 01/05/24 15:06, Greg KH wrote:
> > On Wed, May 01, 2024 at 02:33:39PM +0200, Vincenzo Mezzela wrote:
> > > On 01/05/24 12:48, Greg KH wrote:
> > > > On Wed, May 01, 2024 at 11:43:13AM +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 | 51 +++++++++++++++---------------------
> > > > >    1 file changed, 21 insertions(+), 30 deletions(-)
> > > > How was all of this tested?
> > > > 
> > > > thanks,
> > > > 
> > > > greg k-h
> > > Hi,
> > > 
> > > I just cross-compiled it for RISC-V to enable the config
> > > GENERIC_ARCH_TOPOLOGY
> > > and include arch_topology.c as well.
> > Cross-compile is nice, how about running it?
> > 
> > > Do you have any suggestion to trigger the affected code and perform some
> > > testing?
> > That is up to you to determine if you wish to modify it :)
> > 
> > thanks,
> > 
> > greg k-h
> Hi,
> 
> I've successfully run it on QEMU. There are no differences in the dmesg
> after applying the patches.
>

For this patch, dmesg delta may not be of any use.

> Furthermore, I've tracked the execution of the parse_dt_topology() which is
> calling all the functions that I've modified with the patches and I checked
> that of_node_put was correctly called at the end of each scope.
>

That should be good enough.

> Is there anything else that can be done to further testing this changes?
>

I don't think there is much we can test other than what you have done already.
If you fix the subject and any other comments me and others had suggested, I
am happy to Ack.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature
  2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
                               ` (2 preceding siblings ...)
  2024-05-01 13:19             ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Conor Dooley
@ 2024-05-13  8:13             ` Vincenzo Mezzela
  2024-05-13  8:13               ` [PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops Vincenzo Mezzela
                                 ` (2 more replies)
  3 siblings, 3 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-13  8:13 UTC (permalink / raw)
  To: sudeep.holla
  Cc: vincenzo.mezzela, gregkh, javier.carrasco.cruz, julia.lawall,
	linux-kernel, rafael, skhan

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


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops
  2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
@ 2024-05-13  8:13               ` Vincenzo Mezzela
  2024-05-13  8:13               ` [PATCH 2/2 v4] drivers: arch_topology: use __free attribute instead of of_node_put() Vincenzo Mezzela
  2024-05-13 10:02               ` [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature Sudeep Holla
  2 siblings, 0 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-13  8:13 UTC (permalink / raw)
  To: sudeep.holla
  Cc: vincenzo.mezzela, gregkh, javier.carrasco.cruz, julia.lawall,
	linux-kernel, rafael, skhan

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* [PATCH 2/2 v4] drivers: arch_topology: use __free attribute instead of of_node_put()
  2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
  2024-05-13  8:13               ` [PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops Vincenzo Mezzela
@ 2024-05-13  8:13               ` Vincenzo Mezzela
  2024-05-13 10:02               ` [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature Sudeep Holla
  2 siblings, 0 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-13  8:13 UTC (permalink / raw)
  To: sudeep.holla
  Cc: vincenzo.mezzela, gregkh, javier.carrasco.cruz, julia.lawall,
	linux-kernel, rafael, skhan

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


^ permalink raw reply related	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature
  2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
  2024-05-13  8:13               ` [PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops Vincenzo Mezzela
  2024-05-13  8:13               ` [PATCH 2/2 v4] drivers: arch_topology: use __free attribute instead of of_node_put() Vincenzo Mezzela
@ 2024-05-13 10:02               ` Sudeep Holla
  2024-05-14  7:14                 ` Vincenzo Mezzela
  2024-05-28  8:23                 ` Vincenzo Mezzela
  2 siblings, 2 replies; 29+ messages in thread
From: Sudeep Holla @ 2024-05-13 10:02 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, Sudeep Holla, julia.lawall,
	linux-kernel, rafael, skhan

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature
  2024-05-13 10:02               ` [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature Sudeep Holla
@ 2024-05-14  7:14                 ` Vincenzo Mezzela
  2024-05-28  8:23                 ` Vincenzo Mezzela
  1 sibling, 0 replies; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-14  7:14 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel, rafael, skhan


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



^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature
  2024-05-13 10:02               ` [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature Sudeep Holla
  2024-05-14  7:14                 ` Vincenzo Mezzela
@ 2024-05-28  8:23                 ` Vincenzo Mezzela
  2024-06-03  8:45                   ` Sudeep Holla
  1 sibling, 1 reply; 29+ messages in thread
From: Vincenzo Mezzela @ 2024-05-28  8:23 UTC (permalink / raw)
  To: gregkh
  Cc: Sudeep Holla, javier.carrasco.cruz, julia.lawall, linux-kernel,
	rafael, skhan

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,t
> Sudeep

Hi Greg,

hope this message finds you well. I wanted to kindly follow up on the 
patch [1] I
submitted to introduce the __free attribute in drivers/base/arch_topology.c

Thanks,

Vincenzo

- [1] 
https://lore.kernel.org/lkml/20240513081304.499915-1-vincenzo.mezzela@gmail.com/

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature
  2024-05-28  8:23                 ` Vincenzo Mezzela
@ 2024-06-03  8:45                   ` Sudeep Holla
  0 siblings, 0 replies; 29+ messages in thread
From: Sudeep Holla @ 2024-06-03  8:45 UTC (permalink / raw)
  To: Vincenzo Mezzela
  Cc: gregkh, javier.carrasco.cruz, julia.lawall, linux-kernel,
	Sudeep Holla, rafael, skhan

On Tue, May 28, 2024 at 10:23:37AM +0200, Vincenzo Mezzela wrote:
> On 13/05/24 12:02, Sudeep Holla wrote:

[...]

> > > 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,t
> > Sudeep
> 
> Hi Greg,
> 
> hope this message finds you well. I wanted to kindly follow up on the patch
> [1] I
> submitted to introduce the __free attribute in drivers/base/arch_topology.c
> 

Hi Vincenzo,

Just rebase on -rc1/rc2 and resend the code as the original patch might
have got lost deep in the mbox.

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] drivers: use __free attribute instead of of_node_put()
  2024-04-19 18:26 ` Sudeep Holla
@ 2024-04-19 18:39   ` Shresth Prasad
  0 siblings, 0 replies; 29+ messages in thread
From: Shresth Prasad @ 2024-04-19 18:39 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: vincenzo.mezzela, gregkh, Javier Carrasco, Julia Lawall,
	linux-kernel, rafael, Shuah Khan

19 Apr 2024 11:56:47 pm Sudeep Holla <sudeep.holla@arm.com>:

> On Fri, Apr 19, 2024 at 11:16:37PM +0530, Shresth Prasad wrote:
>>> Please fix the subject line to be "backlight: <driver>: ...". I came
>>> very close to deleting this patch without reading it ;-) .
>> 
>> Really sorry about that, I'll fix it.
>> 
>>> Do we need to get dev->of_node at all? The device, which we are
>>> borrowing, already owns a reference to the node so I don't see
>>> any point in this function taking an extra one.
>>> 
>>> So why not simply make this:
>>> 
>>>     struct device_node *np = dev->of_node;
>> 
>> Looking at it again, I'm not sure why the call to of_node_put is there in
>> the first place. I think removing it would be fine.
>> 
>> I'll fix both of these issues and send a patch v2.
> 
> I assume you are using lore "Reply using the --to, --cc, and..." option
> or something similar.
> 
> You seem to have mixed up 2 different message ID. I was not able to make
> any sense of this email.
> 
> You have wrongly used [1] but you really want [2]. I think both have
> very similar $subject and hence the confusion. Another reason
> why getting subject right is very important on the mailing list.
> 
> -- 
> Regards,
> Sudeep
> 
> [1] 20240419131956.665769-1-vincenzo.mezzela@gmail.com
> (https://lore.kernel.org/all/20240419131956.665769-1-vincenzo.mezzela@gmail.com/)
> [2] 20240419111613.GA12884@aspen.lan
> https://lore.kernel.org/all/20240419111613.GA12884@aspen.lan/
I'm very new to using mailing lists, sorry for any confusion. I was indeed using the reply option on lore. I'll be more careful next.

Is there anyway to remove the incorrect reply from this thread?

Regards,
Shresth Prasad

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] drivers: use __free attribute instead of of_node_put()
  2024-04-19 17:46 [PATCH] drivers: use __free attribute instead of of_node_put() Shresth Prasad
@ 2024-04-19 18:26 ` Sudeep Holla
  2024-04-19 18:39   ` Shresth Prasad
  0 siblings, 1 reply; 29+ messages in thread
From: Sudeep Holla @ 2024-04-19 18:26 UTC (permalink / raw)
  To: Shresth Prasad
  Cc: vincenzo.mezzela, gregkh, Javier Carrasco, Sudeep Holla,
	Julia Lawall, linux-kernel, rafael, Shuah Khan

On Fri, Apr 19, 2024 at 11:16:37PM +0530, Shresth Prasad wrote:
> > Please fix the subject line to be "backlight: <driver>: ...". I came
> > very close to deleting this patch without reading it ;-) .
> 
> Really sorry about that, I'll fix it.
> 
> > Do we need to get dev->of_node at all? The device, which we are
> > borrowing, already owns a reference to the node so I don't see
> > any point in this function taking an extra one.
> >
> > So why not simply make this:
> >
> >     struct device_node *np = dev->of_node;
> 
> Looking at it again, I'm not sure why the call to of_node_put is there in
> the first place. I think removing it would be fine.
> 
> I'll fix both of these issues and send a patch v2.

I assume you are using lore "Reply using the --to, --cc, and..." option
or something similar.

You seem to have mixed up 2 different message ID. I was not able to make
any sense of this email.

You have wrongly used [1] but you really want [2]. I think both have
very similar $subject and hence the confusion. Another reason
why getting subject right is very important on the mailing list.

--
Regards,
Sudeep

[1] 20240419131956.665769-1-vincenzo.mezzela@gmail.com
(https://lore.kernel.org/all/20240419131956.665769-1-vincenzo.mezzela@gmail.com/)
[2] 20240419111613.GA12884@aspen.lan
https://lore.kernel.org/all/20240419111613.GA12884@aspen.lan/

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] drivers: use __free attribute instead of of_node_put()
@ 2024-04-19 17:46 Shresth Prasad
  2024-04-19 18:26 ` Sudeep Holla
  0 siblings, 1 reply; 29+ messages in thread
From: Shresth Prasad @ 2024-04-19 17:46 UTC (permalink / raw)
  To: vincenzo.mezzela
  Cc: gregkh, Javier Carrasco, Julia Lawall, linux-kernel, rafael,
	Shuah Khan, sudeep.holla

> Please fix the subject line to be "backlight: <driver>: ...". I came
> very close to deleting this patch without reading it ;-) .

Really sorry about that, I'll fix it.

> Do we need to get dev->of_node at all? The device, which we are
> borrowing, already owns a reference to the node so I don't see
> any point in this function taking an extra one.
>
> So why not simply make this:
>
>     struct device_node *np = dev->of_node;

Looking at it again, I'm not sure why the call to of_node_put is there in
the first place. I think removing it would be fine.

I'll fix both of these issues and send a patch v2.

Regards,
Shresth

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2024-06-03  8:45 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-19 13:19 [PATCH] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
2024-04-19 14:01 ` Sudeep Holla
2024-04-22  8:27   ` Vincenzo Mezzela
2024-04-22 13:09   ` [PATCH v2] " Vincenzo Mezzela
2024-04-24 10:37     ` Sudeep Holla
2024-04-24 12:42       ` Vincenzo Mezzela
2024-04-24 12:54         ` Sudeep Holla
2024-05-01  9:43           ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Vincenzo Mezzela
2024-05-01  9:43             ` [PATCH 1/2 v3] drivers: reorganize do-while loops Vincenzo Mezzela
2024-05-01 11:04               ` Sudeep Holla
2024-05-01  9:43             ` [PATCH 2/2 v3] drivers: use __free attribute instead of of_node_put() Vincenzo Mezzela
2024-05-01 10:48               ` Greg KH
2024-05-01 12:33                 ` Vincenzo Mezzela
2024-05-01 13:06                   ` Greg KH
2024-05-06 15:30                     ` Vincenzo Mezzela
2024-05-07 10:32                       ` Sudeep Holla
2024-05-01 11:08               ` Sudeep Holla
2024-05-01 11:56                 ` Julia Lawall
2024-05-01 13:19             ` [PATCH 0/2 v3] drivers: introduce automatic cleanup feature Conor Dooley
2024-05-13  8:13             ` [PATCH 0/2 v4] drivers: arch_topology: " Vincenzo Mezzela
2024-05-13  8:13               ` [PATCH 1/2 v4] drivers: arch_topology: Refactor do-while loops Vincenzo Mezzela
2024-05-13  8:13               ` [PATCH 2/2 v4] drivers: arch_topology: use __free attribute instead of of_node_put() Vincenzo Mezzela
2024-05-13 10:02               ` [PATCH 0/2 v4] drivers: arch_topology: introduce automatic cleanup feature Sudeep Holla
2024-05-14  7:14                 ` Vincenzo Mezzela
2024-05-28  8:23                 ` Vincenzo Mezzela
2024-06-03  8:45                   ` Sudeep Holla
2024-04-19 17:46 [PATCH] drivers: use __free attribute instead of of_node_put() Shresth Prasad
2024-04-19 18:26 ` Sudeep Holla
2024-04-19 18:39   ` Shresth Prasad

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.