| Message ID | 1432592573-13743-5-git-send-email-pierre.morrow@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > + } 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
> 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. > >> + } 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
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. > >> >>> + } 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
On 05/26/2015 12:22 AM, Pierre Moreau 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; > + } else { > + acpi_handle_err(handle, "unexpected returned value by Optimus _DSM\n"); > + ACPI_FREE(obj); > + return AE_ERROR; > } > > return 0; How do you handle the case where result is NULL and the type is ACPI_TYPE_BUFFER ? Because you don't return any error. Is that expected?
> 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
On 26 May 2015 at 18:26, Pierre Moreau <pierre.morrow@free.fr> wrote: > >> 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 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@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.
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; + } else { + acpi_handle_err(handle, "unexpected returned value by Optimus _DSM\n"); + ACPI_FREE(obj); + return AE_ERROR; } return 0;
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(-)