Re: [PATCH v22 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 2 potential
issues to consider:
- [Medium] coresight: tmc: unhandled enable failure in
  tmc_etr_switch_buf_node()
- [Low] coresight: tmc: redundant register writes in
  tmc_etr_reset_sysfs_buf()
--

commit 5c5376ae2653b11790a1e4a68e0d311b9dacd681
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 switch an active ETR sink to
a free buffer from etr_buf_list while returning the previous buffer node
for reading.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index e9213f4568341..5032f5598e15b 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)
>  
>  }
>  
> +static void tmc_etr_reset_sysfs_buf(struct tmc_drvdata *drvdata)
> +{
> +	u32 sts;
> +
> +	CS_UNLOCK(drvdata->base);
> +	tmc_write_rrp(drvdata, drvdata->sysfs_buf->hwaddr);
> +	tmc_write_rwp(drvdata, drvdata->sysfs_buf->hwaddr);
> +	sts = readl_relaxed(drvdata->base + TMC_STS) & ~TMC_STS_FULL;
> +	writel_relaxed(sts, drvdata->base + TMC_STS);
> +	CS_LOCK(drvdata->base);
> +}

[Severity: Low]
Is tmc_etr_reset_sysfs_buf() needed when tmc_etr_switch_buf_node() calls
__tmc_etr_enable_hw() immediately afterwards?

__tmc_etr_enable_hw() already resets TMC_RRP, TMC_RWP and TMC_STS_FULL
after waiting for tmc_wait_for_tmcready() and programming TMC_DBA, and
guards those writes with tmc_etr_has_cap(drvdata, TMC_ETR_SAVE_RESTORE).
Calling tmc_etr_reset_sysfs_buf() beforehand duplicates those MMIO writes
and writes TMC_RRP, TMC_RWP and TMC_STS unconditionally before TMCReady is
checked, including on SoC-400 devices without TMC_ETR_SAVE_RESTORE and in
ETR_MODE_ETR_SG where hwaddr is the scatter-gather table base address.

> +
> +/**
> + * tmc_etr_switch_buf_node - switch the ETR to a free buffer of the etr_buf_list.
> + * @drvdata:	drvdata of the TMC device.
> + *
> + * Stop the ETR, hand over the current buffer for reading, switch to a free
> + * buffer and restart the ETR. All of this is done with @drvdata->spinlock held,
> + * so it can't race with the sink disable path.
> + *
> + * Return: the node of the switched out buffer, or NULL if the sink isn't in
> + * sysfs mode or there is no free buffer. The ETR is left untouched in both
> + * cases.
> + */
> +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 && !nd->reading)
> +			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;
> +	drvdata->sysfs_buf = picked_node->sysfs_buf;
> +	drvdata->etr_buf = picked_node->sysfs_buf;
> +
> +	tmc_etr_reset_sysfs_buf(drvdata);
> +	if (__tmc_etr_enable_hw(drvdata))
> +		dev_err(&drvdata->csdev->dev, "Failed to restart the ETR\n");

[Severity: Medium]
If __tmc_etr_enable_hw() fails because tmc_wait_for_tmcready() times out,
does leaving drvdata->sysfs_buf and drvdata->etr_buf pointing to
picked_node->sysfs_buf desynchronize the driver state from the hardware?

Because the sink remains in CS_MODE_SYSFS while the hardware still holds
the previous buffer's registers, a later tmc_disable_etr_sink() or
tmc_etr_switch_buf_node() call will run __tmc_etr_disable_hw() ->
tmc_etr_sync_sysfs_buf() on picked_node->sysfs_buf against stale TMC_RRP
and TMC_RWP values.  In ETR_MODE_FLAT, tmc_etr_sync_flat_buf() computes
etr_buf->offset = rrp - etr_buf->hwaddr using the old RRP, which can exceed
etr_buf->size, underflow etr_buf->size - offset in tmc_etr_buf_get_data(),
and cause an out-of-bounds memory access when inserting the barrier packet
or reading the buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-enable-byte-cntr-for-ctcu-v22-0-05d48c0eb110@oss.qualcomm.com?part=3