[RFC PATCH 0/4] ACPI: processor: refactor acpi_processor_{get_info|remove}

[RFC PATCH 0/4] ACPI: processor: refactor acpi_processor_{get_info|remove}

[Miguel Luis]

Both acpi_processor_get_info and acpi_processor_remove functions have
architecture dependent functionality enabled via CONFIG_ACPI_HOTPLUG_CPU.

Current pre-processor guards are restricting too much of functionality which
makes it dificult to integrate other features such as Virtual CPU
hotplug/unplug for arm64.

This series, applied on top of v6.9-rc3, suggests a refactoring on these two
functions with the intent to understand them better and hopefully ease
integration of more functionality.

Apart from patches 2/4 and 3/4, which could be squashed but left them separated
intentionally so it would ease reviewing, changes are self-contained.

So far I've boot tested it successfully alone and as a prefix for vCPU hotplug/unplug
patches [1], on arm64.

[1]: https://lore.kernel.org/linux-arm-kernel/Zbp5xzmFhKDAgHws@shell.armlinux.org.uk/

Miguel Luis (4):
  ACPI: processor: refactor acpi_processor_get_info: evaluation of
    processor declaration
  ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug
    init delay
  ACPI: processor: refactor acpi_processor_get_info: isolate
    acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU
  ACPI: processor: refactor acpi_processor_remove: isolate
    acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

 drivers/acpi/acpi_processor.c | 138 ++++++++++++++++++++++------------
 1 file changed, 91 insertions(+), 47 deletions(-)

--
2.43.0

[RFC PATCH 1/4] ACPI: processor: refactor acpi_processor_get_info: evaluation of processor declaration

[Miguel Luis]

Isolate the evaluation of processor declaration into its own function.

No functional changes.

Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
---
 drivers/acpi/acpi_processor.c | 78 +++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 7a0dd35d62c9..37e8b69113dd 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -230,15 +230,59 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
 }
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
+static int acpi_evaluate_processor(struct acpi_device *device,
+				   struct acpi_processor *pr,
+				   union acpi_object *object,
+				   int *device_declaration)
+{
+	struct acpi_buffer buffer = { sizeof(union acpi_object), object };
+	acpi_status status = AE_OK;
+	unsigned long long value;
+
+	/*
+	 * Declarations via the ASL "Processor" statement are deprecated.
+	 */
+	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
+		/* Declared with "Processor" statement; match ProcessorID */
+		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
+		if (ACPI_FAILURE(status)) {
+			dev_err(&device->dev,
+				"Failed to evaluate processor object (0x%x)\n",
+				status);
+			return -ENODEV;
+		}
+
+		value = object->processor.proc_id;
+		goto out;
+	}
+
+	/*
+	 * Declared with "Device" statement; match _UID.
+	 */
+	status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
+					NULL, &value);
+	if (ACPI_FAILURE(status)) {
+		dev_err(&device->dev,
+			"Failed to evaluate processor _UID (0x%x)\n",
+			status);
+		return -ENODEV;
+	}
+
+	*device_declaration = 1;
+out:
+	pr->acpi_id = value;
+	return 0;
+}
+
 static int acpi_processor_get_info(struct acpi_device *device)
 {
 	union acpi_object object = { 0 };
-	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
 	struct acpi_processor *pr = acpi_driver_data(device);
 	int device_declaration = 0;
 	acpi_status status = AE_OK;
 	static int cpu0_initialized;
 	unsigned long long value;
+	int ret;
 
 	acpi_processor_errata();
 
@@ -252,32 +296,12 @@ static int acpi_processor_get_info(struct acpi_device *device)
 	} else
 		dev_dbg(&device->dev, "No bus mastering arbitration control\n");
 
-	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
-		/* Declared with "Processor" statement; match ProcessorID */
-		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
-		if (ACPI_FAILURE(status)) {
-			dev_err(&device->dev,
-				"Failed to evaluate processor object (0x%x)\n",
-				status);
-			return -ENODEV;
-		}
-
-		pr->acpi_id = object.processor.proc_id;
-	} else {
-		/*
-		 * Declared with "Device" statement; match _UID.
-		 */
-		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
-						NULL, &value);
-		if (ACPI_FAILURE(status)) {
-			dev_err(&device->dev,
-				"Failed to evaluate processor _UID (0x%x)\n",
-				status);
-			return -ENODEV;
-		}
-		device_declaration = 1;
-		pr->acpi_id = value;
-	}
+	/*
+	 * Evaluate processor declaration.
+	 */
+	ret = acpi_evaluate_processor(device, pr, &object, &device_declaration);
+	if (ret)
+		return ret;
 
 	if (acpi_duplicate_processor_id(pr->acpi_id)) {
 		if (pr->acpi_id == 0xff)
-- 
2.43.0

[RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay

[Miguel Luis]

Delaying a hotplugged CPU initialization depends on
CONFIG_ACPI_HOTPLUG_CPU. Isolate that.

Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
---
 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;
-- 
2.43.0

[RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

mapping and unmaping a cpu at the stage of extra cpu enumeration is
architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
isolate that functionality from architecture independent one.

Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
---
 drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 9ea58b61d741..c6e2f64a056b 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
 	pr_info("CPU%d has been hot-added\n", pr->id);
 	pr->flags.need_hotplug_init = 1;
 }
+static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
+{
+	return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
+}
+static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
+{
+	acpi_unmap_cpu(pr->id);
+}
 #else
 static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
+static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
+{
+	return 0;
+}
+static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
 #endif /* CONFIG_ACPI_HOTPLUG_CPU */
 
 /* Enumerate extra CPUs */
@@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
 	cpu_maps_update_begin();
 	cpus_write_lock();
 
-	ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
+	ret = acpi_processor_hotplug_map_cpu(pr);
 	if (ret)
 		goto out;
 
 	ret = arch_register_cpu(pr->id);
 	if (ret) {
-		acpi_unmap_cpu(pr->id);
+		acpi_processor_hotplug_unmap_cpu(pr);
 		goto out;
 	}
 
-- 
2.43.0

[RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

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>
---
 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,
 	},
-- 
2.43.0

Re: [RFC PATCH 1/4] ACPI: processor: refactor acpi_processor_get_info: evaluation of processor declaration

[Jonathan Cameron]

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

> Isolate the evaluation of processor declaration into its own function.
> 
> No functional changes.
> 
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>

Hi Miguel,

I'd like more description in each patch of 'why' the change is useful. 

A few comments inline.

Jonathan

> ---
>  drivers/acpi/acpi_processor.c | 78 +++++++++++++++++++++++------------
>  1 file changed, 51 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 7a0dd35d62c9..37e8b69113dd 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -230,15 +230,59 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
>  }
>  #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>  
> +static int acpi_evaluate_processor(struct acpi_device *device,
> +				   struct acpi_processor *pr,
> +				   union acpi_object *object,
> +				   int *device_declaration)

I'd use a bool * for device_declaration.

> +{
> +	struct acpi_buffer buffer = { sizeof(union acpi_object), object };
> +	acpi_status status = AE_OK;

Status always written so don't initialize it.

> +	unsigned long long value;
> +
> +	/*
> +	 * Declarations via the ASL "Processor" statement are deprecated.

Be clear where they are deprecated. i.e. the ACPI spec and which version and
under what circumstances. 

Or don't say it. From Linux kernel point of view we need to support this anyway
for a long long time, so knowing they are deprecated in the ACPI spec
isn't really of interest.

> +	 */
> +	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
> +		/* Declared with "Processor" statement; match ProcessorID */
> +		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(&device->dev,
> +				"Failed to evaluate processor object (0x%x)\n",
> +				status);
> +			return -ENODEV;
> +		}
> +
> +		value = object->processor.proc_id;
> +		goto out;

I'd keep the if / else form of the original code. I think it's easier to follow given
this really is choosing between 2 options.

> +	}
> +
> +	/*
> +	 * Declared with "Device" statement; match _UID.
> +	 */
> +	status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> +					NULL, &value);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(&device->dev,
> +			"Failed to evaluate processor _UID (0x%x)\n",
> +			status);
> +		return -ENODEV;
> +	}
> +
> +	*device_declaration = 1;
> +out:
> +	pr->acpi_id = value;

Maybe better to pass in the pr->handle, and return value so
pr->acpi_id is set at the caller rather than setting it in
this helper function?  That will keep the pr->x setting
all in one place.

> +	return 0;
> +}
> +
>  static int acpi_processor_get_info(struct acpi_device *device)
>  {
>  	union acpi_object object = { 0 };
> -	struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>  	struct acpi_processor *pr = acpi_driver_data(device);
>  	int device_declaration = 0;
>  	acpi_status status = AE_OK;
>  	static int cpu0_initialized;
>  	unsigned long long value;
> +	int ret;
>  
>  	acpi_processor_errata();
>  
> @@ -252,32 +296,12 @@ static int acpi_processor_get_info(struct acpi_device *device)
>  	} else
>  		dev_dbg(&device->dev, "No bus mastering arbitration control\n");
>  
> -	if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
> -		/* Declared with "Processor" statement; match ProcessorID */
> -		status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
> -		if (ACPI_FAILURE(status)) {
> -			dev_err(&device->dev,
> -				"Failed to evaluate processor object (0x%x)\n",
> -				status);
> -			return -ENODEV;
> -		}
> -
> -		pr->acpi_id = object.processor.proc_id;
> -	} else {
> -		/*
> -		 * Declared with "Device" statement; match _UID.
> -		 */
> -		status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
> -						NULL, &value);
> -		if (ACPI_FAILURE(status)) {
> -			dev_err(&device->dev,
> -				"Failed to evaluate processor _UID (0x%x)\n",
> -				status);
> -			return -ENODEV;
> -		}
> -		device_declaration = 1;
> -		pr->acpi_id = value;
> -	}
> +	/*
> +	 * Evaluate processor declaration.
Given function name (which is well named!) I don't see the comment adding anything.
So I'd drop the comment.
> +	 */
> +	ret = acpi_evaluate_processor(device, pr, &object, &device_declaration);
> +	if (ret)
> +		return ret;
>  
>  	if (acpi_duplicate_processor_id(pr->acpi_id)) {
>  		if (pr->acpi_id == 0xff)

Re: [RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay

[Jonathan Cameron]

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;

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

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

> mapping and unmaping a cpu at the stage of extra cpu enumeration is
> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
> isolate that functionality from architecture independent one.

Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
to make the arch specific nature of that call more obvious?
I think that has caused more confusion in the discussion than
whether it is hotplug specific or not.

As mentioned in patch 2, fairly sure this needs to go before that
patch.

Jonathan

> 
> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> ---
>  drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> index 9ea58b61d741..c6e2f64a056b 100644
> --- a/drivers/acpi/acpi_processor.c
> +++ b/drivers/acpi/acpi_processor.c
> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>  	pr_info("CPU%d has been hot-added\n", pr->id);
>  	pr->flags.need_hotplug_init = 1;
>  }
> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> +{
> +	return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> +}
> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
> +{
> +	acpi_unmap_cpu(pr->id);
> +}
>  #else
>  static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> +{
> +	return 0;
> +}
> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>  #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>  
>  /* Enumerate extra CPUs */
> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>  	cpu_maps_update_begin();
>  	cpus_write_lock();
>  
> -	ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> +	ret = acpi_processor_hotplug_map_cpu(pr);
>  	if (ret)
>  		goto out;
>  
>  	ret = arch_register_cpu(pr->id);
>  	if (ret) {
> -		acpi_unmap_cpu(pr->id);
> +		acpi_processor_hotplug_unmap_cpu(pr);
>  		goto out;
>  	}
>  

Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

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,
>  	},

Re: [RFC PATCH 0/4] ACPI: processor: refactor acpi_processor_{get_info|remove}

[Jonathan Cameron]

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

> Both acpi_processor_get_info and acpi_processor_remove functions have
> architecture dependent functionality enabled via CONFIG_ACPI_HOTPLUG_CPU.
> 
> Current pre-processor guards are restricting too much of functionality which
> makes it dificult to integrate other features such as Virtual CPU
> hotplug/unplug for arm64.
> 
> This series, applied on top of v6.9-rc3, suggests a refactoring on these two
> functions with the intent to understand them better and hopefully ease
> integration of more functionality.
> 
> Apart from patches 2/4 and 3/4, which could be squashed but left them separated
> intentionally so it would ease reviewing, changes are self-contained.
> 
> So far I've boot tested it successfully alone and as a prefix for vCPU hotplug/unplug
> patches [1], on arm64.

Hi Miguel,

Great to see an attempt to keep this moving. My apologies that I've been rather
quiet on this so far this cycle - a few things came up that ended up more urgent :(

In the thread you link there was a discussion on whether to stub out these functions
as a possible way forwards. I did some analysis of what was going on in 

https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@Huawei.com/

and my conclusion was that to do so would mostly be misleading.
The flows for make present and make enabled are and should be different
(though not as different as they were in v4!)

Jonathan

> 
> [1]: https://lore.kernel.org/linux-arm-kernel/Zbp5xzmFhKDAgHws@shell.armlinux.org.uk/
> 
> Miguel Luis (4):
>   ACPI: processor: refactor acpi_processor_get_info: evaluation of
>     processor declaration
>   ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug
>     init delay
>   ACPI: processor: refactor acpi_processor_get_info: isolate
>     acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU
>   ACPI: processor: refactor acpi_processor_remove: isolate
>     acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU
> 
>  drivers/acpi/acpi_processor.c | 138 ++++++++++++++++++++++------------
>  1 file changed, 91 insertions(+), 47 deletions(-)
> 
> --
> 2.43.0
> 

Re: [RFC PATCH 1/4] ACPI: processor: refactor acpi_processor_get_info: evaluation of processor declaration

[Miguel Luis]

Hi Jonathan,

> On 10 Apr 2024, at 13:13, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> On Tue,  9 Apr 2024 15:05:30 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
>> Isolate the evaluation of processor declaration into its own function.
>> 
>> No functional changes.
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> 
> Hi Miguel,
> 
> I'd like more description in each patch of 'why' the change is useful. 

Ack! Completely agree. This should be throughout the series while relying less
on the cover-letter then.

> 
> A few comments inline.
> 
> Jonathan
> 
>> ---
>> drivers/acpi/acpi_processor.c | 78 +++++++++++++++++++++++------------
>> 1 file changed, 51 insertions(+), 27 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 7a0dd35d62c9..37e8b69113dd 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -230,15 +230,59 @@ static inline int acpi_processor_hotadd_init(struct acpi_processor *pr)
>> }
>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> 
>> +static int acpi_evaluate_processor(struct acpi_device *device,
>> +    struct acpi_processor *pr,
>> +    union acpi_object *object,
>> +    int *device_declaration)
> 
> I'd use a bool * for device_declaration.

Agree.

> 
>> +{
>> + struct acpi_buffer buffer = { sizeof(union acpi_object), object };
>> + acpi_status status = AE_OK;
> 
> Status always written so don't initialize it.

Agree. 

> 
>> + unsigned long long value;
>> +
>> + /*
>> +  * Declarations via the ASL "Processor" statement are deprecated.
> 
> Be clear where they are deprecated. i.e. the ACPI spec and which version and
> under what circumstances. 

Ack.

> 
> Or don't say it. From Linux kernel point of view we need to support this anyway
> for a long long time, so knowing they are deprecated in the ACPI spec
> isn't really of interest.

As the initial effort is to understand it better it might be worth giving it a try.

> 
>> +  */
>> + if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
>> + /* Declared with "Processor" statement; match ProcessorID */
>> + status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
>> + if (ACPI_FAILURE(status)) {
>> + dev_err(&device->dev,
>> + "Failed to evaluate processor object (0x%x)\n",
>> + status);
>> + return -ENODEV;
>> + }
>> +
>> + value = object->processor.proc_id;
>> + goto out;
> 
> I'd keep the if / else form of the original code. I think it's easier to follow given
> this really is choosing between 2 options.

Now there’s only one ASL declaration in the deprecation list so far so the if /
else initial form suits too for readability, albeit thinking the suggested
pattern would help in the future when there’s a new statement to add on the
deprecation scope.

I’ll keep the original if / else form.

> 
>> + }
>> +
>> + /*
>> +  * Declared with "Device" statement; match _UID.
>> +  */
>> + status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
>> + NULL, &value);
>> + if (ACPI_FAILURE(status)) {
>> + dev_err(&device->dev,
>> + "Failed to evaluate processor _UID (0x%x)\n",
>> + status);
>> + return -ENODEV;
>> + }
>> +
>> + *device_declaration = 1;
>> +out:
>> + pr->acpi_id = value;
> 
> Maybe better to pass in the pr->handle, and return value so
> pr->acpi_id is set at the caller rather than setting it in
> this helper function?  That will keep the pr->x setting
> all in one place.

Got it! Let’s leave it to the caller.

> 
>> + return 0;
>> +}
>> +
>> static int acpi_processor_get_info(struct acpi_device *device)
>> {
>> union acpi_object object = { 0 };
>> - struct acpi_buffer buffer = { sizeof(union acpi_object), &object };
>> struct acpi_processor *pr = acpi_driver_data(device);
>> int device_declaration = 0;
>> acpi_status status = AE_OK;
>> static int cpu0_initialized;
>> unsigned long long value;
>> + int ret;
>> 
>> acpi_processor_errata();
>> 
>> @@ -252,32 +296,12 @@ static int acpi_processor_get_info(struct acpi_device *device)
>> } else
>> dev_dbg(&device->dev, "No bus mastering arbitration control\n");
>> 
>> - if (!strcmp(acpi_device_hid(device), ACPI_PROCESSOR_OBJECT_HID)) {
>> - /* Declared with "Processor" statement; match ProcessorID */
>> - status = acpi_evaluate_object(pr->handle, NULL, NULL, &buffer);
>> - if (ACPI_FAILURE(status)) {
>> - dev_err(&device->dev,
>> - "Failed to evaluate processor object (0x%x)\n",
>> - status);
>> - return -ENODEV;
>> - }
>> -
>> - pr->acpi_id = object.processor.proc_id;
>> - } else {
>> - /*
>> -  * Declared with "Device" statement; match _UID.
>> -  */
>> - status = acpi_evaluate_integer(pr->handle, METHOD_NAME__UID,
>> - NULL, &value);
>> - if (ACPI_FAILURE(status)) {
>> - dev_err(&device->dev,
>> - "Failed to evaluate processor _UID (0x%x)\n",
>> - status);
>> - return -ENODEV;
>> - }
>> - device_declaration = 1;
>> - pr->acpi_id = value;
>> - }
>> + /*
>> +  * Evaluate processor declaration.
> Given function name (which is well named!) I don't see the comment adding anything.
> So I'd drop the comment.

I’m glad it passed the naming test :)
I’ll remove the comment.

Thanks!

Miguel

>> +  */
>> + ret = acpi_evaluate_processor(device, pr, &object, &device_declaration);
>> + if (ret)
>> + return ret;
>> 
>> if (acpi_duplicate_processor_id(pr->acpi_id)) {
>> if (pr->acpi_id == 0xff)

Re: [RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay

[Miguel Luis]

> On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> 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.

In agreement.

> Post the full set with the v4 vCPU
> HP patches on top of this so we can see how it is used.
> 

I’ll get a link to a repo for the next version besides would like primarily to
establish acpi_processor_{get_info|remove} first since these changes
would need to live with and without vCPU HP.

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

That would be my understanding from comments on v4. Both present and
enabled paths do have common procedures up to certain point. IIUC, from .1
and .2 from comments [1] and [2] while .3 would be architecture specific code.

[1]: https://lore.kernel.org/linux-arm-kernel/CAJZ5v0iiJpUWq5GMSnKFWQTzn_bdwoQz9m=hDaXNg4Lj_ePF4g@mail.gmail.com/
[2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@Huawei.com/

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

… I agree with the above. It reinforces refactoring acpi_processor_get_info
so it clearly decouples present and enabled paths.

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

Ack. Need to check how we’re differentiating both paths.

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

Yep, it still has. Unless you squash the next patch into this one, which I
didn’t so one could see these changes progressively rather than
self-contained.

Miguel

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

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> On Tue,  9 Apr 2024 15:05:32 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
>> isolate that functionality from architecture independent one.
> 
> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
> to make the arch specific nature of that call more obvious?

Not sure about the pattern to use here but that seems fine to me. Current usage
is architectures export acpi_map_cpu from the acpi interface and do their
thing.

Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
it gets called on the code path?

1) export it and do nothing - it would be creating unnecessary dependency.

2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
into CONFIG_ACPI_HOTPLUG_CPU.

Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
used for CPU HP and the same applies to acpi_unmap_cpu.

> I think that has caused more confusion in the discussion than
> whether it is hotplug specific or not.

Indeed. Within the CPU HP path there are these arch specific intricacies.

> 
> As mentioned in patch 2, fairly sure this needs to go before that
> patch.

2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
CPU initialisation I think.

Miguel

> 
> Jonathan
> 
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>> ---
>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>> 1 file changed, 15 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>> index 9ea58b61d741..c6e2f64a056b 100644
>> --- a/drivers/acpi/acpi_processor.c
>> +++ b/drivers/acpi/acpi_processor.c
>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>> pr_info("CPU%d has been hot-added\n", pr->id);
>> pr->flags.need_hotplug_init = 1;
>> }
>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>> +{
>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>> +}
>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
>> +{
>> + acpi_unmap_cpu(pr->id);
>> +}
>> #else
>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>> +{
>> + return 0;
>> +}
>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>> 
>> /* Enumerate extra CPUs */
>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>> cpu_maps_update_begin();
>> cpus_write_lock();
>> 
>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>> + ret = acpi_processor_hotplug_map_cpu(pr);
>> if (ret)
>> goto out;
>> 
>> ret = arch_register_cpu(pr->id);
>> if (ret) {
>> - acpi_unmap_cpu(pr->id);
>> + acpi_processor_hotplug_unmap_cpu(pr);
>> goto out;
>> }
>> 
> 

Re: [RFC PATCH 2/4] ACPI: processor: refactor acpi_processor_get_info: isolate cpu hotpug init delay

[Jonathan Cameron]

On Wed, 10 Apr 2024 17:20:01 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> > On 10 Apr 2024, at 13:20, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > 
> > 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.  
> 
> In agreement.
> 
> > Post the full set with the v4 vCPU
> > HP patches on top of this so we can see how it is used.
> >   
> 
> I’ll get a link to a repo for the next version besides would like primarily to
> establish acpi_processor_{get_info|remove} first since these changes
> would need to live with and without vCPU HP.

Great.

> 
> > 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.  
> 
> That would be my understanding from comments on v4. Both present and
> enabled paths do have common procedures up to certain point. IIUC, from .1
> and .2 from comments [1] and [2] while .3 would be architecture specific code.
> 
> [1]: https://lore.kernel.org/linux-arm-kernel/CAJZ5v0iiJpUWq5GMSnKFWQTzn_bdwoQz9m=hDaXNg4Lj_ePF4g@mail.gmail.com/
> [2]: https://lore.kernel.org/linux-arm-kernel/20240322185327.00002416@Huawei.com/

3 is not just architecture specific code, it's architecture and action specific.
i.e. What is done in there should not happen in the present path.

From what is in [2] I became much less convinced much code should be shared..
Lightly editing where that thread went today, there is some shared code in
the make_present / make_enabled path, but not that much.
As per that discussion, cpu_maps_update* is harmless, but also pointless
and potentially misleading in the enable case.

static int acpi_processor_make_present(struct acpi_processor *pr)
{
        unsigned long long sta;
        acpi_status status;
        int ret;

        if (!IS_ENABLED(CONFIG_ACPI_HOTPLUG_CPU)) {
                pr_err_once("Changing CPU present bit is not supported\n");
                return -ENODEV;
        }

// The _STA check here is needed still or we need to push it into
// arch_register_cpu() on x86 similarly to proposal on arm64.

	status = acpi_evaluate_integer(pr->handle, "_STA", NULL, &sta);
        if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_PRESENT))
                return -ENODEV;

        if (invalid_phys_cpuid(pr->phys_id))
                return -ENODEV;
        cpu_maps_update_begin();
        cpus_write_lock();

        ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
        if (ret)
                goto out;

        ret = arch_register_cpu(pr->id);
        if (ret) {
                acpi_unmap_cpu(pr->id);
                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;

out:
        cpus_write_unlock();
        cpu_maps_update_done();
        return ret;
}

