| Message ID | 1428047258-15590-2-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | Rejected |
| Headers | show |
Hi Al, Thanks for your comments. I'll provide the 2nd version. On 04/04/2015 05:53 AM, Al Stone wrote: > On 04/03/2015 01:47 AM, Heyi Guo wrote: >> According to ACPI spec 5.1, section 5.2.9, "If the HW_REDUCED_ACPI flag in the table is set, OSPM will ignore fields related to the ACPI HW register interface: Fields at offsets 46 through 108 and 148 through 232, as well as FADT Flag bits 1, 2, 3,7,8,12,13, 14, 16 and 17).", add precondition of checking SMI_CMD, PM_TMR_LEN, etc. >> >> The code may become a little complex; it will be better to split into small functions later. > Hrm...the more I looked at this, maybe it makes sense to do small functions now? > I know it's more work, but it'll be more maintainable, I suspect. > > This way, the logic could be: > > fwts_check_FADT_field_A(); > fwts_check_FADT_field_B(); > .... > fwts_check_FADT_field_n(); > > Each function could then do the proper checks, as you suggest. E.g., > > void fwts_check_FADT_day_alrm(acpi_table_fadt *fadt) > { > if (fwts_acpi_is_reduced_hardware(fadt) { > if (fadt->day_alrm != 0) > fwts_warning(fw, "FADT DAY_ALRM is not zero but should be in reduced hardware > mode."); > } else { > if (fadt->day_alrm == 0) > fwts_warning(fw, "FADT DAY_ALRM is zero, OS will not be able to program day > of month alarm."); > } > } > > ...or something like that.... > > I agree with you -- the code is starting to look a little complex and chunky. > >> Signed-off-by: Heyi Guo <heyi.guo@linaro.org> >> --- >> src/acpi/acpitables/acpitables.c | 164 ++++++++++++++++++++------------------- >> 1 file changed, 83 insertions(+), 81 deletions(-) >> >> diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c >> index e5fffff..acc1505 100644 >> --- a/src/acpi/acpitables/acpitables.c >> +++ b/src/acpi/acpitables/acpitables.c >> @@ -122,92 +122,94 @@ static void acpi_table_check_fadt(fwts_framework *fw, fwts_acpi_table_info *tabl >> } >> } >> >> - /* >> - * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0 >> - * specification states that if SMI_CMD is zero then it is >> - * a system that does not support System Management Mode, so >> - * in that case, don't check SCI_INT being valid. >> - */ >> - if (fadt->smi_cmd != 0) { >> - if (fadt->sci_int == 0) { >> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSCIIRQZero", "FADT SCI Interrupt is 0x00, should be defined."); >> + if (!fwts_acpi_is_reduced_hardware(fadt)) { >> + /* >> + * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0 >> + * specification states that if SMI_CMD is zero then it is >> + * a system that does not support System Management Mode, so >> + * in that case, don't check SCI_INT being valid. >> + */ >> + if (fadt->smi_cmd != 0) { >> + if (fadt->sci_int == 0) { >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSCIIRQZero", "FADT SCI Interrupt is 0x00, should be defined."); >> + } >> + } else { >> + if ((fadt->acpi_enable == 0) && >> + (fadt->acpi_disable == 0) && >> + (fadt->s4bios_req == 0) && >> + (fadt->pstate_cnt == 0) && >> + (fadt->cst_cnt == 0)) { >> + /* Not an error, but intentional, but feedback this finding anyhow */ >> + fwts_log_info(fw, "The FADT SMI_CMD is zero, system does not support System Management Mode."); >> + } >> + else { >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSMICMDZero", >> + "FADT SMI_CMD is 0x00, however, one or more of ACPI_ENABLE, ACPI_DISABLE, " >> + "S4BIOS_REQ, PSTATE_CNT and CST_CNT are defined which means SMI_CMD should be " >> + "defined otherwise SMI commands cannot be sent."); >> + fwts_advice(fw, "The configuration seems to suggest that SMI command should be defined to " >> + "allow the kernel to trigger system management interrupts via the SMD_CMD port. " >> + "The fact that SMD_CMD is zero which is invalid means that SMIs are not possible " >> + "through the normal ACPI mechanisms. This means some firmware based machine " >> + "specific functions will not work."); >> + } >> } >> - } else { >> - if ((fadt->acpi_enable == 0) && >> - (fadt->acpi_disable == 0) && >> - (fadt->s4bios_req == 0) && >> - (fadt->pstate_cnt == 0) && >> - (fadt->cst_cnt == 0)) { >> - /* Not an error, but intentional, but feedback this finding anyhow */ >> - fwts_log_info(fw, "The FADT SMI_CMD is zero, system does not support System Management Mode."); >> + >> + if (fadt->pm_tmr_len != 4) { >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", >> + "FADT PM_TMR_LEN is %" PRIu8 ", should be 4.", fadt->pm_tmr_len); >> + fwts_advice(fw, "FADT field PM_TMR_LEN defines the number of bytes decoded by PM_TMR_BLK. " >> + "This fields value must be 4. If it is not the correct size then the kernel " >> + "will not request a region for the pm timer block. "); >> } >> - else { >> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSMICMDZero", >> - "FADT SMI_CMD is 0x00, however, one or more of ACPI_ENABLE, ACPI_DISABLE, " >> - "S4BIOS_REQ, PSTATE_CNT and CST_CNT are defined which means SMI_CMD should be " >> - "defined otherwise SMI commands cannot be sent."); >> - fwts_advice(fw, "The configuration seems to suggest that SMI command should be defined to " >> - "allow the kernel to trigger system management interrupts via the SMD_CMD port. " >> - "The fact that SMD_CMD is zero which is invalid means that SMIs are not possible " >> - "through the normal ACPI mechanisms. This means some firmware based machine " >> - "specific functions will not work."); >> + if (fadt->gpe0_blk_len & 1) { >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPEBLKLEN", "FADT GPE0_BLK_LEN is %" PRIu8 >> + ", should a multiple of 2.", fadt->gpe0_blk_len); >> + fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " >> + "not map in the GPE0 region. This could mean that General Purpose Events will not " >> + "function correctly (for example lid or ac-power events)."); >> } >> + if (fadt->gpe1_blk_len & 1) { >> + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPE1BLKLEN", "FADT GPE1_BLK_LEN is %" PRIu8 >> + ", should a multiple of 2.", fadt->gpe1_blk_len); >> + fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " >> + "not map in the GPE1 region. This could mean that General Purpose Events will not " >> + "function correctly (for example lid or ac-power events)."); >> + } >> + /* >> + * Bug LP: /833644 >> + * >> + * Remove these tests, really need to put more intelligence into it >> + * perhaps in the cstates test rather than here. For the moment we >> + * shall remove this warning as it's giving users false alarms >> + * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 >> + */ >> + /* >> + if (fadt->p_lvl2_lat > 100) { >> + fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " >> + "system not to support a C2 state.", fadt->p_lvl2_lat); >> + fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " >> + "states that a value > 100 indicates that C2 is not supported and hence the " >> + "ACPI processor idle routine will not use C2 power states."); >> + } >> + if (fadt->p_lvl3_lat > 1000) { >> + fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " >> + "system not to support a C3 state.", fadt->p_lvl3_lat); >> + fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " >> + "states that a value > 1000 indicates that C3 is not supported and hence the " >> + "ACPI processor idle routine will not use C3 power states."); >> + } >> + */ >> + /* >> + if (fadt->day_alrm == 0) >> + fwts_warning(fw, "FADT DAY_ALRM is zero, OS will not be able to program day of month alarm."); >> + if (fadt->mon_alrm == 0) >> + fwts_warning(fw, "FADT MON_ALRM is zero, OS will not be able to program month of year alarm."); >> + if (fadt->century == 0) >> + fwts_warning(fw, "FADT CENTURY is zero, RTC does not support centenary feature is not supported."); >> + */ >> } >> >> - if (fadt->pm_tmr_len != 4) { >> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", >> - "FADT PM_TMR_LEN is %" PRIu8 ", should be 4.", fadt->pm_tmr_len); >> - fwts_advice(fw, "FADT field PM_TMR_LEN defines the number of bytes decoded by PM_TMR_BLK. " >> - "This fields value must be 4. If it is not the correct size then the kernel " >> - "will not request a region for the pm timer block. "); >> - } >> - if (fadt->gpe0_blk_len & 1) { >> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPEBLKLEN", "FADT GPE0_BLK_LEN is %" PRIu8 >> - ", should a multiple of 2.", fadt->gpe0_blk_len); >> - fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " >> - "not map in the GPE0 region. This could mean that General Purpose Events will not " >> - "function correctly (for example lid or ac-power events)."); >> - } >> - if (fadt->gpe1_blk_len & 1) { >> - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPE1BLKLEN", "FADT GPE1_BLK_LEN is %" PRIu8 >> - ", should a multiple of 2.", fadt->gpe1_blk_len); >> - fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " >> - "not map in the GPE1 region. This could mean that General Purpose Events will not " >> - "function correctly (for example lid or ac-power events)."); >> - } >> - /* >> - * Bug LP: /833644 >> - * >> - * Remove these tests, really need to put more intelligence into it >> - * perhaps in the cstates test rather than here. For the moment we >> - * shall remove this warning as it's giving users false alarms >> - * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 >> - */ >> - /* >> - if (fadt->p_lvl2_lat > 100) { >> - fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " >> - "system not to support a C2 state.", fadt->p_lvl2_lat); >> - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " >> - "states that a value > 100 indicates that C2 is not supported and hence the " >> - "ACPI processor idle routine will not use C2 power states."); >> - } >> - if (fadt->p_lvl3_lat > 1000) { >> - fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " >> - "system not to support a C3 state.", fadt->p_lvl3_lat); >> - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " >> - "states that a value > 1000 indicates that C3 is not supported and hence the " >> - "ACPI processor idle routine will not use C3 power states."); >> - } >> - */ >> - /* >> - if (fadt->day_alrm == 0) >> - fwts_warning(fw, "FADT DAY_ALRM is zero, OS will not be able to program day of month alarm."); >> - if (fadt->mon_alrm == 0) >> - fwts_warning(fw, "FADT MON_ALRM is zero, OS will not be able to program month of year alarm."); >> - if (fadt->century == 0) >> - fwts_warning(fw, "FADT CENTURY is zero, RTC does not support centenary feature is not supported."); >> - */ >> - >> if (table->length>=129) { >> if ((fadt->reset_reg.address_space_id != 0) && >> (fadt->reset_reg.address_space_id != 1) && >> >
diff --git a/src/acpi/acpitables/acpitables.c b/src/acpi/acpitables/acpitables.c index e5fffff..acc1505 100644 --- a/src/acpi/acpitables/acpitables.c +++ b/src/acpi/acpitables/acpitables.c @@ -122,92 +122,94 @@ static void acpi_table_check_fadt(fwts_framework *fw, fwts_acpi_table_info *tabl } } - /* - * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0 - * specification states that if SMI_CMD is zero then it is - * a system that does not support System Management Mode, so - * in that case, don't check SCI_INT being valid. - */ - if (fadt->smi_cmd != 0) { - if (fadt->sci_int == 0) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSCIIRQZero", "FADT SCI Interrupt is 0x00, should be defined."); + if (!fwts_acpi_is_reduced_hardware(fadt)) { + /* + * Section 5.2.9 (Fixed ACPI Description Table) of the ACPI 5.0 + * specification states that if SMI_CMD is zero then it is + * a system that does not support System Management Mode, so + * in that case, don't check SCI_INT being valid. + */ + if (fadt->smi_cmd != 0) { + if (fadt->sci_int == 0) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSCIIRQZero", "FADT SCI Interrupt is 0x00, should be defined."); + } + } else { + if ((fadt->acpi_enable == 0) && + (fadt->acpi_disable == 0) && + (fadt->s4bios_req == 0) && + (fadt->pstate_cnt == 0) && + (fadt->cst_cnt == 0)) { + /* Not an error, but intentional, but feedback this finding anyhow */ + fwts_log_info(fw, "The FADT SMI_CMD is zero, system does not support System Management Mode."); + } + else { + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSMICMDZero", + "FADT SMI_CMD is 0x00, however, one or more of ACPI_ENABLE, ACPI_DISABLE, " + "S4BIOS_REQ, PSTATE_CNT and CST_CNT are defined which means SMI_CMD should be " + "defined otherwise SMI commands cannot be sent."); + fwts_advice(fw, "The configuration seems to suggest that SMI command should be defined to " + "allow the kernel to trigger system management interrupts via the SMD_CMD port. " + "The fact that SMD_CMD is zero which is invalid means that SMIs are not possible " + "through the normal ACPI mechanisms. This means some firmware based machine " + "specific functions will not work."); + } } - } else { - if ((fadt->acpi_enable == 0) && - (fadt->acpi_disable == 0) && - (fadt->s4bios_req == 0) && - (fadt->pstate_cnt == 0) && - (fadt->cst_cnt == 0)) { - /* Not an error, but intentional, but feedback this finding anyhow */ - fwts_log_info(fw, "The FADT SMI_CMD is zero, system does not support System Management Mode."); + + if (fadt->pm_tmr_len != 4) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", + "FADT PM_TMR_LEN is %" PRIu8 ", should be 4.", fadt->pm_tmr_len); + fwts_advice(fw, "FADT field PM_TMR_LEN defines the number of bytes decoded by PM_TMR_BLK. " + "This fields value must be 4. If it is not the correct size then the kernel " + "will not request a region for the pm timer block. "); } - else { - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTSMICMDZero", - "FADT SMI_CMD is 0x00, however, one or more of ACPI_ENABLE, ACPI_DISABLE, " - "S4BIOS_REQ, PSTATE_CNT and CST_CNT are defined which means SMI_CMD should be " - "defined otherwise SMI commands cannot be sent."); - fwts_advice(fw, "The configuration seems to suggest that SMI command should be defined to " - "allow the kernel to trigger system management interrupts via the SMD_CMD port. " - "The fact that SMD_CMD is zero which is invalid means that SMIs are not possible " - "through the normal ACPI mechanisms. This means some firmware based machine " - "specific functions will not work."); + if (fadt->gpe0_blk_len & 1) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPEBLKLEN", "FADT GPE0_BLK_LEN is %" PRIu8 + ", should a multiple of 2.", fadt->gpe0_blk_len); + fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " + "not map in the GPE0 region. This could mean that General Purpose Events will not " + "function correctly (for example lid or ac-power events)."); } + if (fadt->gpe1_blk_len & 1) { + fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPE1BLKLEN", "FADT GPE1_BLK_LEN is %" PRIu8 + ", should a multiple of 2.", fadt->gpe1_blk_len); + fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " + "not map in the GPE1 region. This could mean that General Purpose Events will not " + "function correctly (for example lid or ac-power events)."); + } + /* + * Bug LP: /833644 + * + * Remove these tests, really need to put more intelligence into it + * perhaps in the cstates test rather than here. For the moment we + * shall remove this warning as it's giving users false alarms + * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 + */ + /* + if (fadt->p_lvl2_lat > 100) { + fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " + "system not to support a C2 state.", fadt->p_lvl2_lat); + fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " + "states that a value > 100 indicates that C2 is not supported and hence the " + "ACPI processor idle routine will not use C2 power states."); + } + if (fadt->p_lvl3_lat > 1000) { + fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " + "system not to support a C3 state.", fadt->p_lvl3_lat); + fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " + "states that a value > 1000 indicates that C3 is not supported and hence the " + "ACPI processor idle routine will not use C3 power states."); + } + */ + /* + if (fadt->day_alrm == 0) + fwts_warning(fw, "FADT DAY_ALRM is zero, OS will not be able to program day of month alarm."); + if (fadt->mon_alrm == 0) + fwts_warning(fw, "FADT MON_ALRM is zero, OS will not be able to program month of year alarm."); + if (fadt->century == 0) + fwts_warning(fw, "FADT CENTURY is zero, RTC does not support centenary feature is not supported."); + */ } - if (fadt->pm_tmr_len != 4) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadPMTMRLEN", - "FADT PM_TMR_LEN is %" PRIu8 ", should be 4.", fadt->pm_tmr_len); - fwts_advice(fw, "FADT field PM_TMR_LEN defines the number of bytes decoded by PM_TMR_BLK. " - "This fields value must be 4. If it is not the correct size then the kernel " - "will not request a region for the pm timer block. "); - } - if (fadt->gpe0_blk_len & 1) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPEBLKLEN", "FADT GPE0_BLK_LEN is %" PRIu8 - ", should a multiple of 2.", fadt->gpe0_blk_len); - fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " - "not map in the GPE0 region. This could mean that General Purpose Events will not " - "function correctly (for example lid or ac-power events)."); - } - if (fadt->gpe1_blk_len & 1) { - fwts_failed(fw, LOG_LEVEL_MEDIUM, "FADTBadGPE1BLKLEN", "FADT GPE1_BLK_LEN is %" PRIu8 - ", should a multiple of 2.", fadt->gpe1_blk_len); - fwts_advice(fw, "The FADT GPE_BLK_LEN should be a multiple of 2. Because it isn't, the ACPI driver will " - "not map in the GPE1 region. This could mean that General Purpose Events will not " - "function correctly (for example lid or ac-power events)."); - } - /* - * Bug LP: /833644 - * - * Remove these tests, really need to put more intelligence into it - * perhaps in the cstates test rather than here. For the moment we - * shall remove this warning as it's giving users false alarms - * See: https://bugs.launchpad.net/ubuntu/+source/fwts/+bug/833644 - */ - /* - if (fadt->p_lvl2_lat > 100) { - fwts_warning(fw, "FADT P_LVL2_LAT is %" PRIi16 ", a value > 100 indicates a " - "system not to support a C2 state.", fadt->p_lvl2_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C2 latency in microseconds. The ACPI specification " - "states that a value > 100 indicates that C2 is not supported and hence the " - "ACPI processor idle routine will not use C2 power states."); - } - if (fadt->p_lvl3_lat > 1000) { - fwts_warning(fw, "FADT P_LVL3_LAT is %" PRIu16 ", a value > 1000 indicates a " - "system not to support a C3 state.", fadt->p_lvl3_lat); - fwts_advice(fw, "The FADT P_LVL2_LAT setting specifies the C3 latency in microseconds. The ACPI specification " - "states that a value > 1000 indicates that C3 is not supported and hence the " - "ACPI processor idle routine will not use C3 power states."); - } - */ - /* - if (fadt->day_alrm == 0) - fwts_warning(fw, "FADT DAY_ALRM is zero, OS will not be able to program day of month alarm."); - if (fadt->mon_alrm == 0) - fwts_warning(fw, "FADT MON_ALRM is zero, OS will not be able to program month of year alarm."); - if (fadt->century == 0) - fwts_warning(fw, "FADT CENTURY is zero, RTC does not support centenary feature is not supported."); - */ - if (table->length>=129) { if ((fadt->reset_reg.address_space_id != 0) && (fadt->reset_reg.address_space_id != 1) &&
According to ACPI spec 5.1, section 5.2.9, "If the HW_REDUCED_ACPI flag in the table is set, OSPM will ignore fields related to the ACPI HW register interface: Fields at offsets 46 through 108 and 148 through 232, as well as FADT Flag bits 1, 2, 3,7,8,12,13, 14, 16 and 17).", add precondition of checking SMI_CMD, PM_TMR_LEN, etc. The code may become a little complex; it will be better to split into small functions later. Signed-off-by: Heyi Guo <heyi.guo@linaro.org> --- src/acpi/acpitables/acpitables.c | 164 ++++++++++++++++++++------------------- 1 file changed, 83 insertions(+), 81 deletions(-)