Hello Jorge,
The patch you provided fixes the insize buffer issue on an Omen 15 2020
AMD.
The zero sized Field creation issue however, still persists. I could
not think of a better way of handling it than to check if the BIOS
throws an error if the insize parameter != 0 and to use insize = 0 only
if it did.
I looked through the DSDT some more. It seems that all commandtypes
(when used with the HPWMI_READ command, do not actually make use of the
Field DAIN that is created, except for the HPWMI_FAN_SPEED_GET_QUERY
commandtype (used with HPWMI_GM command), which seems to access the
created Field. Since the only call to this command uses insize = sizeof
(char) (meaning it is never called with insize = 0), I think we can get
away with setting args->insize to some arbitrary value, say 1, if 0 is
passed as insize, since the Field is never accessed anyway. This
"solution" however, relies too much on the firmware.
Your input is welcome. Thank you for your time.
On Tue, 2022-06-07 at 13:11 -0500, Jorge Lopez wrote:
> Dropped other participants by accident. Please see my comments
> below.
>
> The original code created an insize 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.
> One additional point was considered. The purpose for introducing
> dynamically allocated buffers was primarily to support WMI calls that
> required buffers size bigger than 128 bytes. The decision was made
> to separate those WMI calls to a separate driver and implement the
> new
> firmware-attribute framework.
>
> The current review request title is "[PATCH] Revert "platform/x86:
> hp-wmi: Changing bios_args.data to be dynamically allocated"
>
> I am testing a cleaner solution to submit instead of reverting the
> changes to hp_wmi_perform_query method. The changes were made and
> tested on business notebooks without any issues. I will submit a new
> patch as soon as I get the test results from a community member who
> owns an Omen 15 system.
>
> The proposed patch which I am pending test results, it is as follows
>
> 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;
> -----
>
> This code eliminates the 0x05 error codes across the other WMI calls.
> Unfortunately, I do not have an Omen system to complete the tests.
>
> zero_insize_support() seems ok for Omen platforms but don't know if
> it
> may cause errors on newer notebooks. I will run the patch locally
> and
> will update the results.
> Bedant if it possible, can you apply the changes described earlier
> and
> let me know if it fixes Omen notebook issues?
>
>
> Regards,
>
> Jorge.
>
> On Tue, Jun 7, 2022 at 12:25 PM Hans de Goede <hdegoede@xxxxxxxxxx>
> wrote:
> >
> > Hi,
> >
> > It looks like you accidentally dropped all the other folks on the
> > Cc,
> > please use reply-to-all. I've re-added those folks now.
> >
> > On 6/7/22 18:00, Jorge Lopez wrote:
> > > Hi Hans
> > >
> > >
> > > On Tue, Jun 7, 2022 at 8:35 AM Hans de Goede
> > > <hdegoede@xxxxxxxxxx> wrote:
> > > >
> > > >
> > > > Hi Bedant,
> > > >
> > > > Adding Jorge from HP to the To: list.
> > > >
> > > > On 6/7/22 15:24, Bedant Patnaik wrote:
> > > > > 4b4967cbd2685f313411e6facf915fb2ae01d796 ("platform/x86: hp-
> > > > > wmi: Changing bios_args.data to be dynamically...")
> > > > > broke WMI queries 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.
> > > > > Fix: allocate at least 128 bytes for args->data
> > > > >
> > > > > be9d73e64957bbd31ee9a0d11adc0f720974c558 ("platform/x86: hp-
> > > > > wmi: Fix 0x05 error code reported by several WMI calls")
> > > > > and 12b19f14a21a2ee6348825d95b642ef2cd16794f ("platform/x86:
> > > > > hp-wmi: Fix hp_wmi_read_int() reporting error (0x05)")
> > > > > caused ACPI BIOS Error (bug): Attempt to CreateField of
> > > > > length zero (20211217/dsopcode-133) because of the ACPI
> > > > > method HWMC, which unconditionally creates a Field of size
> > > > > (insize*8) bits:
> > > > > CreateField (Arg1, 0x80, (Local5 * 0x08), DAIN)
> > > > > In cases where args->insize = 0, the Field size is 0,
> > > > > resulting in an error.
> > > > > Fix: use zero insize only if 0x5 error code is returned
> > > > >
> > > > > Tested on Omen 15 AMD (2020) board ID: 8786.
> > > >
> > > > Thank you for looking into this. The alloc at least
> > > > 128 bytes part for args->data looks good and likely is a better
> > > > fix then the revert of 4b4967cbd2685f3 which Jorge has
> > > > submitted.
> > > >
> > > > I'm not 100% sure about the zero_insize_support() thingie
> > > > though.
> > > >
> > > > Looking at the original fix and then trying to get things
> > > > to work on all models with some requiring insize==0 and
> > > > otheres requiring insize!=0 I guess this also makes sense...
> > > >
> > > > Jorge, any remarks on this patch?
> > > >
> > >
> > > The original code created an insize 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.
> > > One additional point was considered. The purpose for introducing
> > > dynamically allocated buffers was primarily to support WMI calls
> > > that
> > > required buffers size bigger than 128 bytes. The decision was
> > > made
> > > to separate those WMI calls to a separate driver and implement
> > > the new
> > > firmware-attribute framework.
> > >
> > > The current review request title is "[PATCH] Revert
> > > "platform/x86:
> > > hp-wmi: Changing bios_args.data to be dynamically allocated"
> > >
> > > I am testing a cleaner solution to submit instead of reverting
> > > the
> > > changes to hp_wmi_perform_query method. The changes were made
> > > and
> > > tested on business notebooks without any issues. I will submit a
> > > new
> > > patch as soon as I get the test results from a community member
> > > who
> > > owns an Omen 15 system.
> >
> > Right, notice the RFC this thread is a reply to already contains
> > a cleaner version and has been tested on a HP Omen laptop already.
> >
> > The already tested cleaner fix from this RFC is:
> >
> > --- a/drivers/platform/x86/hp-wmi.c
> > +++ b/drivers/platform/x86/hp-wmi.c
> > @@ -297,8 +299,8 @@ static int hp_wmi_perform_query(int query,
> > enum hp_wmi_command command,
> > if (WARN_ON(mid < 0))
> > return mid;
> >
> > - bios_args_size = struct_size(args, data, insize);
> > - args = kmalloc(bios_args_size, GFP_KERNEL);
> > + bios_args_size = max(struct_size(args, data, insize),
> > struct_size(args, data, 128));
> > + args = kzalloc(bios_args_size, GFP_KERNEL);
> > if (!args)
> > return -ENOMEM;
> >
> >
> > Which seems a better (more future proof) solution then your revert.
> >
> > Jorge, my main question from my previous email really is what you
> > make of the zero_if_sup() changes / part of this RFC. Which are
> > necessary because some older ACPI tables don't like it when
> > args->insize is set to 0, which it is after your:
> >
> > be9d73e64957bbd31ee9a0d11adc0f720974c558 ("platform/x86: hp-wmi:
> > Fix 0x05 error code reported by several WMI calls")
> > 12b19f14a21a2ee6348825d95b642ef2cd16794f ("platform/x86: hp-wmi:
> > Fix hp_wmi_read_int() reporting error (0x05)")
> >
> > commits. For details see the original commit msg from this RFC:
> > https://patchwork.kernel.org/project/platform-driver-x86/patch/20220607132428.7221-1-bedant.patnaik@xxxxxxxxx/
> >
> > Even if we go with the revert you submitted that only fixes
> > the passing buffers < 128 bytes issue and not this other issue.
> >
> > I had to take a good look at it, but after doing that I do believe
> > that the proposed zero_if_sup() changes make sense.
> >
> > Bedant, can you split your original RFC into 2 patches please,
> > one for each separate of the two (128 bytes buf-size,
> > resp. zero_if_sup()) fixes which you are doing ?
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >
> >
> > >
> > > Regards,
> > >
> > > Jorge.
> > >
> > >
> > >
> > > > Regards,
> > > >
> > > > Hans
> > > >
> > > > >
> > > > > Signed-off-by: Bedant Patnaik <bedant.patnaik@xxxxxxxxx>
> > > > > Cc: markgross@xxxxxxxxxx
> > > > > Cc: platform-driver-x86@xxxxxxxxxxxxxxx
> > > > >
> > > > > ---
> > > > > drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++++-------
> > > > > ----
> > > > > 1 file changed, 18 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/hp-wmi.c
> > > > > b/drivers/platform/x86/hp-wmi.c
> > > > > index 667f94bba..3ef385f14 100644
> > > > > --- a/drivers/platform/x86/hp-wmi.c
> > > > > +++ b/drivers/platform/x86/hp-wmi.c
> > > > > @@ -38,6 +38,7 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-
> > > > > 3D44E2C707E4");
> > > > > #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-
> > > > > ACCDC67EF61C"
> > > > > #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-
> > > > > 3D44E2C707E4"
> > > > > #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> > > > > +#define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp))
> > > > > // use when zero insize is required
> > > > >
> > > > > /* DMI board names of devices that should use the omen
> > > > > specific path for
> > > > > * thermal profiles.
> > > > > @@ -175,7 +176,7 @@ enum hp_thermal_profile_omen_v1 {
> > > > > enum hp_thermal_profile {
> > > > > HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
> > > > > HP_THERMAL_PROFILE_DEFAULT = 0x01,
> > > > > - HP_THERMAL_PROFILE_COOL = 0x02
> > > > > + HP_THERMAL_PROFILE_COOL = 0x02,
> > > > > };
> > > > >
> > > > > #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) !=
> > > > > HPWMI_POWER_FW_OR_HW)
> > > > > @@ -220,6 +221,7 @@ static struct input_dev
> > > > > *hp_wmi_input_dev;
> > > > > static struct platform_device *hp_wmi_platform_dev;
> > > > > static struct platform_profile_handler
> > > > > platform_profile_handler;
> > > > > static bool platform_profile_support;
> > > > > +static bool zero_insize_support;
> > > > >
> > > > > static struct rfkill *wifi_rfkill;
> > > > > static struct rfkill *bluetooth_rfkill;
> > > > > @@ -297,8 +299,8 @@ static int hp_wmi_perform_query(int
> > > > > query, enum hp_wmi_command command,
> > > > > if (WARN_ON(mid < 0))
> > > > > return mid;
> > > > >
> > > > > - bios_args_size = struct_size(args, data, insize);
> > > > > - args = kmalloc(bios_args_size, GFP_KERNEL);
> > > > > + bios_args_size = max(struct_size(args, data, insize),
> > > > > struct_size(args, data, 128));
> > > > > + args = kzalloc(bios_args_size, GFP_KERNEL);
> > > > > if (!args)
> > > > > return -ENOMEM;
> > > > >
> > > > > @@ -374,7 +376,7 @@ static int hp_wmi_read_int(int query)
> > > > > int val = 0, ret;
> > > > >
> > > > > ret = hp_wmi_perform_query(query, HPWMI_READ, &val,
> > > > > - 0, sizeof(val));
> > > > > + zero_if_sup(val),
> > > > > sizeof(val));
> > > > >
> > > > > if (ret)
> > > > > return ret < 0 ? ret : -EINVAL;
> > > > > @@ -410,7 +412,8 @@ static int hp_wmi_get_tablet_mode(void)
> > > > > return -ENODEV;
> > > > >
> > > > > ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE,
> > > > > HPWMI_READ,
> > > > > - system_device_mode, 0,
> > > > > sizeof(system_device_mode));
> > > > > + system_device_mode,
> > > > > zero_if_sup(system_device_mode),
> > > > > + sizeof(system_device_mode));
> > > > > if (ret < 0)
> > > > > return ret;
> > > > >
> > > > > @@ -497,7 +500,7 @@ static int hp_wmi_fan_speed_max_get(void)
> > > > > int val = 0, ret;
> > > > >
> > > > > ret =
> > > > > hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
> > > > > - &val, 0, sizeof(val));
> > > > > + &val, zero_if_sup(val),
> > > > > sizeof(val));
> > > > >
> > > > > if (ret)
> > > > > return ret < 0 ? ret : -EINVAL;
> > > > > @@ -509,7 +512,7 @@ static int __init
> > > > > hp_wmi_bios_2008_later(void)
> > > > > {
> > > > > int state = 0;
> > > > > int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY,
> > > > > HPWMI_READ, &state,
> > > > > - 0, sizeof(state));
> > > > > + zero_if_sup(state),
> > > > > sizeof(state));
> > > > > if (!ret)
> > > > > return 1;
> > > > >
> > > > > @@ -520,7 +523,7 @@ static int __init
> > > > > hp_wmi_bios_2009_later(void)
> > > > > {
> > > > > u8 state[128];
> > > > > int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY,
> > > > > HPWMI_READ, &state,
> > > > > - 0, sizeof(state));
> > > > > + zero_if_sup(state),
> > > > > sizeof(state));
> > > > > if (!ret)
> > > > > return 1;
> > > > >
> > > > > @@ -598,7 +601,7 @@ static int hp_wmi_rfkill2_refresh(void)
> > > > > int err, i;
> > > > >
> > > > > err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY,
> > > > > HPWMI_READ, &state,
> > > > > - 0, sizeof(state));
> > > > > + zero_if_sup(state),
> > > > > sizeof(state));
> > > > > if (err)
> > > > > return err;
> > > > >
> > > > > @@ -1007,7 +1010,7 @@ static int __init
> > > > > hp_wmi_rfkill2_setup(struct platform_device *device)
> > > > > int err, i;
> > > > >
> > > > > err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY,
> > > > > HPWMI_READ, &state,
> > > > > - 0, sizeof(state));
> > > > > + zero_if_sup(state),
> > > > > sizeof(state));
> > > > > if (err)
> > > > > return err < 0 ? err : -EINVAL;
> > > > >
> > > > > @@ -1483,11 +1486,15 @@ static int __init hp_wmi_init(void)
> > > > > {
> > > > > int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
> > > > > int bios_capable = wmi_has_guid(HPWMI_BIOS_GUID);
> > > > > - int err;
> > > > > + int err, tmp = 0;
> > > > >
> > > > > if (!bios_capable && !event_capable)
> > > > > return -ENODEV;
> > > > >
> > > > > + if (hp_wmi_perform_query(HPWMI_HARDWARE_QUERY,
> > > > > HPWMI_READ, &tmp,
> > > > > + sizeof(tmp), sizeof(tmp)) ==
> > > > > HPWMI_RET_INVALID_PARAMETERS)
> > > > > + zero_insize_support = true;
> > > > > +
> > > > > if (event_capable) {
> > > > > err = hp_wmi_input_setup();
> > > > > if (err)
> > > >
> > >
> >
