Hi,
It looks like you accidentally dropped all the other folks on the Cc,
please use reply-to-all. I've re-added those folks now.
On 6/7/22 18:00, Jorge Lopez wrote:
> Hi Hans
>
>
> On Tue, Jun 7, 2022 at 8:35 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>>
>> Hi Bedant,
>>
>> Adding Jorge from HP to the To: list.
>>
>> On 6/7/22 15:24, Bedant Patnaik wrote:
>>> 4b4967cbd2685f313411e6facf915fb2ae01d796 ("platform/x86: hp-wmi: Changing bios_args.data to be dynamically...")
>>> broke WMI queries on some devices where the ACPI method HWMC unconditionally attempts to create Fields beyond the buffer
>>> if the buffer is too small, this breaks essential features such as power profiles:
>>> CreateByteField (Arg1, 0x10, D008)
>>> CreateByteField (Arg1, 0x11, D009)
>>> CreateByteField (Arg1, 0x12, D010)
>>> CreateDWordField (Arg1, 0x10, D032)
>>> CreateField (Arg1, 0x80, 0x0400, D128)
>>> In cases where args->data had zero length, ACPI BIOS Error (bug): AE_AML_BUFFER_LIMIT, Field [D008] at bit
>>> offset/length 128/8 exceeds size of target Buffer (128 bits) (20211217/dsopcode-198) was obtained.
>>> Fix: allocate at least 128 bytes for args->data
>>>
>>> be9d73e64957bbd31ee9a0d11adc0f720974c558 ("platform/x86: hp-wmi: Fix 0x05 error code reported by several WMI calls")
>>> and 12b19f14a21a2ee6348825d95b642ef2cd16794f ("platform/x86: hp-wmi: Fix hp_wmi_read_int() reporting error (0x05)")
>>> caused ACPI BIOS Error (bug): Attempt to CreateField of length zero (20211217/dsopcode-133) because of the ACPI
>>> method HWMC, which unconditionally creates a Field of size (insize*8) bits:
>>> CreateField (Arg1, 0x80, (Local5 * 0x08), DAIN)
>>> In cases where args->insize = 0, the Field size is 0, resulting in an error.
>>> Fix: use zero insize only if 0x5 error code is returned
>>>
>>> Tested on Omen 15 AMD (2020) board ID: 8786.
>>
>> Thank you for looking into this. The alloc at least
>> 128 bytes part for args->data looks good and likely is a better
>> fix then the revert of 4b4967cbd2685f3 which Jorge has submitted.
>>
>> I'm not 100% sure about the zero_insize_support() thingie though.
>>
>> Looking at the original fix and then trying to get things
>> to work on all models with some requiring insize==0 and
>> otheres requiring insize!=0 I guess this also makes sense...
>>
>> Jorge, any remarks on this patch?
>>
>
> The original code created an insize buffer size of 128 bytes
> regardless if the WMI call required a smaller buffer or not. This
> particular behavior occurs in older BIOS and reproduced in OMEN
> laptops. Newer BIOS handles buffer sizes properly and meets the
> latest specification requirements. This is the reason why testing
> with a dynamically allocated buffer did not uncover any failures with
> the test systems at hand.
> One additional point was considered. The purpose for introducing
> dynamically allocated buffers was primarily to support WMI calls that
> required buffers size bigger than 128 bytes. The decision was made
> to separate those WMI calls to a separate driver and implement the new
> firmware-attribute framework.
>
> The current review request title is "[PATCH] Revert "platform/x86:
> hp-wmi: Changing bios_args.data to be dynamically allocated"
>
> I am testing a cleaner solution to submit instead of reverting the
> changes to hp_wmi_perform_query method. The changes were made and
> tested on business notebooks without any issues. I will submit a new
> patch as soon as I get the test results from a community member who
> owns an Omen 15 system.
Right, notice the RFC this thread is a reply to already contains
a cleaner version and has been tested on a HP Omen laptop already.
The already tested cleaner fix from this RFC is:
--- a/drivers/platform/x86/hp-wmi.c
+++ b/drivers/platform/x86/hp-wmi.c
@@ -297,8 +299,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
if (WARN_ON(mid < 0))
return mid;
- bios_args_size = struct_size(args, data, insize);
- args = kmalloc(bios_args_size, GFP_KERNEL);
+ bios_args_size = max(struct_size(args, data, insize), struct_size(args, data, 128));
+ args = kzalloc(bios_args_size, GFP_KERNEL);
if (!args)
return -ENOMEM;
Which seems a better (more future proof) solution then your revert.
Jorge, my main question from my previous email really is what you
make of the zero_if_sup() changes / part of this RFC. Which are
necessary because some older ACPI tables don't like it when
args->insize is set to 0, which it is after your:
be9d73e64957bbd31ee9a0d11adc0f720974c558 ("platform/x86: hp-wmi: Fix 0x05 error code reported by several WMI calls")
12b19f14a21a2ee6348825d95b642ef2cd16794f ("platform/x86: hp-wmi: Fix hp_wmi_read_int() reporting error (0x05)")
commits. For details see the original commit msg from this RFC:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20220607132428.7221-1-bedant.patnaik@xxxxxxxxx/
Even if we go with the revert you submitted that only fixes
the passing buffers < 128 bytes issue and not this other issue.
I had to take a good look at it, but after doing that I do believe
that the proposed zero_if_sup() changes make sense.
Bedant, can you split your original RFC into 2 patches please,
one for each separate of the two (128 bytes buf-size,
resp. zero_if_sup()) fixes which you are doing ?
Regards,
Hans
>
> Regards,
>
> Jorge.
>
>
>
>> Regards,
>>
>> Hans
>>
>>>
>>> Signed-off-by: Bedant Patnaik <bedant.patnaik@xxxxxxxxx>
>>> Cc: markgross@xxxxxxxxxx
>>> Cc: platform-driver-x86@xxxxxxxxxxxxxxx
>>>
>>> ---
>>> drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++++-----------
>>> 1 file changed, 18 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>>> index 667f94bba..3ef385f14 100644
>>> --- a/drivers/platform/x86/hp-wmi.c
>>> +++ b/drivers/platform/x86/hp-wmi.c
>>> @@ -38,6 +38,7 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4");
>>> #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>>> #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
>>> #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>>> +#define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use when zero insize is required
>>>
>>> /* DMI board names of devices that should use the omen specific path for
>>> * thermal profiles.
>>> @@ -175,7 +176,7 @@ enum hp_thermal_profile_omen_v1 {
>>> enum hp_thermal_profile {
>>> HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
>>> HP_THERMAL_PROFILE_DEFAULT = 0x01,
>>> - HP_THERMAL_PROFILE_COOL = 0x02
>>> + HP_THERMAL_PROFILE_COOL = 0x02,
>>> };
>>>
>>> #define IS_HWBLOCKED(x) ((x & HPWMI_POWER_FW_OR_HW) != HPWMI_POWER_FW_OR_HW)
>>> @@ -220,6 +221,7 @@ static struct input_dev *hp_wmi_input_dev;
>>> static struct platform_device *hp_wmi_platform_dev;
>>> static struct platform_profile_handler platform_profile_handler;
>>> static bool platform_profile_support;
>>> +static bool zero_insize_support;
>>>
>>> static struct rfkill *wifi_rfkill;
>>> static struct rfkill *bluetooth_rfkill;
>>> @@ -297,8 +299,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>> if (WARN_ON(mid < 0))
>>> return mid;
>>>
>>> - bios_args_size = struct_size(args, data, insize);
>>> - args = kmalloc(bios_args_size, GFP_KERNEL);
>>> + bios_args_size = max(struct_size(args, data, insize), struct_size(args, data, 128));
>>> + args = kzalloc(bios_args_size, GFP_KERNEL);
>>> if (!args)
>>> return -ENOMEM;
>>>
>>> @@ -374,7 +376,7 @@ static int hp_wmi_read_int(int query)
>>> int val = 0, ret;
>>>
>>> ret = hp_wmi_perform_query(query, HPWMI_READ, &val,
>>> - 0, sizeof(val));
>>> + zero_if_sup(val), sizeof(val));
>>>
>>> if (ret)
>>> return ret < 0 ? ret : -EINVAL;
>>> @@ -410,7 +412,8 @@ static int hp_wmi_get_tablet_mode(void)
>>> return -ENODEV;
>>>
>>> ret = hp_wmi_perform_query(HPWMI_SYSTEM_DEVICE_MODE, HPWMI_READ,
>>> - system_device_mode, 0, sizeof(system_device_mode));
>>> + system_device_mode, zero_if_sup(system_device_mode),
>>> + sizeof(system_device_mode));
>>> if (ret < 0)
>>> return ret;
>>>
>>> @@ -497,7 +500,7 @@ static int hp_wmi_fan_speed_max_get(void)
>>> int val = 0, ret;
>>>
>>> ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_MAX_GET_QUERY, HPWMI_GM,
>>> - &val, 0, sizeof(val));
>>> + &val, zero_if_sup(val), sizeof(val));
>>>
>>> if (ret)
>>> return ret < 0 ? ret : -EINVAL;
>>> @@ -509,7 +512,7 @@ static int __init hp_wmi_bios_2008_later(void)
>>> {
>>> int state = 0;
>>> int ret = hp_wmi_perform_query(HPWMI_FEATURE_QUERY, HPWMI_READ, &state,
>>> - 0, sizeof(state));
>>> + zero_if_sup(state), sizeof(state));
>>> if (!ret)
>>> return 1;
>>>
>>> @@ -520,7 +523,7 @@ static int __init hp_wmi_bios_2009_later(void)
>>> {
>>> u8 state[128];
>>> int ret = hp_wmi_perform_query(HPWMI_FEATURE2_QUERY, HPWMI_READ, &state,
>>> - 0, sizeof(state));
>>> + zero_if_sup(state), sizeof(state));
>>> if (!ret)
>>> return 1;
>>>
>>> @@ -598,7 +601,7 @@ static int hp_wmi_rfkill2_refresh(void)
>>> int err, i;
>>>
>>> err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
>>> - 0, sizeof(state));
>>> + zero_if_sup(state), sizeof(state));
>>> if (err)
>>> return err;
>>>
>>> @@ -1007,7 +1010,7 @@ static int __init hp_wmi_rfkill2_setup(struct platform_device *device)
>>> int err, i;
>>>
>>> err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state,
>>> - 0, sizeof(state));
>>> + zero_if_sup(state), sizeof(state));
>>> if (err)
>>> return err < 0 ? err : -EINVAL;
>>>
>>> @@ -1483,11 +1486,15 @@ static int __init hp_wmi_init(void)
>>> {
>>> int event_capable = wmi_has_guid(HPWMI_EVENT_GUID);
>>> int bios_capable = wmi_has_guid(HPWMI_BIOS_GUID);
>>> - int err;
>>> + int err, tmp = 0;
>>>
>>> if (!bios_capable && !event_capable)
>>> return -ENODEV;
>>>
>>> + if (hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &tmp,
>>> + sizeof(tmp), sizeof(tmp)) == HPWMI_RET_INVALID_PARAMETERS)
>>> + zero_insize_support = true;
>>> +
>>> if (event_capable) {
>>> err = hp_wmi_input_setup();
>>> if (err)
>>
>
