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;
>> }
>> --
