| Message ID | 20210120124941.73409-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: hp-wmi: Disable tablet-mode reporting by default | expand |
Hi, Thank you for the review. Unrelated, but I would really appreciate a review of the series stating with: "[PATCH 1/4] platform/x86: intel-vbtn: Rework wakeup handling in notify_handler()" On 1/20/21 2:10 PM, Andy Shevchenko wrote: > On Wed, Jan 20, 2021 at 2:49 PM Hans de Goede <hdegoede@redhat.com> 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. >> >> 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, > > claiming Ack, I will fix this up before merging this into review-hans. >> 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 >> 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 > > triplett Ack, I will fix this up before merging this into review-hans. >> better way to detect SW_TABLET_MODE support. ATM the default >> auto option just does the same as off. > > ... > >> +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)"); > > Does it need any documentation update? > There is no documentation for this driver: [hans@x1 linux]$ find Documentation -name '*wmi*' Documentation/ABI/testing/sysfs-platform-intel-wmi-sbl-fw-update Documentation/ABI/testing/sysfs-platform-asus-wmi Documentation/ABI/testing/dell-smbios-wmi Documentation/ABI/testing/sysfs-platform-intel-wmi-thunderbolt > ... > >> + if (enable_tablet_mode_sw > 0) { >> + val = hp_wmi_hw_state(HPWMI_TABLET_MASK); > >> + if (!(val < 0)) { > > Perhaps while at it you may change it to more natural > if (val >= 0) > ? Ack, I will fix this up before merging this into review-hans. Note I plan to let this patch sit for a while before merging it to give people time to complain about me just outright disabing the SW_TABLET_MODE reporting by default. Regards, Hans > >> + __set_bit(SW_TABLET_MODE, hp_wmi_input_dev->swbit); >> + input_report_switch(hp_wmi_input_dev, SW_TABLET_MODE, val); >> + } >> } >
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"? > > 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. --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 >
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. 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 >> >
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 > >> > > >
Hi, On 1/25/21 8:43 PM, mark gross wrote: > 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> Thank you. I've applied this patch to my review-hans branch now, so this will show up in for-next and fixes soon. Regards, Hans
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; +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);
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. 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 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(-)