Re: [PATCH v1 04/17] ACPI: processor: idle: Rearrange acpi_processor_evaluate_lpi()

On Thu, Jul 16, 2026 at 1:52 PM lihuisong (C) <[email protected]> wrote:
>
>
> On 7/9/2026 8:33 PM, Rafael J. Wysocki wrote:
> > From: "Rafael J. Wysocki" <[email protected]>
> >
> > Rearrange acpi_processor_evaluate_lpi() to make it somewhat easier to
> > follow and diagnose (if need be).  In particular:
> >
> >   * Rename some local variables and reorder their definitions.
> >
> >   * Change the type of local variables used for storing firmware-provided
> >     values to unsigned int (they cannot be negative).
> >
> >   * Eliminate local variable "loop" that is redundant.
> >
> >   * Avoid explicit pointer arithmetic.
> >
> >   * Print the correct number of _LPI state packages in the final debug
> >     message.
> >
> > No intentional functional impact beyond debug output.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >   drivers/acpi/processor_idle.c | 64 +++++++++++++++++++----------------
> >   1 file changed, 34 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> > index e8682f779249..cd506e9e5a84 100644
> > --- a/drivers/acpi/processor_idle.c
> > +++ b/drivers/acpi/processor_idle.c
> > @@ -872,12 +872,12 @@ static int obj_get_integer(union acpi_object *obj, u32 *value)
> >   static int acpi_processor_evaluate_lpi(acpi_handle handle,
> >                                      struct acpi_lpi_states_array *info)
> >   {
> > -     acpi_status status;
> > -     int ret = 0;
> > -     int pkg_count, state_idx = 1, loop;
> >       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> > -     union acpi_object *lpi_data;
> > +     union acpi_object *lpi_data, *lpi_pkg;
> > +     unsigned int lpi_pkg_count, state_idx;
> >       struct acpi_lpi_state *lpi_state;
> > +     acpi_status status;
> > +     int ret = 0;
> >
> >       status = acpi_evaluate_object(handle, "_LPI", NULL, &buffer);
> >       if (ACPI_FAILURE(status)) {
> > @@ -895,41 +895,46 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> >               goto end;
> >       }
> >
> > -     pkg_count = lpi_data->package.elements[2].integer.value;
> > +     lpi_pkg_count = lpi_data->package.elements[2].integer.value;
> >
> >       /* Validate number of power states. */
> > -     if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
> > +     if (!lpi_pkg_count || lpi_pkg_count != lpi_data->package.count - 3) {
> >               acpi_handle_debug(handle, "Invalid _LPI state count\n");
> >               ret = -ENODATA;
> >               goto end;
> >       }
> >
> > -     lpi_state = kzalloc_objs(*lpi_state, pkg_count);
> > +     lpi_state = kzalloc_objs(*lpi_state, lpi_pkg_count);
> >       if (!lpi_state) {
> >               ret = -ENOMEM;
> >               goto end;
> >       }
> >
> > -     info->size = pkg_count;
> > +     info->size = lpi_pkg_count;
> >       info->entries = lpi_state;
> >
> > -     /* LPI States start at index 3 */
> > -     for (loop = 3; state_idx <= pkg_count; loop++, state_idx++, lpi_state++) {
> > -             union acpi_object *element, *pkg_elem, *obj;
> > +     /* _LPI State packages start at index 3. */
> > +     lpi_pkg = &lpi_data->package.elements[3];
> >
> > -             element = &lpi_data->package.elements[loop];
> > -             if (element->type != ACPI_TYPE_PACKAGE || element->package.count < 7)
> > +     for (state_idx = 1; state_idx <= lpi_pkg_count;
> > +          state_idx++, lpi_state++, lpi_pkg++) {
> > +             union acpi_object *lpi_pkg_elem, *obj;
> > +
> > +             lpi_state->index = state_idx;
> > +
> > +             if (lpi_pkg->type != ACPI_TYPE_PACKAGE || lpi_pkg->package.count < 7)
> >                       continue;
> >
> > -             pkg_elem = element->package.elements;
> > +             lpi_pkg_elem = lpi_pkg->package.elements;
> >
> > -             obj = pkg_elem + 6;
> > +             /* Get the entry method first and skip the state if that fails. */
> > +             obj = &lpi_pkg_elem[6];
> >               if (obj->type == ACPI_TYPE_BUFFER) {
> >                       struct acpi_power_register *reg;
> >
> >                       if (obj->buffer.length < sizeof(*reg)) {
> >                               acpi_handle_debug(handle,
> > -                                     "Invalid register data for _LPI state %d\n",
> > +                                     "Invalid register data for _LPI state %u\n",
> >                                       state_idx);
> >                               continue;
> >                       }
> > @@ -937,7 +942,7 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> >                       reg = (struct acpi_power_register *)obj->buffer.pointer;
> >                       if (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
> >                               acpi_handle_debug(handle,
> > -                                     "Unsupported entry method for _LPI state %d\n",
> > +                                     "Unsupported entry method for _LPI state %u\n",
> >                                       state_idx);
> >                               continue;
> >                       }
> > @@ -949,36 +954,35 @@ static int acpi_processor_evaluate_lpi(acpi_handle handle,
> >                       lpi_state->address = obj->integer.value;
> >               } else {
> >                       acpi_handle_debug(handle,
> > -                                       "Invalid entry method for _LPI state %d\n",
> > +                                       "Invalid entry method for _LPI state %u\n",
> >                                         state_idx);
> >                       continue;
> >               }
> >
> > -             lpi_state->index = state_idx;
> > -             if (obj_get_integer(pkg_elem + 0, &lpi_state->min_residency)) {
> > +             if (obj_get_integer(&lpi_pkg_elem[0], &lpi_state->min_residency)) {
> >                       acpi_handle_debug(handle,
> > -                                       "Assuming 10 us min. residency for _LPI state %d\n",
> > +                                       "Assuming 10 us min. residency for _LPI state %u\n",
> >                                         state_idx);
> >                       lpi_state->min_residency = 10;
> >               }
> >
> > -             if (obj_get_integer(pkg_elem + 1, &lpi_state->wake_latency)) {
> > +             if (obj_get_integer(&lpi_pkg_elem[1], &lpi_state->wake_latency)) {
> >                       acpi_handle_debug(handle,
> > -                                       "Assuming 10 us wake latency for _LPI state %d\n",
> > +                                       "Assuming 10 us wake latency for _LPI state %u\n",
> >                                         state_idx);
> >                       lpi_state->wake_latency = 10;
> >               }
> >
> > -             if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
> > +             if (obj_get_integer(&lpi_pkg_elem[2], &lpi_state->flags))
> >                       lpi_state->flags = 0;
> >
> > -             if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
> > +             if (obj_get_integer(&lpi_pkg_elem[3], &lpi_state->arch_flags))
> >                       lpi_state->arch_flags = 0;
> >
> > -             if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
> > +             if (obj_get_integer(&lpi_pkg_elem[4], &lpi_state->res_cnt_freq))
> >                       lpi_state->res_cnt_freq = 1;
> >
> > -             if (obj_get_integer(pkg_elem + 5, &lpi_state->enable_parent_state))
> > +             if (obj_get_integer(&lpi_pkg_elem[5], &lpi_state->enable_parent_state))
> >                       lpi_state->enable_parent_state = 0;
> How about move parsing lpi_pkg_elem[6] code to here?

If it fails, the other things don't need to be done at all, so it's
better to do it upfront IMV.