static int acpi_processor_make_enabled(struct acpi_processor *pr)
{
        unsigned long long sta;
        acpi_status status;
        bool present, enabled;
        int ret;

        if (invalid_phys_cpuid(pr->phys_id))
                return -ENODEV;

        cpus_write_lock();
        ret = arch_register_cpu(pr->id);
        cpus_write_unlock();

        return ret;
}

> 
> > 
> > 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.  
> 
> … I agree with the above. It reinforces refactoring acpi_processor_get_info
> so it clearly decouples present and enabled paths.
> 
> > 
> > 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.  
> 
> Ack. Need to check how we’re differentiating both paths.

I haven't looked as much at the remove path recently but for the enable path
the code that should run in the enable path is much less than in the present path.

> 
> > 
> > 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.  
> 
> Yep, it still has. Unless you squash the next patch into this one, which I
> didn’t so one could see these changes progressively rather than
> self-contained.
> 
I think that makes it non bisectable, so you can't do this.  Either don't move
that code until after the next patch, or squash the 2 together.

Less important in an RFC though,

Jonathan


> Miguel
> 
> > 
> > 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;  
> >   
> 

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

On Wed, 10 Apr 2024 18:29:34 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> > On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > 
> > On Tue,  9 Apr 2024 15:05:32 +0000
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >   
> >> mapping and unmaping a cpu at the stage of extra cpu enumeration is
> >> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
> >> isolate that functionality from architecture independent one.  
> > 
> > Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
> > to make the arch specific nature of that call more obvious?  
> 
> Not sure about the pattern to use here but that seems fine to me. Current usage
> is architectures export acpi_map_cpu from the acpi interface and do their
> thing.
> 
> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
> it gets called on the code path?

I'm not sure what you mean by dismisses?

Is missing perhaps?  If that is what you mean, I think it's a mistake to allow
that code to be called from a path that isn't dependent on
CONFIG_ACPI_HOTPLUG_CPU. It makes no sense to do so and stubbing it out to give
the impression that the calling it does make sense (when looking at the caller)
is misleading.

Jonathan


> 
> 1) export it and do nothing - it would be creating unnecessary dependency.
> 
> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
> into CONFIG_ACPI_HOTPLUG_CPU.
> 
> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
> used for CPU HP and the same applies to acpi_unmap_cpu.
> 
> > I think that has caused more confusion in the discussion than
> > whether it is hotplug specific or not.  
> 
> Indeed. Within the CPU HP path there are these arch specific intricacies.
> 
> > 
> > As mentioned in patch 2, fairly sure this needs to go before that
> > patch.  
> 
> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
> CPU initialisation I think.
> 
> Miguel
> 
> > 
> > Jonathan
> >   
> >> 
> >> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> >> ---
> >> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
> >> 1 file changed, 15 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >> index 9ea58b61d741..c6e2f64a056b 100644
> >> --- a/drivers/acpi/acpi_processor.c
> >> +++ b/drivers/acpi/acpi_processor.c
> >> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
> >> pr_info("CPU%d has been hot-added\n", pr->id);
> >> pr->flags.need_hotplug_init = 1;
> >> }
> >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> >> +{
> >> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> >> +}
> >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
> >> +{
> >> + acpi_unmap_cpu(pr->id);
> >> +}
> >> #else
> >> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
> >> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> >> +{
> >> + return 0;
> >> +}
> >> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
> >> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >> 
> >> /* Enumerate extra CPUs */
> >> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
> >> cpu_maps_update_begin();
> >> cpus_write_lock();
> >> 
> >> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> >> + ret = acpi_processor_hotplug_map_cpu(pr);
> >> if (ret)
> >> goto out;
> >> 
> >> ret = arch_register_cpu(pr->id);
> >> if (ret) {
> >> - acpi_unmap_cpu(pr->id);
> >> + acpi_processor_hotplug_unmap_cpu(pr);
> >> goto out;
> >> }
> >>   
> >   
> 

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> 
> On Wed, 10 Apr 2024 18:29:34 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
>>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>>> 
>>> On Tue,  9 Apr 2024 15:05:32 +0000
>>> Miguel Luis <miguel.luis@oracle.com> wrote:
>>> 
>>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
>>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
>>>> isolate that functionality from architecture independent one.  
>>> 
>>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
>>> to make the arch specific nature of that call more obvious?  
>> 
>> Not sure about the pattern to use here but that seems fine to me. Current usage
>> is architectures export acpi_map_cpu from the acpi interface and do their
>> thing.
>> 
>> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
>> it gets called on the code path?
> 
> I'm not sure what you mean by dismisses?
> 

I mean when acpi_map_cpu is not needed.

> Is missing perhaps?

Yes.

>  If that is what you mean, I think it's a mistake to allow
> that code to be called from a path that isn't dependent on
> CONFIG_ACPI_HOTPLUG_CPU.
> It makes no sense to do so and stubbing it out to give
> the impression that the calling it does make sense (when looking at the caller)
> is misleading.

OK, that would be what not to do.

acpi_processor_enumerate_extra could deal with make_present and make_enabled while
a stub would still be needed for make_present since it depends on
CONFIG_ACPI_HOTPLUG_CPU?

Miguel

