[PATCH 1/2] acpi: acpitables: FADT: Ignore fields at offset 46 through 108 for HW_REDUCED_ACPI
Heyi Guo
heyi.guo at linaro.org
Sun Apr 5 09:01:59 UTC 2015
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 at 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) &&
>>
>
More information about the fwts-devel
mailing list