Re: perf: Add Raspberry Pi BCM2835 AXI PMU driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware monitors are not disabled on module unload due to cancel_work_sync().
- [Low] Out-of-tree version compatibility macro blocks upstream integration.
--

--- Patch [1]: perf: Add Raspberry Pi BCM2835 AXI PMU driver ---
commit bbd5d9f81c5427301be257b124dcb9150ad69d8f
Author: Ian Rogers <[email protected]>

perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit adds a new performance monitoring driver for the Raspberry Pi
AXI bus (BCM2835/2711), exposing system-level and VideoCore PMU hardware to
the Linux perf subsystem.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..676e8f873163f
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c
> @@ -0,0 +1,2450 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[ ... ]
> +#include <linux/io.h>
> +#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)
> +{
> +	hrtimer_init(timer, clock_id, mode);
> +	timer->function = function;
> +}
> +#define hrtimer_setup rpi_hrtimer_setup
> +#endif

[Severity: Low]
This isn't a bug, but does this violate upstream kernel submission
guidelines by retaining out-of-tree backward compatibility code?

Upstream policy generally prohibits including LINUX_VERSION_CODE checks for
backward compatibility in mainline submissions.

[ ... ]
> +static void rpi_axi_pmu_del(struct perf_event *event, int flags)
> +{
[ ... ]
> +			if (mon == MON__SYSTEM) {
> +				pmu->monitor[MON__SYSTEM].hw_events.enabled[idx] = false;
> +				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) {
> +				pmu->monitor[MON__VPU].hw_events.enabled[idx] = false;
> +				pmu->monitor[MON__VPU].hw_events.vpu_disable_pending[idx] = true;
> +				schedule_work(&pmu->vpu_work);
> +			}
> +		}
> +	}
[ ... ]
> +static void rpi_axi_pmu__exit(struct rpi_axi_pmu *pmu)
> +{
> +	cpuhp_state_remove_instance_nocalls(rpi_axi_pmu_cpuhp_state, &pmu->cpuhp_node);
> +	perf_pmu_unregister(&pmu->pmu);
> +	hrtimer_cancel(&pmu->hrtimer);
> +	cancel_work_sync(&pmu->vpu_work);

[Severity: Medium]
Will this cancel_work_sync() drop the deferred hardware disable commands
scheduled by rpi_axi_pmu_del() during perf_pmu_unregister()?

If the module is unloaded while VPU perf events are active,
perf_pmu_unregister() detaches events and calls rpi_axi_pmu_del(). For VPU
events, this schedules vpu_work to disable the hardware via Mailbox IPC.
Since cancel_work_sync() drops the pending work instead of executing it,
the VideoCore VPU monitor might be left running indefinitely.

Would flush_work() be more appropriate here to ensure the hardware is properly
disabled?

> +	if (pmu->firmware)
> +		rpi_firmware_put(pmu->firmware);
> +}

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