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

[sashiko-bot@kernel.org]

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.

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