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

[sashiko-bot@kernel.org]

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 <songwei.chai@oss.qualcomm.com>

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?

> +		}
> +	}
> +
> +	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;
> +}

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