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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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."

> 

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)
> 

-- 
 i.