Re: [PATCH v18 2/3] perf: Add Raspberry Pi BCM2835 AXI PMU driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] perf: rpi_axi_pmu: hrtimer_start() in rpi_axi_pmu_add() races
  and resets timer
--

commit 175632998831f232c8904487c2fedb162699a7b8
Author: Ian Rogers <irogers@google.com>

perf: Add Raspberry Pi BCM2835 AXI PMU driver

This commit adds a perf PMU driver for the Raspberry Pi System and VideoCore
AXI performance monitors on Broadcom BCM2835 and BCM2711 platforms.

> diff --git a/drivers/perf/rpi_axi_pmu.c b/drivers/perf/rpi_axi_pmu.c
> new file mode 100644
> index 0000000000000..c75118c7c902b
> --- /dev/null
> +++ b/drivers/perf/rpi_axi_pmu.c

[ ... ]

> +static enum hrtimer_restart rpi_axi_pmu_timer_handler(struct hrtimer *timer)
> +{
> +	struct rpi_axi_pmu *pmu = container_of(timer, struct rpi_axi_pmu, hrtimer);
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&pmu->lock, flags);
> +	if (pmu->active_events == 0) {
> +		raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +		return HRTIMER_NORESTART;
> +	}
> +	for (int i = 0; i < RPI_AXI_MAX_EVENTS; i++) {
> +		struct perf_event *event = pmu->events[i];
> +		enum counter counter;
> +		enum monitor mon;
> +		u32 new_count;
> +
> +		if (!event || (event->hw.state & PERF_HES_STOPPED))
> +			continue;
> +		mon = config_to_monitor(event->attr.config);
> +		if (pmu->monitor[mon].use_mailbox_interface)
> +			continue;
> +		counter = config_to_counter(event->attr.config);
> +		new_count = rpi_axi_pmu_read_counter(pmu, mon,
> +						    event->hw.idx, counter);
> +		rpi_axi_pmu_event_update(event, new_count);
> +	}
> +
> +	if (pmu->active_vpu_events > 0 && pmu->monitor[MON_VPU].use_mailbox_interface)
> +		schedule_work(&pmu->vpu_work);
> +	hrtimer_forward_now(timer, RPI_AXI_PMU_TIMER_INTERVAL);
> +	raw_spin_unlock_irqrestore(&pmu->lock, flags);
> +	return HRTIMER_RESTART;
> +}

[ ... ]

> +static int rpi_axi_pmu_add(struct perf_event *event, int flags)
> +{

[ ... ]

> +	pmu->active_events++;
> +	if (mon == MON_VPU)
> +		pmu->active_vpu_events++;
> +	if (pmu->active_events == 1)
> +		hrtimer_start(&pmu->hrtimer, RPI_AXI_PMU_TIMER_INTERVAL,
> +			      HRTIMER_MODE_REL_SOFT);

[Severity: Medium]
Can calling hrtimer_start() on every 0-to-1 transition of pmu->active_events
race with rpi_axi_pmu_timer_handler() and starve polling during event
multiplexing?

rpi_axi_pmu_del() decrements pmu->active_events to 0 without canceling
pmu->hrtimer.  If pmu->active_events drops to 0 and rises back to 1 (for
example during perf_rotate_context()) after __run_hrtimer() has dequeued
pmu->hrtimer and before rpi_axi_pmu_timer_handler() acquires pmu->lock,
hrtimer_start() re-enqueues the timer while the callback is about to run
hrtimer_forward_now():

<softirq>                             <hardirq> / CPU1
__run_hrtimer()
  timer->is_queued = 0
  rpi_axi_pmu_timer_handler()
                                      perf_mux_hrtimer_handler()
                                        ...
                                        rpi_axi_pmu_del()
                                          pmu->active_events-- (0)
                                        rpi_axi_pmu_add()
                                          pmu->active_events++ (1)
                                          hrtimer_start(&pmu->hrtimer, ...)
                                            timer->is_queued = 1
    raw_spin_lock_irqsave(&pmu->lock)
    hrtimer_forward_now(timer, ...)
      WARN_ON(timer->is_queued)       <-- triggers warning

In addition, because perf_rotate_context() deletes all active events
(pmu->active_events -> 0) and immediately adds the next events
(pmu->active_events -> 1), hrtimer_start() pushes the relative 100ms expiry
forward on every rotation.  Whenever the multiplexing interval is shorter
than 100ms, rpi_axi_pmu_timer_handler() never fires, starving 32-bit MMIO
overflow polling and vpu_work.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107040913.1384033-1-irogers@google.com?part=2