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

[hejunhao <hejunhao3@huawei.com>]

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