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

[Leo Yan]

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
>