Received-SPF: pass (google.com: domain of linux-kernel+bounces-156885-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <d677360a-0f97-412c-8563-1def406061bd@gmail.com>
Date: Wed, 24 Apr 2024 14:42:16 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] drivers: use __free attribute instead of of_node_put()
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: gregkh@linuxfoundation.org, javier.carrasco.cruz@gmail.com,
 julia.lawall@inria.fr, linux-kernel@vger.kernel.org, rafael@kernel.org,
 skhan@linuxfoundation.org
References: <20240419140106.3mkayxriqjt2cz5i@bogus>
 <20240422130931.176635-1-vincenzo.mezzela@gmail.com>
 <20240424103756.jhloae3fcyinyba4@bogus>
Content-Language: en-US
From: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
In-Reply-To: <20240424103756.jhloae3fcyinyba4@bogus>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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