Re: [PATCH v20 6/7] coresight: ctcu: enable byte-cntr for TMC ETR devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 17 potential issue(s) to consider:
- [Critical] coresight: ctcu: race in ctcu_byte_cntr_switch_buffer() corrupts ETR state
- [Critical] coresight: tmc-etr: missing mutual exclusion between byte-cntr and sysfs read
- [High] coresight: ctcu: lost final buffer and missing wakeup on trace stop
- [High] coresight: ctcu: unclocked MMIO access in tmc_read_unprepare_byte_cntr()
- [High] coresight: ctcu: use-after-free and unbind hang in ctcu_platform_remove()
- [High] coresight: ctcu: missing mode check, rollback, and lock in ctcu_enable()
- [High] coresight: ctcu: missing wakeup, tail drain, and NULL check on stop
- [High] coresight: ctcu: unlocked CS_UNLOCK/CS_LOCK in ctcu_program_register()
- [High] coresight: ctcu: unsynchronized concurrent read() in tmc_byte_cntr_get_data()
- [High] coresight: tmc: unhandled errors in tmc_read_prepare_etr() fallback
- [High] coresight: ctcu: race between byte_cntr_sysfs_ops calls and module unload
- [Medium] coresight: ctcu: irq_cnt reset in ctcu_byte_cntr_switch_buffer() breaks check
- [Medium] coresight: ctcu: broken error handling in ctcu_byte_cntr_init()
- [Medium] coresight: ctcu: ctcu_enable() leaves byte-cntr enabled on traceid error
- [Medium] coresight: tmc: tmc_read() swallows negative error codes as EOF
- [Low] coresight: ctcu: missing static qualifier on byte_cntr_sysfs_ops
- [Low] coresight: ctcu: malformed kernel-doc comments in header files
--

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

coresight: ctcu: enable byte-cntr for TMC ETR devices

This patch adds byte-cntr support to the CTCU driver to transfer trace data
from TMC ETR buffers to userspace using threshold interrupts and buffer
switching.

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> new file mode 100644
> index 0000000000000..f10f8897d280e
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
> @@ -0,0 +1,392 @@

[ ... ]

> +static bool ctcu_byte_cntr_switch_buffer(struct tmc_drvdata *etr_drvdata,
> +					 struct ctcu_byte_cntr *byte_cntr_data)
> +{
> +	struct etr_buf_node *nd, *next, *curr_node = NULL, *picked_node = NULL;
> +	struct etr_buf *curr_buf;
> +	bool found_free_buf = false;
> +	unsigned long flags;
> +
> +	if (WARN_ON(!etr_drvdata || !byte_cntr_data))
> +		return false;
> +
> +	/* Stop the ETR before initiating the switch */
> +	tmc_etr_enable_disable_hw(etr_drvdata, false);
> +
> +	/*
> +	 * Serialise the sysfs_buf/etr_buf swap against the ETR sink
> +	 * enable/disable paths which also touch these fields under the
> +	 * spinlock. tmc_etr_enable_disable_hw() takes the same lock, so it
> +	 * must be called outside this critical section.
> +	 */
> +	raw_spin_lock_irqsave(&etr_drvdata->spinlock, flags);
> +	curr_buf = etr_drvdata->sysfs_buf;
> +	list_for_each_entry_safe(nd, next, &etr_drvdata->etr_buf_list, link) {
> +		/* curr_buf is free for next round */
> +		if (nd->sysfs_buf == curr_buf) {
> +			nd->is_free = true;
> +			curr_node = nd;
> +		} else if (!found_free_buf && nd->is_free) {
> +			picked_node = nd;
> +			found_free_buf = true;
> +		}
> +	}
> +
> +	/* The live buffer has to be in the list to be switched out */
> +	if (!curr_node)
> +		found_free_buf = false;
> +
> +	if (found_free_buf) {
> +		curr_node->pos = 0;
> +		curr_node->reading = true;
> +		byte_cntr_data->buf_node = curr_node;
> +		etr_drvdata->sysfs_buf = picked_node->sysfs_buf;
> +		etr_drvdata->etr_buf = picked_node->sysfs_buf;
> +		picked_node->is_free = false;
> +		/* Reset irq_cnt for next etr_buf */
> +		atomic_set(&byte_cntr_data->irq_cnt, 0);
> +	}
> +	raw_spin_unlock_irqrestore(&etr_drvdata->spinlock, flags);
> +
> +	/* Restart the ETR once a free buffer is available */
> +	if (found_free_buf)
> +		tmc_etr_enable_disable_hw(etr_drvdata, true);
> +
> +	return found_free_buf;
> +}

[Severity: Critical]
As reported by Sashiko on v19 [1], can splitting the hardware stop, buffer
pointer swap, and hardware restart across three separate
etr_drvdata->spinlock critical sections in ctcu_byte_cntr_switch_buffer()
race with tmc_disable_etr_sink() and corrupt ETR state?

