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] [day] [month] [year] [list]

Message-ID: <32bbc3dc-d723-6034-ba39-bf01dfbcf81e@huawei.com>
Date: Sat, 19 Jul 2025 19:20:39 +0800
From: hejunhao <hejunhao3@...wei.com>
To: Leo Yan <leo.yan@....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 2025/7/3 1:08, Leo Yan wrote:
> 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);

Sure, I will do that.

>>   
>>   	/*
>> -	 * 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.

A new patch will be added to achieve this.

Thank you for your comments.
Junhao.

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