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.1As 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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.