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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] VPU monitor accounting is broken, causing complete data loss during time-multiplexing and short profiling sessions.
- [High] The driver uses `spinlock_t` for `pmu->lock`, which crashes PREEMPT_RT kernels with "sleeping while atomic".
--

--- Patch [1]: [PATCH v7 1/2] perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit a038f5a0d4388926d4738fa191fa39c1cd2e9f2a
Author: Ian Rogers <[email protected]>
Subject: perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit adds a performance monitoring driver for the Raspberry Pi AXI
bus. It supports both System and VPU monitors.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> index 0000000000000..bd3c79dd2ff8d
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> +struct rpi_axi_pmu {
> +	struct pmu		pmu;
> +	struct platform_device	*pdev;
> +	enum rpi_axi_chip	chip;
> +	struct rpi_firmware	*firmware;
> +
> +	int			cpu;
> +	struct hlist_node	cpuhp_node;
> +
> +	spinlock_t		lock;

[Severity: High]
Will using a standard spinlock_t here cause crashes on PREEMPT_RT kernels?

The perf core invokes PMU callbacks (like add, del, start, stop, and read)
with hard IRQs disabled. On PREEMPT_RT, spinlock_t is mapped to a sleepable
rt_mutex.

Acquiring pmu->lock via spin_lock_irqsave() in these atomic paths, such as
in rpi_axi_pmu_add():

rpi_axi_pmu_add() {
    ...
    spin_lock_irqsave(&pmu->lock, spinflags);
    ...
}

will trigger a "sleeping while atomic" crash. Should this be changed to a
raw_spinlock_t to guarantee it does not sleep?

> +	struct mutex		vpu_mutex;
[ ... ]
> +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) are read synchronously.
> +	 */
> +	if (event->hw.idx < 0)
> +		return;
> +
> +	if (pmu->monitor[mon].use_mailbox_interface)
> +		return;

[Severity: High]
Does returning immediately here lead to complete data loss for VPU events
during time-multiplexing and short profiling sessions?

When the perf core stops an event (for instance, during event rotation or at
the end of a session), it calls rpi_axi_pmu_read() to capture the final count.
Because VPU events bypass synchronous reading here, any counts accumulated
since the last 2-second background timer tick are not recorded.

When the event is subsequently restarted, the background work sets a new
baseline:

rpi_axi_pmu_vpu_work_handler() {
    ...
    if (!(event->hw.state & PERF_HES_UPTODATE)) {
        /* Initial baseline read for newly started/rotated VPU event */
        local64_set(&event->hw.prev_count, new_count);
        event->hw.state |= PERF_HES_UPTODATE;
    }
    ...
}

This permanently strands the uncollected counts from the previous run period.

Could this cause short-lived perf stat sessions (under 2 seconds) and
oversubscribed VPU events to systematically report near zero?

> +
> +	spin_lock_irqsave(&pmu->lock, flags);
[ ... ]

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