> 
> Jonathan
> 
> 
>> 
>> 1) export it and do nothing - it would be creating unnecessary dependency.
>> 
>> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
>> into CONFIG_ACPI_HOTPLUG_CPU.
>> 
>> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
>> used for CPU HP and the same applies to acpi_unmap_cpu.
>> 
>>> I think that has caused more confusion in the discussion than
>>> whether it is hotplug specific or not.  
>> 
>> Indeed. Within the CPU HP path there are these arch specific intricacies.
>> 
>>> 
>>> As mentioned in patch 2, fairly sure this needs to go before that
>>> patch.  
>> 
>> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
>> CPU initialisation I think.
>> 
>> Miguel
>> 
>>> 
>>> Jonathan
>>> 
>>>> 
>>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>>> ---
>>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>>> index 9ea58b61d741..c6e2f64a056b 100644
>>>> --- a/drivers/acpi/acpi_processor.c
>>>> +++ b/drivers/acpi/acpi_processor.c
>>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>>>> pr_info("CPU%d has been hot-added\n", pr->id);
>>>> pr->flags.need_hotplug_init = 1;
>>>> }
>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>> +{
>>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>> +}
>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
>>>> +{
>>>> + acpi_unmap_cpu(pr->id);
>>>> +}
>>>> #else
>>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>>>> 
>>>> /* Enumerate extra CPUs */
>>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>>>> cpu_maps_update_begin();
>>>> cpus_write_lock();
>>>> 
>>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>> + ret = acpi_processor_hotplug_map_cpu(pr);
>>>> if (ret)
>>>> goto out;
>>>> 
>>>> ret = arch_register_cpu(pr->id);
>>>> if (ret) {
>>>> - acpi_unmap_cpu(pr->id);
>>>> + acpi_processor_hotplug_unmap_cpu(pr);
>>>> goto out;
>>>> }
>>>> 
>>> 
>> 
> 

Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> 
> 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.

I agree.

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

Perhaps this got answered already elsewhere but is it OK for the detach handler
to be out of CONFIG_ACPI_HOTPLUG_CPU ?

Miguel

>> ---
>> 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,
>> },
> 
> 

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

On Thu, 11 Apr 2024 10:52:13 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> > On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> > 
> > On Wed, 10 Apr 2024 18:29:34 +0000
> > Miguel Luis <miguel.luis@oracle.com> wrote:
> >   
> >>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >>> 
> >>> On Tue,  9 Apr 2024 15:05:32 +0000
> >>> Miguel Luis <miguel.luis@oracle.com> wrote:
> >>>   
> >>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
> >>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
> >>>> isolate that functionality from architecture independent one.    
> >>> 
> >>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
> >>> to make the arch specific nature of that call more obvious?    
> >> 
> >> Not sure about the pattern to use here but that seems fine to me. Current usage
> >> is architectures export acpi_map_cpu from the acpi interface and do their
> >> thing.
> >> 
> >> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
> >> it gets called on the code path?  
> > 
> > I'm not sure what you mean by dismisses?
> >   
> 
> I mean when acpi_map_cpu is not needed.
> 
> > Is missing perhaps?  
> 
> Yes.
> 
> >  If that is what you mean, I think it's a mistake to allow
> > that code to be called from a path that isn't dependent on
> > CONFIG_ACPI_HOTPLUG_CPU.
> > It makes no sense to do so and stubbing it out to give
> > the impression that the calling it does make sense (when looking at the caller)
> > is misleading.  
> 
> OK, that would be what not to do.
> 
> acpi_processor_enumerate_extra could deal with make_present and make_enabled while
> a stub would still be needed for make_present since it depends on
> CONFIG_ACPI_HOTPLUG_CPU?

Sure, you could make it do that with a bunch of checks on the
config being enabled, but currently I don't see the overlap in
shared code as being sufficient for that to make sense.

The discussion before was assuming that things like the acpi_map_cpu
calls might do stuff that is wanted in the make_enabled() case.

Given they don't do anything that we want there I don't see sharing
the code as useful.

I am however in favor of renaming those hotplug only calls to something
more meaningful so no one 'thinks' they may be relevant in the
enabling only case!

Jonathan

p.s. I'm smashing the outputs of the thread with Rafael into a coherent
patch set at the moment, perhaps seeing that will make it clearer what
is going on.  I got distracted by fixing numa node handling this morning
but that's now pushed out for a follow on series.


> 
> Miguel
> 
> > 
> > Jonathan
> > 
> >   
> >> 
> >> 1) export it and do nothing - it would be creating unnecessary dependency.
> >> 
> >> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
> >> into CONFIG_ACPI_HOTPLUG_CPU.
> >> 
> >> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
> >> used for CPU HP and the same applies to acpi_unmap_cpu.
> >>   
> >>> I think that has caused more confusion in the discussion than
> >>> whether it is hotplug specific or not.    
> >> 
> >> Indeed. Within the CPU HP path there are these arch specific intricacies.
> >>   
> >>> 
> >>> As mentioned in patch 2, fairly sure this needs to go before that
> >>> patch.    
> >> 
> >> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
> >> CPU initialisation I think.
> >> 
> >> Miguel
> >>   
> >>> 
> >>> Jonathan
> >>>   
> >>>> 
> >>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
> >>>> ---
> >>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
> >>>> 1 file changed, 15 insertions(+), 2 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
> >>>> index 9ea58b61d741..c6e2f64a056b 100644
> >>>> --- a/drivers/acpi/acpi_processor.c
> >>>> +++ b/drivers/acpi/acpi_processor.c
> >>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
> >>>> pr_info("CPU%d has been hot-added\n", pr->id);
> >>>> pr->flags.need_hotplug_init = 1;
> >>>> }
> >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> >>>> +{
> >>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> >>>> +}
> >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
> >>>> +{
> >>>> + acpi_unmap_cpu(pr->id);
> >>>> +}
> >>>> #else
> >>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
> >>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
> >>>> +{
> >>>> + return 0;
> >>>> +}
> >>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
> >>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
> >>>> 
> >>>> /* Enumerate extra CPUs */
> >>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
> >>>> cpu_maps_update_begin();
> >>>> cpus_write_lock();
> >>>> 
> >>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
> >>>> + ret = acpi_processor_hotplug_map_cpu(pr);
> >>>> if (ret)
> >>>> goto out;
> >>>> 
> >>>> ret = arch_register_cpu(pr->id);
> >>>> if (ret) {
> >>>> - acpi_unmap_cpu(pr->id);
> >>>> + acpi_processor_hotplug_unmap_cpu(pr);
> >>>> goto out;
> >>>> }
> >>>>   
> >>>   
> >>   
> >   
> 

Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Jonathan Cameron]

