Re: [PATCH v15 5/7] qcom-tgu: Add support to configure next action

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:

New issues:
- [High] Modifying `attr->name` dynamically inside the `is_visible` callback permanently mutates a statically allocated and globally shared sysfs attribute.
- [Low] The dynamically renamed `default` sysfs node is missing from the ABI documentation.

Pre-existing issues:
- [High] Sysfs groups are exposed to userspace before internal data structures are fully allocated, creating a race window for a NULL pointer dereference panic.
- [High] Missing cleanup of sysfs groups on probe error paths leads to orphaned interfaces and Use-After-Free.
- [High] TOCTOU race condition in `enable_tgu_store` causes `pm_runtime_put` underflow.
- [High] Runtime PM usage counter leak on module removal.
- [Medium] Uninitialized variable `index` is used in the return statement when hardware configuration loops are skipped.
- [Medium] `tgu_dataset_show` accesses array elements without holding `drvdata->lock`, leading to data races.
--

commit 321ab56e90d93dd3077807f4f183c67363243eb4
Author: Songwei Chai <songwei.chai@oss.qualcomm.com>

qcom-tgu: Add support to configure next action

This commit adds support to configure the next action for each step by
introducing a select node. It allows configuring triggers, counters, and timers.

> diff --git a/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu b/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
> --- a/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
> +++ b/Documentation/ABI/testing/sysfs-bus-amba-devices-tgu
[ ... ]
> @@ -21,3 +21,10 @@ KernelVersion:	7.2
>  Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Songwei Chai <songwei.chai@oss.qualcomm.com>
>  Description:
>  		(RW) Set/Get the decode mode with specific step for TGU.
> +
> +What:		/sys/bus/amba/devices/<tgu-name>/step[0:7]_condition_select/reg[0:3]
> +Date:		July 2026
> +KernelVersion:	7.2
> +Contact:	Jinlong Mao <jinlong.mao@oss.qualcomm.com>, Songwei Chai <songwei.chai@oss.qualcomm.com>
> +Description:
> +		(RW) Set/Get the next action with specific step for TGU.

[Severity: Low]
Is the default node missing from the ABI documentation?

The logic in tgu_node_visible() renames the final register in the
condition_select region to default, making it available to userspace, but it
does not appear to be documented here.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> --- a/drivers/hwtracing/qcom/tgu.c
> +++ b/drivers/hwtracing/qcom/tgu.c
[ ... ]
> @@ -71,6 +74,9 @@ static ssize_t tgu_dataset_show(struct device *dev,
>  	case TGU_CONDITION_DECODE:
>  		return sysfs_emit(buf, "0x%x\n",
>  				drvdata->value_table->condition_decode[index]);
> +	case TGU_CONDITION_SELECT:
> +		return sysfs_emit(buf, "0x%x\n",
> +				drvdata->value_table->condition_select[index]);

[Severity: Medium]
This is a pre-existing issue, but does tgu_dataset_show() need to hold
drvdata->lock when reading these arrays?

Arrays like condition_select are zeroed and updated under the lock in
reset_tgu_store() and tgu_dataset_store(), so reading them locklessly here
might lead to torn reads or intermediate states being observed.

[ ... ]
> @@ -146,6 +156,13 @@ static umode_t tgu_node_visible(struct kobject *kobject,
>  		if (tgu_attr->reg_num < drvdata->num_condition_decode)
>  			return attr->mode;
>  		break;
> +	case TGU_CONDITION_SELECT:
> +		/* 'default' register is at the end of 'select' region */
> +		if (tgu_attr->reg_num == drvdata->num_condition_select - 1)
> +			attr->name = "default";

[Severity: High]
Does dynamically renaming attr->name here permanently mutate the globally
shared static sysfs attribute?

The struct attribute instances are defined via statically allocated compound
literals at the file scope. Modifying attr->name here changes it for all TGU
instances.

If a system has multiple TGU devices with varying num_condition_select
values, or if the driver is unbound and rebound, the name remains default.
Could this cause a sysfs collision (-EEXIST) when registering subsequent
devices?

[ ... ]
> @@ -184,6 +201,18 @@ static ssize_t tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>  				drvdata->base + CONDITION_DECODE_STEP(i, j));
>  		}
>  	}
> +
> +	for (i = 0; i < drvdata->num_step; i++) {
> +		for (j = 0; j < drvdata->num_condition_select; j++) {
> +			index = check_array_location(drvdata, i,
> +						TGU_CONDITION_SELECT, j);
> +			if (index == -EINVAL)
> +				goto exit;
> +
> +			writel(drvdata->value_table->condition_select[index],
> +				drvdata->base + CONDITION_SELECT_STEP(i, j));
> +		}
> +	}

[Severity: Medium]
This is a pre-existing issue, but is the index variable uninitialized if all
the num_step loops in tgu_write_all_hw_regs() are bypassed?

If drvdata->num_step is 0, the function jumps to the exit label and returns
index >= 0 ? 0 : -EINVAL. This evaluates uninitialized stack garbage and
could result in a non-deterministic return value.

[ ... ]
> @@ -425,6 +464,16 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
[ ... ]
>  	drvdata->value_table->condition_decode = condition;
>  
> +	select_size = drvdata->num_condition_select * drvdata->num_step;
> +
> +	select = devm_kcalloc(dev, select_size,
> +			     sizeof(*(drvdata->value_table->condition_select)),
> +			     GFP_KERNEL);
> +	if (!select)
> +		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does sysfs_create_groups() happen earlier
in tgu_probe() before these internal structures are allocated?

If sysfs nodes are exposed to userspace before memory like condition_select
is fully allocated, could an early concurrent sysfs read or write trigger a
NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but if devm_kcalloc() fails and we return
-ENOMEM here, are the sysfs groups orphaned?

Because sysfs_create_groups() is used instead of a devres managed
alternative, failing probe here without calling sysfs_remove_groups() could
leave sysfs files active while freeing the underlying driver memory,
potentially causing a Use-After-Free if accessed later.


Additionally, there are a couple of pre-existing regressions in the driver
PM handling:

[Severity: High]
This is a pre-existing issue, but is there a race condition in
enable_tgu_store() that causes a pm_runtime_put() underflow?

If two threads concurrently write "0" to enable_tgu, they sequentially pass
the lock and both observe drvdata->enabled == true:

scoped_guard(spinlock, &drvdata->lock) {
    if (!drvdata->enabled)
        return -EINVAL;
}

Both threads then proceed locklessly to call tgu_disable(dev) and
pm_runtime_put(dev). Since there was only one matching
pm_runtime_resume_and_get(), this would result in a PM usage counter
underflow.

[Severity: High]
This is a pre-existing issue, but does tgu_remove() neglect to release the
runtime PM reference if the device is currently enabled?

tgu_remove() calls tgu_disable(dev) to stop the hardware, but does not seem
to call pm_runtime_put() to balance the PM reference acquired during
enablement, which might leak the PM reference and keep the parent bus awake.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701055529.445819-1-songwei.chai@oss.qualcomm.com?part=5