* [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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread
end of thread, other threads:[~2024-06-03 8:45 UTC | newest]
Thread overview: 26+ 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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).