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

[Dave Airlie]

On 26 May 2015 at 18:26, Pierre Moreau <pierre.morrow at free.fr> wrote:
>
>> On 26 May 2015, at 07:17, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>
>> 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 val
ue 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.
>
> The caller doesn’t care about the returned error and just check wether
> it’s non-zero (and sometimes it doesn’t even check).

That's no reason to make it inconsistent, you should probably return
-EINVAL for the AE_ERROR case.

Dave.