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@xxxxxx>
>
> ---
> 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
