Re: [PATCH RESEND v7 1/3] drivers/perf: riscv-iommu: add risc-v iommu pmu driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional event update in PMU IRQ handler corrupts stopped events
--

commit 9e07a4e349b75961d81fbcf21e24a523435b3ee8
Author: Zong Li <zong.li@sifive.com>
Subject: drivers/perf: riscv-iommu: add risc-v iommu pmu driver

This commit adds a new driver to support the RISC-V IOMMU PMU. The driver is
created as an auxiliary device by the parent RISC-V IOMMU driver and
implements the standard perf hardware performance monitor callbacks.

Link: https://lore.kernel.org/linux-riscv/20260618143634.7f3dd6c5@pumpkin/

> diff --git a/drivers/perf/riscv_iommu_pmu.c b/drivers/perf/riscv_iommu_pmu.c
> new file mode 100644
> index 0000000000000..f6acd56f2f616
> --- /dev/null
> +++ b/drivers/perf/riscv_iommu_pmu.c
[ ... ]
> +static int riscv_iommu_pmu_add(struct perf_event *event, int flags)
> +{
[ ... ]
> +	pmu->events[idx] = event;
> +	hwc->idx = idx;
> +	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +	local64_set(&hwc->prev_count, 0);

[Severity: Medium]
Will this lead to a spurious massive count delta if an interrupt occurs before
the event is started?

A newly added event has its state set to PERF_HES_STOPPED and prev_count
initialized to 0 here. If the event is not immediately started (i.e.
PERF_EF_START is not set), it remains stopped but visible in pmu->events[idx].

[ ... ]
> +static irqreturn_t riscv_iommu_pmu_irq_handler(int irq, void *dev_id)
> +{
[ ... ]
> +	ovf = readl(pmu->reg + RISCV_IOMMU_REG_IOCOUNTOVF);
> +	if (ovf) {
> +		bitmap_from_u64(ovf_bitmap, ovf);
> +		for_each_set_bit(idx, ovf_bitmap, pmu->num_counters) {
> +			struct perf_event *event = pmu->events[idx];
> +
> +			/*
> +			 * A counter may overflow just before its event is
> +			 * removed by riscv_iommu_pmu_del(), which leaves the
> +			 * overflow bit set with no event bound to the counter.
> +			 * There is nothing to update in that case, but the
> +			 * overflow bit still has to be cleared below, otherwise
> +			 * the interrupt would stay pending forever.
> +			 */
> +			if (event) {
> +				riscv_iommu_pmu_update(event);
> +				riscv_iommu_pmu_set_period(event);
> +			}

[Severity: Medium]
Does this code update events even if they are in a stopped state?

When a hardware overflow interrupt occurs (e.g. from another active counter or
a stale overflow bit left from a deleted event), the IRQ handler unconditionally
calls riscv_iommu_pmu_update() and riscv_iommu_pmu_set_period() for any mapped
event.

If the event mapped in pmu->events[idx] was added but is currently stopped
(as initialized in riscv_iommu_pmu_add()), this unconditional update calculates
a delta using the hardware counter and a prev_count of 0, adding a large
spurious delta to event->count and reprogramming the hardware period unexpectedly.

Should there be a check like if (!(event->hw.state & PERF_HES_STOPPED)) before
updating?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828085819.4076449-1-zong.li@sifive.com?part=1