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

[Hans de Goede <hdegoede@redhat.com>]

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);
>> +               }
>>         }
>