Re: [PATCH v15 2/7] qcom-tgu: Add TGU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) race condition in `enable_tgu_store` causes unbalanced PM runtime usage counter.
- [High] Missing `pm_runtime_get_sync()` and unbalanced sysfs PM references in `tgu_remove()` lead to PM usage counter underflow or leaks.
--

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

qcom-tgu: Add TGU driver

Add driver to support device TGU (Trigger Generation Unit).
TGU is a Data Engine which can be utilized to sense a plurality of
signals and create a trigger into the CTI or generate interrupts to
processors. Add probe/enable/disable functions for tgu.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> new file mode 100644
> index 0000000000000..49c8f710b9318
> --- /dev/null
> +++ b/drivers/hwtracing/qcom/tgu.c

[ ... ]

> +/* enable_tgu_store - Configure Trace and Gating Unit (TGU) triggers. */
> +static ssize_t enable_tgu_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t size)
> +{
> +	struct tgu_drvdata *drvdata = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &val);
> +	if (ret || val > 1)
> +		return -EINVAL;
> +
> +	if (val) {
> +		scoped_guard(spinlock, &drvdata->lock) {
> +			if (drvdata->enabled)
> +				return -EBUSY;
> +		}
> +
> +		ret = pm_runtime_resume_and_get(dev);

[Severity: High]
Is there a time-of-check to time-of-use (TOCTOU) race condition here?

Because scoped_guard releases the lock immediately after checking
drvdata->enabled, two threads could concurrently write '1' to this sysfs
attribute and both pass the check. 

This would cause pm_runtime_resume_and_get() to be called twice, leaking a PM
runtime usage counter since the internal boolean state only changes once.

[ ... ]

> +	} else {
> +		scoped_guard(spinlock, &drvdata->lock) {
> +			if (!drvdata->enabled)
> +				return -EINVAL;
> +		}
> +
> +		tgu_disable(dev);
> +		pm_runtime_put(dev);
> +	}
> +
> +	return size;
> +}

Similarly, if two threads concurrently write '0' to the sysfs attribute, could
the lock being released early allow both threads to bypass the state check,
causing the PM usage counter to decrement multiple times?

[ ... ]

> +static void tgu_remove(struct amba_device *adev)
> +{
> +	struct device *dev = &adev->dev;
> +
> +	sysfs_remove_groups(&dev->kobj, tgu_attr_groups);
> +
> +	tgu_disable(dev);
> +}

[Severity: High]
Since tgu_probe() called pm_runtime_put(), doesn't tgu_remove() need to call
pm_runtime_get_sync() to balance the counter?

Without this, the AMBA bus framework might underflow the PM usage counter to -1
during remove.

Additionally, if the device is removed while it was enabled via sysfs,
tgu_disable(dev) is called to turn off the hardware, but it doesn't seem to
call pm_runtime_put(). Does this leak the PM runtime reference that was
acquired in enable_tgu_store()?

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