Re: [PATCH v21 8/8] arm64: dts: qcom: lemans: add interrupts to CTCU device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Medium] coresight: tmc: negative error codes converted to EOF in tmc_read()
- [Medium] coresight: tmc-etr: unhandled errors in tmc_read_prepare_etr()
- [Medium] coresight: tmc-etr: buffer mismatch and data loss in tmc_create_etr_buf_list()
- [Medium] coresight: ctcu: active state not checked in irq_enabled_store()
- [Medium] coresight: tmc-etr: unsynced buffer read in tmc_etr_get_sysfs_trace()
- [Medium] coresight: ctcu: active session not checked in ctcu_byte_cntr_start()
--

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

arm64: dts: qcom: lemans: add interrupts to CTCU device

This series enables the CTCU byte-cntr function for TMC ETR devices,
allowing userspace to read trace data from switched ETR buffers while
tracing continues, and wires up the CTCU interrupts on Lemans.

> 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..bff87e776a755
> --- /dev/null
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-byte-cntr.c
[ ... ]
> +/* 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 tmc_drvdata *etr_drvdata;
> +	struct ctcu_byte_cntr *byte_cntr_data;
> +
> +	byte_cntr_data = ctcu_get_byte_cntr(csdev, sink);
> +	if (!byte_cntr_data)
> +		return;
> +
> +	etr_drvdata = dev_get_drvdata(sink->dev.parent);
> +	guard(raw_spinlock_irqsave)(&byte_cntr_data->spin_lock);
> +	/*
> +	 * Don't start byte-cntr function when irq_enabled is not set, or when
> +	 * the CTCU device is being removed.
> +	 */
> +	if (!byte_cntr_data->irq_enabled || byte_cntr_data->removing)
> +		return;
> +
> +	/*
> +	 * A new trace starts with an empty etr_buf. Don't reset the count if
> +	 * the byte-cntr is already started by another path of the same sink.
> +	 */
> +	if (!byte_cntr_data->enable)
> +		atomic_set(&byte_cntr_data->irq_cnt, 0);
> +
> +	/*
> +	 * Configure the byte-cntr register to enable IRQ while the path is
> +	 * enabled, the CTCU is powered then. 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->enable = true;
> +}

[Severity: Medium]
What happens in ctcu_byte_cntr_start() if a previous byte-cntr read session
still has the ETR device open to drain the final stopped buffer when a new
sysfs trace path is enabled?

In coresight_enable_path_from_to(), helpers are enabled before the sink:

  coresight_enable_path_from_to()
    coresight_enable_helpers()
      ctcu_enable()
        ctcu_byte_cntr_start()
    coresight_enable_sink()
      tmc_enable_etr_sink_sysfs()
        tmc_etr_get_sysfs_buffer() -> returns -EBUSY

Because ctcu_byte_cntr_start() does not check byte_cntr_data->reading, it
sets byte_cntr_data->enable = true, resets byte_cntr_data->irq_cnt to 0,
and reprograms the IRQCTRL register before tmc_etr_get_sysfs_buffer()
rejects the new path with -EBUSY (potentially after sleeping in
tmc_etr_setup_sysfs_buf()). During that window, a read by the existing
reader in tmc_byte_cntr_get_data() will see byte_cntr_data->enable == true
and block in wait_event_interruptible_timeout() instead of returning 0 to
drain the final buffer.

Also, when a second path is enabled on an already-active ETR sink where
byte_cntr_data->enable is already true, ctcu_byte_cntr_start()
unconditionally reprograms the IRQCTRL register using etr_drvdata->size /
MAX_IRQ_CNT even if buffer_size was modified in sysfs after the active
sysfs_buf was allocated. Should ctcu_byte_cntr_start() bail out when
byte_cntr_data->reading or byte_cntr_data->enable is already set?

> diff --git a/drivers/hwtracing/coresight/coresight-ctcu-core.c b/drivers/hwtracing/coresight/coresight-ctcu-core.c
> index 9043cad42f01e..cce4e4d76da0d 100644
> --- a/drivers/hwtracing/coresight/coresight-ctcu-core.c
> +++ b/drivers/hwtracing/coresight/coresight-ctcu-core.c
[ ... ]
> +static ssize_t irq_enabled_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf,
> +				 size_t size)
> +{
> +	struct ctcu_byte_cntr_irq_attribute *irq_attr =
> +		container_of(attr, struct ctcu_byte_cntr_irq_attribute, attr);
> +	struct ctcu_drvdata *drvdata = dev_get_drvdata(dev->parent);
> +	u8 port = irq_attr->port;
> +	unsigned long val;
> +
> +	if (kstrtoul(buf, 0, &val))
> +		return -EINVAL;
> +
> +	guard(raw_spinlock_irqsave)(&drvdata->byte_cntr_data[port].spin_lock);
> +	if (drvdata->byte_cntr_data[port].reading)
> +		return -EBUSY;
> +	else if (drvdata->byte_cntr_data[port].irq_ctrl_offset)
> +		drvdata->byte_cntr_data[port].irq_enabled = !!val;
> +
> +	return size;
> +}

[Severity: Medium]
Can toggling irq_enabled in irq_enabled_store() while a trace path is
already active (before /dev/tmc_etr* is opened) leave software state out of
sync with the CTCU hardware?

irq_enabled_store() only checks drvdata->byte_cntr_data[port].reading, not
drvdata->byte_cntr_data[port].enable or whether a trace path is active on
that port.

If irq_enabled is changed from 1 to 0 after ctcu_byte_cntr_start() has
already enabled hardware interrupts and set byte_cntr_data->enable = true,
opening the ETR device causes tmc_read_prepare_byte_cntr() to return
-EINVAL, and tmc_read_prepare_etr() falls back to generic sysfs read
preparation (stopping the ETR hardware and setting drvdata->reading = true)
while CTCU interrupts and byte_cntr_data->enable remain active. Conversely,
if irq_enabled is changed from 0 to 1 after the path was enabled without
starting byte-cntr, tmc_read_prepare_byte_cntr() sees irq_enabled == true
and enable == false, returns -ENODEV, and tmc_read_prepare_etr() similarly
falls through to stopping the ETR hardware.

Should irq_enabled_store() also reject updates when
drvdata->byte_cntr_data[port].enable is true or when the port has active
trace IDs?

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index bc5a133ada3e3..6ad09995ba87c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
[ ... ]
> @@ -287,15 +273,7 @@ static int tmc_open(struct inode *inode, struct file *file)
>  static ssize_t tmc_get_sysfs_trace(struct tmc_drvdata *drvdata, loff_t pos, size_t len,
>  				   char **bufpp)
>  {
> -	switch (drvdata->config_type) {
> -	case TMC_CONFIG_TYPE_ETB:
> -	case TMC_CONFIG_TYPE_ETF:
> -		return tmc_etb_get_sysfs_trace(drvdata, pos, len, bufpp);
> -	case TMC_CONFIG_TYPE_ETR:
> -		return tmc_etr_get_sysfs_trace(drvdata, pos, len, bufpp);
> -	}
> -
> -	return -EINVAL;
> +	return drvdata->sysfs_ops->get_trace_data(drvdata, pos, len, bufpp);
>  }
>  
>  static ssize_t tmc_read(struct file *file, char __user *data, size_t len,
> @@ -315,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]
Doesn't tmc_read() in drivers/hwtracing/coresight/coresight-tmc-core.c turn
negative error codes from byte-cntr reads into 0 (EOF)?

In tmc_byte_cntr_get_data(), wait_event_interruptible_timeout() returns
-ERESTARTSYS when interrupted by a signal, and a failed buffer switch while
the sink is still in CS_MODE_SYSFS returns -ENOMEM. Then
tmc_etr_get_sysfs_trace() propagates those negative error codes:

  if (ret != -EINVAL && ret != 0)
          return ret;

However, tmc_read() does:

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

As a result, -ERESTARTSYS and -ENOMEM are returned to userspace as 0,
causing trace readers to treat a signal interruption or buffer switch error
as end-of-file. Should tmc_read() return actual when actual <= 0?

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 361a433e6f0c5..20d6bc60a446e 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
[ ... ]
> +ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
> +				loff_t pos, size_t len, char **bufpp)
> +{
> +	ssize_t ret;
> +	const struct tmc_sysfs_ops *byte_cntr_ops = READ_ONCE(byte_cntr_sysfs_ops);
> +
> +	if (byte_cntr_ops) {
> +		ret = byte_cntr_ops->get_trace_data(drvdata, pos, len, bufpp);
> +		/*
> +		 * -EINVAL means byte-cntr has no active read session. 0 means
> +		 * byte-cntr has no more data because the sink has stopped, the
> +		 * last etr_buf is read by the generic read. Anything else is
> +		 * the result of the byte-cntr session.
> +		 */
> +		if (ret != -EINVAL && ret != 0)
> +			return ret;
> +	}
> +
> +	return tmc_etr_read_sysfs_buf(drvdata->sysfs_buf, pos, len, bufpp);
>  }
>  
>  static struct etr_buf *
> @@ -1224,6 +1272,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);
> +}
> +
> +/**
> + * 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)
> +			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");
> +out:
> +	raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +	return curr_node;
> +}

[Severity: Medium]
Could tmc_etr_get_sysfs_trace() end up reading an unsynced, live DMA buffer
if ctcu_platform_remove() runs while the ETR sink is still active in
CS_MODE_SYSFS?

In ctcu_platform_remove(), byte_cntr_data->enable is set to false and
byte_cntr_data->wq is woken while the ETR sink can still be running in
CS_MODE_SYSFS. In tmc_byte_cntr_get_data(), seeing !byte_cntr_data->enable
causes it to return 0:

  ctcu_platform_remove()
    byte_cntr_data->enable = false;
    wake_up_all(&byte_cntr_data->wq);

  tmc_etr_get_sysfs_trace()
    tmc_byte_cntr_get_data() -> returns 0
    tmc_etr_read_sysfs_buf(drvdata->sysfs_buf, pos, len, bufpp)

tmc_etr_get_sysfs_trace() treats 0 as meaning the sink has stopped and
immediately calls tmc_etr_read_sysfs_buf(drvdata->sysfs_buf, ...). However,
if at least one buffer switch has already happened in
tmc_etr_switch_buf_node(), picked_node->sysfs_buf->len was never reset to 0
when switching to it, so drvdata->sysfs_buf->len still holds the non-zero
length from its previous fill while the ETR hardware is actively writing to
that DMA buffer without having been stopped or synced.

Should tmc_etr_switch_buf_node() reset picked_node->sysfs_buf->len (and
offset) to 0 before restarting the ETR, and should tmc_byte_cntr_get_data()
return an error (such as -ENODEV) instead of 0 when byte_cntr_data->removing
is set or when the sink is still in CS_MODE_SYSFS?

[ ... ]
> +void tmc_clean_etr_buf_list(struct tmc_drvdata *drvdata)
> +{
> +	struct etr_buf_node *nd, *next;
> +	unsigned long flags;
> +
> +	list_for_each_entry_safe(nd, next, &drvdata->etr_buf_list, link) {
> +		raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +		if (nd->sysfs_buf == drvdata->sysfs_buf) {
> +			if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
> +				/*
> +				 * The sink is still enabled in sysfs mode. Keep
> +				 * the live buffer owned by drvdata->sysfs_buf
> +				 * and only drop the list's reference to it.
> +				 */
> +				nd->sysfs_buf = NULL;
> +			else
> +				/* Free the buffer below through nd->sysfs_buf */
> +				drvdata->sysfs_buf = NULL;
> +		}
> +		list_del(&nd->link);
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +		/* Free the buffer (NULL is ignored) and the node out of the lock */
> +		tmc_etr_free_sysfs_buf(nd->sysfs_buf);
> +		kfree(nd);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(tmc_clean_etr_buf_list);
> +
> +/**
> + * tmc_create_etr_buf_list - create a list to manage the etr_buf_node.
> + * @drvdata:	driver data of the TMC device.
> + * @num_nodes:	number of nodes want to create with the list.
> + *
> + * Locking: must not be called with @drvdata->spinlock held. The spinlock is
> + * taken internally to serialise the @drvdata->sysfs_buf accesses and every
> + * update of @drvdata->etr_buf_list against the ETR sink enable/disable paths
> + * and the generic sysfs read. There must be only one owner of the list at a
> + * time. Must be called from process context: buffers and nodes are allocated
> + * with the lock released.
> + *
> + * Return: 0 upon success, else the error number: -EBUSY if a generic sysfs
> + * read is in progress, -EINVAL if the sink is not enabled in sysfs mode or
> + * the ETR is in the reserved buffer mode, or the error of the allocation.
> + */
> +int tmc_create_etr_buf_list(struct tmc_drvdata *drvdata, int num_nodes)
> +{
> +	struct etr_buf_node *new_node;
> +	struct etr_buf *sysfs_buf;
> +	unsigned long flags;
> +	int i = 0, ret = 0;
> +
> +	/* We don't need a list if there is only one node */
> +	if (num_nodes < 2)
> +		return -EINVAL;
> +
> +	/*
> +	 * All the etr_buf of the reserved mode are mapped to the same reserved
> +	 * region, so they can't be used as separate buffers of the list.
> +	 */
> +	if (drvdata->etr_mode == ETR_MODE_RESRV)
> +		return -EINVAL;
> +
> +	/*
> +	 * We expect that sysfs_buf in drvdata has already been allocated.
> +	 * Wrap the live sysfs_buf into the first node so the captured trace
> +	 * data is preserved. Take the sysfs_buf and add the node under the
> +	 * lock, so that the ETR sink enable path sees a non-empty list and
> +	 * doesn't replace the buffer.
> +	 */
> +	new_node = kzalloc_obj(*new_node, GFP_KERNEL);
> +	if (!new_node)
> +		return -ENOMEM;
> +
> +	raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +	/* The sysfs_buf is in use by a sysfs read session */
> +	if (drvdata->reading) {
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		kfree(new_node);
> +		return -EBUSY;
> +	}
> +
> +	/*
> +	 * The sink enable path reuses or frees the sysfs_buf of a sink which
> +	 * isn't enabled, so only wrap it while the sink is enabled in sysfs mode.
> +	 */
> +	if (coresight_get_mode(drvdata->csdev) != CS_MODE_SYSFS) {
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		kfree(new_node);
> +		return -EINVAL;
> +	}
> +
> +	if (drvdata->sysfs_buf) {
> +		new_node->sysfs_buf = drvdata->sysfs_buf;
> +		new_node->is_free = false;
> +		list_add(&new_node->link, &drvdata->etr_buf_list);
> +		new_node = NULL;
> +		i++;
> +	}
> +	raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +	/* Free the node if there is no live sysfs_buf to wrap */
> +	kfree(new_node);
> +
> +	while (i < num_nodes) {
> +		new_node = kzalloc_obj(*new_node, GFP_KERNEL);
> +		if (!new_node) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		/* Allocate the buffer with the lock released */
> +		sysfs_buf = tmc_alloc_etr_buf(drvdata, drvdata->size, 0, cpu_to_node(0), NULL);
> +		if (IS_ERR(sysfs_buf)) {
> +			kfree(new_node);
> +			ret = PTR_ERR(sysfs_buf);
> +			break;
> +		}
> +
> +		new_node->sysfs_buf = sysfs_buf;
> +		/*
> +		 * The drvdata->sysfs_buf write and the list update need the
> +		 * spinlock, to serialise against the ETR sink enable/disable
> +		 * paths.
> +		 */
> +		raw_spin_lock_irqsave(&drvdata->spinlock, flags);
> +		/* We don't have an available sysfs_buf in drvdata, set one up */
> +		if (!drvdata->sysfs_buf) {
> +			drvdata->sysfs_buf = sysfs_buf;
> +			new_node->is_free = false;
> +		} else {
> +			new_node->is_free = true;
> +		}
> +		list_add_tail(&new_node->link, &drvdata->etr_buf_list);
> +		raw_spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +		i++;
> +	}
> +
> +	/* Clean the list if there is an error */
> +	if (ret)
> +		tmc_clean_etr_buf_list(drvdata);
> +
> +	return ret;
> +}

