Nouveau - May 2015 - [PATCH 5/8] acpi: Check returned object type by Optimus _DSM locally

On Mon, May 25, 2015 at 6:22 PM, Pierre Moreau <pierre.morrow at free.fr>
wrote:
> Most _DSM will return an integer value of 0x80000002 when given an unknown
> UUID, revision ID or function ID. Checking locally allows us to
differentiate
> that case from other ACPI errors, and to not report a "failed to
evaluate _DSM"
> if 0x80000002 is returned which was confusing.
>
> Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
> ---
>  drm/nouveau/nouveau_acpi.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/drm/nouveau/nouveau_acpi.c b/drm/nouveau/nouveau_acpi.c
> index 073f7d7..7aeaf7d 100644
> --- a/drm/nouveau/nouveau_acpi.c
> +++ b/drm/nouveau/nouveau_acpi.c
> @@ -88,12 +88,12 @@ static int nouveau_evaluate_optimus_dsm(acpi_handle
handle, int func, int arg, u
>         for (i = 0; i < 4; i++)
>                 args_buff[i] = (arg >> i * 8) & 0xFF;
>
> -       obj = acpi_evaluate_dsm_typed(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
> -                                     func, &argv4, ACPI_TYPE_BUFFER);
> +       obj = acpi_evaluate_dsm(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
> +                               func, &argv4);
>         if (!obj) {
>                 acpi_handle_info(handle, "failed to evaluate
_DSM\n");
>                 return AE_ERROR;
> -       } else {
> +       } else if (obj->type == ACPI_TYPE_BUFFER) {
>                 if (!result && obj->buffer.length == 4) {
>                         *result  = obj->buffer.pointer[0];
>                         *result |= (obj->buffer.pointer[1] << 8);
> @@ -101,6 +101,15 @@ static int nouveau_evaluate_optimus_dsm(acpi_handle
handle, int func, int arg, u
>                         *result |= (obj->buffer.pointer[3] << 24);
>                 }
>                 ACPI_FREE(obj);
> +       } else if (obj->type == ACPI_TYPE_INTEGER &&
> +                  obj->integer.value == 0x80000002) {
> +               acpi_handle_debug(handle, "failed to query Optimus
_DSM\n");
> +               ACPI_FREE(obj);
> +               return -ENODEV;
should this be AE_ERROR?
> +       } else {
> +               acpi_handle_err(handle, "unexpected returned value by
Optimus _DSM\n");
> +               ACPI_FREE(obj);
> +               return AE_ERROR;
>         }
>
>         return 0;
> --
> 2.4.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

> On 26 May 2015, at 00:39, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
> 
>> On Mon, May 25, 2015 at 6:22 PM, Pierre Moreau <pierre.morrow at
free.fr> wrote:
>> Most _DSM will return an integer value of 0x80000002 when given an
unknown
>> UUID, revision ID or function ID. Checking locally allows us to
differentiate
>> that case from other ACPI errors, and to not report a "failed to
evaluate _DSM"
>> if 0x80000002 is returned which was confusing.
>> 
>> Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
>> ---
>> drm/nouveau/nouveau_acpi.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drm/nouveau/nouveau_acpi.c b/drm/nouveau/nouveau_acpi.c
>> index 073f7d7..7aeaf7d 100644
>> --- a/drm/nouveau/nouveau_acpi.c
>> +++ b/drm/nouveau/nouveau_acpi.c
>> @@ -88,12 +88,12 @@ static int nouveau_evaluate_optimus_dsm(acpi_handle
handle, int func, int arg, u
>>        for (i = 0; i < 4; i++)
>>                args_buff[i] = (arg >> i * 8) & 0xFF;
>> 
>> -       obj = acpi_evaluate_dsm_typed(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
>> -                                     func, &argv4,
ACPI_TYPE_BUFFER);
>> +       obj = acpi_evaluate_dsm(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
>> +                               func, &argv4);
>>        if (!obj) {
>>                acpi_handle_info(handle, "failed to evaluate
_DSM\n");
>>                return AE_ERROR;
>> -       } else {
>> +       } else if (obj->type == ACPI_TYPE_BUFFER) {
>>                if (!result && obj->buffer.length == 4) {
>>                        *result  = obj->buffer.pointer[0];
>>                        *result |= (obj->buffer.pointer[1] <<
8);
>> @@ -101,6 +101,15 @@ static int
nouveau_evaluate_optimus_dsm(acpi_handle handle, int func, int arg, u
>>                        *result |= (obj->buffer.pointer[3] <<
24);
>>                }
>>                ACPI_FREE(obj);
>> +       } else if (obj->type == ACPI_TYPE_INTEGER &&
>> +                  obj->integer.value == 0x80000002) {
>> +               acpi_handle_debug(handle, "failed to query Optimus
_DSM\n");
>> +               ACPI_FREE(obj);
>> +               return -ENODEV;
> 
> should this be AE_ERROR?
I would say no, because ACPI was parsed correctly, just that we didn't it
give the correct arguments, or rather, the _DSM we tested isn't an Optimus
one, but it could a mux or gmux. And I used ENODEV as it is the value returned
by nouveau_evaluate_mux_dsm in the same context.
> 
>> +       } else {
>> +               acpi_handle_err(handle, "unexpected returned value
by Optimus _DSM\n");
>> +               ACPI_FREE(obj);
>> +               return AE_ERROR;
>>        }
>> 
>>        return 0;
>> --
>> 2.4.1
>> 
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau

On Tue, May 26, 2015 at 1:10 AM, Pierre Moreau <pierre.morrow at free.fr>
wrote:
>> On 26 May 2015, at 00:39, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
>>
>>> On Mon, May 25, 2015 at 6:22 PM, Pierre Moreau <pierre.morrow at
free.fr> wrote:
>>> Most _DSM will return an integer value of 0x80000002 when given an
unknown
>>> UUID, revision ID or function ID. Checking locally allows us to
differentiate
>>> that case from other ACPI errors, and to not report a "failed
to evaluate _DSM"
>>> if 0x80000002 is returned which was confusing.
>>>
>>> Signed-off-by: Pierre Moreau <pierre.morrow at free.fr>
>>> ---
>>> drm/nouveau/nouveau_acpi.c | 15 ++++++++++++---
>>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drm/nouveau/nouveau_acpi.c
b/drm/nouveau/nouveau_acpi.c
>>> index 073f7d7..7aeaf7d 100644
>>> --- a/drm/nouveau/nouveau_acpi.c
>>> +++ b/drm/nouveau/nouveau_acpi.c
>>> @@ -88,12 +88,12 @@ static int
nouveau_evaluate_optimus_dsm(acpi_handle handle, int func, int arg, u
>>>        for (i = 0; i < 4; i++)
>>>                args_buff[i] = (arg >> i * 8) & 0xFF;
>>>
>>> -       obj = acpi_evaluate_dsm_typed(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
>>> -                                     func, &argv4,
ACPI_TYPE_BUFFER);
>>> +       obj = acpi_evaluate_dsm(handle, nouveau_op_dsm_muid,
nouveau_op_dsm_rid,
>>> +                               func, &argv4);
>>>        if (!obj) {
>>>                acpi_handle_info(handle, "failed to evaluate
_DSM\n");
>>>                return AE_ERROR;
>>> -       } else {
>>> +       } else if (obj->type == ACPI_TYPE_BUFFER) {
>>>                if (!result && obj->buffer.length == 4) {
>>>                        *result  = obj->buffer.pointer[0];
>>>                        *result |= (obj->buffer.pointer[1]
<< 8);
>>> @@ -101,6 +101,15 @@ static int
nouveau_evaluate_optimus_dsm(acpi_handle handle, int func, int arg, u
>>>                        *result |= (obj->buffer.pointer[3]
<< 24);
>>>                }
>>>                ACPI_FREE(obj);
>>> +       } else if (obj->type == ACPI_TYPE_INTEGER &&
>>> +                  obj->integer.value == 0x80000002) {
>>> +               acpi_handle_debug(handle, "failed to query
Optimus _DSM\n");
>>> +               ACPI_FREE(obj);
>>> +               return -ENODEV;
>>
>> should this be AE_ERROR?
>
> I would say no, because ACPI was parsed correctly, just that we didn't
it give the correct arguments, or rather, the _DSM we tested isn't an
Optimus one, but it could a mux or gmux. And I used ENODEV as it is the value
returned by nouveau_evaluate_mux_dsm in the same context.
Hm ok. It just seemed odd to be returning AE_* in one context, and
-ENODEV in another context -- they're different types of errors.
However if the caller handles it, I guess it's OK... I haven't looked
at the API in depth.
>
>>
>>> +       } else {
>>> +               acpi_handle_err(handle, "unexpected returned
value by Optimus _DSM\n");
>>> +               ACPI_FREE(obj);
>>> +               return AE_ERROR;
>>>        }
>>>
>>>        return 0;
>>> --
>>> 2.4.1
>>>
>>> _______________________________________________
>>> Nouveau mailing list
>>> Nouveau at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/nouveau