Re: [PATCH v2 5/6] perf: add MIPS Coherence Manager 3 (CM3) PMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf: mips_cm3_pmu: counter overflow double-counting in
  cm3_pmu_update()
- [Low] perf: mips_cm3_pmu: broken !MIPS_CPS / !SMP configuration
--

commit 272d1a9241643c479b7f4cabbd1c40edccbe1b16
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

perf: add MIPS Coherence Manager 3 (CM3) PMU driver

This commit adds an uncore PMU driver and documentation for the MIPS
Coherence Manager 3 (CM3) performance and cycle counters.

> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 245e7bb763b99..2dbed2fc7b030 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -75,6 +75,18 @@ config ARM_XSCALE_PMU
>  	depends on ARM_PMU && CPU_XSCALE
>  	def_bool y
>  
> +config MIPS_CM3_PMU
> +	tristate "MIPS Coherence Manager 3 (CM3) PMU"
> +	depends on MIPS_CM

[Severity: Low]
Should MIPS_CM3_PMU depend on MIPS_CPS rather than MIPS_CM?

MIPS_GIC selects MIPS_CM even when CONFIG_MIPS_CPS=n and CONFIG_SMP=n.
On a !CONFIG_SMP build, irq_set_affinity() is a stub returning -EINVAL, so
cm3_pmu_online_cpu() unconditionally triggers WARN_ON() when an overflow
IRQ is present; without CONFIG_MIPS_CPS, cps_smp_setup() also never
populates the cluster ID in cpu_data[].globalnumber.

> diff --git a/drivers/perf/mips_cm3_pmu.c b/drivers/perf/mips_cm3_pmu.c
> new file mode 100644
> index 0000000000000..6d53535473b96
> --- /dev/null
> +++ b/drivers/perf/mips_cm3_pmu.c
[ ... ]
> +static void cm3_pmu_update(struct perf_event *event)
> +{
> +	struct cm3_pmu *cm3_pmu = to_cm3_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	u32 ov_bit = cm3_pmu_overflow_bit(hwc->idx);
> +	u64 delta, prev, new;
> +	bool overflowed = false;
> +
> +	/* Counts were harvested when the cluster went down, nothing since */
> +	if (READ_ONCE(cm3_pmu->paused))
> +		return;
> +
> +	/*
> +	 * When polling, the counter cannot wrap twice between two reads and
> +	 * the 32-bit difference tells the whole story. With the overflow
> +	 * interrupt the counter is only read on demand and may have made a
> +	 * full turn since the last read, which the overflow status tells.
> +	 * It is read and cleared before the counter so that a wrap seen
> +	 * here is also reflected in the counter value; a wrap happening in
> +	 * between is accounted by the next update, which the interrupt it
> +	 * raises makes sure happens. In interrupt mode every access to the
> +	 * event runs on cm3_pmu->on_cpu with interrupts disabled, so the
> +	 * status and the counter are read consistently.
> +	 */
> +	if (cm3_pmu->irq > 0) {
> +		overflowed = CM3_PMU_READ(cm3_pmu, pc_ov) & ov_bit;
> +		if (overflowed)
> +			CM3_PMU_WRITE(cm3_pmu, pc_ov, ov_bit);
> +	}
> +
> +	do {
> +		prev = local64_read(&hwc->prev_count);
> +		new = cm3_pmu_read_counter(cm3_pmu, hwc->idx);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev, new) != prev);
> +
> +	/*
> +	 * If the counter wrapped and now shows a value not below the
> +	 * previous one, it made a full turn on top of the 32-bit difference.
> +	 */
> +	delta = (new - prev) & CM3_PMU_COUNTER_MASK;
> +	if (overflowed && new >= prev)
> +		delta += CM3_PMU_COUNTER_MASK + 1;
> +
> +	local64_add(delta, &event->count);
> +}

[Severity: Medium]
Can cm3_pmu_update() double-count a counter wrap in interrupt mode and
inflate event->count by 2^32?

If hwc->prev_count holds a non-zero value prev and the counter wraps to a
small value (new < prev) between reading pc_ov and reading the counter,
the first cm3_pmu_update() sees overflowed == false, updates prev_count to
new, and already accounts for the wrap in (new - prev) &
CM3_PMU_COUNTER_MASK while leaving ov_bit set in pc_ov.  When the pending
overflow interrupt then runs cm3_pmu_update(), it sees overflowed == true
with the counter >= the new prev_count and adds CM3_PMU_COUNTER_MASK + 1 a
second time.

Similarly, because the hardware sets ov_bit when the counter reaches
0xffffffff rather than when it wraps to 0, handling the interrupt or
reading the event while the counter is still at 0xffffffff satisfies
overflowed && new >= prev, adding 2^32 before the wrap to 0 adds 1 on the
next update.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-i6500-cm3-pmu-v2-0-cd2569571819@mobileye.com?part=5