[Severity: Medium]
Can tmc_create_etr_buf_list() free the pre-existing drvdata->sysfs_buf on
error, or allocate mismatched buffers if sysfs settings changed after the
sink was enabled?

First, tmc_create_etr_buf_list() wraps the live drvdata->sysfs_buf into the
first node and then drops drvdata->spinlock while allocating the second
buffer. If a concurrent tmc_disable_etr_sink() stops the sink during that
window (changing the mode to CS_MODE_DISABLED) and the second allocation
then fails, the error path calls tmc_clean_etr_buf_list(). Because
coresight_get_mode(drvdata->csdev) is no longer CS_MODE_SYSFS,
tmc_clean_etr_buf_list() sets drvdata->sysfs_buf = NULL and frees the
pre-existing drvdata->sysfs_buf, losing the captured trace data on a failed
open().

Second, tmc_create_etr_buf_list() checks drvdata->etr_mode == ETR_MODE_RESRV
without holding drvdata->spinlock and allocates subsequent buffers using
drvdata->size rather than the wrapped drvdata->sysfs_buf->mode and
drvdata->sysfs_buf->size. If buf_mode_preferred or buffer_size was modified
in sysfs after the sink was enabled, tmc_create_etr_buf_list() can wrap an
existing ETR_MODE_RESRV buffer or allocate a second buffer whose size does
not match the first buffer and the programmed CTCU IRQ threshold.

