Received-SPF: pass (google.com: domain of linux-kernel+bounces-138589-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 10 Apr 2024 14:20:31 +0100
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To: Miguel Luis <miguel.luis@oracle.com>
CC: "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <lenb@kernel.org>,
	<linux-acpi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<rmk+kernel@armlinux.org.uk>
Subject: Re: [RFC PATCH 2/4] ACPI: processor: refactor
 acpi_processor_get_info: isolate cpu hotpug init delay
Message-ID: <20240410142031.00007036@Huawei.com>
In-Reply-To: <20240409150536.9933-3-miguel.luis@oracle.com>
References: <20240409150536.9933-1-miguel.luis@oracle.com>
	<20240409150536.9933-3-miguel.luis@oracle.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit

On Tue,  9 Apr 2024 15:05:31 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> Delaying a hotplugged CPU initialization depends on
> CONFIG_ACPI_HOTPLUG_CPU. Isolate that.
> 
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>

Again, needs more explanation. Post the full set with the v4 vCPU
HP patches on top of this so we can see how it is used.

I guess the aim here is to share the bulk of this code between
the present and enabled paths? Whilst I think they should look
more similar actual code sharing seems like a bad idea for a
couple of reasons.

Imagine an arch that supports both present and enabled setting (so vCPU HP and
CPU HP) on that this function will be defined but will not be the right
thing to do for vCPU HP.  Note that in theory this is true of x86 but no one
has added support for the 'online capable bit' yet.

The impression for the _present() path will be that acpi_process_hotplug_delay_init()
should be called, and that's not true.  That should be obvious in the code
not hidden behind a stubbed out function.

Finally, you've pulled acpi_process_enumearte_extra out of the CONFIG_ACPI_HOTPLUG_CPU
block and I'm fairly sure it still has acpi_map_cpu() calls which aren't
defined yet for now ACPI_HOTPLUG_CPU configs.

Jonathan

> ---
>  drivers/acpi/acpi_processor.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 37e8b69113dd..9ea58b61d741 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -184,7 +184,22 @@ static void __init acpi_pcc_cpufreq_init(void) {}
>  
>  /* Initialization */
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> -static int acpi_processor_hotadd_init(struct acpi_processor *pr)
> +static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
> +{
> +	/*
> +	 * CPU got hot-added, but cpu_data is not initialized yet.  Set a flag
> +	 * to delay cpu_idle/throttling initialization and do it when the CPU
> +	 * gets online for the first time.
> +	 */
> +	pr_info("CPU%d has been hot-added\n", pr->id);
> +	pr->flags.need_hotplug_init = 1;
> +}
> +#else
> +static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> +
> +/* Enumerate extra CPUs */
> +static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>  {
>  	unsigned long long sta;
>  	acpi_status status;
> @@ -210,25 +225,12 @@ static int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  		goto out;
>  	}
>  
> -	/*
> -	 * CPU got hot-added, but cpu_data is not initialized yet.  Set a flag
> -	 * to delay cpu_idle/throttling initialization and do it when the CPU
> -	 * gets online for the first time.
> -	 */
> -	pr_info("CPU%d has been hot-added\n", pr->id);
> -	pr->flags.need_hotplug_init = 1;
> -
> +	acpi_processor_hotplug_delay_init(pr);
>  out:
>  	cpus_write_unlock();
>  	cpu_maps_update_done();
>  	return ret;
>  }
> -#else
> -static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
> -{
> -	return -ENODEV;
> -}
> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>  
>  static int acpi_evaluate_processor(struct acpi_device *device,
>  				   struct acpi_processor *pr,
> @@ -347,7 +349,7 @@ static int acpi_processor_get_info(struct acpi_device *device)
>  	 *  because cpuid <-> apicid mapping is persistent now.
>  	 */
>  	if (invalid_logical_cpuid(pr->id) || !cpu_present(pr->id)) {
> -		int ret = acpi_processor_hotadd_init(pr);
> +		int ret = acpi_processor_enumerate_extra(pr);
>  
>  		if (ret)
>  			return ret;