Re: [PATCH V3 8/8] perf/x86/intel/uncore: Implement lazy setup for MSR/MMIO PMUs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The `pmu->activeboxes` counter can underflow during CPU offline if `uncore_box_setup()` fails for an MSR/MMIO PMU lacking an `init_box` callback.
--

commit be5a594990bc5ad6318159289b770e88f7e8958d
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Implement lazy setup for MSR/MMIO PMUs

This commit delays the registration of MSR and MMIO uncore PMUs until the
first box is successfully initialized, applying the same lazy registration
model used by PCI uncore PMUs. Unregistration is deferred until the last
box exits, while box allocation remains at module init/exit time.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index b9ac2f7d31caa..9f8a80c9dcb66 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -1570,8 +1570,13 @@ static void uncore_box_unref(struct intel_uncore_type **types, int die)
>  		pmu = type->pmus;
>  		for (i = 0; i < type->num_boxes; i++, pmu++) {
>  			box = pmu->boxes[die];
> -			if (box && box->cpu >= 0 && atomic_dec_return(&box->refcnt) == 0)
> +			if (box && box->cpu >= 0 &&
> +			    atomic_dec_return(&box->refcnt) == 0) {
> +				if (uncore_box_active(box) &&
> +				    atomic_dec_return(&pmu->activeboxes) == 0)

[Severity: Medium]
Could this atomic decrement cause pmu->activeboxes to underflow during CPU
offline?

If uncore_box_setup() fails during initialization for a PMU that lacks an
init_box callback, the activeboxes counter won't be incremented.

Because uncore_box_active() unconditionally returns true when init_box is
NULL, wouldn't uncore_box_unref() still decrement pmu->activeboxes here and
cause it to become negative?

> +					uncore_pmu_unregister(pmu);
>  				uncore_box_exit(box);
> +			}
>  		}
>  	}
>  }

[ ... ]

> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
> index 0adb477d97086..c8dfa2d21bfd9 100644
> --- a/arch/x86/events/intel/uncore.h
> +++ b/arch/x86/events/intel/uncore.h
> @@ -568,6 +568,12 @@ static inline u64 uncore_read_counter(struct intel_uncore_box *box,
>  	return box->pmu->type->ops->read_counter(box, event);
>  }
>  
> +static inline bool uncore_box_active(struct intel_uncore_box *box)
> +{
> +	return (!box->pmu->type->ops->init_box ||
> +		test_bit(UNCORE_BOX_FLAG_INITIALIZED, &box->flags));
> +}

[Severity: Medium]
Is it safe to unconditionally return true when init_box is NULL?

If uncore_box_setup() fails and marks the PMU as broken, this definition
causes uncore_box_unref() to still consider the box active and perform an
unbalanced decrement on the activeboxes counter.

> +
>  static inline int uncore_box_init(struct intel_uncore_box *box)
>  {
>  	int ret = 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611160033.66760-1-zide.chen@intel.com?part=8