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

["Songwei.Chai" <songwei.chai@oss.qualcomm.com>]

On 7/30/2026 4:17 PM, sashiko-bot@kernel.org wrote:
> 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?

|Clarification：|

The target register is 32 bits wide (|REG_OFFSET = 0x4|). Therefore, any 
value larger than |UINT_MAX| cannot be represented by the hardware and 
is inherently meaningless.

We can switch to |kstrtouint()| for stylistic consistency, but this is 
not a functional bug.

>
>> +	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?

|Clarification：|

The design is intentional.

A complete TGU configuration spans multiple register groups (priority, 
condition_decode, condition_select, timer, counter).

Writing hardware on every store would leave the engine in a transient, 
half-updated state between sysfs writes, which could feed a spurious 
match into CTI/interrupt consumers — exactly the hazard documented in 
tgu_write_all_hw_regs().

>
>> +
>> +	return size;
>> +}