apci: checksum: RSDT and XSDT checksum failures should not be critical (LP: #1013168)

From: Colin Ian King <colin.king@canonical.com>

It seems that the kernel is quite happy to handle RSDT and XSDT tables that
fail on their checksum checks, so lets not fail these as critical failures
anymore.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/checksum/checksum.c |   41 +++++++----------------------------------
 1 file changed, 7 insertions(+), 34 deletions(-)

On 06/14/2012 10:06 PM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> It seems that the kernel is quite happy to handle RSDT and XSDT tables that
> fail on their checksum checks, so lets not fail these as critical failures
> anymore.
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   src/acpi/checksum/checksum.c |   41 +++++++----------------------------------
>   1 file changed, 7 insertions(+), 34 deletions(-)
>
> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
> index 264e7d0..2c41cce 100644
> --- a/src/acpi/checksum/checksum.c
> +++ b/src/acpi/checksum/checksum.c
> @@ -89,16 +89,6 @@ static void checksum_rsdp(fwts_framework *fw, fwts_acpi_table_info *table)
>
>   }
>
> -/*
> - *  The following tables the kernel requires the checksum to be valid otherwise
> - *  it will not load them, so checksum failures here are considered critical errors.
> - */
> -static char *critical_checksum[] = {
> -	"RSDT",
> -	"XSDT",
> -	NULL
> -};
> -
>   static int checksum_scan_tables(fwts_framework *fw)
>   {
>   	int i;
> @@ -131,35 +121,18 @@ static int checksum_scan_tables(fwts_framework *fw)
>   			fwts_passed(fw, "Table %s has correct checksum 0x%x.",
>   				table->name, hdr->checksum);
>   		else {
> -			int i;
> -			int log_level = LOG_LEVEL_LOW;
> -	
> -			for (i = 0; critical_checksum[i]; i++) {
> -				if (!strcmp(table->name, critical_checksum[i])) {
> -					log_level = LOG_LEVEL_CRITICAL;
> -					break;
> -				}
> -			}
> -
> -			fwts_failed(fw, log_level, "ACPITableChecksum",
> +			fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableChecksum",
>   				"Table %s has incorrect checksum, "
>   				"expected 0x%2.2x, got 0x%2.2x.",
>   				table->name, (uint8_t)(hdr->checksum-checksum),
>   				hdr->checksum);
>
> -			/* Give some contextual explanation of the error */
> -			if (log_level == LOG_LEVEL_CRITICAL)
> -				fwts_advice(fw,
> -					"The kernel requires this table to have a "
> -					"valid checksum and will not load it. This "
> -					"will lead to ACPI not working correctly.");
> -			else
> -				fwts_advice(fw,
> -					"The kernel will warn that this table has "
> -					"an invalid checksum but will ignore the "
> -					"error and still load it. This is not a "
> -					"critical issue, but should be fixed if "
> -					"possible to avoid the warning messages.");
> +			fwts_advice(fw,
> +				"The kernel will warn that this table has "
> +				"an invalid checksum but will ignore the "
> +				"error and still load it. This is not a "
> +				"critical issue, but should be fixed if "
> +				"possible to avoid the warning messages.");
>
>   			fwts_tag_failed(fw, FWTS_TAG_ACPI_TABLE_CHECKSUM);
>   		}

If checksum is incorrect, that means it cannot be determined whether the 
table is still trustworthy. It is known that kernel will ignore the 
error and therefore it is not critical; however, will it be better to 
make the level to high or medium so the bug will be highlighted and fixed?

After all, the error means a system does not meet ACPI specification.

Best Regards,
Alex Hung

On 18/06/12 03:38, Alex Hung wrote:
> On 06/14/2012 10:06 PM, Colin King wrote:
>> From: Colin Ian King<colin.king@canonical.com>
>>
>> It seems that the kernel is quite happy to handle RSDT and XSDT tables
>> that
>> fail on their checksum checks, so lets not fail these as critical
>> failures
>> anymore.
>>
>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>> ---
>>   src/acpi/checksum/checksum.c |   41
>> +++++++----------------------------------
>>   1 file changed, 7 insertions(+), 34 deletions(-)
>>
>> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
>> index 264e7d0..2c41cce 100644
>> --- a/src/acpi/checksum/checksum.c
>> +++ b/src/acpi/checksum/checksum.c
>> @@ -89,16 +89,6 @@ static void checksum_rsdp(fwts_framework *fw,
>> fwts_acpi_table_info *table)
>>
>>   }
>>
>> -/*
>> - *  The following tables the kernel requires the checksum to be valid
>> otherwise
>> - *  it will not load them, so checksum failures here are considered
>> critical errors.
>> - */
>> -static char *critical_checksum[] = {
>> -    "RSDT",
>> -    "XSDT",
>> -    NULL
>> -};
>> -
>>   static int checksum_scan_tables(fwts_framework *fw)
>>   {
>>       int i;
>> @@ -131,35 +121,18 @@ static int checksum_scan_tables(fwts_framework *fw)
>>               fwts_passed(fw, "Table %s has correct checksum 0x%x.",
>>                   table->name, hdr->checksum);
>>           else {
>> -            int i;
>> -            int log_level = LOG_LEVEL_LOW;
>> -
>> -            for (i = 0; critical_checksum[i]; i++) {
>> -                if (!strcmp(table->name, critical_checksum[i])) {
>> -                    log_level = LOG_LEVEL_CRITICAL;
>> -                    break;
>> -                }
>> -            }
>> -
>> -            fwts_failed(fw, log_level, "ACPITableChecksum",
>> +            fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableChecksum",
>>                   "Table %s has incorrect checksum, "
>>                   "expected 0x%2.2x, got 0x%2.2x.",
>>                   table->name, (uint8_t)(hdr->checksum-checksum),
>>                   hdr->checksum);
>>
>> -            /* Give some contextual explanation of the error */
>> -            if (log_level == LOG_LEVEL_CRITICAL)
>> -                fwts_advice(fw,
>> -                    "The kernel requires this table to have a "
>> -                    "valid checksum and will not load it. This "
>> -                    "will lead to ACPI not working correctly.");
>> -            else
>> -                fwts_advice(fw,
>> -                    "The kernel will warn that this table has "
>> -                    "an invalid checksum but will ignore the "
>> -                    "error and still load it. This is not a "
>> -                    "critical issue, but should be fixed if "
>> -                    "possible to avoid the warning messages.");
>> +            fwts_advice(fw,
>> +                "The kernel will warn that this table has "
>> +                "an invalid checksum but will ignore the "
>> +                "error and still load it. This is not a "
>> +                "critical issue, but should be fixed if "
>> +                "possible to avoid the warning messages.");
>>
>>               fwts_tag_failed(fw, FWTS_TAG_ACPI_TABLE_CHECKSUM);
>>           }
>
> If checksum is incorrect, that means it cannot be determined whether the
> table is still trustworthy. It is known that kernel will ignore the
> error and therefore it is not critical; however, will it be better to
> make the level to high or medium so the bug will be highlighted and fixed?
>
> After all, the error means a system does not meet ACPI specification.

Well, I'm OK with HIGH, or MEDIUM, I suspect MEDIUM is better because it 
isn't a show-stopper of a bug, but it still need some attention.  I can 
re-send with this level increased if you think that is better.

Colin
>
> Best Regards,
> Alex Hung
>

On 06/18/2012 04:01 PM, Colin Ian King wrote:
> On 18/06/12 03:38, Alex Hung wrote:
>> On 06/14/2012 10:06 PM, Colin King wrote:
>>> From: Colin Ian King<colin.king@canonical.com>
>>>
>>> It seems that the kernel is quite happy to handle RSDT and XSDT tables
>>> that
>>> fail on their checksum checks, so lets not fail these as critical
>>> failures
>>> anymore.
>>>
>>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>>> ---
>>> src/acpi/checksum/checksum.c | 41
>>> +++++++----------------------------------
>>> 1 file changed, 7 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/src/acpi/checksum/checksum.c b/src/acpi/checksum/checksum.c
>>> index 264e7d0..2c41cce 100644
>>> --- a/src/acpi/checksum/checksum.c
>>> +++ b/src/acpi/checksum/checksum.c
>>> @@ -89,16 +89,6 @@ static void checksum_rsdp(fwts_framework *fw,
>>> fwts_acpi_table_info *table)
>>>
>>> }
>>>
>>> -/*
>>> - * The following tables the kernel requires the checksum to be valid
>>> otherwise
>>> - * it will not load them, so checksum failures here are considered
>>> critical errors.
>>> - */
>>> -static char *critical_checksum[] = {
>>> - "RSDT",
>>> - "XSDT",
>>> - NULL
>>> -};
>>> -
>>> static int checksum_scan_tables(fwts_framework *fw)
>>> {
>>> int i;
>>> @@ -131,35 +121,18 @@ static int checksum_scan_tables(fwts_framework
>>> *fw)
>>> fwts_passed(fw, "Table %s has correct checksum 0x%x.",
>>> table->name, hdr->checksum);
>>> else {
>>> - int i;
>>> - int log_level = LOG_LEVEL_LOW;
>>> -
>>> - for (i = 0; critical_checksum[i]; i++) {
>>> - if (!strcmp(table->name, critical_checksum[i])) {
>>> - log_level = LOG_LEVEL_CRITICAL;
>>> - break;
>>> - }
>>> - }
>>> -
>>> - fwts_failed(fw, log_level, "ACPITableChecksum",
>>> + fwts_failed(fw, LOG_LEVEL_LOW, "ACPITableChecksum",
>>> "Table %s has incorrect checksum, "
>>> "expected 0x%2.2x, got 0x%2.2x.",
>>> table->name, (uint8_t)(hdr->checksum-checksum),
>>> hdr->checksum);
>>>
>>> - /* Give some contextual explanation of the error */
>>> - if (log_level == LOG_LEVEL_CRITICAL)
>>> - fwts_advice(fw,
>>> - "The kernel requires this table to have a "
>>> - "valid checksum and will not load it. This "
>>> - "will lead to ACPI not working correctly.");
>>> - else
>>> - fwts_advice(fw,
>>> - "The kernel will warn that this table has "
>>> - "an invalid checksum but will ignore the "
>>> - "error and still load it. This is not a "
>>> - "critical issue, but should be fixed if "
>>> - "possible to avoid the warning messages.");
>>> + fwts_advice(fw,
>>> + "The kernel will warn that this table has "
>>> + "an invalid checksum but will ignore the "
>>> + "error and still load it. This is not a "
>>> + "critical issue, but should be fixed if "
>>> + "possible to avoid the warning messages.");
>>>
>>> fwts_tag_failed(fw, FWTS_TAG_ACPI_TABLE_CHECKSUM);
>>> }
>>
>> If checksum is incorrect, that means it cannot be determined whether the
>> table is still trustworthy. It is known that kernel will ignore the
>> error and therefore it is not critical; however, will it be better to
>> make the level to high or medium so the bug will be highlighted and
>> fixed?
>>
>> After all, the error means a system does not meet ACPI specification.
>
> Well, I'm OK with HIGH, or MEDIUM, I suspect MEDIUM is better because it
> isn't a show-stopper of a bug, but it still need some attention. I can
> re-send with this level increased if you think that is better.

I agree that MEDIUM is a better choice too although I would hope BIOS 
engineers to take it more seriously...

Thanks, Colin.


>
> Colin
>>
>> Best Regards,
>> Alex Hung
>>
>