During a byte-cntr read session, etr_drvdata->reading is false, and in
coresight_disable_path_from_to(), tmc_disable_etr_sink() runs before
coresight_disable_helpers() calls ctcu_byte_cntr_stop().

If tmc_disable_etr_sink() runs before the second critical section in
ctcu_byte_cntr_switch_buffer() (after tmc_etr_enable_disable_hw(etr_drvdata,
false) drops etr_drvdata->spinlock), tmc_disable_etr_sink() disables the ETR,
sets etr_drvdata->etr_buf = NULL, and sets the sink mode to CS_MODE_DISABLED.

The second critical section in ctcu_byte_cntr_switch_buffer() does not check
whether coresight_get_mode(etr_drvdata->csdev) == CS_MODE_SYSFS and
unconditionally assigns etr_drvdata->etr_buf = picked_node->sysfs_buf. Then
tmc_etr_enable_disable_hw(etr_drvdata, true) sees mode != CS_MODE_SYSFS and
returns early. When the file is closed, tmc_clean_etr_buf_list() frees
picked_node->sysfs_buf and clears etr_drvdata->sysfs_buf, leaving
etr_drvdata->etr_buf pointing to freed memory (which can cause a
use-after-free in tmc_panic_sync_etr() and causes WARN_ON(drvdata->etr_buf)
in tmc_etr_enable_hw() to reject all future ETR sessions with -EBUSY).

Conversely, if tmc_disable_etr_sink() runs between the second and third
critical sections, etr_drvdata->sysfs_buf and etr_drvdata->etr_buf already
point to picked_node->sysfs_buf while hardware registers TMC_RRP, TMC_RWP,
and TMC_STS still hold the DMA addresses and full status of the old buffer
curr_node->sysfs_buf. When tmc_disable_etr_sink() calls
__tmc_etr_disable_hw() -> tmc_etr_sync_sysfs_buf(), it computes
etr_buf->offset = rrp - etr_buf->hwaddr across the two distinct buffers,
performing an out-of-bounds DMA sync and, if TMC_STS_FULL was set on the old
buffer, writing a 16-byte barrier packet out of bounds in
tmc_etr_buf_insert_barrier_packet().

> +/*
> + * ctcu_byte_cntr_get_data() - reads data from the deactivated and filled buffer.
> + * The byte-cntr reading work reads data from the deactivated and filled buffer.
> + * The read operation waits for a buffer to become available, either filled or
> + * upon timeout, and then reads trace data from the synced buffer.
> + */
> +static ssize_t tmc_byte_cntr_get_data(struct tmc_drvdata *etr_drvdata, loff_t pos,
> +				      size_t len, char **bufpp)
> +{
> +	struct coresight_device *ctcu = tmc_etr_get_ctcu_device(etr_drvdata);
> +	struct device *dev = &etr_drvdata->csdev->dev;
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct etr_buf *sysfs_buf;
> +	atomic_t *irq_cnt;
> +	ssize_t actual;
> +	int ret;
> +
> +	/* No byte-cntr read session, let the caller use the generic read */
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data || !byte_cntr_data->irq_enabled ||
> +	    !byte_cntr_data->reading)
> +		return -EINVAL;
> +
> +	irq_cnt = &byte_cntr_data->irq_cnt;
> +
> +wait_buffer:
> +	if (!byte_cntr_data->buf_node) {
> +		ret = wait_event_interruptible_timeout(byte_cntr_data->wq,
> +				(atomic_read(irq_cnt) >= MAX_IRQ_CNT - 1) ||
> +				!byte_cntr_data->enable,
> +				BYTE_CNTR_TIMEOUT);
> +		if (ret < 0)
> +			return ret;
> +		/*
> +		 * The current etr_buf is almost full or timeout is triggered,
> +		 * so switch the buffer and mark the switched buffer as reading.
> +		 */
> +		if (byte_cntr_data->enable) {
> +			if (!ctcu_byte_cntr_switch_buffer(etr_drvdata, byte_cntr_data)) {
> +				dev_err(dev, "Switch buffer failed for the byte-cntr\n");
> +				return -ENOMEM;
> +			}
> +		} else {
> +			/* Exit byte-cntr reading */
> +			return 0;
> +		}
> +	}

[Severity: High]
Does returning 0 immediately when byte_cntr_data->enable is false in
tmc_byte_cntr_get_data() discard the final flushed ETR buffer when a trace
session stops?

When tracing is stopped via coresight_disable_sysfs() ->
coresight_disable_path(), tmc_disable_etr_sink() flushes the ETR hardware and
syncs the final trace data into etr_drvdata->sysfs_buf, and ctcu_disable()
calls ctcu_byte_cntr_stop() which sets byte_cntr_data->enable = false.

