Re: [PATCH v21 3/8] coresight: tmc: add a function to switch the ETR to a free etr_buf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] coresight: tmc: active read buffer reuse in tmc_etr_switch_buf_node()
--

commit edde9bcbd54db59cd64f7044e1302e2f89c8fc07
Author: Jie Gan <jie.gan@oss.qualcomm.com>

coresight: tmc: add a function to switch the ETR to a free etr_buf

This commit adds tmc_etr_switch_buf_node() to stop the ETR, switch to a
free etr_buf from etr_buf_list, and restart the ETR under drvdata->spinlock
while returning the switched-out buffer node for reading.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index eee98a9269df4..2de31d73a4b47 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1224,6 +1224,71 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)

[ ... ]

> +struct etr_buf_node *tmc_etr_switch_buf_node(struct tmc_drvdata *drvdata)
> +{
> +	struct etr_buf_node *nd, *curr_node = NULL, *picked_node = NULL;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +	if (coresight_get_mode(drvdata->csdev) != CS_MODE_SYSFS)
> +		goto out;
> +
> +	list_for_each_entry(nd, &drvdata->etr_buf_list, link) {
> +		if (nd->sysfs_buf == drvdata->sysfs_buf)
> +			curr_node = nd;
> +		else if (!picked_node && nd->is_free)
> +			picked_node = nd;
> +	}
> +
> +	if (!curr_node || !picked_node) {
> +		curr_node = NULL;
> +		goto out;
> +	}
> +
> +	__tmc_etr_disable_hw(drvdata);
> +
> +	/* The switched out buffer is free again once it has been read */
> +	curr_node->is_free = true;
> +	curr_node->pos = 0;
> +	curr_node->reading = true;
> +	picked_node->is_free = false;

[Severity: High]
Can tmc_etr_switch_buf_node() re-select a buffer node while it is still
being read by userspace?

When curr_node is switched out for reading, curr_node->is_free is
immediately set to true alongside curr_node->reading = true, while the
selection loop above only checks nd->is_free and does not check
!nd->reading:

	list_for_each_entry(nd, &drvdata->etr_buf_list, link) {
		if (nd->sysfs_buf == drvdata->sysfs_buf)
			curr_node = nd;
		else if (!picked_node && nd->is_free)
			picked_node = nd;
	}

In tmc_create_etr_buf_list(), all nodes other than the active
drvdata->sysfs_buf start with is_free = true, and in the caller added later
in the series (tmc_byte_cntr_get_data() in coresight-ctcu-byte-cntr.c),
finishing a buffer read only clears byte_cntr_data->buf_node->reading =
false without updating is_free.

Because every node other than the active drvdata->sysfs_buf always has
is_free == true even while reading == true, the documented NULL return when
there is no free buffer cannot be reached. If tmc_etr_switch_buf_node() is
called again before the previous buffer has been fully read (for instance,
via concurrent read() calls on the same open descriptor since tmc_open()
uses nonseekable_open(), or when num_nodes > 2 and an earlier node in
etr_buf_list is still being read while a later node is idle), won't it pick
a node with reading == true and restart ETR DMA into that buffer while it
is being read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-enable-byte-cntr-for-ctcu-v21-0-231e069b7853@oss.qualcomm.com?part=3