[ ... ]
>  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: Medium]
In tmc_read_prepare_etr(), what happens when byte_cntr_ops->read_prepare()
fails with an error other than -EBUSY?

When byte-cntr is active on the trace path, tmc_read_prepare_byte_cntr() can
return -ENOMEM (if tmc_create_etr_buf_list() fails to allocate a node or
DMA buffer), -EINVAL (if tmc_create_etr_buf_list() rejects ETR_MODE_RESRV),
or -ENODEV (if try_module_get() fails or ctcu_platform_remove() cleared
byte_cntr_data->enable).

Because tmc_read_prepare_etr() only returns when !ret || ret == -EBUSY and
otherwise resets ret = 0, any of those failures falls through to the generic
ETR read preparation path. When the sink is active in CS_MODE_SYSFS, that
fallback calls __tmc_etr_disable_hw(drvdata), stopping the ETR hardware in
the middle of an active byte-cntr trace session, and sets drvdata->reading =
true. If the trace path is later disabled while the file remains open,
tmc_disable_etr_sink() returns -EBUSY (leaving the sink mode as
CS_MODE_SYSFS), which causes ctcu_byte_cntr_stop() to return early without
clearing byte_cntr_data->enable or disabling the CTCU IRQCTRL register
before coresight_release_path() drops the CTCU runtime PM reference.

Should tmc_read_prepare_byte_cntr() use a distinct return code (or a boolean
out-parameter) when byte-cntr is not applicable, so that
tmc_read_prepare_etr() propagates real errors instead of falling back to
generic sysfs read preparation?

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