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

From: Pierre Moreau <pierre.morrow@free.fr>
To: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: "nouveau@lists.freedesktop.org" <nouveau@lists.freedesktop.org>,
	Ben Skeggs <bskeggs@redhat.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [Nouveau] [PATCH 5/8] acpi: Check returned object type by Optimus _DSM locally
Date: Tue, 26 May 2015 10:26:39 +0200	[thread overview]
Message-ID: <754009E0-60CD-4E58-BA04-5FF8F3315E23@free.fr> (raw)
In-Reply-To: <CAKb7UvjDTgVYM+XYs6ui-vjxy4x50DQ0p9zKKMPePph8M0jZwA@mail.gmail.com>

> On 26 May 2015, at 07:17, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
> 
> On Tue, May 26, 2015 at 1:10 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
>>> On 26 May 2015, at 00:39, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>>> 
>>>> On Mon, May 25, 2015 at 6:22 PM, Pierre Moreau <pierre.morrow@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@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).

> 
>> 
>>> 
>>>> +       } 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@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/nouveau

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel