Ilia Mirkin
2015-May-25 22:39 UTC
[Nouveau] [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
Pierre Moreau
2015-May-26 05:10 UTC
[Nouveau] [PATCH 5/8] acpi: Check returned object type by Optimus _DSM locally
> 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
Ilia Mirkin
2015-May-26 05:17 UTC
[Nouveau] [PATCH 5/8] acpi: Check returned object type by Optimus _DSM locally
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