linux-kernel - Re: [PATCH] coresight: trbe: Use scope-based resource management in arm_trbe_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250912102743.GG12516@e132581.arm.com>
Date: Fri, 12 Sep 2025 11:27:43 +0100
From: Leo Yan <leo.yan@....com>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Markus Elfring <Markus.Elfring@....de>, coresight@...ts.linaro.org,
	linux-arm-kernel@...ts.infradead.org,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	James Clark <james.clark@...aro.org>,
	Mike Leach <mike.leach@...aro.org>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	LKML <linux-kernel@...r.kernel.org>,
	Tamas Zsoldos <tamas.zsoldos@....com>
Subject: Re: [PATCH] coresight: trbe: Use scope-based resource management in
 arm_trbe_alloc_buffer()

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)