When wait_event_interruptible_timeout() returns with
byte_cntr_data->buf_node == NULL, tmc_byte_cntr_get_data() sees
!byte_cntr_data->enable and returns 0 (EOF) without switching or draining
etr_drvdata->sysfs_buf. When the file is later closed,
tmc_read_unprepare_byte_cntr() calls tmc_clean_etr_buf_list(), freeing that
final buffer unread.

> +
> +	/* Check the status of current etr_buf */
> +	if (atomic_read(irq_cnt) >= MAX_IRQ_CNT)
> +		dev_warn(dev, "Data overwrite happened\n");

[Severity: Medium]
Can this overwrite check in tmc_byte_cntr_get_data() ever trigger for the
switched-out buffer after ctcu_byte_cntr_switch_buffer() has already reset
byte_cntr_data->irq_cnt to 0?

In ctcu_byte_cntr_switch_buffer():

	if (found_free_buf) {
		...
		/* Reset irq_cnt for next etr_buf */
		atomic_set(&byte_cntr_data->irq_cnt, 0);
	}

Because irq_cnt is reset to 0 before ctcu_byte_cntr_switch_buffer() returns,
any overwrite on the buffer that was just switched out is cleared before
tmc_byte_cntr_get_data() checks atomic_read(irq_cnt) >= MAX_IRQ_CNT, and
subsequent partial reads of byte_cntr_data->buf_node end up checking the
interrupt count of the newly active hardware buffer instead.

> +
> +	pos = byte_cntr_data->buf_node->pos;
> +	sysfs_buf = byte_cntr_data->buf_node->sysfs_buf;
> +	actual = tmc_etr_read_sysfs_buf(sysfs_buf, pos, len, bufpp);
> +	if (actual <= 0) {
> +		/* Reset buf_node upon reading is finished or failed */
> +		byte_cntr_data->buf_node->reading = false;
> +		byte_cntr_data->buf_node = NULL;
> +
> +		/*
> +		 * Nothing in the buffer, waiting for the next buffer
> +		 * to be filled.
> +		 */
> +		if (actual == 0)
> +			goto wait_buffer;
> +	}
> +
> +	return actual;
> +}

[Severity: High]
As reported by Sashiko on v19 [2], what prevents concurrent read() calls on
the same open /dev/tmc_etrX file descriptor from racing on
byte_cntr_data->buf_node in tmc_byte_cntr_get_data()?

Because /dev/tmc_etrX is a character device opened with nonseekable_open(),
FMODE_ATOMIC_POS is not set and the VFS does not serialize concurrent read()
calls with file->f_pos_lock. Multiple threads or processes sharing the open
file description can enter tmc_byte_cntr_get_data() and
tmc_etr_update_buf_node_pos() simultaneously without any lock:

1. If Thread A reaches the end of the buffer (actual == 0) and sets
   byte_cntr_data->buf_node = NULL while Thread B has just passed the
   if (!byte_cntr_data->buf_node) check, Thread B will dereference a NULL
   byte_cntr_data->buf_node at byte_cntr_data->buf_node->pos.

2. If two threads both see !byte_cntr_data->buf_node and call
   ctcu_byte_cntr_switch_buffer(), the first switch marks curr_node->is_free =
   true while also setting curr_node->reading = true, allowing the second
   switch to pick that same curr_node as picked_node and restart ETR DMA into
   the buffer while the first thread is copying it to userspace.

