Re: [PATCH v4 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:

New issues:
- [Medium] The BCM2712 visibility filter incorrectly hides the JPEG hardware acceleration bus PMU events.
- [Medium] Missing and mismatched PMU event aliases for BCM2712 PERIPHERAL, CPU_UC, and CPU_L2 buses.

Pre-existing issues:
- [High] PMU is exposed to userspace before driver initialization completes, allowing a Use-After-Free if a subsequent step fails.
--

--- Patch [2]: [PATCH v4 2/2] perf: Add Raspberry Pi 5 (BCM2712) AXI PMU support ---
commit ac1314c26eb484b245288019eb6f6293df532017
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
> index 827656b14d85d..02ab73be70b25 100644
> --- a/drivers/perf/rpi_axi_pmu.c
> +++ b/drivers/perf/rpi_axi_pmu.c
[ ... ]
> @@ -723,6 +861,44 @@ PMU_EVENT_ATTR_STRING(isp_system_l2_wtrans, rpi_axi_pmu_event_isp_system_l2_wtra
[ ... ]
> +/*
> + * Quad-Core Arm Cortex-A76 Cores 0-3 I-Cache & D-Cache Filtered Events
> + * (bus=14, BCM2712_SB__CPU_L2)
> + */
> +/* BCM2712 Master ID Filtered Events */
> +PMU_EVENT_ATTR_STRING(arm_rtrans,              rpi_axi_pmu_event_arm_rtrans,              "monitor=0,bus=5,counter=4,filter=10");
> +PMU_EVENT_ATTR_STRING(arm_wtrans,              rpi_axi_pmu_event_arm_wtrans,              "monitor=0,bus=5,counter=2,filter=10");

[Severity: Medium]
Does this mapping correctly reflect the intended hardware bus?

The comment states that the Cortex-A76 Cores L2 cache events correspond to
bus 14 (BCM2712_SB__CPU_L2), but the event strings specify bus=5
(BCM2712_SB__SYSTEM_L2). Will this yield incorrect measurements for the CPU
L2 cache when queried via sysfs?

[ ... ]
> @@ -879,12 +1055,107 @@ 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)
> +{
[ ... ]
> +	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, "v3d_") ||
> +		    strstr(attr->name, "jpeg_") ||

[Severity: Medium]
Should the jpeg_ aliases be hidden on BCM2712?

The BCM2712 hardware definitions (BCM2712_SB__JPEG = 2) indicate that the
JPEG hardware acceleration bus is supported, but this logic explicitly
hides it from userspace on this platform. Does this incorrectly deny access
to valid hardware events?

> +		    strstr(attr->name, "peripheral_") ||
> +		    strstr(attr->name, "cpu_uc_") ||
> +		    strstr(attr->name, "cpu_l2_") ||

[Severity: Medium]
Is it intentional to hide these legacy aliases without providing replacements
for BCM2712?

The aliases for peripheral_, cpu_uc_, and cpu_l2_ are hidden here, which
limits functionality on the new platform since equivalents weren't added in
the newly defined BCM2712 expanded event aliases.

[ ... ]
> @@ -1575,7 +1847,7 @@ static int rpi_axi_pmu__init(struct rpi_axi_pmu *pmu, struct platform_device *pd
>  	spin_lock_init(&pmu->lock);
>  	mutex_init(&pmu->vpu_mutex);
>  
> -	pmu->chip = CHIP_BCM2835;
> +	pmu->chip = (enum rpi_axi_chip)(uintptr_t)of_device_get_match_data(dev);
>  
>  	pmu->pmu = (struct pmu) {
>  		.module = THIS_MODULE,

[Severity: High]
This is a pre-existing issue, but does this function expose the PMU to
userspace before driver initialization is complete?

Looking at rpi_axi_pmu__init(), the PMU is registered before ensuring all
initializations succeed:

drivers/perf/rpi_axi_pmu.c:rpi_axi_pmu__init() {
    ...
    ret = perf_pmu_register(&pmu->pmu, PMU_NAME, /*type=*/-1);
    if (ret) {
        ...
    }

    ret = cpuhp_state_add_instance(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
    if (ret) {
        dev_err(dev, "Failed to add cpuhp instance %d\n", ret);
        goto err_pmu_unregister;
    }
    ...
}

If cpuhp_state_add_instance() fails, the error path calls
perf_pmu_unregister(). However, if userspace opens the event during this
window, it will retain a reference to the struct pmu memory. That memory
gets freed by the devres framework when the probe fails, potentially causing
a use-after-free when userspace interacts with the open event.

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