* [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
* 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, platform-driver-x86
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, platform-driver-x86
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, platform-driver-x86
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 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, platform-driver-x86
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, platform-driver-x86
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
* 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, platform-driver-x86
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 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, platform-driver-x86
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, platform-driver-x86
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, platform-driver-x86
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, platform-driver-x86
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,
platform-driver-x86
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, platform-driver-x86
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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.