Patchwork [2/2] acpi: method: add _PSS test

login
register
mail settings
Submitter Colin King
Date April 18, 2012, 11:41 a.m.
Message ID <1334749269-31162-3-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/153480/
State Rejected
Headers show

Comments

Colin King - April 18, 2012, 11:41 a.m.
From: Colin Ian King <colin.king@canonical.com>

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/method/method.c |  106 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 104 insertions(+), 2 deletions(-)
Alex Hung - April 19, 2012, 3:45 a.m.
On 04/18/2012 07:41 PM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   src/acpi/method/method.c |  106 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 104 insertions(+), 2 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 6797868..fae4b31 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -102,7 +102,7 @@
>    * _PSD  8.4.4.5	N
>    * _PSL  11.4.8		N
>    * _PSR  10.3.1		Y
> - * _PSS  8.4.4.2	N
> + * _PSS  8.4.4.2	Y
>    * _PSV  11.4.9		Y
>    * _PSW  7.2.12		Y
>    * _PTC  8.4.3.1	N
> @@ -307,7 +307,7 @@ static int method_name_check(fwts_framework *fw)
>   	int failed = 0;
>
>    	if ((methods = fwts_method_get_names()) != NULL) {
> -		fwts_log_info(fw, "Found %d Methods\n", methods->len);
> +		fwts_log_info(fw, "Found %d Objects\n", methods->len);
>
>   		fwts_list_foreach(item, methods) {
>   			char *ptr;
> @@ -1846,6 +1846,105 @@ static int method_test_UID(fwts_framework *fw)
>   }
>
>
> +/* Section 8.4 */
> +
> +static void method_test_PSS_return(fwts_framework *fw, char *name, ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
> +{
> +	int i;
> +	bool failed = false;
> +	uint32_t max_freq = 0;
> +	uint32_t prev_power = 0;
> +
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK)
> +		return;
> +
> +	/* Something is really wrong if we don't have any elements in _PSS */
> +	if (obj->Package.Count<  1) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementCount",
> +			"_PSS should return package of at least 1 element, "
> +			"got %d elements instead.",
> +			obj->Package.Count);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +		return;
> +	}
> +
> +	for (i=0; i<  obj->Package.Count; i++) {
> +		ACPI_OBJECT *pstate;
> +
> +		if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementType",
> +			"_PSS package element %d was not a package.", i);
> +			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +			failed = true;
> +			continue;	/* Skip processing sub-package */
> +		}
> +
> +		pstate =&obj->Package.Elements[i];
> +		if (pstate->Package.Count != 6) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementCount",
> +				"_PSS P-State sub-package %d was expected to have "
> +				"6 elements, got %d elements instead.",
> +				i, obj->Package.Count);
> +			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +			failed = true;
> +			continue;	/* Skip processing sub-package */
> +		}
> +
> +		/* Elements need to be all ACPI integer types */
> +		if ((pstate->Package.Elements[0].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[1].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[2].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[3].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[4].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[5].Type != ACPI_TYPE_INTEGER)) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementType",
> +				"_PSS P-State sub-package %d was expected to have "
> +				"6 Integer elements but didn't", i);
> +			failed = true;
> +			continue;
> +		}
> +
> +		fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency %lu us, bus master latency %lu us.",
> +			i,
> +			(unsigned long)pstate->Package.Elements[0].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[1].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[2].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[3].Integer.Value);
> +
> +		if (max_freq<  pstate->Package.Elements[0].Integer.Value)
> +			max_freq = pstate->Package.Elements[0].Integer.Value;
max_freq is supposed to be in the first package, is this if-statement 
necessary?

Do you intend to check max_freq with all frequencies in following 
packages in case the packages are not in descending order?

i.e
if (max_freq <  pstate->Package.Elements[i].Integer.Value)
	max_freq = pstate->Package.Elements[i].Integer.Value;

