[PATCH 2/3] dmi_decode: Only be pedantic if we are sure about the board type (LP:#1021674)
Keng-Yu Lin
kengyu at canonical.com
Mon Jul 9 08:03:41 UTC 2012
On Fri, Jul 6, 2012 at 10:30 PM, Colin King <colin.king at canonical.com> wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> We only should really compare board type and preferred PM profile if we
> are totally sure about the the types. If either are CHASSIS_OTHER then
> don't bother being pedantic and ship the check. Also, because CERT can't
> figure out if this error is bad or not, add some advice text to fully
> explain what this error really means.
>
> To make this more friendly, I've added the a textual description of the
> chassis type and PM preferred profiles rather than just dump out some
> undecipherable magic numbers.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
> src/dmi/dmi_decode/dmi_decode.c | 131 +++++++++++++++++++++++++--------------
> 1 file changed, 84 insertions(+), 47 deletions(-)
>
> diff --git a/src/dmi/dmi_decode/dmi_decode.c b/src/dmi/dmi_decode/dmi_decode.c
> index f8c37ed..67dfea2 100644
> --- a/src/dmi/dmi_decode/dmi_decode.c
> +++ b/src/dmi/dmi_decode/dmi_decode.c
> @@ -73,6 +73,7 @@ typedef struct {
> } fwts_dmi_version;
>
> typedef struct {
> + const char *name;
> uint8_t original;
> uint8_t mapped;
> } fwts_chassis_type_map;
> @@ -90,48 +91,48 @@ static const char *uuid_patterns[] = {
> };
>
> static const fwts_chassis_type_map fwts_dmi_chassis_type[] = {
> - { FWTS_SMBIOS_CHASSIS_INVALID, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_OTHER, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_UNKNOWN, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_DESKTOP, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_LOW_PROFILE_DESKTOP, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_PIZZA_BOX, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_MINI_TOWER, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_TOWER, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_PORTABLE, CHASSIS_MOBILE },
> - { FWTS_SMBIOS_CHASSIS_LAPTOP, CHASSIS_MOBILE },
> - { FWTS_SMBIOS_CHASSIS_NOTEBOOK, CHASSIS_MOBILE },
> - { FWTS_SMBIOS_CHASSIS_HANDHELD, CHASSIS_MOBILE },
> - { FWTS_SMBIOS_CHASSIS_DOCKING_STATION, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_ALL_IN_ONE, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_SUB_NOTEBOOK, CHASSIS_MOBILE },
> - { FWTS_SMBIOS_CHASSIS_SPACE_SAVING, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_LUNCH_BOX, CHASSIS_DESKTOP | CHASSIS_MOBILE},
> - { FWTS_SMBIOS_CHASSIS_MAIN_SERVER_CHASSIS, CHASSIS_SERVER },
> - { FWTS_SMBIOS_CHASSIS_EXPANISON_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_SUB_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_BUS_EXPANSION_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_PERIPHERAL_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_RAID_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_RACK_MOUNT_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_SEALED_CASE_PC, CHASSIS_DESKTOP },
> - { FWTS_SMBIOS_CHASSIS_MULTI_SYSTEM_CHASSIS, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_COMPACT_PCI, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_ADVANCED_TCA, CHASSIS_OTHER },
> - { FWTS_SMBIOS_CHASSIS_BLADE, CHASSIS_SERVER },
> - { FWTS_SMBIOS_CHASSIS_BLASE_ENCLOSURE, CHASSIS_SERVER }
> + { "Invalid", FWTS_SMBIOS_CHASSIS_INVALID, CHASSIS_OTHER },
> + { "Other", FWTS_SMBIOS_CHASSIS_OTHER, CHASSIS_OTHER },
> + { "Unknown", FWTS_SMBIOS_CHASSIS_UNKNOWN, CHASSIS_OTHER },
> + { "Desktop", FWTS_SMBIOS_CHASSIS_DESKTOP, CHASSIS_DESKTOP },
> + { "Low Profile Desktop",FWTS_SMBIOS_CHASSIS_LOW_PROFILE_DESKTOP, CHASSIS_DESKTOP },
> + { "Pizza Box", FWTS_SMBIOS_CHASSIS_PIZZA_BOX, CHASSIS_DESKTOP },
> + { "Mini Tower", FWTS_SMBIOS_CHASSIS_MINI_TOWER, CHASSIS_DESKTOP },
> + { "Chassis Tower", FWTS_SMBIOS_CHASSIS_TOWER, CHASSIS_DESKTOP },
> + { "Portable", FWTS_SMBIOS_CHASSIS_PORTABLE, CHASSIS_MOBILE },
> + { "Laptop", FWTS_SMBIOS_CHASSIS_LAPTOP, CHASSIS_MOBILE },
> + { "Notebook", FWTS_SMBIOS_CHASSIS_NOTEBOOK, CHASSIS_MOBILE },
> + { "Handheld", FWTS_SMBIOS_CHASSIS_HANDHELD, CHASSIS_MOBILE },
> + { "Docking Station", FWTS_SMBIOS_CHASSIS_DOCKING_STATION, CHASSIS_DESKTOP },
> + { "All In One", FWTS_SMBIOS_CHASSIS_ALL_IN_ONE, CHASSIS_DESKTOP },
> + { "Sub Notebook", FWTS_SMBIOS_CHASSIS_SUB_NOTEBOOK, CHASSIS_MOBILE },
> + { "Space Saving", FWTS_SMBIOS_CHASSIS_SPACE_SAVING, CHASSIS_DESKTOP },
> + { "Lunch Box", FWTS_SMBIOS_CHASSIS_LUNCH_BOX, CHASSIS_DESKTOP | CHASSIS_MOBILE},
> + { "Server Chassis", FWTS_SMBIOS_CHASSIS_MAIN_SERVER_CHASSIS, CHASSIS_SERVER },
> + { "Expansion Chassis", FWTS_SMBIOS_CHASSIS_EXPANISON_CHASSIS, CHASSIS_OTHER },
> + { "Sub Chassis", FWTS_SMBIOS_CHASSIS_SUB_CHASSIS, CHASSIS_OTHER },
> + { "Bus Expansion Chassis", FWTS_SMBIOS_CHASSIS_BUS_EXPANSION_CHASSIS, CHASSIS_OTHER },
> + { "Peripheral Chassis", FWTS_SMBIOS_CHASSIS_PERIPHERAL_CHASSIS, CHASSIS_OTHER },
> + { "Raid Chassis", FWTS_SMBIOS_CHASSIS_RAID_CHASSIS, CHASSIS_OTHER },
> + { "Rack Mount Chassis", FWTS_SMBIOS_CHASSIS_RACK_MOUNT_CHASSIS, CHASSIS_OTHER },
> + { "Sealed Case PC", FWTS_SMBIOS_CHASSIS_SEALED_CASE_PC, CHASSIS_DESKTOP },
> + { "Multi System Chassis",FWTS_SMBIOS_CHASSIS_MULTI_SYSTEM_CHASSIS, CHASSIS_OTHER },
> + { "Compact PCI", FWTS_SMBIOS_CHASSIS_COMPACT_PCI, CHASSIS_OTHER },
> + { "Advanced TCA", FWTS_SMBIOS_CHASSIS_ADVANCED_TCA, CHASSIS_OTHER },
> + { "Blade", FWTS_SMBIOS_CHASSIS_BLADE, CHASSIS_SERVER },
> + { "Enclosure", FWTS_SMBIOS_CHASSIS_BLASE_ENCLOSURE, CHASSIS_SERVER }
> };
>
> static const fwts_chassis_type_map fwts_acpi_pm_profile_type[] = {
> - { FWTS_FACP_UNSPECIFIED, CHASSIS_OTHER },
> - { FWTS_FACP_DESKTOP, CHASSIS_DESKTOP },
> - { FWTS_FACP_MOBILE, CHASSIS_MOBILE },
> - { FWTS_FACP_WORKSTATION, CHASSIS_WORKSTATION },
> - { FWTS_FACP_ENTERPRISE_SERVER, CHASSIS_SERVER },
> - { FWTS_FACP_SOHO_SERVER, CHASSIS_SERVER | CHASSIS_DESKTOP },
> - { FWTS_FACP_APPLIANCE_PC, CHASSIS_DESKTOP },
> - { FWTS_FACP_PERFORMANCE_SERVER, CHASSIS_SERVER },
> - { FWTS_FACP_TABLET, CHASSIS_MOBILE }
> + { "Unspecified", FWTS_FACP_UNSPECIFIED, CHASSIS_OTHER },
> + { "Desktop", FWTS_FACP_DESKTOP, CHASSIS_DESKTOP },
> + { "Mobile", FWTS_FACP_MOBILE, CHASSIS_MOBILE },
> + { "Workstation", FWTS_FACP_WORKSTATION, CHASSIS_WORKSTATION },
> + { "Enterprise Server", FWTS_FACP_ENTERPRISE_SERVER, CHASSIS_SERVER },
> + { "SOHO Server", FWTS_FACP_SOHO_SERVER, CHASSIS_SERVER | CHASSIS_DESKTOP },
> + { "Appliance PC", FWTS_FACP_APPLIANCE_PC, CHASSIS_DESKTOP },
> + { "Performance Server", FWTS_FACP_PERFORMANCE_SERVER, CHASSIS_SERVER },
> + { "Tablet", FWTS_FACP_TABLET, CHASSIS_MOBILE }
> };
>
> /* Remapping table from buggy version numbers to correct values */
> @@ -300,8 +301,10 @@ static void dmi_decode_entry(fwts_framework *fw,
> int failed_count = fw->minor_tests.failed;
> int battery_count;
> int ret;
> + int both_ok;
> fwts_acpi_table_info *acpi_table;
> fwts_acpi_table_fadt *fadt;
> + bool advice_given = false;
>
> switch (hdr->type) {
> case 0: /* 7.1 */
> @@ -359,6 +362,7 @@ static void dmi_decode_entry(fwts_framework *fw,
> if (acpi_table == NULL)
> break;
> fadt = (fwts_acpi_table_fadt *)acpi_table->data;
> +
> if (fadt->preferred_pm_profile >=
> (sizeof(fwts_acpi_pm_profile_type) / sizeof(fwts_chassis_type_map))) {
> fwts_failed(fw, LOG_LEVEL_HIGH, DMI_INVALID_HARDWARE_ENTRY,
> @@ -371,18 +375,51 @@ static void dmi_decode_entry(fwts_framework *fw,
> (sizeof(fwts_dmi_chassis_type) / sizeof(fwts_chassis_type_map))) {
> fwts_failed(fw, LOG_LEVEL_HIGH, DMI_INVALID_HARDWARE_ENTRY,
> "Incorrect Chassis Type "
> - "SMBIOS Type 3 reports 0x%x ",
> + "SMBIOS Type 3 reports 0x%x",
> data[5]);
> break;
> }
> - if (!(fwts_acpi_pm_profile_type[fadt->preferred_pm_profile].mapped &
> - fwts_dmi_chassis_type[data[5]].mapped)) {
> +
> + /*
> + * LP: #1021674
> + * We should only check profile and chassis types when we know for sure
> + * that we can compare valid types togther. So it is only valid to do
> + * a check if both aren't CHASSIS_OTHER types
> + */
> + both_ok = (fwts_acpi_pm_profile_type[fadt->preferred_pm_profile].mapped != CHASSIS_OTHER) &
> + (fwts_dmi_chassis_type[data[5]].mapped != CHASSIS_OTHER);
> +
> + if ( both_ok &&
> + !(fwts_acpi_pm_profile_type[fadt->preferred_pm_profile].mapped &
> + fwts_dmi_chassis_type[data[5]].mapped)) {
> fwts_failed(fw, LOG_LEVEL_HIGH, DMI_INVALID_HARDWARE_ENTRY,
> - "Unmatched Chassis Type "
> - "SMBIOS Type 3 reports 0x%x "
> - "ACPI FACP reports 0x%x",
> + "Unmatched Chassis Type: "
> + "SMBIOS Type 3 reports 0x%x '%s' "
> + "ACPI FACP reports 0x%x '%s'",
> data[5],
> - fadt->preferred_pm_profile);
> + fwts_dmi_chassis_type[data[5]].name,
> + fadt->preferred_pm_profile,
> + fwts_acpi_pm_profile_type[fadt->preferred_pm_profile].name);
> + /*
> + * Make it a bit more wordy to explain the ramifications
> + */
> + fwts_advice(fw,
> + "The SMBIOS System Enclosure/Chassis type is defined as "
> + "0x%x '%s' where as the ACPI FACP reports the preferred "
> + "power management profile is 0x%x '%s' so we possibly "
> + "have conflicting definitions of the machine's PM profile "
> + "for the type of machine. "
> + "See Table 16 of section 4.5.1 of the SMBIOS specification "
> + "and Table 5-34 of section 5.2.9 of the ACPI specification "
> + "for more details. "
> + "This kind of mismatch may lead to incorrect power "
> + "management handling for the type of workload expected "
> + "for this hardware.",
> + data[5],
> + fwts_dmi_chassis_type[data[5]].name,
> + fadt->preferred_pm_profile,
> + fwts_acpi_pm_profile_type[fadt->preferred_pm_profile].name);
> + advice_given = true;
> }
> dmi_min_max_mask_uint8_check(fw, table, addr, "Chassis Lock", hdr, 0x5, 0x0, 0x1, 0x7, 0x1);
> dmi_str_check(fw, table, addr, "Version", hdr, 0x6);
> @@ -956,7 +993,7 @@ static void dmi_decode_entry(fwts_framework *fw,
> }
> if (fw->minor_tests.failed == failed_count)
> fwts_passed(fw, "Entry @ 0x%8.8x '%s'", addr, table);
> - else if (hdr->type <= 42)
> + else if (!advice_given && hdr->type <= 42)
> fwts_advice(fw,
> "It may be worth checking against section 7.%d of the "
> "System Management BIOS (SMBIOS) Reference Specification "
> --
> 1.7.10.4
>
Acked-by: Keng-Yu Lin <kengyu at canonical.com>
More information about the fwts-devel
mailing list