Re: [PATCH] ACPI: Correct and clean up the logic of acpi_parse_entries_array()

[Dave Jiang]

On 12/6/23 09:42, Rafael J. Wysocki wrote:
> On Mon, Nov 20, 2023 at 12:42 PM Yuntao Wang <ytcoode@xxxxxxxxx> wrote:
>>
>> The original intention of acpi_parse_entries_array() is to return the
>> number of all matching entries on success. This number may be greater than
>> the value of the max_entries parameter. When this happens, the function
>> will output a warning message, indicating that `count - max_entries`
>> matching entries remain unprocessed and have been ignored.
>>
>> However, commit 4ceacd02f5a1 ("ACPI / table: Always count matched and
>> successfully parsed entries") changed this logic to return the number of
>> entries successfully processed by the handler. In this case, when the
>> max_entries parameter is not zero, the number of entries successfully
>> processed can never be greater than the value of max_entries. In other
>> words, the expression `count > max_entries` will always evaluate to false.
>> This means that the logic in the final if statement will never be executed.
>>
>> Commit 99b0efd7c886 ("ACPI / tables: do not report the number of entries
>> ignored by acpi_parse_entries()") mentioned this issue, but it tried to fix
>> it by removing part of the warning message. This is meaningless because the
>> pr_warn statement will never be executed in the first place.
>>
>> Commit 8726d4f44150 ("ACPI / tables: fix acpi_parse_entries_array() so it
>> traverses all subtables") introduced an errs variable, which is intended to
>> make acpi_parse_entries_array() always traverse all of the subtables,
>> calling as many of the callbacks as possible. However, it seems that the
>> commit does not achieve this goal. For example, when a handler returns an
>> error, none of the handlers will be called again in the subsequent
>> iterations. This result appears to be no different from before the change.
>>
>> This patch corrects and cleans up the logic of acpi_parse_entries_array(),
>> making it return the number of all matching entries, rather than the number
>> of entries successfully processed by handlers. Additionally, if an error
>> occurs when executing a handler, the function will return -EINVAL immediately.
>>
>> This patch should not affect existing users of acpi_parse_entries_array().
>>
>> Signed-off-by: Yuntao Wang <ytcoode@xxxxxxxxx>
> 
> This needs to be ACKed by Dave Jiang or Dan Williams.

Reviewed-by: Dave Jiang <dave.jiang@xxxxxxxxx>

> 
>> ---
>>  lib/fw_table.c | 30 +++++++++---------------------
>>  1 file changed, 9 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/fw_table.c b/lib/fw_table.c
>> index b51f30a28e47..b655e6f4b647 100644
>> --- a/lib/fw_table.c
>> +++ b/lib/fw_table.c
>> @@ -85,11 +85,6 @@ acpi_get_subtable_type(char *id)
>>         return ACPI_SUBTABLE_COMMON;
>>  }
>>
>> -static __init_or_acpilib bool has_handler(struct acpi_subtable_proc *proc)
>> -{
>> -       return proc->handler || proc->handler_arg;
>> -}
>> -
>>  static __init_or_acpilib int call_handler(struct acpi_subtable_proc *proc,
>>                                           union acpi_subtable_headers *hdr,
>>                                           unsigned long end)
>> @@ -133,7 +128,6 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>         unsigned long table_end, subtable_len, entry_len;
>>         struct acpi_subtable_entry entry;
>>         int count = 0;
>> -       int errs = 0;
>>         int i;
>>
>>         table_end = (unsigned long)table_header + table_header->length;
>> @@ -145,25 +139,19 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>             ((unsigned long)table_header + table_size);
>>         subtable_len = acpi_get_subtable_header_length(&entry);
>>
>> -       while (((unsigned long)entry.hdr) + subtable_len  < table_end) {
>> -               if (max_entries && count >= max_entries)
>> -                       break;
>> -
>> +       while (((unsigned long)entry.hdr) + subtable_len < table_end) {
>>                 for (i = 0; i < proc_num; i++) {
>>                         if (acpi_get_entry_type(&entry) != proc[i].id)
>>                                 continue;
>> -                       if (!has_handler(&proc[i]) ||
>> -                           (!errs &&
>> -                            call_handler(&proc[i], entry.hdr, table_end))) {
>> -                               errs++;
>> -                               continue;
>> -                       }
>> +
>> +                       if (!max_entries || count < max_entries)
>> +                               if (call_handler(&proc[i], entry.hdr, table_end))
>> +                                       return -EINVAL;
>>
>>                         proc[i].count++;
>> +                       count++;
>>                         break;
>>                 }
>> -               if (i != proc_num)
>> -                       count++;
>>
>>                 /*
>>                  * If entry->length is 0, break from this loop to avoid
>> @@ -180,9 +168,9 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
>>         }
>>
>>         if (max_entries && count > max_entries) {
>> -               pr_warn("[%4.4s:0x%02x] found the maximum %i entries\n",
>> -                       id, proc->id, count);
>> +               pr_warn("[%4.4s:0x%02x] ignored %i entries of %i found\n",
>> +                       id, proc->id, count - max_entries, count);
>>         }
>>
>> -       return errs ? -EINVAL : count;
>> +       return count;
>>  }
>> --