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

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

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

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/

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

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

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

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

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.