Message ID | 1339682762-26456-1-git-send-email-colin.king@canonical.com |
---|---|
State | Rejected |
Headers | show |
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 >> >
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); }