Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <575e4395-dfe1-4399-9706-cc02f56d7ebe@intel.com>
Date:   Wed, 6 Dec 2023 10:01:50 -0700
User-Agent: Betterbird (Linux)
Subject: Re: [PATCH] ACPI: Correct and clean up the logic of
 acpi_parse_entries_array()
To:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Yuntao Wang <ytcoode@gmail.com>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Len Brown <lenb@kernel.org>, <linux-acpi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20231120114143.95305-1-ytcoode@gmail.com>
 <CAJZ5v0ijJeOLJo=ooru9raj0n=iiGybFCud42Z+EEtncgNk47A@mail.gmail.com>
Content-Language: en-US
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <CAJZ5v0ijJeOLJo=ooru9raj0n=iiGybFCud42Z+EEtncgNk47A@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WUl6QnM4ZXl3c3F2ak5KNVNvaUhqdHloVmdCOG9iajRGZnVKU0xuaHRScUNN?=
 =?utf-8?B?d2I5bWZkSExOUVpDRkxEeEtXdDdWT2J6QjlJeFlZaUVxZWc3TytuRUFzV3Vh?=
 =?utf-8?B?aCtkS1ZXVlloWHJXWmVPa2Fyc285YmJaaU1pWEQrclp1REo1ODVIbmdBTTd1?=
 =?utf-8?B?bHp2Z2lLTmdsaWdXZ1pydHpndTNoNEVjWmZ0cEhSM21FWVNQRWVMSkdRNVZY?=
 =?utf-8?B?b1FoSERJVWpjc0NTR1JjZzJ3MythRnBxMGthKzQ0SmlJM1BIcFpMK05oeVI5?=
 =?utf-8?B?THhGMlJNRGkzcTBmTU40WW94QllQR1pYdEZjcDhvaWlPTHNZekRRMnFyRnlh?=
 =?utf-8?B?TXlmZ2hWWDBjcjFxNDZIZVc2dS95S2dxZm41cFdwSGJaN2ROdk5xeGxwQ2Zp?=
 =?utf-8?B?d0hYZXo0VzVBS0VWaW5KVEhteU1wNzRqTFM3TkRwdlVvOXFBRTFPOU9RdG1O?=
 =?utf-8?B?OU8rMmV3NU9RZFVwdjRNbGpHK25hM2c5QTlyaHdoTmFFcEE3aDM5WFpSbnpT?=
 =?utf-8?B?enRtaUp3dWlhZDEraCt3ZEZndnpRTVEyK1ZqNmtUR2RqeFhqZ3F1ckdGTCtO?=
 =?utf-8?B?VC80dVdueUNUa0x0cWorL1V1SFZkc2Y4YVJYMlUzMnZWTGlqMGNXdFNHL1JC?=
 =?utf-8?B?S0VMSHlpTlFaZlZpbWMvL1RsL2RXY2dHVmg4T1pnR2JSVERSazQzbkJwUXdS?=
 =?utf-8?B?K2gxckE1VnJUaFFiRFo1anJ3OVBsUSs0Ulk2dXRQOGdnc2luR1ZtZlNJbzBL?=
 =?utf-8?B?VDVyU1g0NHdGZnpHK0dtdklLNG1SQTBSM0t6NVA0emRWcmd2QituaXc4d0N5?=
 =?utf-8?B?N2lJTE1BaVRJWnBIeWtLaFJBeDRJZFFISk00am1hTE1ucTQxdSszcGV1dDlj?=
 =?utf-8?B?eXB4N1ZXVTdkeUh4Y3dUMkVlODdjM1Izc3k1RWVNa3B0cDA4eEpxRzNkY3Fu?=
 =?utf-8?B?cm9OUk9LWG1GZzhCc1F0OFZpVzlsQy82NS9qWGxQazN3QWdGUGZNK1h3THM5?=
 =?utf-8?B?cDhYeXBlT01QVW5kWnZVamxZa2FaYkJYQm9sV01rNjBYSHFZQlFYKzdzRkpn?=
 =?utf-8?B?L2RnZFVtV1lmK0Y4YlRKSUg0T0FKRG1SdUEzSklMRTF5dG5tNVZIQWt3aXJq?=
 =?utf-8?B?OVFQKy83dXlqWWJnblZSVjBEK2hNYy91WXRhelNBSXlnUEJDT3NBN3RJNFA5?=
 =?utf-8?B?N3pZbG53ZGV2T1l6ajRSUW5TT0hlaEJIR2R6SG13TERXdThpTWdqM24zWXRr?=
 =?utf-8?B?b1RPM0JhQnRJVzNsODlROFBOWjJrTVBFSDcxYkQra3lXYVdEdXlteWk0a0dQ?=
 =?utf-8?B?K0JlbEhOTXVsVDhMS2tHaC8yK2xzRnFxamIrcmJsNkwzL2xBSzlnUTlNT2xB?=
 =?utf-8?B?V1BPU0pWQjAxNWtYV2FoQXZ1UVN6WWhIV3hvNXpQRVpQVVNIK2VWd1BTdUhM?=
 =?utf-8?B?REJqY2dNQ0JmcVJqQ1dVYjNqa1BROU40cTNvcVZDUnJwdnJMU3NWNUViRndY?=
 =?utf-8?B?OVd1dlVCSVZoRkFQOS83YVhFR3NpK3JodGs5SFFHVGxPdFpHOU5uS1E0Y1hj?=
 =?utf-8?B?UkpBcWRIdTVody9ndXJoeEtuTEJPN21JbU5RNjY3QngvdzhUL0FBdGdUTTRo?=
 =?utf-8?B?bzV4N29WMllKdVJ1YjFIZ09jbmVFMFJmT3hCeGZtN3ZJbldXbEprRmdoOVo2?=
 =?utf-8?B?Mk11cng5TG1RQUpUZkR0WStXTldFWjk1bVJsbTZ0MSthN2oyeFVEWEF6VTlm?=
 =?utf-8?B?c1JSV21hVnhoSy9PWTJZRjAzMjdHRE84Tk4vZE4xR2k3b1BPZzlLV2tHUnNw?=
 =?utf-8?B?Q2dWOUdjTktiWmdtVU50Tm9UNnRKem5Yak9waFo2d3crZ25HN1Z6VTgxaGxy?=
 =?utf-8?B?Q3ZUaDRyNTVpZG1YZWIvOFFUdWhzS1UzMUMyYTVKSUR5OHBRbVovcTV1MDht?=
 =?utf-8?B?TnlnblIvMjl3Wk9zL3FFekJNMVhVUFhsUUJNdmRPSHV4ZVZGdER2UlBld2dY?=
 =?utf-8?B?dVlxbDd5cm1oQUM1eXB1eloxU0lOTHR3OW53dzRMdmgydE0rL0JjUEVGb1FX?=
 =?utf-8?B?N3dmeDdpSTNQZDZqSWlLSG0yeEdWa3pDajFMVFpNRnB5ZEtVZzB2VGZYR2RY?=
 =?utf-8?Q?z1CZV2Av6tuw7+cBymyEZ9gR0?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 81234d37-f09b-42cc-f487-08dbf67d0c2d
X-MS-Exchange-CrossTenant-AuthSource: PH7PR11MB5984.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2023 17:01:54.0563
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: g8MNw4FzmZg2NXgojncyPULdqceDkcvFjEFjyYM6bf9JwGAjmtdiiILuq014Ic9Dtt3LYrwxcZUgPevPZ2U6Kg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6863
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Wed, 06 Dec 2023 09:02:12 -0800 (PST)


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