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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] VPU bus watcher reconfiguration is skipped if a watcher slot is reallocated before asynchronous vpu_work executes, causing silent data corruption.
- [Medium] Broad substring matching in `rpi_axi_pmu_events_is_visible()` unintentionally hides newly added BCM2712-specific event aliases on the BCM2712 platform.
--

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

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

Expand the Raspberry Pi AXI PMU driver to support Broadcom BCM2712
(Raspberry Pi 5) system monitors, PCIe RP1 Southbridge links, HEVC decoder,
and Cortex-A76 DSU L3 interconnects.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> --- a/drivers/perf/rpi_axi_pmu.c
> +++ b/drivers/perf/rpi_axi_pmu.c

[ ... ]

> @@ -880,12 +1062,111 @@ static struct attribute *rpi_axi_pmu_events_attrs[] = {

[ ... ]

> +static umode_t rpi_axi_pmu_events_is_visible(struct kobject *kobj,
> +					      struct attribute *attr, int unused)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	struct rpi_axi_pmu *rpi_pmu = pmu_to_rpi_axi_pmu(pmu);
> +	struct perf_pmu_events_attr *pmu_attr;
> +
> +	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr.attr);
> +
> +	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, "cpu0_") ||
> +		    strstr(attr->name, "cpu1_") ||
> +		    strstr(attr->name, "dma0_") ||
> +		    strstr(attr->name, "dma1_") ||
> +		    strstr(attr->name, "v3d0_") ||
> +		    strstr(attr->name, "v3d1_") ||
> +		    strstr(attr->name, "hvs_system") ||
> +		    strstr(attr->name, "isp_system") ||
> +		    strstr(attr->name, "usb_system") ||
> +		    strstr(attr->name, "ccp2tx_") ||
> +		    strstr(attr->name, "mphi_") ||
> +		    strstr(attr->name, "h264_") ||
> +		    strstr(attr->name, "v3d_") ||
> +		    strstr(attr->name, "peripheral_") ||
> +		    strstr(attr->name, "cpu_uc_") ||

[Severity: Medium]
Does this broad substring matching unintentionally hide the newly added
bcm2712_peripheral_* and bcm2712_cpu_uc_* event aliases on the BCM2712
platform?

Since the new aliases are named bcm2712_peripheral_rtrans and
bcm2712_cpu_uc_rtrans, they contain the exact substrings "peripheral_" and
"cpu_uc_". It looks like they might be incorrectly filtered out here, making
them unavailable in sysfs.

> +		    strstr(attr->name, "cpu_l2_") ||
> +		    strstr(attr->name, "l2_control_") ||
> +		    strstr(attr->name, "system_control_") ||
> +		    strstr(attr->name, "direct_control_") ||
> +		    strstr(attr->name, "direct_data_"))
> +			return 0;
> +	} else {

[ ... ]

> @@ -1507,15 +1831,17 @@ 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--;

[Severity: High]
Could removing this synchronous clear of enabled[idx] cause a race condition
leading to silent data corruption?

If a VPU monitor event is deleted here, refcount[idx] drops to 0,
monitored_bus[idx] becomes -1 (freeing the slot), and we schedule
pmu->vpu_work.

If a new VPU event is added before rpi_axi_pmu_vpu_work_handler() runs,
it will reuse this empty slot and increment refcount[idx] to 1.

Then when rpi_axi_pmu_vpu_work_handler() finally runs:

drivers/perf/rpi_axi_pmu.c:rpi_axi_pmu_vpu_work_handler() {
    ...
    /* If VPU bus watcher is not enabled on hardware, enable it... */
    if (!pmu->monitor[MON__VPU].hw_events.enabled[idx]) {
        ...
    }
    ...
}

It appears the worker will skip configuring the new hardware because
enabled[idx] remained true from the old event.

Furthermore, the teardown of the old hardware is also skipped later in the
handler:

drivers/perf/rpi_axi_pmu.c:rpi_axi_pmu_vpu_work_handler() {
    ...
    for (int idx = 0; idx < NUM_BUS_WATCHERS_PER_MONITOR; idx++) {
        if (pmu->monitor[MON__VPU].hw_events.refcount[idx] == 0 &&
            pmu->monitor[MON__VPU].hw_events.enabled[idx]) {
            ...
        }
    }
}

Since the new event already incremented refcount[idx], this block won't
execute, potentially leaving the hardware misconfigured and causing userspace
to receive PMU counts for a completely different AXI bus.

> -			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);
>  			}
>  		}
>  	}

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