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 <[email protected]>
Signed-off-by: Vincenzo Mezzela <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Vincenzo Mezzela <[email protected]>
> ---
> 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
> Please fix the subject line to be "backlight: <driver>: ...". I came
> very close to deleting this patch without reading it ;-) .
Really sorry about that, I'll fix it.
> Do we need to get dev->of_node at all? The device, which we are
> borrowing, already owns a reference to the node so I don't see
> any point in this function taking an extra one.
>
> So why not simply make this:
>
> struct device_node *np = dev->of_node;
Looking at it again, I'm not sure why the call to of_node_put is there in
the first place. I think removing it would be fine.
I'll fix both of these issues and send a patch v2.
Regards,
Shresth
On Fri, Apr 19, 2024 at 11:16:37PM +0530, Shresth Prasad wrote:
> > Please fix the subject line to be "backlight: <driver>: ...". I came
> > very close to deleting this patch without reading it ;-) .
>
> Really sorry about that, I'll fix it.
>
> > Do we need to get dev->of_node at all? The device, which we are
> > borrowing, already owns a reference to the node so I don't see
> > any point in this function taking an extra one.
> >
> > So why not simply make this:
> >
> > struct device_node *np = dev->of_node;
>
> Looking at it again, I'm not sure why the call to of_node_put is there in
> the first place. I think removing it would be fine.
>
> I'll fix both of these issues and send a patch v2.
I assume you are using lore "Reply using the --to, --cc, and..." option
or something similar.
You seem to have mixed up 2 different message ID. I was not able to make
any sense of this email.
You have wrongly used [1] but you really want [2]. I think both have
very similar $subject and hence the confusion. Another reason
why getting subject right is very important on the mailing list.
--
Regards,
Sudeep
[1] [email protected]
(https://lore.kernel.org/all/[email protected]/)
[2] [email protected]
https://lore.kernel.org/all/[email protected]/
19 Apr 2024 11:56:47 pm Sudeep Holla <[email protected]>:
> On Fri, Apr 19, 2024 at 11:16:37PM +0530, Shresth Prasad wrote:
>>> Please fix the subject line to be "backlight: <driver>: ...". I came
>>> very close to deleting this patch without reading it ;-) .
>>
>> Really sorry about that, I'll fix it.
>>
>>> Do we need to get dev->of_node at all? The device, which we are
>>> borrowing, already owns a reference to the node so I don't see
>>> any point in this function taking an extra one.
>>>
>>> So why not simply make this:
>>>
>>> struct device_node *np = dev->of_node;
>>
>> Looking at it again, I'm not sure why the call to of_node_put is there in
>> the first place. I think removing it would be fine.
>>
>> I'll fix both of these issues and send a patch v2.
>
> I assume you are using lore "Reply using the --to, --cc, and..." option
> or something similar.
>
> You seem to have mixed up 2 different message ID. I was not able to make
> any sense of this email.
>
> You have wrongly used [1] but you really want [2]. I think both have
> very similar $subject and hence the confusion. Another reason
> why getting subject right is very important on the mailing list.
>
> --
> Regards,
> Sudeep
>
> [1] [email protected]
> (https://lore.kernel.org/all/[email protected]/)
> [2] [email protected]
> https://lore.kernel.org/all/[email protected]/
I'm very new to using mailing lists, sorry for any confusion. I was indeed using the reply option on lore. I'll be more careful next.
Is there anyway to remove the incorrect reply from this thread?
Regards,
Shresth Prasad
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 <[email protected]>
>> Signed-off-by: Vincenzo Mezzela <[email protected]>
>> ---
>> 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
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 <[email protected]>
Signed-off-by: Vincenzo Mezzela <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Vincenzo Mezzela <[email protected]>
> ---
> 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
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 <[email protected]>
>> Signed-off-by: Vincenzo Mezzela <[email protected]>
>> ---
>> 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
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
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Vincenzo Mezzela <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Vincenzo Mezzela <[email protected]>
> ---
> drivers/base/arch_topology.c | 51 +++++++++++++++---------------------
> 1 file changed, 21 insertions(+), 30 deletions(-)
How was all of this tested?
thanks,
greg k-h
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> Signed-off-by: Vincenzo Mezzela <[email protected]>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Vincenzo Mezzela <[email protected]>
> > ---
> > 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
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 <[email protected]>
>> Signed-off-by: Vincenzo Mezzela <[email protected]>
>> ---
>> 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
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 <[email protected]>
> > > Signed-off-by: Vincenzo Mezzela <[email protected]>
> > > ---
> > > 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
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.
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 <[email protected]>
>>>> Signed-off-by: Vincenzo Mezzela <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
> > > > > Signed-off-by: Vincenzo Mezzela <[email protected]>
> > > > > ---
> > > > > 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