Re: [PATCH v5 2/2] perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removing `enabled[idx] = false` from `rpi_axi_pmu_del` causes bus watcher misconfiguration when a watcher index is rapidly reused.
- [Medium] Overly broad substring matching inadvertently hides newly added BCM2712 events on RPi 5 while exposing them on RPi 1-4.
- [Medium] The VPU monitor hardware is not reliably disabled during module unload because `cancel_work_sync()` is used instead of `flush_work()`.
--

--- Patch [2]: [PATCH v5 2/2] perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support ---
commit 71567bad9b4b4c6115df3982a734e87c6c908033
Author: Ian Rogers <[email protected]>

perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support

This patch expands the Raspberry Pi AXI PMU driver to support Broadcom
BCM2712 system monitors. It adds new monitored buses, filter options,
and event aliases for the Raspberry Pi 5 platform.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> index e5cc86fc78b25..a24b6587f8718 100644
> --- a/drivers/perf/rpi_axi_pmu.c
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> @@ -1113,31 +1203,66 @@ static umode_t rpi_axi_pmu_events_is_visible(struct kobject *kobj,
>  					      struct attribute *attr, int unused)
>  {
[ ... ]
>  	if (rpi_pmu->chip == CHIP_BCM2712) {
>  		/*
>  		 * On RPi 5 (BCM2712), hide legacy VPU Mailbox IPC events (monitor=1)
>  		 * and legacy RPi 1-4 System Monitor aliases
>  		 */
>  		if (strstr(pmu_attr->event_str, "monitor=1") ||
[ ... ]
>  		    strstr(attr->name, "peripheral_") ||
>  		    strstr(attr->name, "cpu_uc_") ||
[ ... ]
>  			return 0;
>  	} else {
>  		/* On RPi 1-4 (BCM2835-BCM2711), hide RPi 5 specific events */
>  		if (strstr(attr->name, "pcie_rp1") ||
[ ... ]
>  			return 0;
>  	}
>  
>  	return attr->mode;
>  }

[Severity: Medium]
Will these substring matches inadvertently hide the newly added BCM2712 events
on RPi 5?

The new BCM2712 events are named "bcm2712_peripheral_rtrans" and
"bcm2712_cpu_uc_rtrans", which will match "peripheral_" and "cpu_uc_"
and be hidden here. 

Conversely, won't these same new events be mistakenly exposed on older chips
because they don't match any of the substrings in the `else` block?

[ ... ]
> @@ -1810,14 +1831,16 @@ static void rpi_axi_pmu_del(struct perf_event *event, int flags)
[ ... ]
>  	if (pmu->monitor[mon].hw_events.monitored_bus[idx] >= 0) {
>  		pmu->monitor[mon].hw_events.refcount[idx]--;
>  		if (pmu->monitor[mon].hw_events.refcount[idx] == 0) {
> -			if (mon == MON__SYSTEM)
> -				rpi_axi_pmu_disable_bus_watcher(pmu, mon, idx);
>  			pmu->monitor[mon].hw_events.monitored_bus[idx] = -1;
>  			pmu->monitor[mon].hw_events.filter[idx] = BCM2835_FLT__NONE;
> -			pmu->monitor[mon].hw_events.enabled[idx] = false;
>  			pmu->monitor[mon].hw_events.num_monitored--;
> -			if (mon == MON__SYSTEM && pmu->monitor[mon].hw_events.num_monitored == 0) {
> -				set_monitor_control(pmu, mon, GEN_CTL_RESET_BIT);
> -				pmu->monitor[mon].hw_events.monitor_running = false;
> +			if (mon == MON__SYSTEM) {
> +				rpi_axi_pmu_disable_bus_watcher(pmu, mon, idx);
> +				if (pmu->monitor[mon].hw_events.num_monitored == 0) {
> +					set_monitor_control(pmu, mon, GEN_CTL_RESET_BIT);
> +					pmu->monitor[mon].hw_events.monitor_running = false;
> +				}
> +			} else if (mon == MON__VPU) {
> +				schedule_work(&pmu->vpu_work);
>  			}
>  		}
>  	}

[Severity: High]
Is it safe to remove `pmu->monitor[mon].hw_events.enabled[idx] = false` here?

If a VPU event is deleted, the watcher slot is marked as free. If a new VPU
event is added immediately before `vpu_work` runs to perform the teardown,
the new event could claim this slot.

When `vpu_work` runs, it checks `if (!enabled[idx])` to program the hardware.
Since `enabled[idx]` is still true from the deleted event, it skips
initialization, causing the hardware to monitor the old bus while the new
perf event incorrectly reports those counts.

[ ... ]
> @@ -2014,9 +2065,10 @@ static void rpi_axi_pmu__exit(struct rpi_axi_pmu *pmu)
>  {
> -	perf_pmu_unregister(&pmu->pmu);
>  	cpuhp_state_remove_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
> +
> +	perf_pmu_unregister(&pmu->pmu);
>  	hrtimer_cancel(&pmu->hrtimer);
>  	cancel_work_sync(&pmu->vpu_work);
>  	if (pmu->firmware)

[Severity: Medium]
Since `rpi_axi_pmu_del()` was updated to schedule `vpu_work` to disable the
VPU hardware, does this `cancel_work_sync()` prevent the hardware teardown
from completing during module unload?

`perf_pmu_unregister()` will call `rpi_axi_pmu_del()`, which schedules
the asynchronous teardown. Canceling it immediately after means the VPU AXI
bus watchers could be left running. Should this be `flush_work()` instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2