Re: [PATCH v16 3/7] qcom-tgu: Add signal priority support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] tgu_dataset_store allows writes while the device is enabled, causing silent software/hardware state desynchronization.
- [Medium] tgu_dataset_store silently truncates out-of-range priority values on 64-bit systems.
--

--- Patch [3]: [PATCH v16 3/7] qcom-tgu: Add signal priority support ---
commit 8afece15a767b432d65a919119df2e97aca2bed5
Author: Songwei Chai <songwei.chai@oss.qualcomm.com>

qcom-tgu: Add signal priority support

This commit adds signal priority configuration support to the Qualcomm TGU
driver. It introduces sysfs attributes to configure the priority of sensed
signals for each step, and stores these configurations in a dynamically
allocated table.

> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
> index 9c7dd4a09f027..0e88b437f191f 100644
> --- a/drivers/hwtracing/qcom/tgu.c
> +++ b/drivers/hwtracing/qcom/tgu.c

[ ... ]

> +static ssize_t tgu_dataset_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t size)
> +{
> +	struct tgu_drvdata *tgu_drvdata = dev_get_drvdata(dev);
> +	struct tgu_attribute *tgu_attr =
> +		container_of(attr, struct tgu_attribute, attr);
> +	unsigned long val;
> +	int index;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 0, &val);

[Severity: Medium]
Does this code silently truncate out-of-range priority values on 64-bit
systems?

The function uses kstrtoul() to parse user input into an unsigned long.
Later, it directly assigns this value to
tgu_drvdata->value_table->priority[index], which is an array of unsigned int.

On 64-bit systems, inputs exceeding UINT_MAX will be silently truncated
without returning an error like -ERANGE. Would kstrtouint() be more
appropriate here?

> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&tgu_drvdata->lock);
> +	index = calculate_array_location(tgu_drvdata, tgu_attr->step_index,
> +					 tgu_attr->operation_index,
> +					 tgu_attr->reg_num);
> +
> +	tgu_drvdata->value_table->priority[index] = val;

[Severity: Medium]
Can this cause a silent desynchronization between software and hardware
state?

If a user writes to this attribute while the TGU is actively tracing
(tgu_drvdata->enabled == true), the driver updates the software backing array
and returns success, but the hardware is not dynamically updated. Subsequent
reads will return the new value, giving the false impression that the hardware
configuration has changed.

Should this check if the device is enabled and either return an error, or push
the update to the hardware registers?

> +
> +	return size;
> +}

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