Re: [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler

[Richard Cheng <icheng@nvidia.com>]

On Fri, Jul 31, 2026 at 04:28:27PM +0800, Dave Jiang wrote:
> perf_pmu_unregister() frees pmu->dev without clearing the pointer, and
> cxl_pmu_probe() registers its devm actions so that teardown runs
> perf_pmu_unregister() first, then the hotplug instance removal, then
> free_irq(). Nothing before free_irq() masks the interrupt, so the handler
> stays live across a window where info->pmu.dev is freed and its dev_dbg()
> walks that pointer.
> 
> Unsharing the interrupt does not close that window. cxl_pmu_event_stop()
> leaves the counter's overflow status bit set, and only the handler clears
> it, so an overflow taken just before teardown is still delivered and still
> gets past the "did anything overflow" early-out. It lands in the !event
> branch, where the dev_dbg() is.
> 
> Log through info->pmu.parent instead, which is devm-managed and outlives
> every teardown action.
> 
> Clear the overflow status before requesting the interrupt too, since the
> driver never touched it at probe and a counter left enabled with
> INT_ON_OVRFLW by firmware or a previous kernel can raise an interrupt at
> any point.
> 
> Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
> Reported-by: sashiko-bot@kernel.org
> Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> v3:
> - Clear CXL_PMU_OVERFLOW_REG at probe, before the handler is armed. A stale
>   status bit from firmware can cause overflow interrupt. (Robin)
> - Ack dropped as the patch grew a hunk.
> - Drop the cxl_pmu_offline_cpu() hunk. That dev_err() cannot be reached.
> ---
>  drivers/perf/cxl_pmu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
> index 37742ce43d9f..3683427fbb7e 100644
> --- a/drivers/perf/cxl_pmu.c
> +++ b/drivers/perf/cxl_pmu.c
> @@ -806,7 +806,7 @@ static irqreturn_t cxl_pmu_irq(int irq, void *data)
>  		struct perf_event *event = info->hw_events[i];
>  
>  		if (!event) {
> -			dev_dbg(info->pmu.dev,
> +			dev_dbg(info->pmu.parent,
>  				"overflow but on non enabled counter %d\n", i);
>  			continue;
>  		}
> @@ -903,6 +903,15 @@ static int cxl_pmu_probe(struct device *dev)
>  	if (!irq_name)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Clear any overflow status left set by firmware or a previous kernel
> +	 * before the handler goes live, so it cannot mistake a stale bit for an
> +	 * overflow on a counter no event owns yet. The register is RW1C, and
> +	 * bits above the implemented counters are RsvdZ, so only write those.
> +	 */
> +	writeq(GENMASK_ULL(info->num_counters - 1, 0),
> +	       info->base + CXL_PMU_OVERFLOW_REG);
> +

Hi Dave,

I have a question here, since you are adding a clear, event_start() covers more than probe does. The block is frozen there, and it closes the reuse case as well as the boot one.
If a counter's overflow MSI is still in flight when perf reschedule that counter
to another event, the handler charges the full period to an event whose
prev_count was just zeroed.

Also if FW can leave a counter enabled, I'm not sure whether it's possible or not, but if that's the case, is clearing the status bit enough?
A leftoever counter with Global Freeze on Overflow but not Interrupt on Overflow
will wrap, freeze the whole CPMU, and raise nothing.

Best regards,
Richard Cheng.



>  	/*
>  	 * The handler must run on info->on_cpu, so the interrupt cannot be
>  	 * shared - IRQF_NOBALANCING is only honoured for the first action on a
> -- 
> 2.55.0
> 
>