[PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 9 Sep 2025 14:20:06 +0200

Scope-based resource management became supported for some
programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
Introduce __cleanup() based infrastructure").

* Thus use the attribute “__free(kfree)”.

* Reduce the scopes for the local variables “buf” and “pglist”.

* Omit four kfree() calls accordingly.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hwtracing/coresight/coresight-trbe.c | 21 ++++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 8f9bbef71f23..1b0d58bf8613 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -733,8 +733,6 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
 				   struct perf_event *event, void **pages,
 				   int nr_pages, bool snapshot)
 {
-	struct trbe_buf *buf;
-	struct page **pglist;
 	int i;
 
 	/*
@@ -746,32 +744,29 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
 	if (nr_pages < 2)
 		return NULL;
 
-	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, trbe_alloc_node(event));
+	struct trbe_buf *buf __free(kfree) = kzalloc_node(sizeof(*buf),
+							  GFP_KERNEL,
+							  trbe_alloc_node(event));
 	if (!buf)
 		return NULL;
 
-	pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
-	if (!pglist) {
-		kfree(buf);
+	struct page **pglist __free(kfree) = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
+	if (!pglist)
 		return NULL;
-	}
 
 	for (i = 0; i < nr_pages; i++)
 		pglist[i] = virt_to_page(pages[i]);
 
 	buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
-	if (!buf->trbe_base) {
-		kfree(pglist);
-		kfree(buf);
+	if (!buf->trbe_base)
 		return NULL;
-	}
+
 	buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
 	buf->trbe_write = buf->trbe_base;
 	buf->snapshot = snapshot;
 	buf->nr_pages = nr_pages;
 	buf->pages = pages;
-	kfree(pglist);
-	return buf;
+	return_ptr(buf);
 }
 
 static void arm_trbe_free_buffer(void *config)
-- 
2.51.0

Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[Anshuman Khandual]

On 09/09/25 5:58 PM, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 9 Sep 2025 14:20:06 +0200
> 
> Scope-based resource management became supported for some
> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:

			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

SHA ID here just needs first 12 digits not the entire string.
> Introduce __cleanup() based infrastructure").
> 
> * Thus use the attribute “__free(kfree)”.
> 
> * Reduce the scopes for the local variables “buf” and “pglist”.

What is that required ?
> 
> * Omit four kfree() calls accordingly.

Right.

The commit message should be re-written with little more
context from arm_trbe_alloc_buffer(), after describing
the new scope-based resource management.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/hwtracing/coresight/coresight-trbe.c | 21 ++++++++------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 8f9bbef71f23..1b0d58bf8613 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -733,8 +733,6 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>  				   struct perf_event *event, void **pages,
>  				   int nr_pages, bool snapshot)
>  {
> -	struct trbe_buf *buf;
> -	struct page **pglist;
>  	int i;
>  
>  	/*
> @@ -746,32 +744,29 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>  	if (nr_pages < 2)
>  		return NULL;
>  
> -	buf = kzalloc_node(sizeof(*buf), GFP_KERNEL, trbe_alloc_node(event));
> +	struct trbe_buf *buf __free(kfree) = kzalloc_node(sizeof(*buf),
> +							  GFP_KERNEL,
> +							  trbe_alloc_node(event));
>  	if (!buf)
>  		return NULL;
>  
> -	pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> -	if (!pglist) {
> -		kfree(buf);
> +	struct page **pglist __free(kfree) = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> +	if (!pglist)
>  		return NULL;
> -	}
>  
>  	for (i = 0; i < nr_pages; i++)
>  		pglist[i] = virt_to_page(pages[i]);
>  
>  	buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
> -	if (!buf->trbe_base) {
> -		kfree(pglist);
> -		kfree(buf);
> +	if (!buf->trbe_base)
>  		return NULL;
> -	}
> +
>  	buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
>  	buf->trbe_write = buf->trbe_base;
>  	buf->snapshot = snapshot;
>  	buf->nr_pages = nr_pages;
>  	buf->pages = pages;
> -	kfree(pglist);
> -	return buf;
> +	return_ptr(buf);
>  }
>  
>  static void arm_trbe_free_buffer(void *config)

Seems like a good idea though. But please keep the local variable
declaration in the current place which is bit cleaner and reduces
code churn as well.

diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 7a226316c48f..7babba1a9afb 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -733,8 +733,8 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
                                   struct perf_event *event, void **pages,
                                   int nr_pages, bool snapshot)
 {
-       struct trbe_buf *buf;
-       struct page **pglist;
+       struct trbe_buf *buf __free(kfree);
+       struct page **pglist __free(kfree);
        int i;

        /*
@@ -751,27 +751,22 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
                return NULL;

        pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
-       if (!pglist) {
-               kfree(buf);
+       if (!pglist)
                return NULL;
-       }

        for (i = 0; i < nr_pages; i++)
                pglist[i] = virt_to_page(pages[i]);

        buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
-       if (!buf->trbe_base) {
-               kfree(pglist);
-               kfree(buf);
+       if (!buf->trbe_base)
                return NULL;
-       }
+
        buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
        buf->trbe_write = buf->trbe_base;
        buf->snapshot = snapshot;
        buf->nr_pages = nr_pages;
        buf->pages = pages;
-       kfree(pglist);
-       return buf;
+       return_ptr(buf);
 }

 static void arm_trbe_free_buffer(void *config)

Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[Markus Elfring]

>> Scope-based resource management became supported for some
>> programming interfaces by contributions of Peter Zijlstra on 2023-05-26.
>> See also the commit 54da6a0924311c7cf5015533991e44fb8eb12773 ("locking:
> 
> 			^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> SHA ID here just needs first 12 digits not the entire string.

See also once more:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.17-rc5#n109


>> Introduce __cleanup() based infrastructure").
>>
>> * Thus use the attribute “__free(kfree)”.
>>
>> * Reduce the scopes for the local variables “buf” and “pglist”.
> 
> What is that required ?

The needed variables should be defined in a succinct way.


>> * Omit four kfree() calls accordingly.
> 
> Right.
> 
> The commit message should be re-written with little more
> context from arm_trbe_alloc_buffer(), after describing
> the new scope-based resource management.

Which background information do you miss so far?

Regards,
Markus

Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[Leo Yan]

On Fri, Sep 12, 2025 at 09:55:57AM +0530, Anshuman Khandual wrote:

[...]

> Seems like a good idea though. But please keep the local variable
> declaration in the current place which is bit cleaner and reduces
> code churn as well.

Though include/linux/cleanup.h suggests to "always define and assign
variables in one statement" to avoid potential interdependency problem
with lock, this is not concerned in arm_trbe_alloc_buffer().

So I bias to Anshuman's suggestion of declaring variables at the top
of the function, as this is the style widely used in the kernel.

> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 7a226316c48f..7babba1a9afb 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -733,8 +733,8 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>                                    struct perf_event *event, void **pages,
>                                    int nr_pages, bool snapshot)
>  {
> -       struct trbe_buf *buf;
> -       struct page **pglist;
> +       struct trbe_buf *buf __free(kfree);

    struct trbe_buf *buf __free(kfree) = NULL;

> +       struct page **pglist __free(kfree);

    struct page **pglist __free(kfree) = NULL;

>         int i;
> 
>         /*
> @@ -751,27 +751,22 @@ static void *arm_trbe_alloc_buffer(struct coresight_device *csdev,
>                 return NULL;
> 
>         pglist = kcalloc(nr_pages, sizeof(*pglist), GFP_KERNEL);
> -       if (!pglist) {
> -               kfree(buf);
> +       if (!pglist)
>                 return NULL;
> -       }
> 
>         for (i = 0; i < nr_pages; i++)
>                 pglist[i] = virt_to_page(pages[i]);
> 
>         buf->trbe_base = (unsigned long)vmap(pglist, nr_pages, VM_MAP, PAGE_KERNEL);
> -       if (!buf->trbe_base) {
> -               kfree(pglist);
> -               kfree(buf);
> +       if (!buf->trbe_base)
>                 return NULL;
> -       }
> +
>         buf->trbe_limit = buf->trbe_base + nr_pages * PAGE_SIZE;
>         buf->trbe_write = buf->trbe_base;
>         buf->snapshot = snapshot;
>         buf->nr_pages = nr_pages;
>         buf->pages = pages;
> -       kfree(pglist);
> -       return buf;
> +       return_ptr(buf);
>  }
> 
>  static void arm_trbe_free_buffer(void *config)

Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc_buffer()

[Markus Elfring]

>> Seems like a good idea though. But please keep the local variable
>> declaration in the current place which is bit cleaner and reduces
>> code churn as well.
> 
> Though include/linux/cleanup.h suggests to "always define and assign
> variables in one statement" to avoid potential interdependency problem
> with lock, this is not concerned in arm_trbe_alloc_buffer().
> 
> So I bias to Anshuman's suggestion of declaring variables at the top
> of the function, as this is the style widely used in the kernel.

Does anything hinder you to follow the mentioned cleanup advice more?

Regards,
Markus