Re: [PATCH v15 6/7] qcom-tgu: Add timer/counter functionality for TGU

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Incorrect bitmask definition for TGU_DEVID2_TIMER1 skips bit 12.
- [Low] Empty sysfs directories for timers and counters are created when the hardware does not support them.

Pre-existing issues:
- [High] Sysfs attributes are exposed to userspace before their underlying data structures are fully allocated, leading to a possible NULL pointer dereference.
- [High] Probe error paths leak sysfs groups and leave dangling pointers, causing a Use-After-Free if accessed.
- [High] Unsynchronized PM runtime operations in enable_tgu_store() cause PM usage counter leaks.
- [High] Driver unbind leaks a PM runtime reference if the TGU device was left enabled.
--

commit 358a24e20604be5f3d933b0c130bc3468d07b832
Author: Songwei Chai <songwei.chai@oss.qualcomm.com>

qcom-tgu: Add timer/counter functionality for TGU
    
This commit adds counter and timer nodes for each TGU step. It provides sysfs
interfaces to program these components for use in trigger events or sequences.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> index fefe932059cbb..6d5bf2621cb05 100644
> --- a/drivers/hwtracing/qcom/tgu.c
> +++ b/drivers/hwtracing/qcom/tgu.c
[ ... ]
> @@ -163,6 +181,18 @@ static umode_t tgu_node_visible(struct kobject *kobject,
>  		if (tgu_attr->reg_num < drvdata->num_condition_select)
>  			return attr->mode;
>  		break;
> +	case TGU_COUNTER:
> +		if (!drvdata->num_counter)
> +			break;

[Severity: Low]
When drvdata->num_counter is 0, this breaks out and returns 0 instead of
SYSFS_GROUP_INVISIBLE. Could this leave empty sysfs directories behind when
the hardware doesn't support counters? Returning SYSFS_GROUP_INVISIBLE would
properly suppress the creation of the named group directory.

> +		if (tgu_attr->reg_num < drvdata->num_counter)
> +			return attr->mode;
> +		break;
> +	case TGU_TIMER:
> +		if (!drvdata->num_timer)
> +			break;

[Severity: Low]
Similarly, should this return SYSFS_GROUP_INVISIBLE when drvdata->num_timer
is 0 to prevent creating empty directories?

> +		if (tgu_attr->reg_num < drvdata->num_timer)
> +			return attr->mode;
> +		break;
>  	default:
>  		break;
>  	}
[ ... ]
> @@ -412,8 +503,8 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	struct device *dev = &adev->dev;
>  	struct tgu_drvdata *drvdata;
> -	unsigned int *priority, *condition, *select;
> -	size_t priority_size, condition_size, select_size;
> +	unsigned int *priority, *condition, *select, *timer, *counter;
> +	size_t priority_size, condition_size, select_size, timer_size, counter_size;
>  	int ret;
>  
>  	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> @@ -432,6 +523,7 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
>  	tgu_set_reg_number(drvdata);
>  	tgu_set_steps(drvdata);
>  	tgu_set_conditions(drvdata);
> +	tgu_set_timer_counter(drvdata);
>  
>  	ret = sysfs_create_groups(&dev->kobj, tgu_attr_groups);
>  	if (ret) {
> @@ -474,6 +566,26 @@ static int tgu_probe(struct amba_device *adev, const struct amba_id *id)

[Severity: High]
This is a pre-existing issue, but sysfs_create_groups() is called before the
value_table and its arrays are allocated. If a concurrent userspace process
reads a sysfs file, could tgu_dataset_show() dereference an unallocated
drvdata->value_table, resulting in a NULL pointer dereference?

>  
>  	drvdata->value_table->condition_select = select;
>  
> +	timer_size = drvdata->num_step * drvdata->num_timer;
> +
> +	timer = devm_kcalloc(dev, timer_size,
> +			    sizeof(*(drvdata->value_table->timer)),
> +			    GFP_KERNEL);
> +	if (!timer)
> +		return -ENOMEM;

[Severity: High]
This isn't a bug introduced by this patch, but if devm_kcalloc() fails here,
we return -ENOMEM without cleaning up the sysfs groups created earlier. 

The driver core will free devres-managed memory, but the active sysfs files
will remain. If userspace reads these files afterward, could it trigger a
use-after-free or NULL pointer dereference when accessing drvdata?

> +
> +	drvdata->value_table->timer = timer;
> +
> +	counter_size = drvdata->num_step * drvdata->num_counter;
> +
> +	counter = devm_kcalloc(dev, counter_size,
> +			      sizeof(*(drvdata->value_table->counter)),
> +			      GFP_KERNEL);
> +	if (!counter)
> +		return -ENOMEM;
> +
> +	drvdata->value_table->counter = counter;
> +
>  	drvdata->enabled = false;
>  
>  	pm_runtime_put(&adev->dev);
> diff --git a/drivers/hwtracing/qcom/tgu.h b/drivers/hwtracing/qcom/tgu.h
> index c61aa8dc51b0d..1bcbc99169def 100644
> --- a/drivers/hwtracing/qcom/tgu.h
> +++ b/drivers/hwtracing/qcom/tgu.h
[ ... ]
> @@ -18,6 +19,16 @@
>  	((int)FIELD_GET(GENMASK(6, 3), devid_val))
>  #define TGU_DEVID_CONDITIONS(devid_val) \
>  	((int)FIELD_GET(GENMASK(2, 0), devid_val))
> +#define TGU_DEVID2_TIMER0(devid_val)	\
> +	((int)FIELD_GET(GENMASK(23, 18), devid_val))
> +#define TGU_DEVID2_TIMER1(devid_val)	\
> +	((int)FIELD_GET(GENMASK(17, 13), devid_val))

[Severity: Medium]
Does GENMASK(17, 13) correctly represent the TIMER1 mask? TIMER0, COUNTER0,
and COUNTER1 are all defined as 6 bits wide, but GENMASK(17, 13) is only 5
bits wide, skipping bit 12. Should this be GENMASK(17, 12) instead?

> +#define TGU_DEVID2_COUNTER0(devid_val)	\
> +	((int)FIELD_GET(GENMASK(11, 6), devid_val))
> +#define TGU_DEVID2_COUNTER1(devid_val)	\
> +	((int)FIELD_GET(GENMASK(5, 0), devid_val))
> +
> +

I also noticed a couple of pre-existing issues in other parts of the driver
unrelated to this specific patch:

[Severity: High]
This is a pre-existing issue, but in enable_tgu_store():

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

ret = pm_runtime_resume_and_get(dev);

The lock is dropped before the PM runtime reference is acquired. Could two
concurrent writers both bypass the !drvdata->enabled check, allowing both
to acquire PM references, but only one to be released during disable, leaking
the PM usage counter?

[Severity: High]
This is a pre-existing issue, but in tgu_remove():

sysfs_remove_groups(&dev->kobj, tgu_attr_groups);

tgu_disable(dev);

The tgu_disable() function disables the hardware but doesn't call
pm_runtime_put(). If the device was left enabled via sysfs, does this leak
the PM runtime reference upon driver unbind?

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