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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Concurrent execution of `rpi_axi_pmu_del()`, `rpi_axi_pmu_add()`, and the background workqueue can leave a VPU hardware bus watcher permanently disabled due to unprotected mutation of the `vpu_disable_pending` flag.
- [Low] The BCM2712 (Raspberry Pi 5) specific event aliases referenced in the `rpi_axi_pmu_events_attrs` array are missing their macro definitions, causing compilation to fail.
- [Low] Usage of LINUX_VERSION_CODE check for out-of-tree backward compatibility in a mainline patch.
--

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

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/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..f7e5520d02eb9
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
> @@ -0,0 +1,2009 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/**
> + * DOC: Raspberry Pi AXI Bus Performance Monitoring Unit (PMU) Driver
> + *
> + * This driver exposes the performance monitoring hardware on Raspberry Pi

[ ... ]

> +#include <linux/version.h>
> +
> +#if LINUX_VERSION_CODE < KERNEL_VERSION(6, 13, 0)
> +static inline void rpi_hrtimer_setup(struct hrtimer *timer,
> +				     enum hrtimer_restart (*function)(struct hrtimer *),
> +				     clockid_t clock_id, enum hrtimer_mode mode)
> +{

[Severity: Low]
Should this out-of-tree backward compatibility check using LINUX_VERSION_CODE
be included in a mainline patch? Typically, mainline submissions only target
the current kernel version and avoid legacy version checks.

> +	hrtimer_init(timer, clock_id, mode);
> +	timer->function = function;
> +}
> +#define hrtimer_setup rpi_hrtimer_setup
> +#endif

[ ... ]

> +static int rpi_axi_hw_events__get_alloc_event_idx(struct rpi_axi_hw_events *hw_events,
> +						  const struct perf_event *event)
> +{
> +	int bus = config_to_bus(event->attr.config);
> +	int filter = config_to_filter(event->attr.config);
> +
> +	for (int i = 0; i < NUM_BUS_WATCHERS_PER_MONITOR; i++) {
> +		if (hw_events->monitored_bus[i] == bus && hw_events->filter[i] == filter) {
> +			hw_events->refcount[i]++;
> +			return i;
> +		}
> +	}
> +	if (hw_events->num_monitored == NUM_BUS_WATCHERS_PER_MONITOR)
> +		return -1;
> +
> +	for (int i = 0; i < NUM_BUS_WATCHERS_PER_MONITOR; i++) {
> +		if (hw_events->monitored_bus[i] == -1) {
> +			hw_events->monitored_bus[i] = bus;
> +			hw_events->filter[i] = filter;
> +			hw_events->refcount[i] = 1;

[Severity: Medium]
Can a newly allocated VPU watcher be immediately and permanently disabled due
to a stale vpu_disable_pending flag?

If a concurrent rpi_axi_pmu_del() frees a watcher and sets:
   vpu_disable_pending[idx] = true;
   enabled[idx] = false;
   schedule_work(&pmu->vpu_work);

And then rpi_axi_hw_events__get_alloc_event_idx() reuses this idx for a new
event before the workqueue runs, it does not clear vpu_disable_pending here.

When rpi_axi_pmu_vpu_work_handler() later processes the new event, it will
enable it in hardware and set enabled[idx] = true. However, on the next run,
the handler will see the stale vpu_disable_pending == true, disable the
hardware, but skip re-enabling it because enabled[idx] is already true,
leaving the hardware counter silently stopped.

> +			hw_events->num_monitored++;
> +			return i;
> +		}
> +	}
> +	return -1;
> +}

[ ... ]

> +	/* Filtered Event Aliases (RPi 1-4) */
> +	&rpi_axi_pmu_event_cpu0_icache_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_cpu0_dcache_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_cpu0_dcache_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_cpu1_icache_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_cpu1_dcache_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_cpu1_dcache_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_dma0_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_dma0_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_dma1_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_dma1_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_v3d0_system_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_v3d0_system_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_v3d1_system_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_v3d1_system_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_hvs_system_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_hvs_system_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_isp_system_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_isp_system_l2_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_usb_system_l2_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_usb_system_l2_wtrans.attr.attr,
> +
> +	/* RPi 5 (BCM2712) Expanded Event Aliases */
> +	&rpi_axi_pmu_event_pcie_rp1_atrans.attr.attr,
> +	&rpi_axi_pmu_event_pcie_rp1_atwait.attr.attr,

[Severity: Low]
Are these BCM2712 event aliases missing their corresponding macro definitions?
It looks like the array references variables like
rpi_axi_pmu_event_pcie_rp1_atrans that were never declared with
PMU_EVENT_ATTR_STRING, which would cause a compiler error for undefined
identifiers when building the driver.

> +	&rpi_axi_pmu_event_pcie_rp1_wtrans.attr.attr,
> +	&rpi_axi_pmu_event_pcie_rp1_wtwait.attr.attr,
> +	&rpi_axi_pmu_event_pcie_rp1_rtrans.attr.attr,
> +	&rpi_axi_pmu_event_pcie_rp1_rtwait.attr.attr,
> +
> +	&rpi_axi_pmu_event_hevc_dec_atrans.attr.attr,

[ ... ]

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