Re: [PATCH] platform/x86/intel-uncore-freq: fix inconsistent state on init failure

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Mon, 2025-04-14 at 13:41 +0300, Ilpo Järvinen wrote:
> On Mon, 14 Apr 2025, shouyeliu wrote:
> 
> > When uncore_event_cpu_online() fails to initialize a control CPU
> > (e.g.,
> > due to memory allocation failure or uncore_freq_add_entry()
> > errors),
> > the code leaves stale entries in uncore_cpu_mask after that online
> > CPU
> > will not try to call uncore_freq_add_entry, resulting in no sys
> > interface.
> 
> Please add () after any name that refers to a C function (you're not
> even 
> being consistent here as you had it in some cases but not here).
> 
> Please try to split the very long sentence a bit and make it more
> obvious 
> what causes what as the current wording is a bit vague, did you mean:
> uncore_event_cpu_online() will not call uncore_freq_add_entry() for
> another CPU that is being onlined or something along those lines?
> 
> Will this change work/matter? Documentation/core-api/cpu_hotplug.rst
> says 
> about cpuhp_setup_state():
> 
> "If a callback fails for CPU N then the teardown callback for CPU
>  0 .. N-1 is invoked to rollback the operation. The state setup
> fails,
>  the callbacks for the state are not installed and in case of dynamic
>  allocation the allocated state is freed."
> 

Yes, cpuhp_setup_state() will fail and which will result in clean up.
So any fail of any fail uncore_event_cpu_online() will result in no sys
entries.

I think here the intention is to keep sys entries, which will not
happen with this patch.

For confirmation on 6.14 kernel, I forced failure on CPU 10:

[595799.696873] intel_uncore_init 
[595799.700102] uncore_event_cpu_online cpu:0
[595799.704240] uncore_event_cpu_online cpu:1
[595799.708360] uncore_event_cpu_online cpu:2
[595799.712505] uncore_event_cpu_online cpu:3
[595799.716633] uncore_event_cpu_online cpu:4
[595799.720755] uncore_event_cpu_online cpu:5
[595799.724953] uncore_event_cpu_online cpu:6
[595799.729158] uncore_event_cpu_online cpu:7
[595799.733409] uncore_event_cpu_online cpu:8
[595799.737674] uncore_event_cpu_online cpu:9
[595799.741954] uncore_event_cpu_online cpu:10
[595799.746134] Force CPU 10 to fail online
[595799.750182] uncore_event_cpu_offline cpu:0
[595799.754508] uncore_event_cpu_offline cpu:1
[595799.758834] uncore_event_cpu_offline cpu:2
[595799.763238] uncore_event_cpu_offline cpu:3
[595799.767558] uncore_event_cpu_offline cpu:4
[595799.771832] uncore_event_cpu_offline cpu:5
[595799.776178] uncore_event_cpu_offline cpu:6
[595799.780506] uncore_event_cpu_offline cpu:7
[595799.784862] uncore_event_cpu_offline cpu:8
[595799.789247] uncore_event_cpu_offline cpu:9
[595799.793540] intel_uncore_init cpuhp_setup_state failed
[595799.798776] intel_uncore_init failed


Thanks,
Srinivas



> > 
> 
> Fixes tag?
> 
> > Signed-off-by: shouyeliu <shouyeliu@gmail.com>
> 
> The correct format for tags is documented in 
> Documentation/process/5.Posting.rst:
> 
> tag: Full Name <email address>
> 
> > ---
> >  .../x86/intel/uncore-frequency/uncore-frequency.c    | 12
> > ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > index 40bbf8e45fa4..1de0a4a9d6cd 100644
> > --- a/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > +++ b/drivers/platform/x86/intel/uncore-frequency/uncore-
> > frequency.c
> > @@ -146,15 +146,13 @@ static int uncore_event_cpu_online(unsigned
> > int cpu)
> >  {
> >  	struct uncore_data *data;
> >  	int target;
> > +	int ret;
> >  
> >  	/* Check if there is an online cpu in the package for
> > uncore MSR */
> >  	target = cpumask_any_and(&uncore_cpu_mask,
> > topology_die_cpumask(cpu));
> >  	if (target < nr_cpu_ids)
> >  		return 0;
> >  
> > -	/* Use this CPU on this die as a control CPU */
> > -	cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > -
> >  	data = uncore_get_instance(cpu);
> >  	if (!data)
> >  		return 0;
> > @@ -163,7 +161,13 @@ static int uncore_event_cpu_online(unsigned
> > int cpu)
> >  	data->die_id = topology_die_id(cpu);
> >  	data->domain_id = UNCORE_DOMAIN_ID_INVALID;
> >  
> > -	return uncore_freq_add_entry(data, cpu);
> > +	ret = uncore_freq_add_entry(data, cpu);
> > +	if (!ret) {
> > +		/* Use this CPU on this die as a control CPU */
> > +		cpumask_set_cpu(cpu, &uncore_cpu_mask);
> > +	}
> > +
> > +	return ret;
> 
> Please reverse to logic such that you return early on error, which is
> the 
> usual error handling pattern.
> 
> >  }
> >  
> >  static int uncore_event_cpu_offline(unsigned int cpu)
> > 
>