[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