Re: [PATCH 6/6] perf/x86/intel/uncore: Implement global init callback for GNR uncore

["Chen, Zide" <zide.chen@intel.com>]

On 5/12/2026 9:25 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The return value of `wrmsrq_on_cpu()` is ignored in `uncore_msr_global_init()`, masking hardware initialization failures caused by a CPU hotplug TOCTOU race.
> --
> 
> commit d795e124f297e8f2c0cfee9efd29cdf4ec1993ce
> Author: Zide Chen <zide.chen@intel.com>
> 
> perf/x86/intel/uncore: Implement global init callback for GNR uncore
> 
> This commit adds a generic uncore_msr_global_init() callback for GNR, GRR,
> SRF, and CWF. It writes 0 to the global control register during boot to
> ensure the UBOX PMON units are properly initialized.
> 
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index 19056514b0812..a7780c5cd419a 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
> 
> [ ... ]
> 
>> @@ -1731,6 +1731,17 @@ static int uncore_mmio_global_init(u64 ctl)
>>  	return 0;
>>  }
>>  
>> +static int uncore_msr_global_init(int die, u64 msr)
>> +{
>> +	int cpu = uncore_die_to_cpu(die);
>> +
>> +	if (cpu == -1)
>> +		return -ENODEV;
>> +
>> +	wrmsrq_on_cpu(cpu, msr, 0);
>> +	return 0;
>> +}
> 
> uncore_die_to_cpu() acquires and releases cpus_read_lock() to find an
> online CPU, meaning the selected CPU could be hot-unplugged concurrently
> before wrmsrq_on_cpu() executes.
> 
> If the CPU goes offline, the underlying smp_call_function_single() will
> fail and wrmsrq_on_cpu() will return an error.

As you pointed out in PATCH 4/6, I would move the lock out of
uncore_die_to_cpu() and rely on the caller to manage the lock, then
between uncore_die_to_cpu() and wrmsrq_on_cpu(), CPU hot-unplug cannot
occur.
> Because this function unconditionally returns 0, the caller will assume
> success and proceed to register the PMON units. As noted in the commit
> message, the units will not work unless the MSR is written, which could
> lead to a silent functional failure.
> 
> Does this code need to check and propagate the return value of
> wrmsrq_on_cpu() to prevent this regression?

No, wrmsrq_on_cpu() returns 0 always.

>