> +
> +static int tmc_read_prepare_byte_cntr(struct tmc_drvdata *etr_drvdata)
> +{
> +	struct coresight_device *ctcu = tmc_etr_get_ctcu_device(etr_drvdata);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* byte-cntr is operating with SYSFS mode being enabled only */
> +	if (coresight_get_mode(etr_drvdata->csdev) != CS_MODE_SYSFS)
> +		return -EINVAL;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data)
> +		return -EINVAL;
> +
> +	raw_spin_lock_irqsave(&byte_cntr_data->spin_lock, flags);
> +	/*
> +	 * irq_enabled can only be changed with the lock held and reading
> +	 * cleared, so check it here to keep it stable once reading is set.
> +	 */
> +	if (!byte_cntr_data->irq_enabled) {
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	if (byte_cntr_data->reading) {
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		return -EBUSY;
> +	}

[Severity: Critical]
Can a byte-cntr read session and a generic ETR sysfs read session be opened
concurrently on the same /dev/tmc_etrX device?

When a byte-cntr read session is opened via tmc_read_prepare_byte_cntr(),
byte_cntr_data->reading is set to true, but tmc_read_prepare_etr() returns
early without setting drvdata->reading = true. Conversely,
tmc_read_prepare_byte_cntr() never checks etr_drvdata->reading.

In addition, tmc_read_prepare_byte_cntr() checks
coresight_get_mode(etr_drvdata->csdev) != CS_MODE_SYSFS and returns -EINVAL
before checking if (byte_cntr_data->reading) return -EBUSY. If a user opens
/dev/tmc_etrX in byte-cntr mode, stops tracing (so the sink mode becomes
CS_MODE_DISABLED), and opens /dev/tmc_etrX a second time,
tmc_read_prepare_byte_cntr() returns -EINVAL and tmc_read_prepare_etr() falls
through to the generic read prepare path, which succeeds because
drvdata->reading is false and drvdata->sysfs_buf is non-NULL.

Similarly, if /dev/tmc_etrX is first opened in generic mode (setting
drvdata->reading = true) while CS_MODE_SYSFS is active, enabling irq_enabled
and opening /dev/tmc_etrX a second time succeeds in
tmc_read_prepare_byte_cntr() because it never checks etr_drvdata->reading.

When either open file descriptor is then closed, tmc_read_unprepare_etr()
calls tmc_read_unprepare_byte_cntr(), which runs
tmc_clean_etr_buf_list(etr_drvdata) and frees all buffers in etr_buf_list and
sets drvdata->sysfs_buf = NULL while the other file descriptor is still open
and reading, leading to a use-after-free or a NULL pointer dereference in
tmc_etr_read_sysfs_buf().

> +
> +	/* byte_cntr_data->enable may race with ctcu_platform_remove() */
> +	if (!byte_cntr_data->enable) {
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		return -ENODEV;
> +	}
> +
> +	/*
> +	 * Pin the module for the whole read session, so that rmmod fails with
> +	 * -EBUSY instead of blocking in ctcu_platform_remove() until the file
> +	 * is closed.
> +	 */
> +	if (!try_module_get(THIS_MODULE)) {
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		return -ENODEV;
> +	}
> +
> +	byte_cntr_data->reading = true;
> +	raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +	/* Setup an available etr_buf_list for byte-cntr */
> +	ret = tmc_create_etr_buf_list(etr_drvdata, 2);
> +	if (ret) {
> +		scoped_guard(raw_spinlock_irqsave, &byte_cntr_data->spin_lock)
> +			byte_cntr_data->reading = false;
> +
> +		module_put(THIS_MODULE);
> +		/* ctcu_platform_remove() may be waiting for reading to be cleared */
> +		wake_up(&byte_cntr_data->wq);
> +		return ret;
> +	}
> +
> +	scoped_guard(raw_spinlock_irqsave, &byte_cntr_data->spin_lock) {
> +		atomic_set(&byte_cntr_data->irq_cnt, 0);
> +		/*
> +		 * Configure the byte-cntr register to enable IRQ. The
> +		 * configured size is 5% of the buffer_size.
> +		 */
> +		ctcu_cfg_byte_cntr_reg(byte_cntr_data->ctcu_drvdata,
> +				       etr_drvdata->size / MAX_IRQ_CNT,
> +				       byte_cntr_data->irq_ctrl_offset);
> +		byte_cntr_data->buf_node = NULL;
> +	}
> +	/* enable_irq_wake() may sleep on slow-bus irqchips, call it unlocked */
> +	enable_irq_wake(byte_cntr_data->irq);
> +
> +	return 0;
> +}
> +
> +static int tmc_read_unprepare_byte_cntr(struct tmc_drvdata *etr_drvdata)
> +{
> +	struct coresight_device *ctcu = tmc_etr_get_ctcu_device(etr_drvdata);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	/*
> +	 * Do the unprepare operation only when the byte_cntr_data->reading
> +	 * is truly set
> +	 */
> +	byte_cntr_data = ctcu_get_byte_cntr(ctcu, etr_drvdata->csdev);
> +	if (!byte_cntr_data || !byte_cntr_data->irq_enabled ||
> +	    !byte_cntr_data->reading)
> +		return -EINVAL;
> +
> +	tmc_clean_etr_buf_list(etr_drvdata);
> +	scoped_guard(raw_spinlock_irqsave, &byte_cntr_data->spin_lock) {
> +		/* Configure the byte-cntr register to disable IRQ */
> +		ctcu_cfg_byte_cntr_reg(byte_cntr_data->ctcu_drvdata, 0,
> +				       byte_cntr_data->irq_ctrl_offset);
> +		byte_cntr_data->buf_node = NULL;
> +		byte_cntr_data->reading = false;
> +	}

[Severity: High]
Does tmc_read_unprepare_byte_cntr() (and tmc_read_prepare_byte_cntr() if
racing with path disable) write to CTCU MMIO registers while drvdata->apb_clk
is disabled?

A runtime PM reference on the CTCU device is only held while a CoreSight path
is active (_coresight_build_path() -> coresight_grab_device(), released in
coresight_release_path() -> coresight_drop_device()). When the user stops
tracing via coresight_disable_sysfs(), coresight_release_path() drops the
CTCU runtime PM reference and ctcu_runtime_suspend() disables
drvdata->apb_clk while /dev/tmc_etrX is still open.

When userspace subsequently closes /dev/tmc_etrX (matching the workflow in
the commit message where tmc_disable_etr_sink and ctcu_disable precede
tmc_read_unprepare_etr), tmc_read_unprepare_byte_cntr() calls
ctcu_cfg_byte_cntr_reg() -> ctcu_program_register() without holding a runtime
PM reference on the CTCU device, performing unclocked MMIO writes to
drvdata->base. The same unclocked access can happen in
tmc_read_prepare_byte_cntr() if the path is disabled while the spinlock is
dropped across tmc_create_etr_buf_list().

> +	/*
> +	 * The threshold IRQ is already disabled by the register write above,
> +	 * so no wake event can arrive here. disable_irq_wake() may sleep on
> +	 * slow-bus irqchips, so call it outside the spin_lock.
> +	 */
> +	disable_irq_wake(byte_cntr_data->irq);
> +	wake_up(&byte_cntr_data->wq);
> +	module_put(THIS_MODULE);
> +
> +	return 0;
> +}
> +
> +const struct tmc_sysfs_ops byte_cntr_sysfs_ops = {
> +	.read_prepare	= tmc_read_prepare_byte_cntr,
> +	.read_unprepare	= tmc_read_unprepare_byte_cntr,
> +	.get_trace_data	= tmc_byte_cntr_get_data,
> +};

