[PATCH] acpi: method: only get pedantic about therm returns if values are hard coded

[IvanHu]

On 08/01/2012 05:25 PM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> The thermal evaluations are producing unnecessary noisy advice and warnings
> because the evals are from emulated memory or I/O space regions, which of
> course will return dummy values.   Instead of doing this, make it more
> intelligent by keeping track of any memory space accesses and only checking
> the evaluated return value if we didn't access any memory or I/O space regions.
> This way we can catch any bogus hard-coded incorrect therm return values.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>   src/acpi/method/method.c      |   54 +++++++++++++++++++++++++++++------------
>   src/acpica/fwts_acpica.c      |   14 +++++++++++
>   src/lib/include/fwts_acpica.h |    2 ++
>   3 files changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 9e81f07..377b7bc 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -1222,22 +1222,43 @@ static void method_test_THERM_return(fwts_framework *fw, char *name, ACPI_BUFFER
>   {
>   	if (method_check_type(fw, name, buf, ACPI_TYPE_INTEGER) == FWTS_OK) {
>   		char *method = (char*)private;
> -		if (obj->Integer.Value >= 2732)
> -			fwts_passed(fw, "%s correctly returned sane looking value 0x%8.8x (%5.1f degrees K)",
> -				method,
> -				(uint32_t)obj->Integer.Value,
> -				(float)((uint32_t)obj->Integer.Value) / 10.0);
> -		else {
> -			fwts_failed(fw, LOG_LEVEL_MEDIUM, "MethodBadTemp",
> -				"%s returned a dubious value below 0 degrees C: 0x%8.8x (%5.1f degrees K)",
> -				method,
> -				(uint32_t)obj->Integer.Value,
> -				(float)((uint32_t)obj->Integer.Value) / 10.0);
> -			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> -			fwts_advice(fw, "An incorrect value could be because the method requires interaction with "
> -					"I/O ports or the embedded controller and this test frame work is spoofing "
> -					"these operations. However, it is worth sanity checking these values in "
> +		
> +		if (fwts_acpi_region_handler_called_get()) {
> +			/*
> +			 *  We accessed some memory or I/O region during the evaluation
> +			 *  which returns spoofed values, so we should not test the value
> +			 *  being returned. In this case, just pass this as a valid
> +			 *  return type.
> +			 */
> +			fwts_passed(fw, "%s correctly returned sane looking return type.", name);
> +		} else {
> +			/*
> +			 *  The evaluation probably was a hard-coded value, so sanity check it
> +			 */
> +			if (obj->Integer.Value >= 2732)
> +				fwts_passed(fw,
> +					"%s correctly returned sane looking value "
> +					"0x%8.8x (%5.1f degrees K)",
> +					method,
> +					(uint32_t)obj->Integer.Value,
> +					(float)((uint32_t)obj->Integer.Value) / 10.0);
> +			else {
> +				fwts_failed(fw, LOG_LEVEL_MEDIUM, "MethodBadTemp",
> +					"%s returned a dubious value below 0 degrees C: "
> +					"0x%8.8x (%5.1f degrees K)",
> +					method,
> +					(uint32_t)obj->Integer.Value,
> +					(float)((uint32_t)obj->Integer.Value) / 10.0);
> +				fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +				fwts_advice(fw,
> +					"The value returned was probably a hard-coded "
> +					"thermal value which is out of range because "
> +					"fwts did not detect any ACPI region handler "
> +					"accesses of I/O or system memeory to evaluate "
> +					"the thermal value. "
> +					"It is worth sanity checking these values in "
>   					"/sys/class/thermal/thermal_zone*.");
> +			}
>   		}
>   	}
>   }
> @@ -1245,7 +1266,8 @@ static void method_test_THERM_return(fwts_framework *fw, char *name, ACPI_BUFFER
>   #define method_test_THERM(name, type)			\
>   static int method_test ## name(fwts_framework *fw)	\
>   {							\
> -	return method_evaluate_method(fw, type, # name, NULL, 0, method_test_THERM_return, # name);				\
> +	fwts_acpi_region_handler_called_set(false);	\
> +	return method_evaluate_method(fw, type, # name, NULL, 0, method_test_THERM_return, # name);	\
>   }
>
>   method_test_THERM(_CRT, METHOD_OPTIONAL)
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index 08b012d..4e28d60 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -74,6 +74,8 @@ static sem_hash			sem_hash_table[MAX_SEMAPHORES];
>
>   static ACPI_TABLE_DESC		Tables[ACPI_MAX_INIT_TABLES];
>
> +static bool			region_handler_called;
> +
>   /*
>    *  Used to account for threads used by AcpiOsExecute
>    */
> @@ -305,6 +307,16 @@ static ACPI_STATUS fwts_region_init(
>   	return AE_OK;
>   }
>
> +void fwts_acpi_region_handler_called_set(bool val)
> +{
> +	region_handler_called = val;
> +}
> +
> +bool fwts_acpi_region_handler_called_get(void)
> +{
> +	return region_handler_called;
> +}
> +
>   static ACPI_STATUS fwts_region_handler(
>   	UINT32                  function,
>   	ACPI_PHYSICAL_ADDRESS   address,
> @@ -323,6 +335,8 @@ static ACPI_STATUS fwts_region_handler(
>   	if (regionobject->Region.Type != ACPI_TYPE_REGION)
>   		return AE_OK;
>
> +	fwts_acpi_region_handler_called_set(true);
> +
>   	context = ACPI_CAST_PTR (ACPI_CONNECTION_INFO, handlercontext);
>   	length = (ACPI_SIZE)regionobject->Region.Length;
>
> diff --git a/src/lib/include/fwts_acpica.h b/src/lib/include/fwts_acpica.h
> index e7008be..bfea77e 100644
> --- a/src/lib/include/fwts_acpica.h
> +++ b/src/lib/include/fwts_acpica.h
> @@ -32,5 +32,7 @@ fwts_list *fwts_acpica_get_object_names(int type);
>   void fwts_acpica_sem_count_clear(void);
>   void fwts_acpica_sem_count_get(int *acquired, int *released);
>   void fwts_acpica_simulate_sem_timeout(int flag);
> +void fwts_acpi_region_handler_called_set(bool val);
> +bool fwts_acpi_region_handler_called_get(void);
>
>   #endif
>

Acked-by: Ivan Hu <ivan.hu at canonical.com>