On Thu, 11 Apr 2024 11:02:37 +0000
Miguel Luis <miguel.luis@oracle.com> wrote:

> > On 10 Apr 2024, at 13:31, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> > 
> > 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.  
> 
> I agree.
> 
> >  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.
> >   
> 
> Perhaps this got answered already elsewhere but is it OK for the detach handler
> to be out of CONFIG_ACPI_HOTPLUG_CPU ?

There is code that is again specific to CONFIG_ACPI_HOTPLUG_CPU and
code that is specific to the disabling only case.  So I think the conditions
will end up looking pretty similar to the attach path.
> 
> Miguel
> 
> >> ---
> >> 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,
> >> },  
> > 
> >   
> 

Re: [RFC PATCH 3/4] ACPI: processor: refactor acpi_processor_get_info: isolate acpi_{map|unmap}_cpu under CONFIG_ACPI_HOTPLUG_CPU

[Miguel Luis]

> On 11 Apr 2024, at 13:57, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
> 
> On Thu, 11 Apr 2024 10:52:13 +0000
> Miguel Luis <miguel.luis@oracle.com> wrote:
> 
>>> On 10 Apr 2024, at 19:44, Jonathan Cameron <jonathan.cameron@huawei.com> wrote:
>>> 
>>> On Wed, 10 Apr 2024 18:29:34 +0000
>>> Miguel Luis <miguel.luis@oracle.com> wrote:
>>> 
>>>>> On 10 Apr 2024, at 13:23, Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>>>>> 
>>>>> On Tue,  9 Apr 2024 15:05:32 +0000
>>>>> Miguel Luis <miguel.luis@oracle.com> wrote:
>>>>> 
>>>>>> mapping and unmaping a cpu at the stage of extra cpu enumeration is
>>>>>> architecture specific which depends on CONFIG_ACPI_HOTPLUG_CPU so let's
>>>>>> isolate that functionality from architecture independent one.    
>>>>> 
>>>>> Should we consider renaming acpi_map_cpu() to arch_acpi_map_cpu()
>>>>> to make the arch specific nature of that call more obvious?    
>>>> 
>>>> Not sure about the pattern to use here but that seems fine to me. Current usage
>>>> is architectures export acpi_map_cpu from the acpi interface and do their
>>>> thing.
>>>> 
>>>> Question is what to do when there’s a use-case which dismisses acpi_map_cpu and
>>>> it gets called on the code path?  
>>> 
>>> I'm not sure what you mean by dismisses?
>>> 
>> 
>> I mean when acpi_map_cpu is not needed.
>> 
>>> Is missing perhaps?  
>> 
>> Yes.
>> 
>>> If that is what you mean, I think it's a mistake to allow
>>> that code to be called from a path that isn't dependent on
>>> CONFIG_ACPI_HOTPLUG_CPU.
>>> It makes no sense to do so and stubbing it out to give
>>> the impression that the calling it does make sense (when looking at the caller)
>>> is misleading.  
>> 
>> OK, that would be what not to do.
>> 
>> acpi_processor_enumerate_extra could deal with make_present and make_enabled while
>> a stub would still be needed for make_present since it depends on
>> CONFIG_ACPI_HOTPLUG_CPU?
> 
> Sure, you could make it do that with a bunch of checks on the
> config being enabled, but currently I don't see the overlap in
> shared code as being sufficient for that to make sense.
> 
> The discussion before was assuming that things like the acpi_map_cpu
> calls might do stuff that is wanted in the make_enabled() case.
> 
> Given they don't do anything that we want there I don't see sharing
> the code as useful.
> 
> I am however in favor of renaming those hotplug only calls to something
> more meaningful so no one 'thinks' they may be relevant in the
> enabling only case!
> 
> Jonathan
> 
> p.s. I'm smashing the outputs of the thread with Rafael into a coherent
> patch set at the moment, perhaps seeing that will make it clearer what
> is going on.  I got distracted by fixing numa node handling this morning
> but that's now pushed out for a follow on series.
> 

Thanks! Looking forward to see v5.

Miguel

