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)
