| Message ID | 1454981583-31872-9-git-send-email-al.stone@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
On 09/02/16 01:32, Al Stone wrote: > The primary purpose of this patch is to catch some very minor white > space edits while adding in two very simple compliance tests -- is the > table checksum correct, and is the revision number current? > > Signed-off-by: Al Stone <al.stone@linaro.org> > --- > src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 47 insertions(+), 4 deletions(-) > > diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c > index 5106e4e..6e5ee26 100644 > --- a/src/acpi/fadt/fadt.c > +++ b/src/acpi/fadt/fadt.c > @@ -20,8 +20,8 @@ > */ > #include "fwts.h" > > -#include <stdlib.h> > #include <stdio.h> > +#include <stdlib.h> > #include <sys/types.h> > #include <sys/stat.h> > #ifdef FWTS_ARCH_INTEL > @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) > fwts_log_error(fw, "ACPI table FACP does not exist!"); > return FWTS_ERROR; > } > - fadt = (const fwts_acpi_table_fadt*)table->data; > + fadt = (const fwts_acpi_table_fadt *)table->data; > fadt_size = table->length; > > /* Not having a FADT is not a failure on x86 */ > @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) > return FWTS_OK; > } > > +static int fadt_checksum(fwts_framework *fw) > +{ > + const uint8_t *data = (const uint8_t *)fadt; > + ssize_t length = fadt->header.length; > + uint8_t checksum = 0; > + > + /* verify the table checksum */ > + checksum = fwts_checksum(data, length); > + if (checksum == 0) > + fwts_passed(fw, "FADT checksum is correct"); > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "SPECMADTChecksum", > + "FADT checksum is incorrect: 0x%x", checksum); > + > + return FWTS_OK; > +} > + > +static int fadt_revision(fwts_framework *fw) > +{ > + uint8_t major; > + uint8_t minor; > + > + major = fadt->header.revision; > + minor = 0; > + if (major >= 5 && fadt->header.length >= 268) > + minor = fadt->minor_version; /* field added ACPI 5.1 */ > + > + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); > + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); > + > + if (major == 6) > + fwts_passed(fw, "FADT revision is up to date."); > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", > + "FADT revision is outdated: %d.%d", major, minor); > + > + return FWTS_OK; > +} > + > + > static void acpi_table_check_fadt_firmware_control( > fwts_framework *fw, > const fwts_acpi_table_fadt *fadt, > @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) > } > > static fwts_framework_minor_test fadt_tests[] = { > - { fadt_info, "FADT ACPI Description Table flag info." }, > - { fadt_test1, "Test FADT ACPI Description Table tests." }, > + { fadt_info, "ACPI FADT Description Table flag info." }, > + { fadt_checksum, "FADT checksum test." }, > + { fadt_revision, "FADT revision test." }, > + { fadt_test1, "ACPI FADT Description Table tests." }, > { fadt_test2, "Test FADT SCI_EN bit is enabled." }, > { fadt_test3, "Test FADT reset register." }, > { NULL, NULL } > Acked-by: Colin Ian King <colin.king@canonucal.com>
On 2016-02-09 09:32 AM, Al Stone wrote: > The primary purpose of this patch is to catch some very minor white > space edits while adding in two very simple compliance tests -- is the > table checksum correct, and is the revision number current? > > Signed-off-by: Al Stone <al.stone@linaro.org> > --- > src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 47 insertions(+), 4 deletions(-) > > diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c > index 5106e4e..6e5ee26 100644 > --- a/src/acpi/fadt/fadt.c > +++ b/src/acpi/fadt/fadt.c > @@ -20,8 +20,8 @@ > */ > #include "fwts.h" > > -#include <stdlib.h> > #include <stdio.h> > +#include <stdlib.h> > #include <sys/types.h> > #include <sys/stat.h> > #ifdef FWTS_ARCH_INTEL > @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) > fwts_log_error(fw, "ACPI table FACP does not exist!"); > return FWTS_ERROR; > } > - fadt = (const fwts_acpi_table_fadt*)table->data; > + fadt = (const fwts_acpi_table_fadt *)table->data; > fadt_size = table->length; > > /* Not having a FADT is not a failure on x86 */ > @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) > return FWTS_OK; > } > > +static int fadt_checksum(fwts_framework *fw) > +{ > + const uint8_t *data = (const uint8_t *)fadt; > + ssize_t length = fadt->header.length; > + uint8_t checksum = 0; > + > + /* verify the table checksum */ > + checksum = fwts_checksum(data, length); > + if (checksum == 0) > + fwts_passed(fw, "FADT checksum is correct"); > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, > + "SPECMADTChecksum", > + "FADT checksum is incorrect: 0x%x", checksum); > + > + return FWTS_OK; > +} > + > +static int fadt_revision(fwts_framework *fw) > +{ > + uint8_t major; > + uint8_t minor; > + > + major = fadt->header.revision; > + minor = 0; > + if (major >= 5 && fadt->header.length >= 268) > + minor = fadt->minor_version; /* field added ACPI 5.1 */ > + > + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); > + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); > + > + if (major == 6) > + fwts_passed(fw, "FADT revision is up to date."); > + else > + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", > + "FADT revision is outdated: %d.%d", major, minor); If fwts is run on older systems with FADT revision not equal to 6, this generates an error even though the systems were cutting-edge at their shipping time. I am not very sure a medium failure is the best choice. How about an info or a warning with an advice so either a firmware developer can update it or an user can check for a firmware update? Any comments from anyone? > + > + return FWTS_OK; > +} > + > + > static void acpi_table_check_fadt_firmware_control( > fwts_framework *fw, > const fwts_acpi_table_fadt *fadt, > @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) > } > > static fwts_framework_minor_test fadt_tests[] = { > - { fadt_info, "FADT ACPI Description Table flag info." }, > - { fadt_test1, "Test FADT ACPI Description Table tests." }, > + { fadt_info, "ACPI FADT Description Table flag info." }, > + { fadt_checksum, "FADT checksum test." }, > + { fadt_revision, "FADT revision test." }, > + { fadt_test1, "ACPI FADT Description Table tests." }, > { fadt_test2, "Test FADT SCI_EN bit is enabled." }, > { fadt_test3, "Test FADT reset register." }, > { NULL, NULL } >
On 02/16/2016 10:35 PM, Alex Hung wrote: > On 2016-02-09 09:32 AM, Al Stone wrote: >> The primary purpose of this patch is to catch some very minor white >> space edits while adding in two very simple compliance tests -- is the >> table checksum correct, and is the revision number current? >> >> Signed-off-by: Al Stone <al.stone@linaro.org> >> --- >> src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 47 insertions(+), 4 deletions(-) >> >> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c >> index 5106e4e..6e5ee26 100644 >> --- a/src/acpi/fadt/fadt.c >> +++ b/src/acpi/fadt/fadt.c >> @@ -20,8 +20,8 @@ >> */ >> #include "fwts.h" >> >> -#include <stdlib.h> >> #include <stdio.h> >> +#include <stdlib.h> >> #include <sys/types.h> >> #include <sys/stat.h> >> #ifdef FWTS_ARCH_INTEL >> @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) >> fwts_log_error(fw, "ACPI table FACP does not exist!"); >> return FWTS_ERROR; >> } >> - fadt = (const fwts_acpi_table_fadt*)table->data; >> + fadt = (const fwts_acpi_table_fadt *)table->data; >> fadt_size = table->length; >> >> /* Not having a FADT is not a failure on x86 */ >> @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) >> return FWTS_OK; >> } >> >> +static int fadt_checksum(fwts_framework *fw) >> +{ >> + const uint8_t *data = (const uint8_t *)fadt; >> + ssize_t length = fadt->header.length; >> + uint8_t checksum = 0; >> + >> + /* verify the table checksum */ >> + checksum = fwts_checksum(data, length); >> + if (checksum == 0) >> + fwts_passed(fw, "FADT checksum is correct"); >> + else >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, >> + "SPECMADTChecksum", >> + "FADT checksum is incorrect: 0x%x", checksum); >> + >> + return FWTS_OK; >> +} >> + >> +static int fadt_revision(fwts_framework *fw) >> +{ >> + uint8_t major; >> + uint8_t minor; >> + >> + major = fadt->header.revision; >> + minor = 0; >> + if (major >= 5 && fadt->header.length >= 268) >> + minor = fadt->minor_version; /* field added ACPI 5.1 */ >> + >> + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); >> + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); >> + >> + if (major == 6) >> + fwts_passed(fw, "FADT revision is up to date."); >> + else >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", >> + "FADT revision is outdated: %d.%d", major, minor); > > If fwts is run on older systems with FADT revision not equal to 6, this > generates an error even though the systems were cutting-edge at their shipping > time. I am not very sure a medium failure is the best choice. > > How about an info or a warning with an advice so either a firmware developer can > update it or an user can check for a firmware update? > > Any comments from anyone? > Hrm. I'll change this to a warning and an advice. This does bring up a much more difficult problem that I've been trying to think through: what I would like to be able to do is something like this: # ./fwts --acpicompliance --spec=5.1 and check for to make sure the ACPI tables are 5.1 compliant, or --spec=6.0 or --spec=6.1 .... unfortunately, that seems to touch on a level of complexity that may ultimately not be useful ... It would, however, make this particular test clearer. An FADT version 1 would be incorrect for the ACPI 6.0 spec, but okay for ACPI 2.0, and so on. My biggest concern with this is that some quick sampling of machines shows that this is _not_ the way firmware vendors -- nor OS developers -- have been using the ACPI tables historically. The test results would probably end up far more confusing than useful. In the meantime, I'll touch up this test in particular for now. >> + >> + return FWTS_OK; >> +} >> + >> + >> static void acpi_table_check_fadt_firmware_control( >> fwts_framework *fw, >> const fwts_acpi_table_fadt *fadt, >> @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) >> } >> >> static fwts_framework_minor_test fadt_tests[] = { >> - { fadt_info, "FADT ACPI Description Table flag info." }, >> - { fadt_test1, "Test FADT ACPI Description Table tests." }, >> + { fadt_info, "ACPI FADT Description Table flag info." }, >> + { fadt_checksum, "FADT checksum test." }, >> + { fadt_revision, "FADT revision test." }, >> + { fadt_test1, "ACPI FADT Description Table tests." }, >> { fadt_test2, "Test FADT SCI_EN bit is enabled." }, >> { fadt_test3, "Test FADT reset register." }, >> { NULL, NULL } >> > >
On Thu, Feb 18, 2016 at 5:45 AM, Al Stone <al.stone@linaro.org> wrote: > On 02/16/2016 10:35 PM, Alex Hung wrote: >> On 2016-02-09 09:32 AM, Al Stone wrote: >>> The primary purpose of this patch is to catch some very minor white >>> space edits while adding in two very simple compliance tests -- is the >>> table checksum correct, and is the revision number current? >>> >>> Signed-off-by: Al Stone <al.stone@linaro.org> >>> --- >>> src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 47 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c >>> index 5106e4e..6e5ee26 100644 >>> --- a/src/acpi/fadt/fadt.c >>> +++ b/src/acpi/fadt/fadt.c >>> @@ -20,8 +20,8 @@ >>> */ >>> #include "fwts.h" >>> >>> -#include <stdlib.h> >>> #include <stdio.h> >>> +#include <stdlib.h> >>> #include <sys/types.h> >>> #include <sys/stat.h> >>> #ifdef FWTS_ARCH_INTEL >>> @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) >>> fwts_log_error(fw, "ACPI table FACP does not exist!"); >>> return FWTS_ERROR; >>> } >>> - fadt = (const fwts_acpi_table_fadt*)table->data; >>> + fadt = (const fwts_acpi_table_fadt *)table->data; >>> fadt_size = table->length; >>> >>> /* Not having a FADT is not a failure on x86 */ >>> @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) >>> return FWTS_OK; >>> } >>> >>> +static int fadt_checksum(fwts_framework *fw) >>> +{ >>> + const uint8_t *data = (const uint8_t *)fadt; >>> + ssize_t length = fadt->header.length; >>> + uint8_t checksum = 0; >>> + >>> + /* verify the table checksum */ >>> + checksum = fwts_checksum(data, length); >>> + if (checksum == 0) >>> + fwts_passed(fw, "FADT checksum is correct"); >>> + else >>> + fwts_failed(fw, LOG_LEVEL_MEDIUM, >>> + "SPECMADTChecksum", >>> + "FADT checksum is incorrect: 0x%x", checksum); >>> + >>> + return FWTS_OK; >>> +} >>> + >>> +static int fadt_revision(fwts_framework *fw) >>> +{ >>> + uint8_t major; >>> + uint8_t minor; >>> + >>> + major = fadt->header.revision; >>> + minor = 0; >>> + if (major >= 5 && fadt->header.length >= 268) >>> + minor = fadt->minor_version; /* field added ACPI 5.1 */ >>> + >>> + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); >>> + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); >>> + >>> + if (major == 6) >>> + fwts_passed(fw, "FADT revision is up to date."); >>> + else >>> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", >>> + "FADT revision is outdated: %d.%d", major, minor); >> >> If fwts is run on older systems with FADT revision not equal to 6, this >> generates an error even though the systems were cutting-edge at their shipping >> time. I am not very sure a medium failure is the best choice. >> >> How about an info or a warning with an advice so either a firmware developer can >> update it or an user can check for a firmware update? >> >> Any comments from anyone? >> I think the argument --spec=5.1 is too much for this purpose, too. After all, we will begin seeing more systems shipping for ACPI 6 or 6.1 one soon. A warning and an advice should work as a good reminder for firmware developer. > > Hrm. I'll change this to a warning and an advice. > > This does bring up a much more difficult problem that I've been trying to > think through: what I would like to be able to do is something like this: > > # ./fwts --acpicompliance --spec=5.1 > > and check for to make sure the ACPI tables are 5.1 compliant, or --spec=6.0 > or --spec=6.1 .... unfortunately, that seems to touch on a level of complexity > that may ultimately not be useful ... > > It would, however, make this particular test clearer. An FADT version 1 would > be incorrect for the ACPI 6.0 spec, but okay for ACPI 2.0, and so on. My > biggest concern with this is that some quick sampling of machines shows that > this is _not_ the way firmware vendors -- nor OS developers -- have been using > the ACPI tables historically. The test results would probably end up far more > confusing than useful. > > In the meantime, I'll touch up this test in particular for now. > >>> + >>> + return FWTS_OK; >>> +} >>> + >>> + >>> static void acpi_table_check_fadt_firmware_control( >>> fwts_framework *fw, >>> const fwts_acpi_table_fadt *fadt, >>> @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) >>> } >>> >>> static fwts_framework_minor_test fadt_tests[] = { >>> - { fadt_info, "FADT ACPI Description Table flag info." }, >>> - { fadt_test1, "Test FADT ACPI Description Table tests." }, >>> + { fadt_info, "ACPI FADT Description Table flag info." }, >>> + { fadt_checksum, "FADT checksum test." }, >>> + { fadt_revision, "FADT revision test." }, >>> + { fadt_test1, "ACPI FADT Description Table tests." }, >>> { fadt_test2, "Test FADT SCI_EN bit is enabled." }, >>> { fadt_test3, "Test FADT reset register." }, >>> { NULL, NULL } >>> >> >> > > > -- > ciao, > al > ----------------------------------- > Al Stone > Software Engineer > Linaro Enterprise Group > al.stone@linaro.org > ----------------------------------- > > -- > fwts-devel mailing list > fwts-devel@lists.ubuntu.com > Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
diff --git a/src/acpi/fadt/fadt.c b/src/acpi/fadt/fadt.c index 5106e4e..6e5ee26 100644 --- a/src/acpi/fadt/fadt.c +++ b/src/acpi/fadt/fadt.c @@ -20,8 +20,8 @@ */ #include "fwts.h" -#include <stdlib.h> #include <stdio.h> +#include <stdlib.h> #include <sys/types.h> #include <sys/stat.h> #ifdef FWTS_ARCH_INTEL @@ -53,7 +53,7 @@ static int fadt_init(fwts_framework *fw) fwts_log_error(fw, "ACPI table FACP does not exist!"); return FWTS_ERROR; } - fadt = (const fwts_acpi_table_fadt*)table->data; + fadt = (const fwts_acpi_table_fadt *)table->data; fadt_size = table->length; /* Not having a FADT is not a failure on x86 */ @@ -152,6 +152,47 @@ static int fadt_info(fwts_framework *fw) return FWTS_OK; } +static int fadt_checksum(fwts_framework *fw) +{ + const uint8_t *data = (const uint8_t *)fadt; + ssize_t length = fadt->header.length; + uint8_t checksum = 0; + + /* verify the table checksum */ + checksum = fwts_checksum(data, length); + if (checksum == 0) + fwts_passed(fw, "FADT checksum is correct"); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, + "SPECMADTChecksum", + "FADT checksum is incorrect: 0x%x", checksum); + + return FWTS_OK; +} + +static int fadt_revision(fwts_framework *fw) +{ + uint8_t major; + uint8_t minor; + + major = fadt->header.revision; + minor = 0; + if (major >= 5 && fadt->header.length >= 268) + minor = fadt->minor_version; /* field added ACPI 5.1 */ + + fwts_log_info(fw, "FADT revision: %d.%d", major, minor); + fwts_log_info(fw, "FADT table length: %d", fadt->header.length); + + if (major == 6) + fwts_passed(fw, "FADT revision is up to date."); + else + fwts_failed(fw, LOG_LEVEL_MEDIUM, "SPECFADTRevision", + "FADT revision is outdated: %d.%d", major, minor); + + return FWTS_OK; +} + + static void acpi_table_check_fadt_firmware_control( fwts_framework *fw, const fwts_acpi_table_fadt *fadt, @@ -635,8 +676,10 @@ static int fadt_test3(fwts_framework *fw) } static fwts_framework_minor_test fadt_tests[] = { - { fadt_info, "FADT ACPI Description Table flag info." }, - { fadt_test1, "Test FADT ACPI Description Table tests." }, + { fadt_info, "ACPI FADT Description Table flag info." }, + { fadt_checksum, "FADT checksum test." }, + { fadt_revision, "FADT revision test." }, + { fadt_test1, "ACPI FADT Description Table tests." }, { fadt_test2, "Test FADT SCI_EN bit is enabled." }, { fadt_test3, "Test FADT reset register." }, { NULL, NULL }
The primary purpose of this patch is to catch some very minor white space edits while adding in two very simple compliance tests -- is the table checksum correct, and is the revision number current? Signed-off-by: Al Stone <al.stone@linaro.org> --- src/acpi/fadt/fadt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 47 insertions(+), 4 deletions(-)