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