Re: [PATCH v17 09/10] qcom-tgu: Add timer/counter functionality for TGU

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

On 11/13/2026 11:01 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] qcom-tgu: swapped step_index and operation_index in
>    tgu_dataset_rw()
> --
>
> commit 55838204d9ea620c8df4d9d4233a3561aeb9ea39
> Author: Songwei Chai <songwei.chai@oss.qualcomm.com>
>
> qcom-tgu: Add timer/counter functionality for TGU
>
> This commit adds per-step timer and counter sysfs attributes and hardware
> register programming for the Qualcomm Trigger Generation Unit (TGU).
>
>> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
>> index 6e66752a7b8dd..1cb131a3c5213 100644
>> --- a/drivers/hwtracing/qcom/tgu.c
>> +++ b/drivers/hwtracing/qcom/tgu.c
> [ ... ]
>
>> @@ -496,6 +566,38 @@ static const struct attribute_group tgu_common_grp = {
>>   		.name = "step" #step "_condition_select" \
>>   	})
>>   
>> +#define STEP_TIMER(step_index, reg_num) \
>> +	tgu_dataset_rw(reg##reg_num, step_index, TGU_TIMER, reg_num)
>> +
>> +#define STEP_TIMER_LIST(step_index) \
>> +	{STEP_TIMER(step_index, 0), \
>> +	 STEP_TIMER(step_index, 1), \
>> +	 NULL                       \
>> +	}
>> +
>> +#define TIMER_ATTRIBUTE_GROUP_INIT(step)\
>> +	(&(const struct attribute_group){\
>> +		.attrs = (struct attribute*[])STEP_TIMER_LIST(step),\
>> +		.is_visible = tgu_node_visible,\
>> +		.name = "step" #step "_timer" \
>> +	})
>> +
>> +#define STEP_COUNTER(step_index, reg_num) \
>> +	tgu_dataset_rw(reg##reg_num, step_index, TGU_COUNTER, reg_num)
> [Severity: High]
> Do STEP_TIMER() and STEP_COUNTER() initialize operation_index and
> step_index in the wrong order via tgu_dataset_rw()?
>
> drivers/hwtracing/qcom/tgu.c:
>      struct tgu_attribute {
>      	struct device_attribute attr;
>      	enum operation_index operation_index;
>      	u8 step_index;
>      	u8 reg_num;
>      };
>      ...
>      #define tgu_dataset_rw(name, step_index, priority, reg_num)              \
>      	(&((struct tgu_attribute[]){ {                                   \
>      		__ATTR(name, 0644, tgu_dataset_show, tgu_dataset_store), \
>      		step_index,                                              \
>      		priority,                                                \
>      		reg_num,                                                 \
>      	} })[0].attr.attr)
>
> Because tgu_dataset_rw() initializes struct tgu_attribute positionally,
> every timer and counter attribute gets tgu_attr->operation_index set to
> step_index (0..7) and tgu_attr->step_index set to TGU_TIMER (6) or
> TGU_COUNTER (7).
>
> In tgu_node_visible(), tgu_attr->step_index >= drvdata->num_step hides all
> timer groups when num_step <= 6 and all counter groups when num_step <= 7.
> When visible, tgu_dataset_show() and tgu_dataset_store() switch on
> operation_index and read or write priority, condition_decode, and
> condition_select for steps 6 and 7 instead of drvdata->timer and
> drvdata->counter.  Conversely, adding TGU_TIMER (6) and TGU_COUNTER (7) to
> enum operation_index causes the existing step 6 and step 7 priority,
> condition_decode, and condition_select attributes (which also use
> tgu_dataset_rw()) to be dispatched as TGU_TIMER and TGU_COUNTER.
Same issue as patch 6
>