ACPI: Correct and clean up the logic of acpi_parse_entries_array()

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@gmail.com>
---
 lib/fw_table.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

On Mon, Nov 20, 2023 at 12:42 PM Yuntao Wang <ytcoode@gmail.com> 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@gmail.com>

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

On 12/6/23 09:42, Rafael J. Wysocki wrote:
> On Mon, Nov 20, 2023 at 12:42 PM Yuntao Wang <ytcoode@gmail.com> 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@gmail.com>
> 
> This needs to be ACKed by Dave Jiang or Dan Williams.

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

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

On Wed, Dec 6, 2023 at 6:02 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
>
>
> On 12/6/23 09:42, Rafael J. Wysocki wrote:
> > On Mon, Nov 20, 2023 at 12:42 PM Yuntao Wang <ytcoode@gmail.com> 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@gmail.com>
> >
> > This needs to be ACKed by Dave Jiang or Dan Williams.
>
> Reviewed-by: Dave Jiang <dave.jiang@intel.com>

Applied as 6.8 material, thanks!