linux-kernel - Re: [PATCH v2 3/3] coresight: tmc: Decouple the perf buffer allocation from sysfs mode

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: <20250702170827.GB1039028@e132581.arm.com>
Date: Wed, 2 Jul 2025 18:08:27 +0100
From: Leo Yan <leo.yan@....com>
To: Junhao He <hejunhao3@...wei.com>
Cc: suzuki.poulose@....com, james.clark@....com, anshuman.khandual@....com,
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org, linuxarm@...wei.com,
	jonathan.cameron@...wei.com, yangyicong@...wei.com,
	prime.zeng@...ilicon.com
Subject: Re: [PATCH v2 3/3] coresight: tmc: Decouple the perf buffer
 allocation from sysfs mode

On Fri, Jun 20, 2025 at 03:54:12PM +0800, Junhao He wrote:

[..]

> @@ -1341,33 +1339,24 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,
>  	unsigned long size;
>  
>  	node = (event->cpu == -1) ? NUMA_NO_NODE : cpu_to_node(event->cpu);
> -	/*
> -	 * Try to match the perf ring buffer size if it is larger
> -	 * than the size requested via sysfs.
> -	 */
> -	if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
> -		etr_buf = tmc_alloc_etr_buf(drvdata, ((ssize_t)nr_pages << PAGE_SHIFT),
> -					    0, node, NULL);
> -		if (!IS_ERR(etr_buf))
> -			goto done;
> -	}
> +
> +	/* Use the minimum limit if the required size is smaller */
> +	size = (unsigned long)nr_pages << PAGE_SHIFT;

Please change the size's type to ssize_t, then:

        size = nr_pages << PAGE_SHIFT;

> +	if (size < TMC_ETR_PERF_MIN_BUF_SIZE)
> +		size = TMC_ETR_PERF_MIN_BUF_SIZE;

size = min_t(ssize_t, size, TMC_ETR_PERF_MIN_BUF_SIZE);
>  
>  	/*
> -	 * Else switch to configured size for this ETR
> -	 * and scale down until we hit the minimum limit.
> +	 * Try to allocate the required size for this ETR, if failed scale
> +	 * down until we hit the minimum limit.
>  	 */
> -	size = drvdata->size;
>  	do {
>  		etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
>  		if (!IS_ERR(etr_buf))
> -			goto done;
> +			return etr_buf;
>  		size /= 2;
>  	} while (size >= TMC_ETR_PERF_MIN_BUF_SIZE);

Do we really need to scale down buffer size for failure cases?

I would like a straightforward code:

        etr_buf = tmc_alloc_etr_buf(drvdata, size, 0, node, NULL);
        if (IS_ERR_OR_NULL(etr_buf))
                return etr_buf;

Just a side topic, we know tmc_alloc_etr_buf() should not return NULL
pointer. For a sanity check, the callers (alloc_etr_buf(),
tmc_etr_get_sysfs_buffer(), etc) should valid a buffer pointer with
IS_ERR_OR_NULL() rather than IS_ERR(). This can be a separate patch.

Thanks,
Leo

>  	return ERR_PTR(-ENOMEM);
> -
> -done:
> -	return etr_buf;
>  }
>  
>  static struct etr_buf *
> -- 
> 2.33.0
>