> +
> +		/* Sanity check descending power dissipation levels */
> +		if ((i>  0)&&  (prev_power != 0)&&
> +		    (pstate->Package.Elements[1].Integer.Value>= prev_power)) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackagePowerNotDecending",
> +				"_PSS P-State sub-package %d has a larger power dissipation "
> +				"setting than the previous sub-package.", i);
> +			fwts_advice(fw, "_PSS P-States must be ordered in decending order of "
> +				"power dissipation, so that the zero'th entry has the highest "
> +				"power dissipation level and the Nth has the lowest.");
> +			failed = true;
> +		}
> +		prev_power = pstate->Package.Elements[1].Integer.Value;
> +	}
> +
> +	if (max_freq<  1000) {
Will it be better if we use
"if (!failed && (max_freq = Package.Elements[0].Integer.Value) < 1000)"
and "max_freq" needs not be set in the for-loop?

If the sanity check fails in for-loop, may it not be necessary to check 
max_freq?

> +		fwts_warning(fw,
> +			"Maximum CPU frequency is %dHz and this is low for "
> +			"a modern processor. This may indicate the _PSS PStates "
> +			"are incorrect\n", max_freq);
> +		failed = true;
> +	}
> +
> +	if (!failed)
> +		fwts_passed(fw, "_PSS correctly returned sane looking package.");
> +}
> +
> +static int method_test_PSS(fwts_framework *fw)
> +{
> +	return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL, 0, method_test_PSS_return, NULL);
> +}
> +
>   /* Tests */
>
>   static fwts_framework_minor_test method_tests[] = {
> @@ -1973,6 +2072,9 @@ static fwts_framework_minor_test method_tests[] = {
>   	{ method_test_ON,  "Check _ON  (Set resource on)." },
>   	{ method_test_OFF, "Check _OFF (Set resource off)." },
>
> +	/* Section 8.4 */
> +	{ method_test_PSS, "Check _PSS (Performance Supported States)." },
> +
>   	/* Appendix B, ACPI Extensions for Display Adapters */
>
>   	{ method_test_DOS, "Check _DOS (Enable/Disable Output Switching)." },
Keng-Yu Lin - April 19, 2012, 6:33 a.m.
On Thu, Apr 19, 2012 at 11:45 AM, Alex Hung <alex.hung@canonical.com> wrote:
> On 04/18/2012 07:41 PM, Colin King wrote:
>>
>> From: Colin Ian King<colin.king@canonical.com>
>>
>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>> ---
>>  src/acpi/method/method.c |  106
>> +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 104 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
>> index 6797868..fae4b31 100644
>> --- a/src/acpi/method/method.c
>> +++ b/src/acpi/method/method.c
>> @@ -102,7 +102,7 @@
>>   * _PSD  8.4.4.5      N
>>   * _PSL  11.4.8               N
>>   * _PSR  10.3.1               Y
>> - * _PSS  8.4.4.2       N
>> + * _PSS  8.4.4.2       Y
>>   * _PSV  11.4.9               Y
>>   * _PSW  7.2.12               Y
>>   * _PTC  8.4.3.1      N
>> @@ -307,7 +307,7 @@ static int method_name_check(fwts_framework *fw)
>>        int failed = 0;
>>
>>        if ((methods = fwts_method_get_names()) != NULL) {
>> -               fwts_log_info(fw, "Found %d Methods\n", methods->len);
>> +               fwts_log_info(fw, "Found %d Objects\n", methods->len);
>>
>>                fwts_list_foreach(item, methods) {
>>                        char *ptr;
>> @@ -1846,6 +1846,105 @@ static int method_test_UID(fwts_framework *fw)
>>  }
>>
>>
>> +/* Section 8.4 */
>> +
>> +static void method_test_PSS_return(fwts_framework *fw, char *name,
>> ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
>> +{
>> +       int i;
>> +       bool failed = false;
>> +       uint32_t max_freq = 0;
>> +       uint32_t prev_power = 0;
>> +
>> +       if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) !=
>> FWTS_OK)
>> +               return;
>> +
>> +       /* Something is really wrong if we don't have any elements in _PSS
>> */
>> +       if (obj->Package.Count<  1) {
>> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSElementCount",
>> +                       "_PSS should return package of at least 1 element,
>> "
>> +                       "got %d elements instead.",
>> +                       obj->Package.Count);
>> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> +               return;
>> +       }
>> +
>> +       for (i=0; i<  obj->Package.Count; i++) {
>> +               ACPI_OBJECT *pstate;
>> +
>> +               if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSElementType",
>> +                       "_PSS package element %d was not a package.", i);
>> +                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> +                       failed = true;
>> +                       continue;       /* Skip processing sub-package */
>> +               }
>> +
>> +               pstate =&obj->Package.Elements[i];

pstate points to the i-th sub-package.

>>
>> +               if (pstate->Package.Count != 6) {
>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackageElementCount",
>> +                               "_PSS P-State sub-package %d was expected
>> to have "
>> +                               "6 elements, got %d elements instead.",
>> +                               i, obj->Package.Count);
>> +                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>> +                       failed = true;
>> +                       continue;       /* Skip processing sub-package */
>> +               }
>> +
>> +               /* Elements need to be all ACPI integer types */
>> +               if ((pstate->Package.Elements[0].Type !=
>> ACPI_TYPE_INTEGER) ||
>> +                   (pstate->Package.Elements[1].Type !=
>> ACPI_TYPE_INTEGER) ||
>> +                   (pstate->Package.Elements[2].Type !=
>> ACPI_TYPE_INTEGER) ||
>> +                   (pstate->Package.Elements[3].Type !=
>> ACPI_TYPE_INTEGER) ||
>> +                   (pstate->Package.Elements[4].Type !=
>> ACPI_TYPE_INTEGER) ||
>> +                   (pstate->Package.Elements[5].Type !=
>> ACPI_TYPE_INTEGER)) {
>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackageElementType",
>> +                               "_PSS P-State sub-package %d was expected
>> to have "
>> +                               "6 Integer elements but didn't", i);
>> +                       failed = true;
>> +                       continue;
>> +               }
>> +
>> +               fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency
>> %lu us, bus master latency %lu us.",
>> +                       i,
>> +                       (unsigned
>> long)pstate->Package.Elements[0].Integer.Value,
>> +                       (unsigned
>> long)pstate->Package.Elements[1].Integer.Value,
>> +                       (unsigned
>> long)pstate->Package.Elements[2].Integer.Value,
>> +                       (unsigned
>> long)pstate->Package.Elements[3].Integer.Value);
>> +
>> +               if (max_freq<  pstate->Package.Elements[0].Integer.Value)
>> +                       max_freq =
>> pstate->Package.Elements[0].Integer.Value;
>
> max_freq is supposed to be in the first package, is this if-statement
> necessary?
>

