Received-SPF: pass (google.com: domain of linux-kernel+bounces-138606-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:31:01 +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 4/4] ACPI: processor: refactor
 acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU
Message-ID: <20240410143101.00001f5a@Huawei.com>
In-Reply-To: <20240409150536.9933-5-miguel.luis@oracle.com>
References: <20240409150536.9933-1-miguel.luis@oracle.com>
	<20240409150536.9933-5-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:33 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> acpi_unmap_cpu is architecture dependent. Isolate it.
> The pre-processor guard for detach may now be restricted to
> cpu unmap.
> 
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
Again the why question isn't answered by the patch description.

I assume this is to try and resolve the remove question of releasing
resources that was outstanding on vCPU HP v4 series Russell posted.

I've not looked as closely at the remove path as the add one yet, but
my gut feeling is same issue applies.
This code that runs in here should not be dependent on whether
CONFIG_ACPI_HOTPLUG_CPU is enabled or not.  What we do for the
make disabled flow should not run a few of the steps in 
acpi_processor_remove() we should make that clear by calling
a different function that doesn't have those steps.

> ---
>  drivers/acpi/acpi_processor.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index c6e2f64a056b..edcd6a8d4735 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -492,6 +492,14 @@ static int acpi_processor_add(struct acpi_device *device,
>  }
>  
>  #ifdef CONFIG_ACPI_HOTPLUG_CPU
> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr)
> +{
> +	acpi_unmap_cpu(pr->id);
> +}
> +#else
> +static void acpi_processor_hotunplug_unmap_cpu(struct acpi_processor *pr) {}
> +#endif /* CONFIG_ACPI_HOTPLUG_CPU */
> +
>  /* Removal */
>  static void acpi_processor_remove(struct acpi_device *device)
>  {
> @@ -524,7 +532,7 @@ static void acpi_processor_remove(struct acpi_device *device)
>  
>  	/* Remove the CPU. */
>  	arch_unregister_cpu(pr->id);
> -	acpi_unmap_cpu(pr->id);
> +	acpi_processor_hotunplug_unmap_cpu(pr);
>  
>  	cpus_write_unlock();
>  	cpu_maps_update_done();
> @@ -535,7 +543,6 @@ static void acpi_processor_remove(struct acpi_device *device)
>  	free_cpumask_var(pr->throttling.shared_cpu_map);
>  	kfree(pr);
>  }
> -#endif /* CONFIG_ACPI_HOTPLUG_CPU */
>  
>  #ifdef CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC
>  bool __init processor_physically_present(acpi_handle handle)
> @@ -660,9 +667,7 @@ static const struct acpi_device_id processor_device_ids[] = {
>  static struct acpi_scan_handler processor_handler = {
>  	.ids = processor_device_ids,
>  	.attach = acpi_processor_add,
> -#ifdef CONFIG_ACPI_HOTPLUG_CPU
>  	.detach = acpi_processor_remove,
> -#endif
>  	.hotplug = {
>  		.enabled = true,
>  	},