[Severity: Low]
Should byte_cntr_sysfs_ops in coresight-ctcu-byte-cntr.c be declared static,
since it is only referenced within this file?

> +
> +/* Start the byte-cntr function when the path is enabled. */
> +void ctcu_byte_cntr_start(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(csdev, sink);
> +	if (!byte_cntr_data)
> +		return;
> +
> +	/* Don't start byte-cntr function when irq_enabled is not set. */
> +	if (!byte_cntr_data->irq_enabled || byte_cntr_data->enable)
> +		return;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	byte_cntr_data->enable = true;
> +}
> +
> +/* Stop the byte-cntr function when the path is disabled. */
> +void ctcu_byte_cntr_stop(struct coresight_device *csdev, struct coresight_path *path)
> +{
> +	struct coresight_device *sink = coresight_get_sink(path);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	if (coresight_get_mode(sink) == CS_MODE_SYSFS)
> +		return;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(csdev, sink);
> +	if (!byte_cntr_data)
> +		return;
> +
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	byte_cntr_data->enable = false;
> +}

[Severity: High]
In ctcu_byte_cntr_stop(), could sink be NULL when passed to
coresight_get_mode(sink) (whereas ctcu_set_etr_traceid() checks sink == NULL
first), and shouldn't ctcu_byte_cntr_stop() also call
wake_up(&byte_cntr_data->wq) after clearing byte_cntr_data->enable so a
reader blocked in wait_event_interruptible_timeout() in
tmc_byte_cntr_get_data() wakes up immediately instead of stalling until
BYTE_CNTR_TIMEOUT expires?

> +
> +void ctcu_byte_cntr_init(struct device *dev, struct ctcu_drvdata *drvdata, int etr_num)
> +{
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	struct device_node *nd = dev->of_node;
> +	int irq_num, ret, i, irq_registered = 0;
> +
> +	for (i = 0; i < etr_num; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		/*
> +		 * Initialize the lock and the waitqueue before requesting the
> +		 * IRQ, and for every port as ctcu_platform_remove() uses them
> +		 * even if the IRQ of the port is not registered.
> +		 */
> +		byte_cntr_data->ctcu_drvdata = drvdata;
> +		init_waitqueue_head(&byte_cntr_data->wq);
> +		raw_spin_lock_init(&byte_cntr_data->spin_lock);
> +
> +		irq_num = of_irq_get(nd, i);
> +		if (irq_num < 0) {
> +			dev_err(dev, "Failed to get IRQ from DT for port%d\n", i);
> +			continue;
> +		}
> +
> +		ret = devm_request_irq(dev, irq_num, byte_cntr_handler,
> +				       IRQF_TRIGGER_RISING,
> +				       dev_name(dev), byte_cntr_data);
> +		if (ret) {
> +			dev_err(dev, "Failed to register IRQ for port%d\n", i);
> +			continue;
> +		}
> +
> +		byte_cntr_data->irq = irq_num;
> +		irq_registered++;
> +	}
> +
> +	if (!irq_registered)
> +		return;
> +
> +	scoped_guard(mutex, &byte_cntr_ops_lock) {
> +		tmc_etr_set_byte_cntr_sysfs_ops(&byte_cntr_sysfs_ops);
> +		byte_cntr_ops_users++;
> +	}
> +
> +	/* Drop the ops user when the CTCU device is removed or probe fails */
> +	if (devm_add_action_or_reset(dev, ctcu_byte_cntr_put_ops, NULL))
> +		dev_err(dev, "Failed to add the byte-cntr devm action\n");
> +}

