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.