Re: [PATCH] platform/x86: hp-wmi: Disable tablet-mode reporting by default

[mark gross <mgross@linux.intel.com>]

On Fri, Jan 22, 2021 at 01:15:05AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 1/22/21 12:48 AM, mark gross wrote:
> > On Wed, Jan 20, 2021 at 01:49:41PM +0100, Hans de Goede wrote:
> >> Recently userspace has started making more use of SW_TABLET_MODE
> >> (when an input-dev reports this).
> >>
> >> Specifically recent GNOME3 versions will:
> >>
> >> 1.  When SW_TABLET_MODE is reported and is reporting 0:
> >> 1.1 Disable accelerometer-based screen auto-rotation
> >> 1.2 Disable automatically showing the on-screen keyboard when a
> >>     text-input field is focussed
> >>
> >> 2.  When SW_TABLET_MODE is reported and is reporting 1:
> >> 2.1 Ignore input-events from the builtin keyboard and touchpad
> >>     (this is for 360° hinges style 2-in-1s where the keyboard and
> >>      touchpads are accessible on the back of the tablet when folded
> >>      into tablet-mode)
> >>
> >> This means that claiming to support SW_TABLET_MODE when it does not
> >> actually work / reports correct values has bad side-effects.
> > did you mean "reports incorrect values"?
> 
> Yes and no, I meant this to be read as "does not (...) report correct values"
> but your suggestion is better I will fix this for v2.
> 
> > 
> >>
> >> The check in the hp-wmi code which is used to decide if the input-dev
> >> should claim SW_TABLET_MODE support, only checks if the
> >> HPWMI_HARDWARE_QUERY is supported. It does *not* check if the hardware
> >> actually is capable of reporting SW_TABLET_MODE.
> >>
> >> This leads to the hp-wmi input-dev claming SW_TABLET_MODE support,
> >> while in reality it will always report 0 as SW_TABLET_MODE value.
> >> This has been seen on a "HP ENVY x360 Convertible 15-cp0xxx" and
> >> this likely is the case on a whole lot of other HP models.
> >>
> >> This problem causes both auto-rotation and on-screen keyboard
> >> support to not work on affected x360 models.
> >>
> >> There is no easy fix for this, but since userspace expects
> >> SW_TABLET_MODE reporting to be reliable when advertised it is
> >> better to not claim/report SW_TABLET_MODE support at all, then
> >                                                             than
> >> to claim to support it while it does not work.
> >>
> >> To avoid the mentioned problems, add a new enable_tablet_mode_sw
> >> module-parameter which defaults to false.
> >>
> >> Note I've made this an int using the standard -1=auto, 0=off, 1=on
> >> tripplet, with the hope that in the future we can come up with a
> >> better way to detect SW_TABLET_MODE support. ATM the default
> >> auto option just does the same as off.
> >>
> >> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1918255
> >> Cc: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/hp-wmi.c | 14 ++++++++++----
> >>  1 file changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> >> index 18bf8aeb5f87..ff028587cd21 100644
> >> --- a/drivers/platform/x86/hp-wmi.c
> >> +++ b/drivers/platform/x86/hp-wmi.c
> >> @@ -32,6 +32,10 @@ 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;
> > So busted HW gets the default while working HW will need to add a boot time
> > parameter.  If there are no working tablet_mode devices I guess its ok but, if
> > I had a working platform I'd be a little miffed at the choice to make my life
> > harder (by forcing me to add a enable_tablet_mode_sw=1 to my kernel
> > command line) while making life easier for those with busted hardware.
> > 
> > I'm not saying change it but, it should be considered.
> 
> Until recently userspace pretty much ignored SW_TABLET_MODE, so reporting
> it while it did not work was not a big deal, but as I tried to explain
> in the commit message always reporting SW_TABLET_MODE=0 does cause some
> real issues:
> 
> >> 1.  When SW_TABLET_MODE is reported and is reporting 0:
> >> 1.1 Disable accelerometer-based screen auto-rotation
> >> 1.2 Disable automatically showing the on-screen keyboard when a
> >>     text-input field is focussed
> 
> By defaulting to not reporting SW_TABLET_MODE at all we go back to the
> (slightly) older userspace behavior of always doing auto-rotation and
> always popping up the onscreen-keyboard on text-field focus (on devices
> with a touchscreen).
> 
> So basically the bad side-effects of reporting SW_TABLET_MODE while it
> is not working are much worse then the bad side-effects of not reporting
> it on devices where it does work.
> 
> More in general the way userspace uses SW_TABLET_MODE means that if
> we report it, then it MUST be reliable. If it is not reliable then it
> is better to not support it at all.
Yeah, thats all too true as well.  Making user mode work correctly takes
priority over my sense of fairness.

Acked-by: Mark Gross <mgross@linux.intel.com>


--mark

> 
> For this reason the intel-vbtn, intel-hid and asus-wmi driver have all
> 3 already been moved over to using a DMI based whitelist. And now I
> guess it is hp-wmi's turn to follow in their footsteps.
> 
> Ideally there is some WMI query other then the HPWMI_HARDWARE_QUERY
> which actually tells us which bits in the HPWMI_HARDWARE_QUERY result
> are valid and which bits are simply always 0.  I hope someone who
> actually has this hardware can spend some time figuring this out.
> 
> In the mean time disabling SW_TABLET_MODE reporting is the safe
> fallback option; and if people come forward where this does work
> then we can do a DMI based whitelist (*).
> 
> Regards,
> 
> Hans
> 
> *) Which will hopefully be a temporary solution but there is absolutely 
> no documentation for all this crap and most vendors don't seem to care
> about helping us with this, so...
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> > 
> > --mark
> > 
> > 
> >> +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"
> >>  
> >> @@ -654,10 +658,12 @@ static int __init hp_wmi_input_setup(void)
> >>  	}
> >>  
> >>  	/* Tablet mode */
> >> -	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);
> >> +	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);
> >> +			input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val);
> >> +		}
> >>  	}
> >>  
> >>  	err = sparse_keymap_setup(hp_wmi_input_dev, hp_wmi_keymap, NULL);
> >> -- 
> >> 2.28.0
> >>
> > 
>