| Message ID | 20220608170220.5751-2-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Resolve-WMI-query-failures-on-some-devices | expand |
On Wed, Jun 8, 2022 at 7:20 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: > > WMI queries fail on some devices where the ACPI method HWMC > unconditionally attempts to create Fields beyond the buffer > if the buffer is too small, this breaks essential features > such as power profiles: > > CreateByteField (Arg1, 0x10, D008) > CreateByteField (Arg1, 0x11, D009) > CreateByteField (Arg1, 0x12, D010) > CreateDWordField (Arg1, 0x10, D032) > CreateField (Arg1, 0x80, 0x0400, D128) > > In cases where args->data had zero length, ACPI BIOS Error > (bug): AE_AML_BUFFER_LIMIT, Field [D008] at bit > offset/length 128/8 exceeds size of target Buffer (128 bits) > (20211217/dsopcode-198) was obtained. > > ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [D009] at bit > offset/length 136/8 exceeds size of target Buffer (136bits) > (20211217/dsopcode-198) > > The original code created a buffer size of 128 bytes regardless if > the WMI call required a smaller buffer or not. This particular > behavior occurs in older BIOS and reproduced in OMEN laptops. Newer > BIOS handles buffer sizes properly and meets the latest specification > requirements. This is the reason why testing with a dynamically > allocated buffer did not uncover any failures with the test systems at > hand. > > This patch was tested on several OMEN, Elite, and Zbooks. It was > confirmed the patch resolves HPWMI_FAN GET/SET calls in an OMEN > Laptop 15-ek0xxx. No problems were reported when testing on several Elite > and Zbooks notebooks. ... > struct bios_args *args = NULL; > int mid, actual_outsize, ret; > size_t bios_args_size; > + int actual_insize; Same comment as per v1. Please, be careful and read all comments you have been given and react to them either by explaining why it's not worth to address or with an addressed changes.
Hi Andy,
Failure to run your tool and include all the appropriate parties in
the review was an oversight on my part. I will make sure it is done
in the following patches.
Regarding the statement ...
Please, be careful and read all comments you have been given and react
to them either by explaining why it's not worth to address or with an
addressed changes.
All other comments have been addressed in the commit notes and via
email. The comments addressed were
- As a quick fix it's good, but have you had a chance to understand why
this failure happened in the first place?
- Can you check my theory that is expressed in the code below?
- Leverage ge2maintainer tool to include all appropriate parties.
(See earlier comment)
Did I address all the comments? If not, please accept my apologies
and kindly point to the question(s) I need to address.
Regards,
Jorge
On Wed, Jun 8, 2022 at 1:45 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jun 8, 2022 at 7:20 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote:
> >
> > WMI queries fail on some devices where the ACPI method HWMC
> > unconditionally attempts to create Fields beyond the buffer
> > if the buffer is too small, this breaks essential features
> > such as power profiles:
> >
> > CreateByteField (Arg1, 0x10, D008)
> > CreateByteField (Arg1, 0x11, D009)
> > CreateByteField (Arg1, 0x12, D010)
> > CreateDWordField (Arg1, 0x10, D032)
> > CreateField (Arg1, 0x80, 0x0400, D128)
> >
> > In cases where args->data had zero length, ACPI BIOS Error
> > (bug): AE_AML_BUFFER_LIMIT, Field [D008] at bit
> > offset/length 128/8 exceeds size of target Buffer (128 bits)
> > (20211217/dsopcode-198) was obtained.
> >
> > ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [D009] at bit
> > offset/length 136/8 exceeds size of target Buffer (136bits)
> > (20211217/dsopcode-198)
> >
> > The original code created a buffer size of 128 bytes regardless if
> > the WMI call required a smaller buffer or not. This particular
> > behavior occurs in older BIOS and reproduced in OMEN laptops. Newer
> > BIOS handles buffer sizes properly and meets the latest specification
> > requirements. This is the reason why testing with a dynamically
> > allocated buffer did not uncover any failures with the test systems at
> > hand.
> >
> > This patch was tested on several OMEN, Elite, and Zbooks. It was
> > confirmed the patch resolves HPWMI_FAN GET/SET calls in an OMEN
> > Laptop 15-ek0xxx. No problems were reported when testing on several Elite
> > and Zbooks notebooks.
>
> ...
>
> > struct bios_args *args = NULL;
> > int mid, actual_outsize, ret;
> > size_t bios_args_size;
> > + int actual_insize;
>
> Same comment as per v1.
>
> Please, be careful and read all comments you have been given and react
> to them either by explaining why it's not worth to address or with an
> addressed changes.
>
> --
> With Best Regards,
> Andy Shevchenko
On Wed, Jun 8, 2022 at 11:04 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: > > Hi Andy, > > Failure to run your tool and include all the appropriate parties in > the review was an oversight on my part. I will make sure it is done > in the following patches. Hmm... It uses get_maintainer.pl which I believe uses the MAINTAINERS database more or less correctly. I use the script on a daily basis. > Regarding the statement ... > > Please, be careful and read all comments you have been given and react > to them either by explaining why it's not worth to address or with an > addressed changes. > > All other comments have been addressed in the commit notes and via > email. I have noticed that by reading the next patch. As I mentioned there, it should be squashed to the first one, I never expected to see two patches on this topic. > The comments addressed were > > - As a quick fix it's good, but have you had a chance to understand why > this failure happened in the first place? > > - Can you check my theory that is expressed in the code below? > - Leverage ge2maintainer tool to include all appropriate parties. > (See earlier comment) > > Did I address all the comments? If not, please accept my apologies > and kindly point to the question(s) I need to address.
Hi Andy, On Thu, Jun 9, 2022 at 5:50 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Jun 8, 2022 at 11:04 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: > > > > Hi Andy, > > > > Failure to run your tool and include all the appropriate parties in > > the review was an oversight on my part. I will make sure it is done > > in the following patches. > > Hmm... It uses get_maintainer.pl which I believe uses the MAINTAINERS > database more or less correctly. I use the script on a daily basis. > > > Regarding the statement ... > > > > Please, be careful and read all comments you have been given and react > > to them either by explaining why it's not worth to address or with an > > addressed changes. > > > > All other comments have been addressed in the commit notes and via > > email. > > I have noticed that by reading the next patch. As I mentioned there, > it should be squashed to the first one, I never expected to see two > patches on this topic. This is my first year working with kernel upstream teams and reviewers hence I am a newcomer. For this reason, I was being cautious and separated the changes instead of squashing them. Please excuse my inexperience in the matter. > > > The comments addressed were > > > > - As a quick fix it's good, but have you had a chance to understand why > > this failure happened in the first place? > > > > - Can you check my theory that is expressed in the code below? > > - Leverage ge2maintainer tool to include all appropriate parties. > > (See earlier comment) > > > > Did I address all the comments? If not, please accept my apologies > > and kindly point to the question(s) I need to address. > > > > -- > With Best Regards, > Andy Shevchenko
On Thu, Jun 9, 2022 at 3:45 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: > On Thu, Jun 9, 2022 at 5:50 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Jun 8, 2022 at 11:04 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: ... > > > Regarding the statement ... > > > > > > Please, be careful and read all comments you have been given and react > > > to them either by explaining why it's not worth to address or with an > > > addressed changes. > > > > > > All other comments have been addressed in the commit notes and via > > > email. > > > > I have noticed that by reading the next patch. As I mentioned there, > > it should be squashed to the first one, I never expected to see two > > patches on this topic. > > This is my first year working with kernel upstream teams and reviewers > hence I am a newcomer. For this reason, I was being cautious and > separated the changes instead of squashing them. Please excuse my > inexperience in the matter. No problem, everything is good enough, just a couple of technical improvements on the process. You are doing right in the general case and it's rare, in contrast to awful github, that kernel maintainers ask for a squash.
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 0e9a25b56e0e..7bcfa07cc6ab 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -292,12 +292,14 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, struct bios_args *args = NULL; int mid, actual_outsize, ret; size_t bios_args_size; + int actual_insize; mid = encode_outsize_for_pvsz(outsize); if (WARN_ON(mid < 0)) return mid; - bios_args_size = struct_size(args, data, insize); + actual_insize = max(insize, 128); + bios_args_size = struct_size(args, data, actual_insize); args = kmalloc(bios_args_size, GFP_KERNEL); if (!args) return -ENOMEM;
WMI queries fail on some devices where the ACPI method HWMC unconditionally attempts to create Fields beyond the buffer if the buffer is too small, this breaks essential features such as power profiles: CreateByteField (Arg1, 0x10, D008) CreateByteField (Arg1, 0x11, D009) CreateByteField (Arg1, 0x12, D010) CreateDWordField (Arg1, 0x10, D032) CreateField (Arg1, 0x80, 0x0400, D128) In cases where args->data had zero length, ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [D008] at bit offset/length 128/8 exceeds size of target Buffer (128 bits) (20211217/dsopcode-198) was obtained. ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [D009] at bit offset/length 136/8 exceeds size of target Buffer (136bits) (20211217/dsopcode-198) The original code created a buffer size of 128 bytes regardless if the WMI call required a smaller buffer or not. This particular behavior occurs in older BIOS and reproduced in OMEN laptops. Newer BIOS handles buffer sizes properly and meets the latest specification requirements. This is the reason why testing with a dynamically allocated buffer did not uncover any failures with the test systems at hand. This patch was tested on several OMEN, Elite, and Zbooks. It was confirmed the patch resolves HPWMI_FAN GET/SET calls in an OMEN Laptop 15-ek0xxx. No problems were reported when testing on several Elite and Zbooks notebooks. Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)