[PATCH v2 0/2] Resolve-WMI-query-failures-on-some-devices

[PATCH v2 0/2] Resolve-WMI-query-failures-on-some-devices

[Jorge Lopez]

The intention for this patch is resolve WMI query failures reported
in several HP OMEN notebooks.  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.


Description of changes between version 1 and version 2
------------------------------------------------------
v2 patch 1: Resolve-WMI-query-failures-on-some-devices
            - No new changes to the logic.

v2 patch 2: Organize-declaration-variables
            - Organize declared variables in hp_wmi_perform_query method

Jorge Lopez (2):
  Resolve WMI query failures on some devices
  Organize declaration variables

 drivers/platform/x86/hp-wmi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] Resolve WMI query failures on some devices

[Jorge Lopez]

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(-)

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;
-- 
2.25.1

[PATCH v2 2/2] Organize declaration variables

[Jorge Lopez]

This patch organizes declaration variables in method
hp_wmi_perform_query as requested.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next

V1 - Original submission
V2 - Organize declaration variables
---
 drivers/platform/x86/hp-wmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
index 7bcfa07cc6ab..d3540dd62d06 100644
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -290,9 +290,9 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
 	struct bios_return *bios_return;
 	union acpi_object *obj = NULL;
 	struct bios_args *args = NULL;
-	int mid, actual_outsize, ret;
+	int mid, actual_insize, actual_outsize;
 	size_t bios_args_size;
-	int actual_insize;
+	int ret;
 
 	mid = encode_outsize_for_pvsz(outsize);
 	if (WARN_ON(mid < 0))
-- 
2.25.1

Re: [PATCH v2 0/2] Resolve-WMI-query-failures-on-some-devices

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 7:21 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote:
>
> The intention for this patch is resolve WMI query failures reported
> in several HP OMEN notebooks.  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.

Maintainers are still missed. Try first to send patches to yourself
(another address or so) and check.

> Description of changes between version 1 and version 2
> ------------------------------------------------------
> v2 patch 1: Resolve-WMI-query-failures-on-some-devices
>             - No new changes to the logic.
>
> v2 patch 2: Organize-declaration-variables
>             - Organize declared variables in hp_wmi_perform_query method
>
> Jorge Lopez (2):
>   Resolve WMI query failures on some devices
>   Organize declaration variables
>
>  drivers/platform/x86/hp-wmi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> --
> 2.25.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] Resolve WMI query failures on some devices

[Andy Shevchenko]

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

Re: [PATCH v2 2/2] Organize declaration variables

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 7:20 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote:
>
> This patch organizes declaration variables in method
> hp_wmi_perform_query as requested.

Ah, I see now. Please squash these changes in one patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] Resolve-WMI-query-failures-on-some-devices

[Jorge Lopez]

Will do, thanks!

On Wed, Jun 8, 2022 at 1:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Jun 8, 2022 at 7:21 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote:
> >
> > The intention for this patch is resolve WMI query failures reported
> > in several HP OMEN notebooks.  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.
>
> Maintainers are still missed. Try first to send patches to yourself
> (another address or so) and check.
>
> > Description of changes between version 1 and version 2
> > ------------------------------------------------------
> > v2 patch 1: Resolve-WMI-query-failures-on-some-devices
> >             - No new changes to the logic.
> >
> > v2 patch 2: Organize-declaration-variables
> >             - Organize declared variables in hp_wmi_perform_query method
> >
> > Jorge Lopez (2):
> >   Resolve WMI query failures on some devices
> >   Organize declaration variables
> >
> >  drivers/platform/x86/hp-wmi.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > --
> > 2.25.1
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 2/2] Organize declaration variables

[Jorge Lopez]

Will do. Thank you

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:
> >
> > This patch organizes declaration variables in method
> > hp_wmi_perform_query as requested.
>
> Ah, I see now. Please squash these changes in one patch.
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 1/2] Resolve WMI query failures on some devices

[Jorge Lopez]

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

Re: [PATCH v2 1/2] Resolve WMI query failures on some devices

[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.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] Resolve WMI query failures on some devices

[Jorge Lopez]

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

Re: [PATCH v2 1/2] Resolve WMI query failures on some devices

[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.

-- 
With Best Regards,
Andy Shevchenko