Re: [PATCH v16 6/7] qcom-tgu: Add timer/counter functionality for TGU

On 7/30/2026 4:18 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Incorrect mapping of missing hardware timer and counter registers causes out-of-order register writes.
> --
>
> --- Patch [6]: [PATCH v16 6/7] qcom-tgu: Add timer/counter functionality for TGU ---
> commit db9df0865ab2140ab0eb7823166108ce956f637e
> Author: Songwei Chai <[email protected]>
>
> qcom-tgu: Add timer/counter functionality for TGU
>
> This commit adds timer and counter functionality for each step in the TGU
> driver. It determines the number of timers and counters by reading the DEVID2
> register, allocates necessary memory, and exposes sysfs attributes for user
> configuration.
>
>> diff --git a/drivers/hwtracing/qcom/tgu.c b/drivers/hwtracing/qcom/tgu.c
>> index 7f0a74514e40f..ea83273a628c5 100644
>> --- a/drivers/hwtracing/qcom/tgu.c
>> +++ b/drivers/hwtracing/qcom/tgu.c
> [ ... ]
>> @@ -236,6 +266,30 @@ static ssize_t tgu_write_all_hw_regs(struct tgu_drvdata *drvdata)
>>   				drvdata->base + CONDITION_SELECT_STEP(i, j));
>>   		}
>>   	}
>> +
>> +	for (i = 0; i < drvdata->num_step; i++) {
>> +		for (j = 0; j < drvdata->num_timer; j++) {
>> +			index = check_array_location(drvdata, i, TGU_TIMER, j);
>> +
>> +			if (index == -EINVAL)
>> +				goto exit;
>> +
>> +			writel(drvdata->value_table->timer[index],
>> +				drvdata->base + TIMER_COMPARE_STEP(i, j));
> [Severity: High]
> Does this code handle sparse timers correctly? If the hardware implements a
> non-contiguous set of timers (e.g. TIMER1 is present but TIMER0 is absent),
> tgu_set_timer_counter() will set num_timers to 1.
>
> Since num_timers is 1, j will be 0 here. This writes to TIMER_COMPARE_STEP(i, 0)
> instead of TIMER_COMPARE_STEP(i, 1). Could writing to an unimplemented
> or unclocked register cause an asynchronous external abort?


Clarification：
All three comments point to the same underlying concern: that sparse 
timer/counter allocation (e.g. TIMER1 present but TIMER0 absent) could 
cause the count-based index to map to the wrong hardware register.
However, according to the hardware specification, timers and counters 
are allocated contiguously — TIMER1 cannot be present without TIMER0.
The same applies to counters. Therefore the sparse case cannot occur, 
and the count-based approach is correct.
>
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < drvdata->num_step; i++) {
>> +		for (j = 0; j < drvdata->num_counter; j++) {
>> +			index = check_array_location(drvdata, i, TGU_COUNTER, j);
>> +
>> +			if (index == -EINVAL)
>> +				goto exit;
>> +
>> +			writel(drvdata->value_table->counter[index],
>> +				drvdata->base + COUNTER_COMPARE_STEP(i, j));
> [Severity: High]
> Can the same issue happen for sparse counters, where we map to the wrong
> hardware register if COUNTER1 is present but COUNTER0 is absent?
>
>> +		}
>> +	}
>>   	/* Enable TGU to program the triggers */
>>   	writel(1, drvdata->base + TGU_CONTROL);
>>   exit:
> [ ... ]
>> @@ -316,6 +370,27 @@ static void tgu_set_conditions(struct tgu_drvdata *drvdata)
>>   		drvdata->num_condition_select = TGU_MAX_CONDITION_SELECT;
>>   }
>>   
>> +static void tgu_set_timer_counter(struct tgu_drvdata *drvdata)
>> +{
>> +	int num_timers = 0, num_counters = 0;
>> +	u32 devid2;
>> +
>> +	devid2 = readl(drvdata->base + CORESIGHT_DEVID2);
>> +
>> +	if (TGU_DEVID2_TIMER0(devid2))
>> +		num_timers++;
>> +	if (TGU_DEVID2_TIMER1(devid2))
>> +		num_timers++;
> [Severity: High]
> If TIMER0 is absent but TIMER1 is present, this collapses the presence bits
> into a single count of 1, losing the index mapping used when writing to the
> hardware registers above.
>
>> +
>> +	if (TGU_DEVID2_COUNTER0(devid2))
>> +		num_counters++;
>> +	if (TGU_DEVID2_COUNTER1(devid2))
>> +		num_counters++;
>> +
>> +	drvdata->num_timer = num_timers;
>> +	drvdata->num_counter = num_counters;
>> +}