diff mbox

acpi: method: relax mandatory requirement for _PTS, _TTS and _WAK (LP: #1296737)

Message ID 1395669576-25789-1-git-send-email-colin.king@canonical.com
State Rejected
Headers show

Commit Message

Colin Ian King March 24, 2014, 1:59 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

The kernel does not throw any errors if these controls don't exist,
so we probably should relax the method test so it does not report
a failure if these controls don't exist.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/acpi/method/method.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Ivan Hu March 25, 2014, 2:49 a.m. UTC | #1 
On 03/24/2014 09:59 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The kernel does not throw any errors if these controls don't exist,
> so we probably should relax the method test so it does not report
> a failure if these controls don't exist.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/acpi/method/method.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 6e6c5c2..377d2ef 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -4305,7 +4305,7 @@ static int method_test_PTS(fwts_framework *fw)
>
>   		fwts_log_info(fw, "Test _PTS(%d).", i);
>
> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_PTS", arg, 1,
> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_PTS", arg, 1,
>   			method_test_NULL_return, NULL) == FWTS_NOT_EXIST) {
>   			fwts_advice(fw,
>   				"Could not find _PTS. This method provides a "
> @@ -4336,7 +4336,7 @@ static int method_test_TTS(fwts_framework *fw)
>   			fwts_log_info(fw,
>   				"Test _TTS(%d) Transition To State S%d.", i, i);
>
> -			if (method_evaluate_method(fw, METHOD_MANDITORY,
> +			if (method_evaluate_method(fw, METHOD_OPTIONAL,
>   				"_TTS", arg, 1, method_test_NULL_return,
>   				NULL) == FWTS_NOT_EXIST)
>   				break;
> @@ -4412,7 +4412,7 @@ static int method_test_WAK(fwts_framework *fw)
>   		arg[0].Type = ACPI_TYPE_INTEGER;
>   		arg[0].Integer.Value = i;
>   		fwts_log_info(fw, "Test _WAK(%d) System Wake, State S%d.", i, i);
> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_WAK", arg, 1,
> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_WAK", arg, 1,
>   			method_test_WAK_return, &i) == FWTS_NOT_EXIST)
>   			break;
>   		fwts_log_nl(fw);
>

Acked-by: Ivan Hu <ivan.hu@canonical.com>
Alex Hung March 25, 2014, 3:13 a.m. UTC | #2 
On 03/24/2014 09:59 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The kernel does not throw any errors if these controls don't exist,
> so we probably should relax the method test so it does not report
> a failure if these controls don't exist.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/acpi/method/method.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
> index 6e6c5c2..377d2ef 100644
> --- a/src/acpi/method/method.c
> +++ b/src/acpi/method/method.c
> @@ -4305,7 +4305,7 @@ static int method_test_PTS(fwts_framework *fw)
>  
>  		fwts_log_info(fw, "Test _PTS(%d).", i);
>  
> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_PTS", arg, 1,
> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_PTS", arg, 1,
>  			method_test_NULL_return, NULL) == FWTS_NOT_EXIST) {
>  			fwts_advice(fw,
>  				"Could not find _PTS. This method provides a "
> @@ -4336,7 +4336,7 @@ static int method_test_TTS(fwts_framework *fw)
>  			fwts_log_info(fw,
>  				"Test _TTS(%d) Transition To State S%d.", i, i);
>  
> -			if (method_evaluate_method(fw, METHOD_MANDITORY,
> +			if (method_evaluate_method(fw, METHOD_OPTIONAL,
>  				"_TTS", arg, 1, method_test_NULL_return,
>  				NULL) == FWTS_NOT_EXIST)
>  				break;
> @@ -4412,7 +4412,7 @@ static int method_test_WAK(fwts_framework *fw)
>  		arg[0].Type = ACPI_TYPE_INTEGER;
>  		arg[0].Integer.Value = i;
>  		fwts_log_info(fw, "Test _WAK(%d) System Wake, State S%d.", i, i);
> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_WAK", arg, 1,
> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_WAK", arg, 1,
>  			method_test_WAK_return, &i) == FWTS_NOT_EXIST)
>  			break;
>  		fwts_log_nl(fw);
> 


This may not the best idea since they are needed according to ACPI spec.
Changing them to optional makes it kernel-dependent and is not exactly
testing firmware.

Will it be better to change the advise or lower the severity so users
know the impacts are minimal?
Colin Ian King March 25, 2014, 8:06 a.m. UTC | #3 
On 25/03/14 03:13, Alex Hung wrote:
> On 03/24/2014 09:59 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The kernel does not throw any errors if these controls don't exist,
>> so we probably should relax the method test so it does not report
>> a failure if these controls don't exist.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  src/acpi/method/method.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
>> index 6e6c5c2..377d2ef 100644
>> --- a/src/acpi/method/method.c
>> +++ b/src/acpi/method/method.c
>> @@ -4305,7 +4305,7 @@ static int method_test_PTS(fwts_framework *fw)
>>  
>>  		fwts_log_info(fw, "Test _PTS(%d).", i);
>>  
>> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_PTS", arg, 1,
>> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_PTS", arg, 1,
>>  			method_test_NULL_return, NULL) == FWTS_NOT_EXIST) {
>>  			fwts_advice(fw,
>>  				"Could not find _PTS. This method provides a "
>> @@ -4336,7 +4336,7 @@ static int method_test_TTS(fwts_framework *fw)
>>  			fwts_log_info(fw,
>>  				"Test _TTS(%d) Transition To State S%d.", i, i);
>>  
>> -			if (method_evaluate_method(fw, METHOD_MANDITORY,
>> +			if (method_evaluate_method(fw, METHOD_OPTIONAL,
>>  				"_TTS", arg, 1, method_test_NULL_return,
>>  				NULL) == FWTS_NOT_EXIST)
>>  				break;
>> @@ -4412,7 +4412,7 @@ static int method_test_WAK(fwts_framework *fw)
>>  		arg[0].Type = ACPI_TYPE_INTEGER;
>>  		arg[0].Integer.Value = i;
>>  		fwts_log_info(fw, "Test _WAK(%d) System Wake, State S%d.", i, i);
>> -		if (method_evaluate_method(fw, METHOD_MANDITORY, "_WAK", arg, 1,
>> +		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_WAK", arg, 1,
>>  			method_test_WAK_return, &i) == FWTS_NOT_EXIST)
>>  			break;
>>  		fwts_log_nl(fw);
>>
> 
> 
> This may not the best idea since they are needed according to ACPI spec.

OK, can you advise me where abouts in the spec? I can then pass that
back to the party that requested I tweaked this.

> Changing them to optional makes it kernel-dependent and is not exactly
> testing firmware.

+1

> 
> Will it be better to change the advise or lower the severity so users
> know the impacts are minimal?
> 

How about I add some contextual advice on this instead explaining what
the kernel does in these cases?

Colin
Alex Hung March 25, 2014, 8:21 a.m. UTC | #4 
> OK, can you advise me where abouts in the spec? I can then pass that
> back to the party that requested I tweaked this.

I guess it is more indirectly definition: in Section 73. of ACPI spec
5.0, it explicitly specifies optional control methods (ex. _BFS and
_GTS), but it does not state _PTS and _WAK as optional.

>
>> Changing them to optional makes it kernel-dependent and is not exactly
>> testing firmware.
>
> +1
>
>>
>> Will it be better to change the advise or lower the severity so users
>> know the impacts are minimal?
>>
>
> How about I add some contextual advice on this instead explaining what
> the kernel does in these cases?

That sounds good.

On Tue, Mar 25, 2014 at 4:06 PM, Colin Ian King
<colin.king@canonical.com> wrote:
> On 25/03/14 03:13, Alex Hung wrote:
>> On 03/24/2014 09:59 PM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The kernel does not throw any errors if these controls don't exist,
>>> so we probably should relax the method test so it does not report
>>> a failure if these controls don't exist.
>>>
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  src/acpi/method/method.c | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
>>> index 6e6c5c2..377d2ef 100644
>>> --- a/src/acpi/method/method.c
>>> +++ b/src/acpi/method/method.c
>>> @@ -4305,7 +4305,7 @@ static int method_test_PTS(fwts_framework *fw)
>>>
>>>              fwts_log_info(fw, "Test _PTS(%d).", i);
>>>
>>> -            if (method_evaluate_method(fw, METHOD_MANDITORY, "_PTS", arg, 1,
>>> +            if (method_evaluate_method(fw, METHOD_OPTIONAL, "_PTS", arg, 1,
>>>                      method_test_NULL_return, NULL) == FWTS_NOT_EXIST) {
>>>                      fwts_advice(fw,
>>>                              "Could not find _PTS. This method provides a "
>>> @@ -4336,7 +4336,7 @@ static int method_test_TTS(fwts_framework *fw)
>>>                      fwts_log_info(fw,
>>>                              "Test _TTS(%d) Transition To State S%d.", i, i);
>>>
>>> -                    if (method_evaluate_method(fw, METHOD_MANDITORY,
>>> +                    if (method_evaluate_method(fw, METHOD_OPTIONAL,
>>>                              "_TTS", arg, 1, method_test_NULL_return,
>>>                              NULL) == FWTS_NOT_EXIST)
>>>                              break;
>>> @@ -4412,7 +4412,7 @@ static int method_test_WAK(fwts_framework *fw)
>>>              arg[0].Type = ACPI_TYPE_INTEGER;
>>>              arg[0].Integer.Value = i;
>>>              fwts_log_info(fw, "Test _WAK(%d) System Wake, State S%d.", i, i);
>>> -            if (method_evaluate_method(fw, METHOD_MANDITORY, "_WAK", arg, 1,
>>> +            if (method_evaluate_method(fw, METHOD_OPTIONAL, "_WAK", arg, 1,
>>>                      method_test_WAK_return, &i) == FWTS_NOT_EXIST)
>>>                      break;
>>>              fwts_log_nl(fw);
>>>
>>
>>
>> This may not the best idea since they are needed according to ACPI spec.
>
> OK, can you advise me where abouts in the spec? I can then pass that
> back to the party that requested I tweaked this.
>
>> Changing them to optional makes it kernel-dependent and is not exactly
>> testing firmware.
>
> +1
>
>>
>> Will it be better to change the advise or lower the severity so users
>> know the impacts are minimal?
>>
>
> How about I add some contextual advice on this instead explaining what
> the kernel does in these cases?
>
> Colin
>
> --
> fwts-devel mailing list
> fwts-devel@lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/fwts-devel
diff mbox

Patch

diff --git a/src/acpi/method/method.c b/src/acpi/method/method.c
index 6e6c5c2..377d2ef 100644
--- a/src/acpi/method/method.c
+++ b/src/acpi/method/method.c
@@ -4305,7 +4305,7 @@  static int method_test_PTS(fwts_framework *fw)
 
 		fwts_log_info(fw, "Test _PTS(%d).", i);
 
-		if (method_evaluate_method(fw, METHOD_MANDITORY, "_PTS", arg, 1,
+		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_PTS", arg, 1,
 			method_test_NULL_return, NULL) == FWTS_NOT_EXIST) {
 			fwts_advice(fw,
 				"Could not find _PTS. This method provides a "
@@ -4336,7 +4336,7 @@  static int method_test_TTS(fwts_framework *fw)
 			fwts_log_info(fw,
 				"Test _TTS(%d) Transition To State S%d.", i, i);
 
-			if (method_evaluate_method(fw, METHOD_MANDITORY,
+			if (method_evaluate_method(fw, METHOD_OPTIONAL,
 				"_TTS", arg, 1, method_test_NULL_return,
 				NULL) == FWTS_NOT_EXIST)
 				break;
@@ -4412,7 +4412,7 @@  static int method_test_WAK(fwts_framework *fw)
 		arg[0].Type = ACPI_TYPE_INTEGER;
 		arg[0].Integer.Value = i;
 		fwts_log_info(fw, "Test _WAK(%d) System Wake, State S%d.", i, i);
-		if (method_evaluate_method(fw, METHOD_MANDITORY, "_WAK", arg, 1,
+		if (method_evaluate_method(fw, METHOD_OPTIONAL, "_WAK", arg, 1,
 			method_test_WAK_return, &i) == FWTS_NOT_EXIST)
 			break;
 		fwts_log_nl(fw);