* [PATCH v2 0/3] Fix SW_TABLET_MODE detection method
@ 2022-02-18 16:09 Jorge Lopez
2022-02-18 16:09 ` [PATCH v2 1/3] " Jorge Lopez
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jorge Lopez @ 2022-02-18 16:09 UTC (permalink / raw)
To: platform-driver-x86
The intension for this patch was to address SW_TABLET_MODE detection
problem. It is during the initial investigation; two other issues were
identified and are related to the initial task.
First, several WMI queries were reporting error 0x05 including
HPWMI_HARDWARE_QUERY that is responsible for returning dock and table
modes values. See patch v2 part 2 comments for list of WMI queries
affected. The driver now reports the appropriate states and values
correctly.
Lastly, a limiting data size restriction was discovered.
struct bios_args data member size limits all possible WMI commands
to those requiring buffer size of 128 bytes or less. Several WMI
commands and queries require a buffer size larger than 128 bytes
hence limiting current and new feature supported by the driver.
hp_wmi_perform_query function changed to handle the memory
allocation and release of any required buffer size.
Jorge Lopez (3):
Fix SW_TABLET_MODE detection method
Fix 0x05 error code reported by several WMI calls
Changing bios_args.data to be dynamically allocated
drivers/platform/x86/hp-wmi.c | 160 ++++++++++++++++++++++------------
1 file changed, 104 insertions(+), 56 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v2 1/3] Fix SW_TABLET_MODE detection method 2022-02-18 16:09 [PATCH v2 0/3] Fix SW_TABLET_MODE detection method Jorge Lopez @ 2022-02-18 16:09 ` Jorge Lopez 2022-02-22 16:13 ` Hans de Goede 2022-02-18 16:09 ` [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls Jorge Lopez 2022-02-18 16:09 ` [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated Jorge Lopez 2 siblings, 1 reply; 7+ messages in thread From: Jorge Lopez @ 2022-02-18 16:09 UTC (permalink / raw) To: platform-driver-x86 The purpose of this patch is to introduce a fix and removal of the current hack when determining tablet mode status. Determining the tablet mode status requires reading Byte 0 bit 2 and 3 as reported by HPWMI_HARDWARE_QUERY. The investigation identified the failure was rooted in two areas; HPWMI_HARDWARE_QUERY failure (0x05) and reading Byte 0, bit 2 only to determine the table mode status. HPWMI_HARDWARE_QUERY WMI failure also rendered the dock state value invalid. All changes were validated on a ZBook Workstation notebook. Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 48a46466f086..544fce906ce7 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -35,9 +35,6 @@ MODULE_LICENSE("GPL"); MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C"); MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); -static int enable_tablet_mode_sw = -1; -module_param(enable_tablet_mode_sw, int, 0444); -MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=auto, 0=no, 1=yes)"); #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C" #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4" @@ -127,6 +124,7 @@ enum hp_wmi_command { enum hp_wmi_hardware_mask { HPWMI_DOCK_MASK = 0x01, HPWMI_TABLET_MASK = 0x04, + HPWMI_DETACHABLE_MASK = 0x08, }; struct bios_return { @@ -347,12 +345,19 @@ static int hp_wmi_read_int(int query) static int hp_wmi_hw_state(int mask) { - int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY); + int state = 0, ret; - if (state < 0) - return state; + ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state, + 0, sizeof(state)); - return !!(state & mask); + if (ret) + return ret < 0 ? ret : -EINVAL; + + /* determine if Detachable mode is enabled */ + if (HPWMI_TABLET_MASK == mask) + state = (state & HPWMI_DETACHABLE_MASK ); + + return (state & mask); } static int omen_thermal_profile_set(int mode) @@ -781,18 +786,16 @@ static int __init hp_wmi_input_setup(void) /* Dock */ val = hp_wmi_hw_state(HPWMI_DOCK_MASK); - if (!(val < 0)) { + if (val > 0) { __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); input_report_switch(hp_wmi_input_dev, SW_DOCK, val); } /* Tablet mode */ - if (enable_tablet_mode_sw > 0) { - val = hp_wmi_hw_state(HPWMI_TABLET_MASK); - if (val >= 0) { - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); + val = hp_wmi_hw_state(HPWMI_TABLET_MASK); + if (val > 0) { + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); - } } err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL); -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/3] Fix SW_TABLET_MODE detection method 2022-02-18 16:09 ` [PATCH v2 1/3] " Jorge Lopez @ 2022-02-22 16:13 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2022-02-22 16:13 UTC (permalink / raw) To: Jorge Lopez, platform-driver-x86 Hi Jorge, On 2/18/22 17:09, Jorge Lopez wrote: > The purpose of this patch is to introduce a fix and removal > of the current hack when determining tablet mode status. > > Determining the tablet mode status requires reading Byte 0 bit 2 and 3 > as reported by HPWMI_HARDWARE_QUERY. The investigation identified the > failure was rooted in two areas; HPWMI_HARDWARE_QUERY failure (0x05) > and reading Byte 0, bit 2 only to determine the table mode status. > HPWMI_HARDWARE_QUERY WMI failure also rendered the dock state value invalid. > > All changes were validated on a ZBook Workstation notebook. > > Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> > > --- > Based on the latest platform-drivers-x86.git/for-next > --- > drivers/platform/x86/hp-wmi.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 48a46466f086..544fce906ce7 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -35,9 +35,6 @@ MODULE_LICENSE("GPL"); > MODULE_ALIAS("wmi:95F24279-4D7B-4334-9387-ACCDC67EF61C"); > MODULE_ALIAS("wmi:5FB7F034-2C63-45e9-BE91-3D44E2C707E4"); > > -static int enable_tablet_mode_sw = -1; > -module_param(enable_tablet_mode_sw, int, 0444); > -MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=auto, 0=no, 1=yes)"); > > #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C" > #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4" > @@ -127,6 +124,7 @@ enum hp_wmi_command { > enum hp_wmi_hardware_mask { > HPWMI_DOCK_MASK = 0x01, > HPWMI_TABLET_MASK = 0x04, > + HPWMI_DETACHABLE_MASK = 0x08, > }; > > struct bios_return { > @@ -347,12 +345,19 @@ static int hp_wmi_read_int(int query) > > static int hp_wmi_hw_state(int mask) > { > - int state = hp_wmi_read_int(HPWMI_HARDWARE_QUERY); > + int state = 0, ret; > > - if (state < 0) > - return state; > + ret = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &state, > + 0, sizeof(state)); > > - return !!(state & mask); > + if (ret) > + return ret < 0 ? ret : -EINVAL; > + > + /* determine if Detachable mode is enabled */ > + if (HPWMI_TABLET_MASK == mask) > + state = (state & HPWMI_DETACHABLE_MASK ); If you do: "state &= 0x08" as you do here then after that "state & HPWMI_TABLET_MASK" aka "state & 0x04" as done below will always be 0 since you have masked out the 0x04 bit when applying the 0x08 mask. I think you probably want something like this instead: if (HPWMI_TABLET_MASK == mask) if (!(state & HPWMI_DETACHABLE_MASK)) return 0; So when asked to check the HPWMI_TABLET_MASK then if the device is not a detachable always return 0, does that sound right ? Note it is probable better to just makes this a generic helper which no longer takes the mask and let the caller handle all this. Or maybe just remove this function all together since if you remove the masking I don't think there will be much left of it. > + > + return (state & mask); > } > > static int omen_thermal_profile_set(int mode) > @@ -781,18 +786,16 @@ static int __init hp_wmi_input_setup(void) > > /* Dock */ > val = hp_wmi_hw_state(HPWMI_DOCK_MASK); > - if (!(val < 0)) { > + if (val > 0) { > __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); > input_report_switch(hp_wmi_input_dev, SW_DOCK, val); > } I'm not sure why you made this change here, the original code here actually is correct. This: __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); tells userspace that the input-device has a SW_DOCK switch, where as this: input_report_switch(hp_wmi_input_dev, SW_DOCK, val); Set the initial value of the dock-switch. After your change we will not only do this bit: __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); When the machine is docked at boot, which means that if it gets docked later we will not report it since SW_DOCK is never set in hp_wmi_input_dev->swbit in that case (swbit must be initialized before registering the input-device). > > /* Tablet mode */ > - if (enable_tablet_mode_sw > 0) { > - val = hp_wmi_hw_state(HPWMI_TABLET_MASK); > - if (val >= 0) { > - __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > + val = hp_wmi_hw_state(HPWMI_TABLET_MASK); > + if (val > 0) { > + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); > input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); > - } > } Dropping the module option is fine, but otherwise this needs to be changed. I think we want something like this here: err = hp_wmi_perform_query(HPWMI_HARDWARE_QUERY, HPWMI_READ, &val, 0, sizeof(val)); if (!err) { __set_bit(SW_DOCK, hp_wmi_input_dev->swbit); input_report_switch(hp_wmi_input_dev, SW_DOCK, (val & HPWMI_DOCK_MASK)); } if (!err && (val & HPWMI_DETACHABLE_MASK)) { __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, (val & HPWMI_TABLET_MASK)); } This also avoids unnecessarily doing the HPWMI_HARDWARE_QUERY twice. And we probably want something similar in hp_wmi_notify() case HPWMI_DOCK_EVENT: to also avoid unnecessarily doing the HPWMI_HARDWARE_QUERY twice there. Regards, Hans Reg ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls 2022-02-18 16:09 [PATCH v2 0/3] Fix SW_TABLET_MODE detection method Jorge Lopez 2022-02-18 16:09 ` [PATCH v2 1/3] " Jorge Lopez @ 2022-02-18 16:09 ` Jorge Lopez 2022-02-22 16:22 ` Hans de Goede 2022-02-18 16:09 ` [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated Jorge Lopez 2 siblings, 1 reply; 7+ messages in thread From: Jorge Lopez @ 2022-02-18 16:09 UTC (permalink / raw) To: platform-driver-x86 Several WMI queries leverage hp_wmi_read_int function to read their data. hp_wmi_read_int function returns the appropiate value if the WMI command requires an input and output buffer size values greater than zero. WMI queries such HPWMI_HARDWARE_QUERY, HPWMI_WIRELESS2_QUERY, HPWMI_FEATURE2_QUERY, HPWMI_DISPLAY_QUERY, HPWMI_ALS_QUERY, and HPWMI_POSTCODEERROR_QUERY requires calling hp_wmi_perform_query function with input buffer size value of zero. Any input buffer size greater than zero will cause error 0x05 to be returned. All changes were validated on a ZBook Workstation notebook. Additional validation was included to ensure no other commands were failing or incorrectly handled. Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 64 ++++++++++++++++++++++++----------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index 544fce906ce7..de715687021a 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -442,7 +442,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, - sizeof(state), sizeof(state)); + 0, sizeof(state)); if (!ret) return 1; @@ -477,25 +477,37 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = { static bool hp_wmi_get_sw_state(enum hp_wmi_radio r) { int mask = 0x200 << (r * 8); + int ret= 0; + int wireless = 0; + + ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, + 0, sizeof(wireless)); - int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY); + if (ret < 0) + return -EINVAL; /* TBD: Pass error */ WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY"); - return !(wireless & mask); + return (wireless & mask); } static bool hp_wmi_get_hw_state(enum hp_wmi_radio r) { int mask = 0x800 << (r * 8); + int ret= 0; + int wireless = 0; - int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY); + ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, + 0, sizeof(wireless)); + + if (ret < 0) + return -EINVAL; /* TBD: Pass error */ WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY"); - return !(wireless & mask); + return (wireless & mask); } static int hp_wmi_rfkill2_set_block(void *data, bool blocked) @@ -520,7 +532,7 @@ static int hp_wmi_rfkill2_refresh(void) int err, i; err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state, - sizeof(state), sizeof(state)); + 0, sizeof(state)); if (err) return err; @@ -546,27 +558,36 @@ static int hp_wmi_rfkill2_refresh(void) static ssize_t display_show(struct device *dev, struct device_attribute *attr, char *buf) { - int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY); - if (value < 0) - return value; + int value = 0; + int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &value, + 0, sizeof(value)); + if (ret) + return ret < 0 ? ret : -EINVAL; + return sprintf(buf, "%d\n", value); } static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr, char *buf) { - int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY); - if (value < 0) - return value; + int value = 0; + int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &value, + 0, sizeof(value)); + if (ret) + return ret < 0 ? ret : -EINVAL; + return sprintf(buf, "%d\n", value); } static ssize_t als_show(struct device *dev, struct device_attribute *attr, char *buf) { - int value = hp_wmi_read_int(HPWMI_ALS_QUERY); - if (value < 0) - return value; + int value = 0; + int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &value, + 0, sizeof(value)); + if (ret) + return ret < 0 ? ret : -EINVAL; + return sprintf(buf, "%d\n", value); } @@ -592,9 +613,12 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr, char *buf) { /* Get the POST error code of previous boot failure. */ - int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY); - if (value < 0) - return value; + int value = 0; + int ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &value, + 0, sizeof(value)); + if (ret) + return ret < 0 ? ret : -EINVAL; + return sprintf(buf, "0x%x\n", value); } @@ -609,7 +633,7 @@ static ssize_t als_store(struct device *dev, struct device_attribute *attr, return ret; ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp, - sizeof(tmp), sizeof(tmp)); + sizeof(tmp), 0); if (ret) return ret < 0 ? ret : -EINVAL; @@ -922,7 +946,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, - sizeof(state), sizeof(state)); + 0, sizeof(state)); if (err) return err < 0 ? err : -EINVAL; -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls 2022-02-18 16:09 ` [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls Jorge Lopez @ 2022-02-22 16:22 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2022-02-22 16:22 UTC (permalink / raw) To: Jorge Lopez, platform-driver-x86 Hi, On 2/18/22 17:09, Jorge Lopez wrote: > Several WMI queries leverage hp_wmi_read_int function to read their data. > hp_wmi_read_int function returns the appropiate value if the WMI command > requires an input and output buffer size values greater than zero. > WMI queries such HPWMI_HARDWARE_QUERY, HPWMI_WIRELESS2_QUERY, > HPWMI_FEATURE2_QUERY, HPWMI_DISPLAY_QUERY, HPWMI_ALS_QUERY, and > HPWMI_POSTCODEERROR_QUERY requires calling hp_wmi_perform_query function > with input buffer size value of zero. Any input buffer size greater > than zero will cause error 0x05 to be returned. > > All changes were validated on a ZBook Workstation notebook. Additional > validation was included to ensure no other commands were failing or > incorrectly handled. > > Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> So after this the only remaining callers of hp_wmi_read_int() are: 1. hp_wmi_notify() case HPWMI_BEZEL_BUTTON 2. hp_wmi_rfkill_setup() 3. thermal_profile_get() Where 2. looks like you just forgot to convert it since it does a hp_wmi_read_int(HPWMI_WIRELESS_QUERY) which you do convert in the hp_wmi_get_sw_state() and hp_wmi_get_hw_state() helpers, suggesting that it should be converted in hp_wmi_rfkill_setup() too. This leaves just 1 and 3. So IMHO it would be better to fix hp_wmi_read_int() and if 1. and 3. indeed need the old behavior convert them to call hp_wmi_perform_query() directly. Regards, Hans > > --- > Based on the latest platform-drivers-x86.git/for-next > --- > drivers/platform/x86/hp-wmi.c | 64 ++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 20 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index 544fce906ce7..de715687021a 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -442,7 +442,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, > - sizeof(state), sizeof(state)); > + 0, sizeof(state)); > if (!ret) > return 1; > > @@ -477,25 +477,37 @@ static const struct rfkill_ops hp_wmi_rfkill_ops = { > static bool hp_wmi_get_sw_state(enum hp_wmi_radio r) > { > int mask = 0x200 << (r * 8); > + int ret= 0; > + int wireless = 0; > + > + ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, > + 0, sizeof(wireless)); > > - int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY); > + if (ret < 0) > + return -EINVAL; > > /* TBD: Pass error */ > WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY"); > > - return !(wireless & mask); > + return (wireless & mask); > } > > static bool hp_wmi_get_hw_state(enum hp_wmi_radio r) > { > int mask = 0x800 << (r * 8); > + int ret= 0; > + int wireless = 0; > > - int wireless = hp_wmi_read_int(HPWMI_WIRELESS_QUERY); > + ret = hp_wmi_perform_query(HPWMI_WIRELESS_QUERY, HPWMI_READ, &wireless, > + 0, sizeof(wireless)); > + > + if (ret < 0) > + return -EINVAL; > > /* TBD: Pass error */ > WARN_ONCE(wireless < 0, "error executing HPWMI_WIRELESS_QUERY"); > > - return !(wireless & mask); > + return (wireless & mask); > } > > static int hp_wmi_rfkill2_set_block(void *data, bool blocked) > @@ -520,7 +532,7 @@ static int hp_wmi_rfkill2_refresh(void) > int err, i; > > err = hp_wmi_perform_query(HPWMI_WIRELESS2_QUERY, HPWMI_READ, &state, > - sizeof(state), sizeof(state)); > + 0, sizeof(state)); > if (err) > return err; > > @@ -546,27 +558,36 @@ static int hp_wmi_rfkill2_refresh(void) > static ssize_t display_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > - int value = hp_wmi_read_int(HPWMI_DISPLAY_QUERY); > - if (value < 0) > - return value; > + int value = 0; > + int ret = hp_wmi_perform_query(HPWMI_DISPLAY_QUERY, HPWMI_READ, &value, > + 0, sizeof(value)); > + if (ret) > + return ret < 0 ? ret : -EINVAL; > + > return sprintf(buf, "%d\n", value); > } > > static ssize_t hddtemp_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > - int value = hp_wmi_read_int(HPWMI_HDDTEMP_QUERY); > - if (value < 0) > - return value; > + int value = 0; > + int ret = hp_wmi_perform_query(HPWMI_HDDTEMP_QUERY, HPWMI_READ, &value, > + 0, sizeof(value)); > + if (ret) > + return ret < 0 ? ret : -EINVAL; > + > return sprintf(buf, "%d\n", value); > } > > static ssize_t als_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > - int value = hp_wmi_read_int(HPWMI_ALS_QUERY); > - if (value < 0) > - return value; > + int value = 0; > + int ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_READ, &value, > + 0, sizeof(value)); > + if (ret) > + return ret < 0 ? ret : -EINVAL; > + > return sprintf(buf, "%d\n", value); > } > > @@ -592,9 +613,12 @@ static ssize_t postcode_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > /* Get the POST error code of previous boot failure. */ > - int value = hp_wmi_read_int(HPWMI_POSTCODEERROR_QUERY); > - if (value < 0) > - return value; > + int value = 0; > + int ret = hp_wmi_perform_query(HPWMI_POSTCODEERROR_QUERY, HPWMI_READ, &value, > + 0, sizeof(value)); > + if (ret) > + return ret < 0 ? ret : -EINVAL; > + > return sprintf(buf, "0x%x\n", value); > } > > @@ -609,7 +633,7 @@ static ssize_t als_store(struct device *dev, struct device_attribute *attr, > return ret; > > ret = hp_wmi_perform_query(HPWMI_ALS_QUERY, HPWMI_WRITE, &tmp, > - sizeof(tmp), sizeof(tmp)); > + sizeof(tmp), 0); > if (ret) > return ret < 0 ? ret : -EINVAL; > > @@ -922,7 +946,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, > - sizeof(state), sizeof(state)); > + 0, sizeof(state)); > if (err) > return err < 0 ? err : -EINVAL; > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated 2022-02-18 16:09 [PATCH v2 0/3] Fix SW_TABLET_MODE detection method Jorge Lopez 2022-02-18 16:09 ` [PATCH v2 1/3] " Jorge Lopez 2022-02-18 16:09 ` [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls Jorge Lopez @ 2022-02-18 16:09 ` Jorge Lopez 2022-02-22 17:14 ` Hans de Goede 2 siblings, 1 reply; 7+ messages in thread From: Jorge Lopez @ 2022-02-18 16:09 UTC (permalink / raw) To: platform-driver-x86 The purpose of this patch is to remove 128 bytes buffer limitation imposed in bios_args structure. A limiting factor discovered during this investigation was the struct bios_args.data size restriction. The data member size limits all possible WMI commands to those requiring buffer size of 128 bytes or less. Several WMI commands and queries require a buffer size larger than 128 bytes hence limiting current and feature supported by the driver. It is for this reason, struct bios_args.data changed and is dynamically allocated. hp_wmi_perform_query function changed to handle the memory allocation and release of any required buffer size. All changes were validated on a ZBook Workstation notebook. Additional validation was included to ensure no other commands were failing or incorrectly handled. Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 67 +++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index de715687021a..11c8e9b6e64a 100644 --- a/drivers/platform/x86/hp-wmi.c +++ b/drivers/platform/x86/hp-wmi.c @@ -83,12 +83,17 @@ enum hp_wmi_event_ids { HPWMI_BATTERY_CHARGE_PERIOD = 0x10, }; +/** + * struct bios_args buffer is dynamically allocated. New WMI command types + * were introduced that exceeds 128-byte data size. Changes to handle + * the data size allocation scheme were kept in hp_wmi_perform_qurey function. + */ struct bios_args { u32 signature; u32 command; u32 commandtype; u32 datasize; - u8 data[128]; + u8 data[0]; }; enum hp_wmi_commandtype { @@ -258,39 +263,47 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, struct bios_return *bios_return; int actual_outsize; union acpi_object *obj; - struct bios_args args = { - .signature = 0x55434553, - .command = command, - .commandtype = query, - .datasize = insize, - .data = { 0 }, - }; - struct acpi_buffer input = { sizeof(struct bios_args), &args }; + size_t bios_args_size = sizeof(struct bios_args) + insize; + struct bios_args *args = NULL; + struct acpi_buffer input; struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; int ret = 0; - mid = encode_outsize_for_pvsz(outsize); - if (WARN_ON(mid < 0)) - return mid; + args = kmalloc(bios_args_size, GFP_KERNEL); + if (!args) + return -ENOMEM; - if (WARN_ON(insize > sizeof(args.data))) - return -EINVAL; - memcpy(&args.data[0], buffer, insize); + input.length = bios_args_size; + input.pointer = args; - wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output); + mid = encode_outsize_for_pvsz(outsize); + if (WARN_ON(mid < 0)) { + ret = mid; + goto in_free; + } - obj = output.pointer; + /* Avoid unnecessary copy to the data buffer if input buffer size is zero */ + if (insize > 0) + memcpy(args->data, buffer, insize); - if (!obj) - return -EINVAL; + args->signature = 0x55434553; + args->command = command; + args->commandtype = query; + args->datasize = insize; - if (obj->type != ACPI_TYPE_BUFFER) { + ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output); + + obj = output.pointer; + if (!obj) { + pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret); ret = -EINVAL; - goto out_free; + goto in_free; } - bios_return = (struct bios_return *)obj->buffer.pointer; - ret = bios_return->return_code; + if (!ret && obj->type == ACPI_TYPE_BUFFER) { + bios_return = (struct bios_return *)obj->buffer.pointer; + ret = bios_return->return_code; + } if (ret) { if (ret != HPWMI_RET_UNKNOWN_COMMAND && @@ -299,6 +312,12 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, goto out_free; } + if (obj->type != ACPI_TYPE_BUFFER) { + pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret); + ret = -EINVAL; + goto out_free; + } + /* Ignore output data of zero size */ if (!outsize) goto out_free; @@ -309,6 +328,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, out_free: kfree(obj); +in_free: + kfree(args); return ret; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated 2022-02-18 16:09 ` [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated Jorge Lopez @ 2022-02-22 17:14 ` Hans de Goede 0 siblings, 0 replies; 7+ messages in thread From: Hans de Goede @ 2022-02-22 17:14 UTC (permalink / raw) To: Jorge Lopez, platform-driver-x86 Hi, On 2/18/22 17:09, Jorge Lopez wrote: > The purpose of this patch is to remove 128 bytes buffer limitation > imposed in bios_args structure. > > A limiting factor discovered during this investigation was the struct > bios_args.data size restriction. The data member size limits all possible > WMI commands to those requiring buffer size of 128 bytes or less. > Several WMI commands and queries require a buffer size larger than 128 > bytes hence limiting current and feature supported by the driver. > It is for this reason, struct bios_args.data changed and is dynamically > allocated. hp_wmi_perform_query function changed to handle the memory > allocation and release of any required buffer size. > > All changes were validated on a ZBook Workstation notebook. Additional > validation was included to ensure no other commands were failing or > incorrectly handled. > > Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com> > > --- > Based on the latest platform-drivers-x86.git/for-next > --- > drivers/platform/x86/hp-wmi.c | 67 +++++++++++++++++++++++------------ > 1 file changed, 44 insertions(+), 23 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index de715687021a..11c8e9b6e64a 100644 > --- a/drivers/platform/x86/hp-wmi.c > +++ b/drivers/platform/x86/hp-wmi.c > @@ -83,12 +83,17 @@ enum hp_wmi_event_ids { > HPWMI_BATTERY_CHARGE_PERIOD = 0x10, > }; > > +/** > + * struct bios_args buffer is dynamically allocated. New WMI command types > + * were introduced that exceeds 128-byte data size. Changes to handle > + * the data size allocation scheme were kept in hp_wmi_perform_qurey function. > + */ > struct bios_args { > u32 signature; > u32 command; > u32 commandtype; > u32 datasize; > - u8 data[128]; > + u8 data[0]; > }; > > enum hp_wmi_commandtype { > @@ -258,39 +263,47 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, > struct bios_return *bios_return; > int actual_outsize; > union acpi_object *obj; > - struct bios_args args = { > - .signature = 0x55434553, > - .command = command, > - .commandtype = query, > - .datasize = insize, > - .data = { 0 }, > - }; > - struct acpi_buffer input = { sizeof(struct bios_args), &args }; > + size_t bios_args_size = sizeof(struct bios_args) + insize; > + struct bios_args *args = NULL; > + struct acpi_buffer input; > struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL }; > int ret = 0; > > - mid = encode_outsize_for_pvsz(outsize); > - if (WARN_ON(mid < 0)) > - return mid; > + args = kmalloc(bios_args_size, GFP_KERNEL); > + if (!args) > + return -ENOMEM; > > - if (WARN_ON(insize > sizeof(args.data))) > - return -EINVAL; > - memcpy(&args.data[0], buffer, insize); > + input.length = bios_args_size; > + input.pointer = args; > > - wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output); > + mid = encode_outsize_for_pvsz(outsize); > + if (WARN_ON(mid < 0)) { > + ret = mid; > + goto in_free; > + } > > - obj = output.pointer; > + /* Avoid unnecessary copy to the data buffer if input buffer size is zero */ > + if (insize > 0) This if is not necessary, memcpy should touch neither dst nor src when called with size==0. > + memcpy(args->data, buffer, insize); > > - if (!obj) > - return -EINVAL; > + args->signature = 0x55434553; > + args->command = command; > + args->commandtype = query; > + args->datasize = insize; > > - if (obj->type != ACPI_TYPE_BUFFER) { > + ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output); Add: if (ret) goto in_free; Here ? AFAIK if ret is set &output is not touched, so other wise you end up reading unitialized mem when doing: obj = output.pointer; > + if (!obj) { > + pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret); Are you sure this can never happen when e.g. querying features on older models ? I believe it is probably best to just drop this as it is an unrelated behavior change and if you really think this is a good idea, put the adding of the pr_warn in a separate commit with an explanation why this is being added. > ret = -EINVAL; > - goto out_free; > + goto in_free; > } > > - bios_return = (struct bios_return *)obj->buffer.pointer; > - ret = bios_return->return_code; > + if (!ret && obj->type == ACPI_TYPE_BUFFER) { > + bios_return = (struct bios_return *)obj->buffer.pointer; > + ret = bios_return->return_code; > + } Please keep this in the form of: if (unexpected_condition) { ret = -EINVAL; goto out_free; } straight-path-statements-here; This form is consistently used in most kernel code so that the "straight-path" code is always indented only 1 level which makes it easier to follow. And this would also mean that you end up making less changes in this patch. > > if (ret) { > if (ret != HPWMI_RET_UNKNOWN_COMMAND && > @@ -299,6 +312,12 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, > goto out_free; > } > > + if (obj->type != ACPI_TYPE_BUFFER) { > + pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret); > + ret = -EINVAL; > + goto out_free; > + } > + > /* Ignore output data of zero size */ > if (!outsize) > goto out_free; > @@ -309,6 +328,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, > > out_free: > kfree(obj); > +in_free: > + kfree(args); > return ret; IMHO it would be better to have a single "out" label here and initialize obj to NULL when declared, kfree(NULL) is a no-op, so this way you don't need 2 labels and you also avoid potentially jumping to the wrong label. Regards, Hans ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-22 17:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-18 16:09 [PATCH v2 0/3] Fix SW_TABLET_MODE detection method Jorge Lopez 2022-02-18 16:09 ` [PATCH v2 1/3] " Jorge Lopez 2022-02-22 16:13 ` Hans de Goede 2022-02-18 16:09 ` [PATCH v2 2/3] Fix 0x05 error code reported by several WMI calls Jorge Lopez 2022-02-22 16:22 ` Hans de Goede 2022-02-18 16:09 ` [PATCH v2 3/3] Changing bios_args.data to be dynamically allocated Jorge Lopez 2022-02-22 17:14 ` Hans de Goede
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.