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@xxxxxxxxxx> wrote:
> 
> On Tue,  9 Apr 2024 15:05:30 +0000
> Miguel Luis <miguel.luis@xxxxxxxxxx> wrote:
> 
>> Isolate the evaluation of processor declaration into its own function.
>> 
>> No functional changes.
>> 
>> Signed-off-by: Miguel Luis <miguel.luis@xxxxxxxxxx>
> 
> 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)