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