[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d677360a-0f97-412c-8563-1def406061bd@gmail.com>
Date: Wed, 24 Apr 2024 14:42:16 +0200
From: Vincenzo Mezzela <vincenzo.mezzela@...il.com>
To: Sudeep Holla <sudeep.holla@....com>
Cc: gregkh@...uxfoundation.org, javier.carrasco.cruz@...il.com,
julia.lawall@...ia.fr, linux-kernel@...r.kernel.org, rafael@...nel.org,
skhan@...uxfoundation.org
Subject: Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()
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@...ia.fr>
>> Signed-off-by: Vincenzo Mezzela <vincenzo.mezzela@...il.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
Powered by blists - more mailing lists