Re: [PATCH v5 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] rpi_axi_pmu_del() calls hrtimer_cancel() while holding the perf core's ctx->lock (a raw spinlock) and with IRQs disabled, leading to deadlocks.
- [High] VPU hardware bus watchers are never disabled when their associated perf events are deleted, resulting in a permanent hardware resource leak.
- [Medium] Final VPU event counts are lost when an event is stopped, leading to silent under-reporting.
- [Low] RASPBERRYPI_AXI_PMU conditionally selects RASPBERRYPI_FIRMWARE without ensuring its dependencies are met, causing Kconfig warnings.
--

--- Patch [1]: [PATCH v5 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit cadc1bb2e8b72c5287e86258dff1cee13436365d
Author: Ian Rogers <[email protected]>
Date:   Wed Aug 12 23:07:59 2026 +0000

    perf: Add Raspberry Pi BCM2835 AXI PMU driver
    
    This commit adds an uncore performance monitoring unit (PMU) driver
    for Broadcom BCM2835-BCM2711 AXI system monitors and VideoCore VPU firmware
    mailbox monitors found on Raspberry Pi 1 through 4.

> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 245e7bb763b99..97c8bf34f4633 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -325,4 +325,14 @@ config NVIDIA_TEGRA410_C2C_PMU
>  	  Enable perf support for counters in NVIDIA C2C interface of NVIDIA
>  	  Tegra410 SoC.
>  
> +config RASPBERRYPI_AXI_PMU
> +	tristate "Raspberry Pi AXI PMU support"
> +	depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST
> +	select RASPBERRYPI_FIRMWARE if ARCH_BCM2835

[Severity: Low]
Does this conditionally select RASPBERRYPI_FIRMWARE without ensuring its
dependencies are met? If a user configures ARCH_BCM2835=y but MAILBOX=n,
could this cause Kconfig unmet dependency warnings and silent build failures?

[ ... ]

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..e5cc86fc78b25
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c

[ ... ]

> +static void rpi_axi_pmu_read(struct perf_event *event)
> +{
> +	struct rpi_axi_pmu *pmu = pmu_to_rpi_axi_pmu(event->pmu);
> +	enum monitor mon = config_to_monitor(event->attr.config);
> +	enum counter counter = config_to_counter(event->attr.config);
> +	u64 prev_count, new_count;
> +	unsigned long flags;
> +	u32 delta;
> +
> +	/* Mailbox VPU counters are polled asynchronously in background vpu_work.
> +	 * MMIO monitors (System and BCM2712 VPU) are read synchronously.
> +	 */
> +	if (pmu->monitor[mon].use_mailbox_interface || event->hw.idx < 0)
> +		return;

[Severity: Medium]
When a VPU event is stopped, it looks like rpi_axi_pmu_read() bails out here
without capturing the final delta.

Additionally, in rpi_axi_pmu_vpu_work_handler():

        if (!event || (event->hw.state & PERF_HES_STOPPED) ||
            config_to_monitor(event->attr.config) != MON__VPU)
                continue;

The background poller skips stopped events. If rpi_axi_pmu_stop() marks the
event as stopped, does this result in the final VPU counts being completely
discarded? Could short-lived processes consistently report zero events because
of this?

[ ... ]

> +static void rpi_axi_pmu_del(struct perf_event *event, int flags)
> +{
> +	struct rpi_axi_pmu *pmu = pmu_to_rpi_axi_pmu(event->pmu);
> +	enum monitor mon = config_to_monitor(event->attr.config);
> +	unsigned long spinflags;
> +	int idx = event->hw.idx;
> +
> +	if (idx < 0)
> +		return;
> +
> +	rpi_axi_pmu_stop(event, PERF_EF_UPDATE);
> +
> +	spin_lock_irqsave(&pmu->lock, spinflags);
> +	for (int i = 0; i < RPI_AXI_MAX_EVENTS; i++) {
> +		if (pmu->events[i] == event) {
> +			pmu->events[i] = NULL;
> +			pmu->event_gen[i]++;
> +			break;
> +		}
> +	}
> +
> +	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;

[Severity: High]
Does this cause a permanent hardware resource leak for VPU events? Since
rpi_axi_pmu_disable_bus_watcher() is only called for MON__SYSTEM, the VPU
bus watchers are never disabled. Furthermore, because enabled[idx] is set to
false, the hardware is left running indefinitely with no way for any background
polling logic to track or disable it later.

> +			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;
> +			}
> +		}
> +	}
> +
> +	event->hw.idx = -1;
> +	pmu->active_events--;
> +	if (mon == MON__VPU)
> +		pmu->active_vpu_events--;
> +
> +	if (pmu->active_events == 0) {
> +		spin_unlock_irqrestore(&pmu->lock, spinflags);
> +		if (in_hardirq())
> +			hrtimer_try_to_cancel(&pmu->hrtimer);
> +		else
> +			hrtimer_cancel(&pmu->hrtimer);

[Severity: Critical]
Can this lead to deadlocks or scheduling while atomic panics? 

rpi_axi_pmu_del() is called with the perf core's ctx->lock held (a raw spinlock) 
and local IRQs disabled. If in_hardirq() is false, this branch calls 
hrtimer_cancel(). 

Because this is a SOFT hrtimer (HRTIMER_MODE_REL_SOFT), hrtimer_cancel() 
can sleep on PREEMPT_RT, causing an immediate panic. Even without PREEMPT_RT, 
if the user thread preempted ksoftirqd on the same CPU, spin-waiting for 
the timer to finish while preemption is disabled causes a permanent 
self-deadlock.

> +		spin_lock_irqsave(&pmu->lock, spinflags);
> +		if (pmu->active_events > 0)
> +			hrtimer_start(&pmu->hrtimer, RPI_AXI_PMU_TIMER_INTERVAL,
> +				      HRTIMER_MODE_REL_SOFT);
> +		spin_unlock_irqrestore(&pmu->lock, spinflags);
> +		return;
> +	}
> +	spin_unlock_irqrestore(&pmu->lock, spinflags);
> +}

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