[Severity: Medium]
In ctcu_byte_cntr_init(), of_irq_get() returns 0 on IRQ mapping failure as
well as negative error codes on lookup failure. Should the check be
if (irq_num <= 0) so that IRQ 0 is not passed to devm_request_irq()?

Also, if devm_add_action_or_reset() fails at the end of
ctcu_byte_cntr_init(), ctcu_byte_cntr_put_ops() resets byte_cntr_sysfs_ops,
but ctcu_byte_cntr_init() returns void without propagating the error or
clearing byte_cntr_data[i].irq, leaving the IRQs and sysfs attributes active
without registered ops.

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index 9043cad42f01e..7738ae4fbf14c 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c

[ ... ]

> @@ -62,6 +68,87 @@ static const struct ctcu_config sa8775p_cfgs = {
>  	.num_etr_config	= ARRAY_SIZE(sa8775p_etr_cfgs),
>  };
>  
> +void ctcu_program_register(struct ctcu_drvdata *drvdata, u32 val, u32 offset)
> +{
> +	CS_UNLOCK(drvdata->base);
> +	ctcu_writel(drvdata, val, offset);
> +	CS_LOCK(drvdata->base);
> +}

[Severity: High]
Can calling ctcu_program_register() under the per-port
byte_cntr_data->spin_lock instead of the device-wide drvdata->spin_lock race
on the shared CORESIGHT_LAR lock register?

In coresight-ctcu-core.c, __ctcu_set_etr_traceid() acquires
drvdata->spin_lock before calling ctcu_program_atid_register() (which also
does CS_UNLOCK(drvdata->base) and CS_LOCK(drvdata->base)). However,
tmc_read_prepare_byte_cntr() and tmc_read_unprepare_byte_cntr() call
ctcu_cfg_byte_cntr_reg() -> ctcu_program_register() while holding only
byte_cntr_data->spin_lock.

If ctcu_program_register() runs concurrently with __ctcu_set_etr_traceid() or
with ctcu_program_register() on the other ETR port, one thread can execute
CS_LOCK(drvdata->base) right after the other thread's
CS_UNLOCK(drvdata->base) and before its ctcu_writel(), causing the write to
IRQCTRL or ATID to be ignored by hardware.

[ ... ]

