diff mbox series

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

Message ID 20240409150536.9933-2-miguel.luis@oracle.com (mailing list archive)
State RFC, archived
Headers show
Series ACPI: processor: refactor acpi_processor_{get_info|remove} | expand

Commit Message

Miguel Luis April 9, 2024, 3:05 p.m. UTC 
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(-)

Comments

Jonathan Cameron April 10, 2024, 1:13 p.m. UTC | #1 
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)
Miguel Luis April 10, 2024, 3:35 p.m. UTC | #2 
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)
diff mbox series

Patch

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)