All of lore.kernel.org
 help / color / mirror / Atom feed
From: Vincenzo Mezzela <vincenzo.mezzela@gmail.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: gregkh@linuxfoundation.org, rafael@kernel.org,
	linux-kernel@vger.kernel.org, julia.lawall@inria.fr,
	javier.carrasco.cruz@gmail.com, skhan@linuxfoundation.org
Subject: Re: [PATCH] drivers: use __free attribute instead of of_node_put()
Date: Mon, 22 Apr 2024 10:27:38 +0200	[thread overview]
Message-ID: <589b0185-c620-4797-82da-5443ab707a68@gmail.com> (raw)
In-Reply-To: <20240419140106.3mkayxriqjt2cz5i@bogus>

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


  reply	other threads:[~2024-04-22  8:27 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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
2024-04-19 17:46 [PATCH] drivers: use __free attribute instead of of_node_put() Shresth Prasad
2024-04-19 18:26 ` Sudeep Holla
2024-04-19 18:39   ` Shresth Prasad

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=589b0185-c620-4797-82da-5443ab707a68@gmail.com \
    --to=vincenzo.mezzela@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=javier.carrasco.cruz@gmail.com \
    --cc=julia.lawall@inria.fr \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rafael@kernel.org \
    --cc=skhan@linuxfoundation.org \
    --cc=sudeep.holla@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.