I think pstate is modified in each iteration as a pointer to a
sub-package (see above).
pstate->Package.Elements[0] is CoreFrequency

> Do you intend to check max_freq with all frequencies in following packages
> in case the packages are not in descending order?
>
> i.e
> if (max_freq <  pstate->Package.Elements[i].Integer.Value)
>        max_freq = pstate->Package.Elements[i].Integer.Value;
>
suppose you mean obj->Package.Elements[i]->Package.Elements[0].Integer.Value.

>
>> +
>> +               /* Sanity check descending power dissipation levels */
>> +               if ((i>  0)&&  (prev_power != 0)&&
>> +                   (pstate->Package.Elements[1].Integer.Value>=
>> prev_power)) {
>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>> "Method_PSSSubPackagePowerNotDecending",
>> +                               "_PSS P-State sub-package %d has a larger
>> power dissipation "
>> +                               "setting than the previous sub-package.",
>> i);
>> +                       fwts_advice(fw, "_PSS P-States must be ordered in
>> decending order of "
>> +                               "power dissipation, so that the zero'th
>> entry has the highest "
>> +                               "power dissipation level and the Nth has
>> the lowest.");
>> +                       failed = true;
>> +               }
>> +               prev_power = pstate->Package.Elements[1].Integer.Value;
>> +       }
>> +
>> +       if (max_freq<  1000) {
>
> Will it be better if we use
> "if (!failed && (max_freq = Package.Elements[0].Integer.Value) < 1000)"
> and "max_freq" needs not be set in the for-loop?
>
> If the sanity check fails in for-loop, may it not be necessary to check
> max_freq?
>
>
>> +               fwts_warning(fw,
>> +                       "Maximum CPU frequency is %dHz and this is low for
>> "
>> +                       "a modern processor. This may indicate the _PSS
>> PStates "
>> +                       "are incorrect\n", max_freq);
>> +               failed = true;
>> +       }
>> +
>> +       if (!failed)
>> +               fwts_passed(fw, "_PSS correctly returned sane looking
>> package.");
>> +}
>> +
>> +static int method_test_PSS(fwts_framework *fw)
>> +{
>> +       return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL,
>> 0, method_test_PSS_return, NULL);
>> +}
>> +
>>  /* Tests */
>>
>>  static fwts_framework_minor_test method_tests[] = {
>> @@ -1973,6 +2072,9 @@ static fwts_framework_minor_test method_tests[] = {
>>        { method_test_ON,  "Check _ON  (Set resource on)." },
>>        { method_test_OFF, "Check _OFF (Set resource off)." },
>>
>> +       /* Section 8.4 */
>> +       { method_test_PSS, "Check _PSS (Performance Supported States)." },
>> +
>>        /* Appendix B, ACPI Extensions for Display Adapters */
>>
>>        { method_test_DOS, "Check _DOS (Enable/Disable Output Switching)."
>> },
>
Alex Hung - April 19, 2012, 6:36 a.m.
On 04/19/2012 02:33 PM, Keng-Yu Lin wrote:
> On Thu, Apr 19, 2012 at 11:45 AM, Alex Hung<alex.hung@canonical.com>  wrote:
>> On 04/18/2012 07:41 PM, Colin King wrote:
>>>
>>> From: Colin Ian King<colin.king@canonical.com>
>>>
>>> Signed-off-by: Colin Ian King<colin.king@canonical.com>
>>> ---
>>>   src/acpi/method/method.c |  106
>>> +++++++++++++++++++++++++++++++++++++++++++++-
>>>   1 file changed, 104 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
>>> index 6797868..fae4b31 100644
>>> --- a/src/acpi/method/method.c
>>> +++ b/src/acpi/method/method.c
>>> @@ -102,7 +102,7 @@
>>>    * _PSD  8.4.4.5      N
>>>    * _PSL  11.4.8               N
>>>    * _PSR  10.3.1               Y
>>> - * _PSS  8.4.4.2       N
>>> + * _PSS  8.4.4.2       Y
>>>    * _PSV  11.4.9               Y
>>>    * _PSW  7.2.12               Y
>>>    * _PTC  8.4.3.1      N
>>> @@ -307,7 +307,7 @@ static int method_name_check(fwts_framework *fw)
>>>         int failed = 0;
>>>
>>>         if ((methods = fwts_method_get_names()) != NULL) {
>>> -               fwts_log_info(fw, "Found %d Methods\n", methods->len);
>>> +               fwts_log_info(fw, "Found %d Objects\n", methods->len);
>>>
>>>                 fwts_list_foreach(item, methods) {
>>>                         char *ptr;
>>> @@ -1846,6 +1846,105 @@ static int method_test_UID(fwts_framework *fw)
>>>   }
>>>
>>>
>>> +/* Section 8.4 */
>>> +
>>> +static void method_test_PSS_return(fwts_framework *fw, char *name,
>>> ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
>>> +{
>>> +       int i;
>>> +       bool failed = false;
>>> +       uint32_t max_freq = 0;
>>> +       uint32_t prev_power = 0;
>>> +
>>> +       if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) !=
>>> FWTS_OK)
>>> +               return;
>>> +
>>> +       /* Something is really wrong if we don't have any elements in _PSS
>>> */
>>> +       if (obj->Package.Count<    1) {
>>> +               fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "Method_PSSElementCount",
>>> +                       "_PSS should return package of at least 1 element,
>>> "
>>> +                       "got %d elements instead.",
>>> +                       obj->Package.Count);
>>> +               fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>>> +               return;
>>> +       }
>>> +
>>> +       for (i=0; i<    obj->Package.Count; i++) {
>>> +               ACPI_OBJECT *pstate;
>>> +
>>> +               if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
>>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "Method_PSSElementType",
>>> +                       "_PSS package element %d was not a package.", i);
>>> +                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>>> +                       failed = true;
>>> +                       continue;       /* Skip processing sub-package */
>>> +               }
>>> +
>>> +               pstate =&obj->Package.Elements[i];
>
> pstate points to the i-th sub-package.
>
>>>
>>> +               if (pstate->Package.Count != 6) {
>>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "Method_PSSSubPackageElementCount",
>>> +                               "_PSS P-State sub-package %d was expected
>>> to have "
>>> +                               "6 elements, got %d elements instead.",
>>> +                               i, obj->Package.Count);
>>> +                       fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
>>> +                       failed = true;
>>> +                       continue;       /* Skip processing sub-package */
>>> +               }
>>> +
>>> +               /* Elements need to be all ACPI integer types */
>>> +               if ((pstate->Package.Elements[0].Type !=
>>> ACPI_TYPE_INTEGER) ||
>>> +                   (pstate->Package.Elements[1].Type !=
>>> ACPI_TYPE_INTEGER) ||
>>> +                   (pstate->Package.Elements[2].Type !=
>>> ACPI_TYPE_INTEGER) ||
>>> +                   (pstate->Package.Elements[3].Type !=
>>> ACPI_TYPE_INTEGER) ||
>>> +                   (pstate->Package.Elements[4].Type !=
>>> ACPI_TYPE_INTEGER) ||
>>> +                   (pstate->Package.Elements[5].Type !=
>>> ACPI_TYPE_INTEGER)) {
>>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "Method_PSSSubPackageElementType",
>>> +                               "_PSS P-State sub-package %d was expected
>>> to have "
>>> +                               "6 Integer elements but didn't", i);
>>> +                       failed = true;
>>> +                       continue;
>>> +               }
>>> +
>>> +               fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency
>>> %lu us, bus master latency %lu us.",
>>> +                       i,
>>> +                       (unsigned
>>> long)pstate->Package.Elements[0].Integer.Value,
>>> +                       (unsigned
>>> long)pstate->Package.Elements[1].Integer.Value,
>>> +                       (unsigned
>>> long)pstate->Package.Elements[2].Integer.Value,
>>> +                       (unsigned
>>> long)pstate->Package.Elements[3].Integer.Value);
>>> +
>>> +               if (max_freq<    pstate->Package.Elements[0].Integer.Value)
>>> +                       max_freq =
>>> pstate->Package.Elements[0].Integer.Value;
>>
>> max_freq is supposed to be in the first package, is this if-statement
>> necessary?
>>
>
> I think pstate is modified in each iteration as a pointer to a
> sub-package (see above).
> pstate->Package.Elements[0] is CoreFrequency
>

