[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

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

[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

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

[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

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

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