* [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices
@ 2022-06-07 13:24 Bedant Patnaik
2022-06-07 13:35 ` Hans de Goede
0 siblings, 1 reply; 14+ messages in thread
From: Bedant Patnaik @ 2022-06-07 13:24 UTC (permalink / raw)
To: hdegoede; +Cc: Bedant Patnaik, markgross, platform-driver-x86
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.
Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com>
Cc: markgross@kernel.org
Cc: platform-driver-x86@vger.kernel.org
---
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)
--
2.36.1
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 13:24 [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices Bedant Patnaik @ 2022-06-07 13:35 ` Hans de Goede [not found] ` <CAOOmCE8QYEwh6TrgA=_sTcm4spkuk3rjMS4g78nbBbWXWUB2aQ@mail.gmail.com> 0 siblings, 1 reply; 14+ messages in thread From: Hans de Goede @ 2022-06-07 13:35 UTC (permalink / raw) To: Bedant Patnaik, Jorge Lopez, Andy Shevchenko Cc: markgross, platform-driver-x86 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? Regards, Hans > > Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> > Cc: markgross@kernel.org > Cc: platform-driver-x86@vger.kernel.org > > --- > 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) ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <CAOOmCE8QYEwh6TrgA=_sTcm4spkuk3rjMS4g78nbBbWXWUB2aQ@mail.gmail.com>]
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices [not found] ` <CAOOmCE8QYEwh6TrgA=_sTcm4spkuk3rjMS4g78nbBbWXWUB2aQ@mail.gmail.com> @ 2022-06-07 17:25 ` Hans de Goede 2022-06-07 18:11 ` Jorge Lopez ` (2 more replies) 0 siblings, 3 replies; 14+ messages in thread From: Hans de Goede @ 2022-06-07 17:25 UTC (permalink / raw) To: Jorge Lopez Cc: Bedant Patnaik, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org 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@redhat.com> 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@gmail.com/ 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@gmail.com> >>> Cc: markgross@kernel.org >>> Cc: platform-driver-x86@vger.kernel.org >>> >>> --- >>> 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) >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 17:25 ` Hans de Goede @ 2022-06-07 18:11 ` Jorge Lopez 2022-06-07 18:37 ` Bedant Patnaik 2022-06-07 19:20 ` Hans de Goede 2022-06-07 18:52 ` Bedant Patnaik 2022-06-07 18:54 ` Bedant Patnaik 2 siblings, 2 replies; 14+ messages in thread From: Jorge Lopez @ 2022-06-07 18:11 UTC (permalink / raw) To: Hans de Goede Cc: Bedant Patnaik, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org 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@redhat.com> 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@redhat.com> 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@gmail.com/ > > 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@gmail.com> > >>> Cc: markgross@kernel.org > >>> Cc: platform-driver-x86@vger.kernel.org > >>> > >>> --- > >>> 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) > >> > > > ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 18:11 ` Jorge Lopez @ 2022-06-07 18:37 ` Bedant Patnaik 2022-06-07 19:47 ` Jorge Lopez 2022-06-07 19:20 ` Hans de Goede 1 sibling, 1 reply; 14+ messages in thread From: Bedant Patnaik @ 2022-06-07 18:37 UTC (permalink / raw) To: Jorge Lopez, Hans de Goede Cc: Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org 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@redhat.com> > 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@redhat.com> 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@gmail.com/ > > > > 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@gmail.com> > > > > > Cc: markgross@kernel.org > > > > > Cc: platform-driver-x86@vger.kernel.org > > > > > > > > > > --- > > > > > 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) > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 18:37 ` Bedant Patnaik @ 2022-06-07 19:47 ` Jorge Lopez 2022-06-08 19:28 ` Bedant Patnaik 0 siblings, 1 reply; 14+ messages in thread From: Jorge Lopez @ 2022-06-07 19:47 UTC (permalink / raw) To: Bedant Patnaik Cc: Hans de Goede, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org Hi Bedant, Thank you for the update. I applied your patch (zero_if_sup) and ran the driver on several Zbook and Elitebook notebooks. With exception of getting error 0x05 once when executing HPWMI_HARDWARE_QUERY (0x04) while loading, all other functions remain unaffected and working fine. No other errors were reported. I don't feel comfortable with setting args->insize to some arbitrary value. It will introduce more problems that it will solve. Patching the driver with zero_if_sup definition is the best option to ensure support on Omen, Zbooks, and EliteBooks alike. BTW, I received confirmation from another community member and also confirmed the patch resolves HPWMI_FAN GET/SET calls in an OMEN Laptop 15-ek0xxx. If you are ok with it, I will submit a patch for the insize buffer size of 128 bytes and you can proceed to submit zero_if_sup patch. Let me know if you are ok to proceed in this manner. Regards, Jorge On Tue, Jun 7, 2022 at 1:38 PM Bedant Patnaik <bedant.patnaik@gmail.com> wrote: > > 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@redhat.com> > > 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@redhat.com> 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@gmail.com/ > > > > > > 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@gmail.com> > > > > > > Cc: markgross@kernel.org > > > > > > Cc: platform-driver-x86@vger.kernel.org > > > > > > > > > > > > --- > > > > > > 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) > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 19:47 ` Jorge Lopez @ 2022-06-08 19:28 ` Bedant Patnaik 2022-06-08 19:41 ` Bedant Patnaik 2022-06-10 20:27 ` Hans de Goede 0 siblings, 2 replies; 14+ messages in thread From: Bedant Patnaik @ 2022-06-08 19:28 UTC (permalink / raw) To: Jorge Lopez Cc: Hans de Goede, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org From 26f84d835819fa38872c673f49f1b77774927568 Mon Sep 17 00:00:00 2001 From: Bedant Patnaik <bedant.patnaik@gmail.com> Date: Thu, 9 Jun 2022 00:50:53 +0530 Subject: [PATCH] Use zero insize parameter only when supported 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)") cause 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. Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> --- drivers/platform/x86/hp-wmi.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 277397de5..0e07581b8 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. @@ -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; @@ -376,7 +378,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; @@ -412,7 +414,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; @@ -499,7 +502,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; @@ -511,7 +514,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; @@ -522,7 +525,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; @@ -600,7 +603,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; @@ -1009,7 +1012,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; @@ -1485,11 +1488,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) -- 2.36.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-08 19:28 ` Bedant Patnaik @ 2022-06-08 19:41 ` Bedant Patnaik 2022-06-09 13:50 ` Jorge Lopez 2022-06-10 20:27 ` Hans de Goede 1 sibling, 1 reply; 14+ messages in thread From: Bedant Patnaik @ 2022-06-08 19:41 UTC (permalink / raw) To: Jorge Lopez Cc: Hans de Goede, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org Greetings Hans, Jorge, I have sent the zero_if_supp() patch that can be applied over Jorge's buffer size patch in the mail that I'm replying to. This *should* be enough as the final patch. Please let me know if anything needs to be rectified. All your input is welcome. Thank you. On Thu, 2022-06-09 at 00:58 +0530, Bedant Patnaik wrote: > From 26f84d835819fa38872c673f49f1b77774927568 Mon Sep 17 00:00:00 2001 > From: Bedant Patnaik <bedant.patnaik@gmail.com> > Date: Thu, 9 Jun 2022 00:50:53 +0530 > Subject: [PATCH] Use zero insize parameter only when supported > > 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)") > cause 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. > > Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> > --- > drivers/platform/x86/hp-wmi.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 277397de5..0e07581b8 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. > @@ -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; > @@ -376,7 +378,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; > @@ -412,7 +414,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; > > @@ -499,7 +502,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; > @@ -511,7 +514,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; > > @@ -522,7 +525,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; > > @@ -600,7 +603,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; > > @@ -1009,7 +1012,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; > > @@ -1485,11 +1488,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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-08 19:41 ` Bedant Patnaik @ 2022-06-09 13:50 ` Jorge Lopez 2022-06-09 15:09 ` Andy Shevchenko 0 siblings, 1 reply; 14+ messages in thread From: Jorge Lopez @ 2022-06-09 13:50 UTC (permalink / raw) To: Bedant Patnaik Cc: Hans de Goede, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org On Wed, Jun 8, 2022 at 2:42 PM Bedant Patnaik <bedant.patnaik@gmail.com> wrote: > > Greetings Hans, Jorge, > > I have sent the zero_if_supp() patch that can be applied over Jorge's buffer size patch in the mail that I'm replying to. > This *should* be enough as the final patch. Please let me know if anything needs to be rectified. The patch looks good to me. It matches the one tested earlier. Thank you > > All your input is welcome. > > Thank you. > > On Thu, 2022-06-09 at 00:58 +0530, Bedant Patnaik wrote: > > From 26f84d835819fa38872c673f49f1b77774927568 Mon Sep 17 00:00:00 2001 > > From: Bedant Patnaik <bedant.patnaik@gmail.com> > > Date: Thu, 9 Jun 2022 00:50:53 +0530 > > Subject: [PATCH] Use zero insize parameter only when supported > > > > 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)") > > cause 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. > > > > Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> > > --- > > drivers/platform/x86/hp-wmi.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > > index 277397de5..0e07581b8 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. > > @@ -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; > > @@ -376,7 +378,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; > > @@ -412,7 +414,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; > > > > @@ -499,7 +502,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; > > @@ -511,7 +514,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; > > > > @@ -522,7 +525,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; > > > > @@ -600,7 +603,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; > > > > @@ -1009,7 +1012,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; > > > > @@ -1485,11 +1488,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) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-09 13:50 ` Jorge Lopez @ 2022-06-09 15:09 ` Andy Shevchenko 0 siblings, 0 replies; 14+ messages in thread From: Andy Shevchenko @ 2022-06-09 15:09 UTC (permalink / raw) To: Jorge Lopez Cc: Bedant Patnaik, Hans de Goede, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org On Thu, Jun 9, 2022 at 3:50 PM Jorge Lopez <jorgealtxwork@gmail.com> wrote: > On Wed, Jun 8, 2022 at 2:42 PM Bedant Patnaik <bedant.patnaik@gmail.com> wrote: > > I have sent the zero_if_supp() patch that can be applied over Jorge's buffer size patch in the mail that I'm replying to. > > This *should* be enough as the final patch. Please let me know if anything needs to be rectified. > > The patch looks good to me. It matches the one tested earlier. Thank you If you wish a bit of advance process, you may do the following: 1) squash your patches (as I mentioned in the other thread); 2) apply this patch on top; 3) send two patches as a v3 of your series. Git should preserve authorship correctly. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-08 19:28 ` Bedant Patnaik 2022-06-08 19:41 ` Bedant Patnaik @ 2022-06-10 20:27 ` Hans de Goede 1 sibling, 0 replies; 14+ messages in thread From: Hans de Goede @ 2022-06-10 20:27 UTC (permalink / raw) To: Bedant Patnaik, Jorge Lopez Cc: Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org Hi, On 6/8/22 21:28, Bedant Patnaik wrote: > From 26f84d835819fa38872c673f49f1b77774927568 Mon Sep 17 00:00:00 2001 > From: Bedant Patnaik <bedant.patnaik@gmail.com> > Date: Thu, 9 Jun 2022 00:50:53 +0530 > Subject: [PATCH] Use zero insize parameter only when supported > > 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)") > cause 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. > > Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/hp-wmi.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 277397de5..0e07581b8 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. > @@ -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; > @@ -376,7 +378,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; > @@ -412,7 +414,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; > > @@ -499,7 +502,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; > @@ -511,7 +514,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; > > @@ -522,7 +525,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; > > @@ -600,7 +603,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; > > @@ -1009,7 +1012,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; > > @@ -1485,11 +1488,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) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 18:11 ` Jorge Lopez 2022-06-07 18:37 ` Bedant Patnaik @ 2022-06-07 19:20 ` Hans de Goede 1 sibling, 0 replies; 14+ messages in thread From: Hans de Goede @ 2022-06-07 19:20 UTC (permalink / raw) To: Jorge Lopez Cc: Bedant Patnaik, Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org Hi, On 6/7/22 20:11, 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. This in essence is the same as the RFC from Bedant, which was already tested on an Omen, so I believe that it is safe to say that this variant (which is a bit cleaner) will also work on Bedant's Omen model. > 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. Thanks in advance for testing the zero_insize_support(). Regards, Hans > 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@redhat.com> 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@redhat.com> 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@gmail.com/ >> >> 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@gmail.com> >>>>> Cc: markgross@kernel.org >>>>> Cc: platform-driver-x86@vger.kernel.org >>>>> >>>>> --- >>>>> 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) >>>> >>> >> > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 17:25 ` Hans de Goede 2022-06-07 18:11 ` Jorge Lopez @ 2022-06-07 18:52 ` Bedant Patnaik 2022-06-07 18:54 ` Bedant Patnaik 2 siblings, 0 replies; 14+ messages in thread From: Bedant Patnaik @ 2022-06-07 18:52 UTC (permalink / raw) To: Hans de Goede, Jorge Lopez Cc: Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org From: Bedant Patnaik <bedant.patnaik@gmail.com> Date: Tue, 7 Jun 2022 23:16:05 +0530 Subject: [PATCH] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 4b4967cbd2685f313411e6facf915fb2ae01d796 ("platform/x86: hp-wmi: Changing bios_args.data to be dynamically allocated") 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: 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 Tested on Omen 15 AMD (2020) board ID: 8786. Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> --- drivers/platform/x86/hp-wmi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 667f94bba..7a54191f0 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -175,7 +175,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) @@ -297,8 +297,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; -- 2.36.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices 2022-06-07 17:25 ` Hans de Goede 2022-06-07 18:11 ` Jorge Lopez 2022-06-07 18:52 ` Bedant Patnaik @ 2022-06-07 18:54 ` Bedant Patnaik 2 siblings, 0 replies; 14+ messages in thread From: Bedant Patnaik @ 2022-06-07 18:54 UTC (permalink / raw) To: Hans de Goede, Jorge Lopez Cc: Andy Shevchenko, markgross@kernel.org, platform-driver-x86@vger.kernel.org From 59c10c3007e52694318324fc5e9cfbce83533491 Mon Sep 17 00:00:00 2001 From: Bedant Patnaik <bedant.patnaik@gmail.com> Date: Tue, 7 Jun 2022 23:30:57 +0530 Subject: [PATCH] Use zero insize parameter only when supported 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)") cause 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. Signed-off-by: Bedant Patnaik <bedant.patnaik@gmail.com> --- drivers/platform/x86/hp-wmi.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 7a54191f0..809294e36 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. @@ -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; @@ -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) -- 2.36.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-06-10 20:28 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-06-07 13:24 [RFC] platform/x86: hp-wmi: make hp_wmi_perform_query() work with certain devices Bedant Patnaik
2022-06-07 13:35 ` Hans de Goede
[not found] ` <CAOOmCE8QYEwh6TrgA=_sTcm4spkuk3rjMS4g78nbBbWXWUB2aQ@mail.gmail.com>
2022-06-07 17:25 ` Hans de Goede
2022-06-07 18:11 ` Jorge Lopez
2022-06-07 18:37 ` Bedant Patnaik
2022-06-07 19:47 ` Jorge Lopez
2022-06-08 19:28 ` Bedant Patnaik
2022-06-08 19:41 ` Bedant Patnaik
2022-06-09 13:50 ` Jorge Lopez
2022-06-09 15:09 ` Andy Shevchenko
2022-06-10 20:27 ` Hans de Goede
2022-06-07 19:20 ` Hans de Goede
2022-06-07 18:52 ` Bedant Patnaik
2022-06-07 18:54 ` Bedant Patnaik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox