[PATCH v2 7/8] ACPICA: AML Parser: ignore dispatcher error status during table load

[Aaron.Ma]

On March 29, 2019 8:06:00 AM UTC, Khaled Elmously <khalid.elmously at canonical.com> wrote:
>On 2019-03-29 15:37:31 , Aaron Ma wrote:
>> From: "Schmauss, Erik" <erik.schmauss at intel.com>
>> 
>> BugLink: https://bugs.launchpad.net/bugs/1820153
>> 
>> The dispatcher and the executer process the parse nodes During table
>> load. Error status from the evaluation confuses the AML parser. This
>> results in the parser failing to complete parsing of the current
>> scope op which becomes problematic. For the incorrect AML below, _ADR
>> never gets created.
>> 
>> definition_block(...)
>> {
>>    Scope (\_SB)
>>    {
>>      Device (PCI0){...}
>>      Name (OBJ1, 0x0)
>>      OBJ1 = PCI0 + 5 // Results in an operand error.
>>    } // \_SB not closed
>> 
>>    // parser looks for \_SB._SB.PCI0, results in AE_NOT_FOUND error
>>    // Entire scope block gets skipped.
>>    Scope (\_SB.PCI0)
>>    {
>>        Name (_ADR, 0x0)
>>    }
>> }
>> 
>> Fix the above error by properly completing the initial \_SB scope
>> after an error by clearing errors that occur during table load. In
>> the above case, this means that OBJ1 = PIC0 + 5 is skipped.
>> 
>> Fixes: 5088814a6e93 (ACPICA: AML parser: attempt to continue loading
>table after error)
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=200363
>> Tested-by: Bastien Nocera <hadess at hadess.net>
>> Signed-off-by: Erik Schmauss <erik.schmauss at intel.com>
>> Cc: 4.17+ <stable at vger.kernel.org> # 4.17+
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki at intel.com>
>> (cherry picked from commit 73c2a01c52b657f4a0ead6c95f64c5279efbd000)
>> Signed-off-by: Aaron Ma <aaron.ma at canonical.com>
>> ---
>>  drivers/acpi/acpica/psloop.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>> 
>> diff --git a/drivers/acpi/acpica/psloop.c
>b/drivers/acpi/acpica/psloop.c
>> index f437c3d54086..a5f35ff54510 100644
>> --- a/drivers/acpi/acpica/psloop.c
>> +++ b/drivers/acpi/acpica/psloop.c
>> @@ -513,6 +513,18 @@ acpi_status acpi_ps_parse_loop(struct
>acpi_walk_state *walk_state)
>>  			status =
>>  			    acpi_ps_create_op(walk_state, aml_op_start, &op);
>>  			if (ACPI_FAILURE(status)) {
>> +				/*
>> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
>> +				 * executing it as a control method. However, if we encounter
>> +				 * an error while loading the table, we need to keep trying to
>> +				 * load the table rather than aborting the table load. Set the
>> +				 * status to AE_OK to proceed with the table load.
>> +				 */
>> +				if ((walk_state->
>> +				     parse_flags & ACPI_PARSE_MODULE_LEVEL)
>> +				    && status == AE_ALREADY_EXISTS) {
>> +					status = AE_OK;
>> +				}
>>  				if (status == AE_CTRL_PARSE_CONTINUE) {
>>  					continue;
>>  				}
>> @@ -710,6 +722,20 @@ acpi_status acpi_ps_parse_loop(struct
>acpi_walk_state *walk_state)
>>  			    acpi_ps_next_parse_state(walk_state, op, status);
>>  			if (status == AE_CTRL_PENDING) {
>>  				status = AE_OK;
>> +			} else
>> +			    if ((walk_state->
>> +				 parse_flags & ACPI_PARSE_MODULE_LEVEL)
>> +				&& ACPI_FAILURE(status)) {
>> +				/*
>> +				 * ACPI_PARSE_MODULE_LEVEL means that we are loading a table by
>> +				 * executing it as a control method. However, if we encounter
>> +				 * an error while loading the table, we need to keep trying to
>> +				 * load the table rather than aborting the table load. Set the
>> +				 * status to AE_OK to proceed with the table load. If we get a
>> +				 * failure at this point, it means that the dispatcher got an
>> +				 * error while processing Op (most likely an AML operand error.
>> +				 */
>> +				status = AE_OK;
>>  			}
>>  		}
>> 
>
>I believe I have Tyler's ACK for patch 8/8 based on his comment but I
>have no ACKs yet for 7/8.
>
>I'm assuming these 2 patches don't necessarily need to come right after
>patch 1-6? Please let me know if that's not right.
>

I think Tyler's ack for both patches is in bionic sru.

Just applying them are fine, no need to rebase branch.

Thanks,
Aaron


>Thanks