ah.. Keng-yu you are right. I did make mistake about the pstate package.

Thanks for pointing out.

>> Do you intend to check max_freq with all frequencies in following packages
>> in case the packages are not in descending order?
>>
>> i.e
>> if (max_freq<    pstate->Package.Elements[i].Integer.Value)
>>         max_freq = pstate->Package.Elements[i].Integer.Value;
>>
> suppose you mean obj->Package.Elements[i]->Package.Elements[0].Integer.Value.
>
>>
>>> +
>>> +               /* Sanity check descending power dissipation levels */
>>> +               if ((i>    0)&&    (prev_power != 0)&&
>>> +                   (pstate->Package.Elements[1].Integer.Value>=
>>> prev_power)) {
>>> +                       fwts_failed(fw, LOG_LEVEL_MEDIUM,
>>> "Method_PSSSubPackagePowerNotDecending",
>>> +                               "_PSS P-State sub-package %d has a larger
>>> power dissipation "
>>> +                               "setting than the previous sub-package.",
>>> i);
>>> +                       fwts_advice(fw, "_PSS P-States must be ordered in
>>> decending order of "
>>> +                               "power dissipation, so that the zero'th
>>> entry has the highest "
>>> +                               "power dissipation level and the Nth has
>>> the lowest.");
>>> +                       failed = true;
>>> +               }
>>> +               prev_power = pstate->Package.Elements[1].Integer.Value;
>>> +       }
>>> +
>>> +       if (max_freq<    1000) {
>>
>> Will it be better if we use
>> "if (!failed&&  (max_freq = Package.Elements[0].Integer.Value)<  1000)"
>> and "max_freq" needs not be set in the for-loop?
>>
>> If the sanity check fails in for-loop, may it not be necessary to check
>> max_freq?
>>
>>
>>> +               fwts_warning(fw,
>>> +                       "Maximum CPU frequency is %dHz and this is low for
>>> "
>>> +                       "a modern processor. This may indicate the _PSS
>>> PStates "
>>> +                       "are incorrect\n", max_freq);
>>> +               failed = true;
>>> +       }
>>> +
>>> +       if (!failed)
>>> +               fwts_passed(fw, "_PSS correctly returned sane looking
>>> package.");
>>> +}
>>> +
>>> +static int method_test_PSS(fwts_framework *fw)
>>> +{
>>> +       return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL,
>>> 0, method_test_PSS_return, NULL);
>>> +}
>>> +
>>>   /* Tests */
>>>
>>>   static fwts_framework_minor_test method_tests[] = {
>>> @@ -1973,6 +2072,9 @@ static fwts_framework_minor_test method_tests[] = {
>>>         { method_test_ON,  "Check _ON  (Set resource on)." },
>>>         { method_test_OFF, "Check _OFF (Set resource off)." },
>>>
>>> +       /* Section 8.4 */
>>> +       { method_test_PSS, "Check _PSS (Performance Supported States)." },
>>> +
>>>         /* Appendix B, ACPI Extensions for Display Adapters */
>>>
>>>         { method_test_DOS, "Check _DOS (Enable/Disable Output Switching)."
>>> },
>>
Alex Hung - April 19, 2012, 8:15 a.m.
On 04/18/2012 07:41 PM, Colin King wrote:
> From: Colin Ian King<colin.king@canonical.com>
>
> Signed-off-by: Colin Ian King<colin.king@canonical.com>
> ---
>   src/acpi/method/method.c |  106 +++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 104 insertions(+), 2 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 6797868..fae4b31 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -102,7 +102,7 @@
>    * _PSD  8.4.4.5	N
>    * _PSL  11.4.8		N
>    * _PSR  10.3.1		Y
> - * _PSS  8.4.4.2	N
> + * _PSS  8.4.4.2	Y
>    * _PSV  11.4.9		Y
>    * _PSW  7.2.12		Y
>    * _PTC  8.4.3.1	N
> @@ -307,7 +307,7 @@ static int method_name_check(fwts_framework *fw)
>   	int failed = 0;
>
>    	if ((methods = fwts_method_get_names()) != NULL) {
> -		fwts_log_info(fw, "Found %d Methods\n", methods->len);
> +		fwts_log_info(fw, "Found %d Objects\n", methods->len);
>
>   		fwts_list_foreach(item, methods) {
>   			char *ptr;
> @@ -1846,6 +1846,105 @@ static int method_test_UID(fwts_framework *fw)
>   }
>
>
> +/* Section 8.4 */
> +
> +static void method_test_PSS_return(fwts_framework *fw, char *name, ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
> +{
> +	int i;
> +	bool failed = false;
> +	uint32_t max_freq = 0;
> +	uint32_t prev_power = 0;
> +
> +	if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK)
> +		return;
> +
> +	/* Something is really wrong if we don't have any elements in _PSS */
> +	if (obj->Package.Count<  1) {
> +		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementCount",
> +			"_PSS should return package of at least 1 element, "
> +			"got %d elements instead.",
> +			obj->Package.Count);
> +		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +		return;
> +	}
> +
> +	for (i=0; i<  obj->Package.Count; i++) {
> +		ACPI_OBJECT *pstate;
> +
> +		if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementType",
> +			"_PSS package element %d was not a package.", i);
> +			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +			failed = true;
> +			continue;	/* Skip processing sub-package */
> +		}
> +
> +		pstate =&obj->Package.Elements[i];
> +		if (pstate->Package.Count != 6) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementCount",
> +				"_PSS P-State sub-package %d was expected to have "
> +				"6 elements, got %d elements instead.",
> +				i, obj->Package.Count);
> +			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
> +			failed = true;
> +			continue;	/* Skip processing sub-package */
> +		}
> +
> +		/* Elements need to be all ACPI integer types */
> +		if ((pstate->Package.Elements[0].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[1].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[2].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[3].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[4].Type != ACPI_TYPE_INTEGER) ||
> +		    (pstate->Package.Elements[5].Type != ACPI_TYPE_INTEGER)) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementType",
> +				"_PSS P-State sub-package %d was expected to have "
> +				"6 Integer elements but didn't", i);
> +			failed = true;
> +			continue;
> +		}
> +
> +		fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency %lu us, bus master latency %lu us.",
> +			i,
> +			(unsigned long)pstate->Package.Elements[0].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[1].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[2].Integer.Value,
> +			(unsigned long)pstate->Package.Elements[3].Integer.Value);
> +
> +		if (max_freq<  pstate->Package.Elements[0].Integer.Value)
> +			max_freq = pstate->Package.Elements[0].Integer.Value;
> +
> +		/* Sanity check descending power dissipation levels */
> +		if ((i>  0)&&  (prev_power != 0)&&
> +		    (pstate->Package.Elements[1].Integer.Value>= prev_power)) {
> +			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackagePowerNotDecending",
> +				"_PSS P-State sub-package %d has a larger power dissipation "
> +				"setting than the previous sub-package.", i);
> +			fwts_advice(fw, "_PSS P-States must be ordered in decending order of "
> +				"power dissipation, so that the zero'th entry has the highest "
> +				"power dissipation level and the Nth has the lowest.");
> +			failed = true;
> +		}
> +		prev_power = pstate->Package.Elements[1].Integer.Value;
> +	}
> +
> +	if (max_freq<  1000) {
> +		fwts_warning(fw,
> +			"Maximum CPU frequency is %dHz and this is low for "
> +			"a modern processor. This may indicate the _PSS PStates "
> +			"are incorrect\n", max_freq);
> +		failed = true;
> +	}
> +
> +	if (!failed)
> +		fwts_passed(fw, "_PSS correctly returned sane looking package.");
> +}
> +
> +static int method_test_PSS(fwts_framework *fw)
> +{
> +	return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL, 0, method_test_PSS_return, NULL);
> +}
> +
>   /* Tests */
>
>   static fwts_framework_minor_test method_tests[] = {
> @@ -1973,6 +2072,9 @@ static fwts_framework_minor_test method_tests[] = {
>   	{ method_test_ON,  "Check _ON  (Set resource on)." },
>   	{ method_test_OFF, "Check _OFF (Set resource off)." },
>
> +	/* Section 8.4 */
> +	{ method_test_PSS, "Check _PSS (Performance Supported States)." },
> +
>   	/* Appendix B, ACPI Extensions for Display Adapters */
>
>   	{ method_test_DOS, "Check _DOS (Enable/Disable Output Switching)." },

Acked-by: Alex Hung <alex.hung@canonical.com>

Patch

diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
index 6797868..fae4b31 100644
--- a/src/acpi/method/method.c
+++ b/src/acpi/method/method.c
@@ -102,7 +102,7 @@ 
  * _PSD  8.4.4.5	N
  * _PSL  11.4.8		N
  * _PSR  10.3.1		Y
- * _PSS  8.4.4.2	N
+ * _PSS  8.4.4.2	Y
  * _PSV  11.4.9		Y
  * _PSW  7.2.12		Y
  * _PTC  8.4.3.1	N
@@ -307,7 +307,7 @@  static int method_name_check(fwts_framework *fw)
 	int failed = 0;
 
  	if ((methods = fwts_method_get_names()) != NULL) {
-		fwts_log_info(fw, "Found %d Methods\n", methods->len);
+		fwts_log_info(fw, "Found %d Objects\n", methods->len);
 
 		fwts_list_foreach(item, methods) {
 			char *ptr;
@@ -1846,6 +1846,105 @@  static int method_test_UID(fwts_framework *fw)
 }
 
 
+/* Section 8.4 */
+
+static void method_test_PSS_return(fwts_framework *fw, char *name, ACPI_BUFFER *buf, ACPI_OBJECT *obj, void *private)
+{
+	int i;
+	bool failed = false;
+	uint32_t max_freq = 0;
+	uint32_t prev_power = 0;
+
+	if (method_check_type(fw, name, buf, ACPI_TYPE_PACKAGE) != FWTS_OK)
+		return;
+
+	/* Something is really wrong if we don't have any elements in _PSS */
+	if (obj->Package.Count < 1) {
+		fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementCount",
+			"_PSS should return package of at least 1 element, "
+			"got %d elements instead.",
+			obj->Package.Count);
+		fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+		return;
+	}
+
+	for (i=0; i < obj->Package.Count; i++) {
+		ACPI_OBJECT *pstate;
+
+		if (obj->Package.Elements[i].Type != ACPI_TYPE_PACKAGE) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSElementType",
+			"_PSS package element %d was not a package.", i);
+			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+			failed = true;
+			continue;	/* Skip processing sub-package */
+		}
+
+		pstate = &obj->Package.Elements[i];
+		if (pstate->Package.Count != 6) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementCount",
+				"_PSS P-State sub-package %d was expected to have "
+				"6 elements, got %d elements instead.",
+				i, obj->Package.Count);
+			fwts_tag_failed(fw, FWTS_TAG_ACPI_METHOD_RETURN);
+			failed = true;
+			continue;	/* Skip processing sub-package */
+		}
+
+		/* Elements need to be all ACPI integer types */
+		if ((pstate->Package.Elements[0].Type != ACPI_TYPE_INTEGER) ||
+		    (pstate->Package.Elements[1].Type != ACPI_TYPE_INTEGER) ||
+		    (pstate->Package.Elements[2].Type != ACPI_TYPE_INTEGER) ||
+		    (pstate->Package.Elements[3].Type != ACPI_TYPE_INTEGER) ||
+		    (pstate->Package.Elements[4].Type != ACPI_TYPE_INTEGER) ||
+		    (pstate->Package.Elements[5].Type != ACPI_TYPE_INTEGER)) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackageElementType",
+				"_PSS P-State sub-package %d was expected to have "
+				"6 Integer elements but didn't", i);
+			failed = true;
+			continue;
+		}
+
+		fwts_log_info(fw, "PState %d: CPU %ld Mhz, %lu mW, latency %lu us, bus master latency %lu us.",
+			i,
+			(unsigned long)pstate->Package.Elements[0].Integer.Value,
+			(unsigned long)pstate->Package.Elements[1].Integer.Value,
+			(unsigned long)pstate->Package.Elements[2].Integer.Value,
+			(unsigned long)pstate->Package.Elements[3].Integer.Value);
+
+		if (max_freq < pstate->Package.Elements[0].Integer.Value)
+			max_freq = pstate->Package.Elements[0].Integer.Value;
+
+		/* Sanity check descending power dissipation levels */
+		if ((i > 0) && (prev_power != 0) &&
+		    (pstate->Package.Elements[1].Integer.Value >= prev_power)) {
+			fwts_failed(fw, LOG_LEVEL_MEDIUM, "Method_PSSSubPackagePowerNotDecending",
+				"_PSS P-State sub-package %d has a larger power dissipation "
+				"setting than the previous sub-package.", i);
+			fwts_advice(fw, "_PSS P-States must be ordered in decending order of "
+				"power dissipation, so that the zero'th entry has the highest "
+				"power dissipation level and the Nth has the lowest.");
+			failed = true;
+		}
+		prev_power = pstate->Package.Elements[1].Integer.Value;
+	}
+
+	if (max_freq < 1000) {
+		fwts_warning(fw,
+			"Maximum CPU frequency is %dHz and this is low for "
+			"a modern processor. This may indicate the _PSS PStates "
+			"are incorrect\n", max_freq);
+		failed = true;
+	}
+
+	if (!failed)
+		fwts_passed(fw, "_PSS correctly returned sane looking package.");
+}
+
+static int method_test_PSS(fwts_framework *fw)
+{
+	return method_evaluate_method(fw, METHOD_OPTIONAL, "_PSS", NULL, 0, method_test_PSS_return, NULL);
+}
+
 /* Tests */
 
 static fwts_framework_minor_test method_tests[] = {
@@ -1973,6 +2072,9 @@  static fwts_framework_minor_test method_tests[] = {
 	{ method_test_ON,  "Check _ON  (Set resource on)." },
 	{ method_test_OFF, "Check _OFF (Set resource off)." },
 
+	/* Section 8.4 */
+	{ method_test_PSS, "Check _PSS (Performance Supported States)." },
+
 	/* Appendix B, ACPI Extensions for Display Adapters */
 
 	{ method_test_DOS, "Check _DOS (Enable/Disable Output Switching)." },