| Message ID | 20240409150536.9933-2-miguel.luis@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI: processor: refactor acpi_processor_{get_info|remove} | expand |
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)
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 --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)
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(-)