| Message ID | 1343813120-13282-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Aug 1, 2012 at 5:25 PM, Colin King <colin.king@canonical.com> wrote: > From: Colin Ian King <colin.king@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@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 > -- > 1.7.10.4 > Acked-by: Keng-Yu Lin <kengyu@canonical.com>
On 08/01/2012 05:25 PM, Colin King wrote: > From: Colin Ian King <colin.king@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@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@canonical.com>
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