> @@ -157,11 +227,15 @@ static int ctcu_set_etr_traceid(struct coresight_device *csdev, struct coresight
>  static int ctcu_enable(struct coresight_device *csdev, enum cs_mode mode,
>  		       struct coresight_path *path)
>  {
> +	ctcu_byte_cntr_start(csdev, path);
> +
>  	return ctcu_set_etr_traceid(csdev, path, true);
>  }

[Severity: Medium]
If ctcu_set_etr_traceid() fails in ctcu_enable(), does byte_cntr_data->enable
remain leaked as true?

When a helper's enable callback returns an error in
coresight_enable_helpers(), the core only calls coresight_disable_helper() on
the previously enabled helpers in the path, so ctcu_disable() ->
ctcu_byte_cntr_stop() is not called for the failing CTCU device.

[Severity: High]
Furthermore, in ctcu_enable() and ctcu_byte_cntr_start(), what prevents
byte_cntr_data->enable from being set during CS_MODE_PERF sessions or racing
with irq_enabled_store() and ctcu_platform_remove()?

1. ctcu_enable() calls ctcu_byte_cntr_start(csdev, path) without checking
   whether mode == CS_MODE_SYSFS, so starting a perf session while
   irq_enabled is set will set byte_cntr_data->enable = true.

2. ctcu_byte_cntr_start() checks !byte_cntr_data->irq_enabled and
   byte_cntr_data->enable outside byte_cntr_data->spin_lock before acquiring
   the lock to set byte_cntr_data->enable = true, which can race with
   irq_enabled_store() or ctcu_platform_remove().

[ ... ]

> @@ -261,10 +339,42 @@ static int ctcu_platform_probe(struct platform_device *pdev)
>  static void ctcu_platform_remove(struct platform_device *pdev)
>  {
>  	struct ctcu_drvdata *drvdata = platform_get_drvdata(pdev);
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +	unsigned long flags;
> +	int i;
>  
>  	if (WARN_ON(!drvdata))
>  		return;
>  
> +	/*
> +	 * Signal all active byte-cntr readers to exit, then wait for them to
> +	 * finish before resetting the ops pointer and freeing driver data.
> +	 * Without this, a reader blocked in wait_event_interruptible_timeout()
> +	 * would access the freed ctcu_drvdata wait-queue head (use-after-free).
> +	 *
> +	 * The wait ends only when the userspace closes the tmc_etr file, so an
> +	 * unbind with the file left open blocks until then. Unloading the module
> +	 * is rejected with -EBUSY while a read session is open, so only a manual
> +	 * unbind can block here.
> +	 */
> +	for (i = 0; i < ETR_MAX_NUM; i++) {
> +		byte_cntr_data = &drvdata->byte_cntr_data[i];
> +		/* The port was not initialized by ctcu_byte_cntr_init() */
> +		if (!byte_cntr_data->irq)
> +			continue;
> +
> +		raw_spin_lock_irqsave(&byte_cntr_data->spin_lock, flags);
> +		/* Set enable=false for all ports to signal teardown to racing readers */
> +		byte_cntr_data->enable = false;
> +		if (!byte_cntr_data->reading) {
> +			raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +			continue;
> +		}
> +		raw_spin_unlock_irqrestore(&byte_cntr_data->spin_lock, flags);
> +		wake_up_all(&byte_cntr_data->wq);
> +		wait_event(byte_cntr_data->wq, !byte_cntr_data->reading);
> +	}
> +
>  	ctcu_remove(pdev);
>  	pm_runtime_disable(&pdev->dev);
>  }

[Severity: High]
As reported by Sashiko on v19 [3], can ctcu_platform_remove() still trigger a
use-after-free or hang indefinitely on device unbind?

1. In tmc_read_unprepare_byte_cntr(), byte_cntr_data->reading = false is set
   inside the spinlock, and the lock is released before calling
   disable_irq_wake(byte_cntr_data->irq) and wake_up(&byte_cntr_data->wq)
   (and similarly before wake_up(&byte_cntr_data->wq) in the error path of
   tmc_read_prepare_byte_cntr()). As soon as byte_cntr_data->reading becomes
   false, ctcu_platform_remove() can finish and devres can free drvdata while
   tmc_read_unprepare_byte_cntr() or tmc_read_prepare_byte_cntr() is still
   accessing byte_cntr_data->irq and byte_cntr_data->wq.

2. Because this teardown loop runs before ctcu_remove(pdev) unregisters the
   CoreSight helper device, a concurrent coresight_enable_sysfs() ->
   ctcu_enable() -> ctcu_byte_cntr_start() can set byte_cntr_data->enable =
   true again and start a new read session on a port that the loop has
   already checked.

3. Waiting in wait_event(byte_cntr_data->wq, !byte_cntr_data->reading) puts
   the unbind task into TASK_UNINTERRUPTIBLE until userspace closes
   /dev/tmc_etrX, which will trigger hung-task warnings if a process keeps
   the file descriptor open.

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu.h b/drivers/hwtracing/coresight/coresight-ctcu.h
> index e9594c38dd91c..a2ae0a0d91d0f 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu.h
> +++ b/drivers/hwtracing/coresight/coresight-ctcu.h
> @@ -1,23 +1,31 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  /*
> - * Copyright (c) 2024-2025 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2024-2026 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>   */
>  
>  #ifndef _CORESIGHT_CTCU_H
>  #define _CORESIGHT_CTCU_H
> +
> +#include <linux/time.h>
>  #include "coresight-trace-id.h"
>  
>  /* Maximum number of supported ETR devices for a single CTCU. */
>  #define ETR_MAX_NUM	2
>  
> +#define BYTE_CNTR_TIMEOUT	(3 * HZ)
> +#define MAX_IRQ_CNT		20
> +
>  /**
>   * struct ctcu_etr_config
>   * @atid_offset:	offset to the ATID0 Register.
> - * @port_num:		in-port number of CTCU device that connected to ETR.
> + * @port_num:		in-port number of the CTCU device that connected to ETR.
> + * @irq_ctrl_offset:    offset to the BYTECNTRVAL register.
>   */
>  struct ctcu_etr_config {
>  	const u32 atid_offset;
>  	const u32 port_num;
> +	const u32 irq_ctrl_offset;
>  };
>  
>  struct ctcu_config {
> @@ -25,15 +33,68 @@ struct ctcu_config {
>  	int num_etr_config;
>  };
>  
> -struct ctcu_drvdata {
> -	void __iomem		*base;
> -	struct clk		*apb_clk;
> -	struct device		*dev;
> -	struct coresight_device	*csdev;
> +/**
> + * struct ctcu_byte_cntr
> + * @enable:		indicates that byte_cntr function is enabled or not.

[ ... ]

> +/**
> + * struct ctcu_byte_cntr_irq_attribute
> + * @attr:	The device attribute.
> + * @port:	port number.
> + */

[Severity: Low]
This isn't a bug, but the kernel-doc comments for struct ctcu_etr_config,
struct ctcu_byte_cntr, and struct ctcu_byte_cntr_irq_attribute in
coresight-ctcu.h are missing the "- short description" on the struct line,
and struct etr_buf_node in coresight-tmc.h is missing the
"struct etr_buf_node - short description" line, which triggers kernel-doc
warnings under W=1 builds.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 4b40b692be4d8..6ad09995ba87c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -293,7 +293,8 @@ static ssize_t tmc_read(struct file *file, char __user *data, size_t len,
>  		return -EFAULT;
>  	}
>  
> -	*ppos += actual;
> +	if (!tmc_etr_update_buf_node_pos(drvdata, actual))
> +		*ppos += actual;
>  	dev_dbg(&drvdata->csdev->dev, "%zu bytes copied\n", actual);
>  
>  	return actual;

[Severity: Medium]
As reported by Sashiko on v19 [4], doesn't tmc_read() still swallow negative
error codes such as -ERESTARTSYS and -ENOMEM returned by
tmc_etr_get_sysfs_trace() and convert them into 0 (EOF)?

In tmc_etr_get_sysfs_trace(), any return value from
byte_cntr_ops->get_trace_data() other than -EINVAL (including -ERESTARTSYS on
signal interruption and -ENOMEM on buffer switch failure) is returned to
tmc_get_sysfs_trace(). However, tmc_read() has:

	actual = tmc_get_sysfs_trace(drvdata, *ppos, len, &bufp);
	if (actual <= 0)
		return 0;

So any signal delivered to a process waiting in tmc_byte_cntr_get_data()
causes read() to return 0 (EOF) instead of -ERESTARTSYS.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 7d41fdf83c5bb..253fe8eb6cbe1 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c

[ ... ]

> @@ -2076,15 +2142,54 @@ int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes)

[ ... ]

>  int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
>  {
>  	int ret = 0;
>  	unsigned long flags;
> +	const struct tmc_sysfs_ops *byte_cntr_ops;
>  
>  	/* config types are set a boot time and never change */
>  	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
>  		return -EINVAL;
>  
> +	byte_cntr_ops = READ_ONCE(byte_cntr_sysfs_ops);
> +	if (byte_cntr_ops) {
> +		ret = byte_cntr_ops->read_prepare(drvdata);
> +		if (!ret || ret == -EBUSY)
> +			return ret;
> +
> +		ret = 0;
> +	}

[Severity: High]
As reported by Sashiko on v19 [5], why does tmc_read_prepare_etr() ignore
errors like -ENOMEM and -ENODEV from byte_cntr_ops->read_prepare(drvdata) and
fall through to the generic ETR read path?

Unlike tmc_etr_get_sysfs_trace(), which only falls back when ret == -EINVAL,
tmc_read_prepare_etr() masks -ENOMEM (when tmc_create_etr_buf_list() fails)
and -ENODEV (when !byte_cntr_data->enable or try_module_get() fails) by
resetting ret = 0 and continuing into the generic ETR read path. During an
active CS_MODE_SYSFS session, falling through calls __tmc_etr_disable_hw(),
which halts the running ETR hardware capture and sets drvdata->reading = true
while byte_cntr_data->reading remains false.

> +
>  	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
>  	if (drvdata->reading) {
>  		ret = -EBUSY;
> @@ -2116,11 +2221,17 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
>  {
>  	unsigned long flags;
>  	struct etr_buf *sysfs_buf = NULL;
> +	const struct tmc_sysfs_ops *byte_cntr_ops;
>  
>  	/* config types are set a boot time and never change */
>  	if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
>  		return -EINVAL;
>  
> +	byte_cntr_ops = READ_ONCE(byte_cntr_sysfs_ops);
> +	if (byte_cntr_ops)
> +		if (!byte_cntr_ops->read_unprepare(drvdata))
> +			return 0;

[Severity: High]
As reported by Sashiko on v19 [6], can reading byte_cntr_sysfs_ops locklessly
in tmc_read_prepare_etr(), tmc_etr_get_sysfs_trace(), and
tmc_read_unprepare_etr() still race with unloading the coresight-ctcu module?

CONFIG_CORESIGHT_LINK_AND_SINK_TMC and CONFIG_CORESIGHT_CTCU can be built as
separate modules. Although tmc_read_prepare_byte_cntr() calls
try_module_get(THIS_MODULE), that happens only after tmc_read_prepare_etr()
has already loaded byte_cntr_ops, dereferenced byte_cntr_ops->read_prepare,
and entered coresight-ctcu code.

Furthermore, for normal (non-byte-cntr) ETR sysfs read sessions where
tmc_read_prepare_byte_cntr() returns -EINVAL without pinning THIS_MODULE,
every subsequent tmc_etr_get_sysfs_trace() and tmc_read_unprepare_etr() call
still loads and dereferences byte_cntr_sysfs_ops and calls into
coresight-ctcu while its module refcount is 0. If rmmod coresight_ctcu runs
concurrently, byte_cntr_sysfs_ops and the module text can be unmapped after
READ_ONCE(byte_cntr_sysfs_ops) and before or during the callback execution.

[1] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[2] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[3] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[4] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[5] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org
[6] https://lore.kernel.org/r/20260625110924.BA8EA1F000E9@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-enable-byte-cntr-for-ctcu-v20-0-bd5c3ac9f916@oss.qualcomm.com?part=6