> 
>> 
>> Miguel
>> 
>>> 
>>> Jonathan
>>> 
>>> 
>>>> 
>>>> 1) export it and do nothing - it would be creating unnecessary dependency.
>>>> 
>>>> 2) evaluate whether calling it is exclusive to the CPU HP path and keep it wrapped
>>>> into CONFIG_ACPI_HOTPLUG_CPU.
>>>> 
>>>> Option (2) is the current approach on this RFC. IIUC acpi_map_cpu is solely
>>>> used for CPU HP and the same applies to acpi_unmap_cpu.
>>>> 
>>>>> I think that has caused more confusion in the discussion than
>>>>> whether it is hotplug specific or not.    
>>>> 
>>>> Indeed. Within the CPU HP path there are these arch specific intricacies.
>>>> 
>>>>> 
>>>>> As mentioned in patch 2, fairly sure this needs to go before that
>>>>> patch.    
>>>> 
>>>> 2 and 3 depend on each to be self-contained as CPU HP wouldn’t work without late
>>>> CPU initialisation I think.
>>>> 
>>>> Miguel
>>>> 
>>>>> 
>>>>> Jonathan
>>>>> 
>>>>>> 
>>>>>> Signed-off-by: Miguel Luis <miguel.luis@oracle.com>
>>>>>> ---
>>>>>> drivers/acpi/acpi_processor.c | 17 +++++++++++++++--
>>>>>> 1 file changed, 15 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
>>>>>> index 9ea58b61d741..c6e2f64a056b 100644
>>>>>> --- a/drivers/acpi/acpi_processor.c
>>>>>> +++ b/drivers/acpi/acpi_processor.c
>>>>>> @@ -194,8 +194,21 @@ static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr)
>>>>>> pr_info("CPU%d has been hot-added\n", pr->id);
>>>>>> pr->flags.need_hotplug_init = 1;
>>>>>> }
>>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + return acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>>>> +}
>>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + acpi_unmap_cpu(pr->id);
>>>>>> +}
>>>>>> #else
>>>>>> static void acpi_processor_hotplug_delay_init(struct acpi_processor *pr) {}
>>>>>> +static int acpi_processor_hotplug_map_cpu(struct acpi_processor *pr)
>>>>>> +{
>>>>>> + return 0;
>>>>>> +}
>>>>>> +static void acpi_processor_hotplug_unmap_cpu(struct acpi_processor *pr) {}
>>>>>> #endif /* CONFIG_ACPI_HOTPLUG_CPU */
>>>>>> 
>>>>>> /* Enumerate extra CPUs */
>>>>>> @@ -215,13 +228,13 @@ static int acpi_processor_enumerate_extra(struct acpi_processor *pr)
>>>>>> cpu_maps_update_begin();
>>>>>> cpus_write_lock();
>>>>>> 
>>>>>> - ret = acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, &pr->id);
>>>>>> + ret = acpi_processor_hotplug_map_cpu(pr);
>>>>>> if (ret)
>>>>>> goto out;
>>>>>> 
>>>>>> ret = arch_register_cpu(pr->id);
>>>>>> if (ret) {
>>>>>> - acpi_unmap_cpu(pr->id);
>>>>>> + acpi_processor_hotplug_unmap_cpu(pr);
>>>>>> goto out;
>>>>>> }

Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[kernel test robot]

Hi Miguel,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.9-rc3 next-20240412]
[cannot apply to rafael-pm/acpi-bus rafael-pm/devprop]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Miguel-Luis/ACPI-processor-refactor-acpi_processor_get_info-evaluation-of-processor-declaration/20240409-231441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240409150536.9933-5-miguel.luis%40oracle.com
patch subject: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU
config: i386-randconfig-004-20240413 (https://download.01.org/0day-ci/archive/20240413/202404131936.uSjT5AhF-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240413/202404131936.uSjT5AhF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404131936.uSjT5AhF-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/acpi/acpi_processor.o: in function `acpi_processor_remove':
>> drivers/acpi/acpi_processor.c:534:(.text+0x5e): undefined reference to `arch_unregister_cpu'


vim +534 drivers/acpi/acpi_processor.c

e8d0d9a5afb50b Miguel Luis               2024-04-09  502  
c8deb1c2576237 Xiaofei Tan               2021-03-27  503  /* Removal */
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  504  static void acpi_processor_remove(struct acpi_device *device)
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  505  {
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  506  	struct acpi_processor *pr;
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  507  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  508  	if (!device || !acpi_driver_data(device))
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  509  		return;
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  510  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  511  	pr = acpi_driver_data(device);
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  512  	if (pr->id >= nr_cpu_ids)
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  513  		goto out;
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  514  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  515  	/*
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  516  	 * The only reason why we ever get here is CPU hot-removal.  The CPU is
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  517  	 * already offline and the ACPI device removal locking prevents it from
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  518  	 * being put back online at this point.
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  519  	 *
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  520  	 * Unbind the driver from the processor device and detach it from the
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  521  	 * ACPI companion object.
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  522  	 */
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  523  	device_release_driver(pr->dev);
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  524  	acpi_unbind_one(pr->dev);
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  525  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  526  	/* Clean up. */
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  527  	per_cpu(processor_device_array, pr->id) = NULL;
2e4f1db49d9722 Rafael J. Wysocki         2013-05-30  528  	per_cpu(processors, pr->id) = NULL;
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  529  
b9d10be7a8e88f Toshi Kani                2013-08-12  530  	cpu_maps_update_begin();
95ac706744de78 Sebastian Andrzej Siewior 2021-08-03  531  	cpus_write_lock();
b9d10be7a8e88f Toshi Kani                2013-08-12  532  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  533  	/* Remove the CPU. */
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03 @534  	arch_unregister_cpu(pr->id);
e8d0d9a5afb50b Miguel Luis               2024-04-09  535  	acpi_processor_hotunplug_unmap_cpu(pr);
b9d10be7a8e88f Toshi Kani                2013-08-12  536  
95ac706744de78 Sebastian Andrzej Siewior 2021-08-03  537  	cpus_write_unlock();
b9d10be7a8e88f Toshi Kani                2013-08-12  538  	cpu_maps_update_done();
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  539  
1e385f6f97b8ab Yasuaki Ishimatsu         2013-08-06  540  	try_offline_node(cpu_to_node(pr->id));
1e385f6f97b8ab Yasuaki Ishimatsu         2013-08-06  541  
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  542   out:
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  543  	free_cpumask_var(pr->throttling.shared_cpu_map);
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  544  	kfree(pr);
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  545  }
ac212b6980d8d5 Rafael J. Wysocki         2013-05-03  546  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU

[kernel test robot]

Hi Miguel,

[This is a private test report for your RFC patch.]
kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.9-rc3 next-20240412]
[cannot apply to rafael-pm/acpi-bus rafael-pm/devprop]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Miguel-Luis/ACPI-processor-refactor-acpi_processor_get_info-evaluation-of-processor-declaration/20240409-231441
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240409150536.9933-5-miguel.luis%40oracle.com
patch subject: [RFC PATCH 4/4] ACPI: processor: refactor acpi_processor_remove: isolate acpi_unmap_cpu under CONFIG_ACPI_HOTPLUG_CPU
config: i386-randconfig-001-20240413 (https://download.01.org/0day-ci/archive/20240413/202404132034.7uJtpbGD-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240413/202404132034.7uJtpbGD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404132034.7uJtpbGD-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: arch_unregister_cpu
   >>> referenced by acpi_processor.c:534 (drivers/acpi/acpi_processor.c:534)
   >>>               drivers/acpi/acpi_processor.o:(acpi_processor_remove) in archive vmlinux.a

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki