Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <20231120114143.95305-1-ytcoode@gmail.com>
In-Reply-To: <20231120114143.95305-1-ytcoode@gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Wed, 6 Dec 2023 17:42:15 +0100
Message-ID: <CAJZ5v0ijJeOLJo=ooru9raj0n=iiGybFCud42Z+EEtncgNk47A@mail.gmail.com>
Subject: Re: [PATCH] ACPI: Correct and clean up the logic of acpi_parse_entries_array()
To:     Yuntao Wang <ytcoode@gmail.com>, Dave Jiang <dave.jiang@intel.com>,
        Dan Williams <dan.j.williams@intel.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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