Re: [PATCH] Process tablet mode events on Dell Inspiron 2n1

[Pali Rohár]

On Monday 13 September 2021 14:48:46 Hans de Goede wrote:
> Hi,
> 
> On 9/13/21 2:44 PM, Pali Rohár wrote:
> > On Monday 13 September 2021 14:40:28 Hans de Goede wrote:
> >> Hi,
> >>
> >> Overall this patch looks good, I have 2 remarks:
> >>
> >> 1. Your commit message needs some work, the first line (Subject) should
> >> briefly describe the change with a subsystem prefix, so something like this:
> >>
> >> platform/x86: dell-wmi: Add SW_TABLET_MODE support for Dell Inspiron 2in1
> >>
> >> And then an empty line followed by a "body" with a bit more detailed description.
> >>
> >> And last but not least you need to add a Signed-off-by line like this:
> >>
> >> Singed-off-by: Troy Rollo <linux2021@xxxxxxxxxxxxxxx>
> >>
> >> Which indicates that you authored the patch and are submitting it under
> >> the standard kernel license, see:
> >> https://elinux.org/Developer_Certificate_Of_Origin
> >>
> >> 2. Also have 1 remark about the code, see below.
> >>
> >>
> >> On 9/9/21 5:35 AM, Troy Rollo wrote:
> >>> ---
> >>>  drivers/platform/x86/dell/dell-wmi-base.c | 36 ++++++++++++++++-------
> >>>  1 file changed, 26 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/dell/dell-wmi-base.c b/drivers/platform/x86/dell/dell-wmi-base.c
> >>> index 089c125e18f7..474ca05055ab 100644
> >>> --- a/drivers/platform/x86/dell/dell-wmi-base.c
> >>> +++ b/drivers/platform/x86/dell/dell-wmi-base.c
> >>> @@ -309,6 +309,9 @@ static const struct key_entry dell_wmi_keymap_type_0010[] = {
> >>>   * Keymap for WMI events of type 0x0011
> >>>   */
> >>>  static const struct key_entry dell_wmi_keymap_type_0011[] = {
> >>> +	/* Reflex keyboard switch on 2n1 devices */
> >>> +	{ KE_VSW,    0xe070, { .sw  = { SW_TABLET_MODE } } },
> >>> +
> >>
> >> By adding this here, any device loading the dell-wmi driver will now advertise
> >> that it supports SW_TABLET_MODE reporting, even when it does not. This will
> >> cause e.g. GNOME40 to disable accelerometer based rotation since when
> >> SW_TABLET_MODE=0 is being reported then GNOME40 assumes it is running on a
> >> 2in1 in laptop mode and thus disable auto-rotation (and also the on-screen-kbd).
> >>
> >> So advertising a non functional (always reporting 0) SW_TABLET_MODE switch
> >> is harmful and this change will do this.
> > 
> > Good catch. This code needs some rework as based on above gnome
> > behavior, this change breaks support for all existing older Dell
> > laptops.
> > 
> > It it problem only with SW_TABLET_MODE? Or with any KE_VSW?
> 
> It is a problem for any KE_SW / KE_VSW since unlike keys/buttons which
> are always unpressed by default and don't cause userspace to do anything
> until a press is reported, switches have a meaning in both their
> 0 and 1 state, so merely reporting the presence of a switch can cause
> userspace to change behavior.
> 
> Note I'm not aware of any SW other then SW_TABLET_MODE actually causing
> issues, but in theory this is an issue with any SW because of the reason
> which I just gave.

Well, this argument makes sense for any switch.

But all other defined keys in this driver are "standard" keys which may
or may not be present on de-facto standard keyboard: capslock,
brightness keys, volume keys, wifi key, eject key, ...
Probably generic AT keyboard driver may also report more standard keys
than it has.

So I'm not sure if it makes sense to rewrite wmi driver to dynamically
allocate presence of also every key based on first press.

If some userspace software depends on capabilities of "standard" keys
and reacts differently then I guess it is broken...

But for switches this dynamic behavior as you described is needed.

> Regards,
> 
> Hans
> 
> 
> > 
> >> Since you already have special handling for the tablet-mode WMI events,
> >> it would be better to use a separate input-device which just reports
> >> SW_TABLET_MODE and which gets dynamically created upon receiving the
> >> first 0x0011 0xe070 event. See the
> >> drivers/platform/x86/intel/vbtn.c
> >> drivers/platform/x86/intel/hid.c
> >>
> >> Drivers for an example of 2 drivers already doing this.
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>>  	/* Battery unplugged */
> >>>  	{ KE_IGNORE, 0xfff0, { KEY_RESERVED } },
> >>>  
> >>> @@ -344,17 +347,19 @@ static const struct key_entry dell_wmi_keymap_type_0012[] = {
> >>>  	{ KE_IGNORE, 0xe035, { KEY_RESERVED } },
> >>>  };
> >>>  
> >>> -static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
> >>> +static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16 *buffer, int remaining)
> >>>  {
> >>>  	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
> >>>  	const struct key_entry *key;
> >>> +	int used = 0;
> >>> +	int value = 1;
> >>>  
> >>>  	key = sparse_keymap_entry_from_scancode(priv->input_dev,
> >>>  						(type << 16) | code);
> >>>  	if (!key) {
> >>>  		pr_info("Unknown key with type 0x%04x and code 0x%04x pressed\n",
> >>>  			type, code);
> >>> -		return;
> >>> +		return 0;
> >>>  	}
> >>>  
> >>>  	pr_debug("Key with type 0x%04x and code 0x%04x pressed\n", type, code);
> >>> @@ -363,16 +368,22 @@ static void dell_wmi_process_key(struct wmi_device *wdev, int type, int code)
> >>>  	if ((key->keycode == KEY_BRIGHTNESSUP ||
> >>>  	     key->keycode == KEY_BRIGHTNESSDOWN) &&
> >>>  	    acpi_video_handles_brightness_key_presses())
> >>> -		return;
> >>> +		return 0;
> >>>  
> >>>  	if (type == 0x0000 && code == 0xe025 && !wmi_requires_smbios_request)
> >>> -		return;
> >>> +		return 0;
> >>>  
> >>> -	if (key->keycode == KEY_KBDILLUMTOGGLE)
> >>> +	if (key->keycode == KEY_KBDILLUMTOGGLE) {
> >>>  		dell_laptop_call_notifier(
> >>>  			DELL_LAPTOP_KBD_BACKLIGHT_BRIGHTNESS_CHANGED, NULL);
> >>> +	} else if (type == 0x0011 && code == 0xe070 && remaining > 0) {
> >>> +		value = !buffer[0];
> >>> +		used = 1;
> >>> +	}
> >>> +
> >>> +	sparse_keymap_report_entry(priv->input_dev, key, value, true);
> >>>  
> >>> -	sparse_keymap_report_entry(priv->input_dev, key, 1, true);
> >>> +	return used;
> >>>  }
> >>>  
> >>>  static void dell_wmi_notify(struct wmi_device *wdev,
> >>> @@ -430,21 +441,26 @@ static void dell_wmi_notify(struct wmi_device *wdev,
> >>>  		case 0x0000: /* One key pressed or event occurred */
> >>>  			if (len > 2)
> >>>  				dell_wmi_process_key(wdev, buffer_entry[1],
> >>> -						     buffer_entry[2]);
> >>> +						     buffer_entry[2],
> >>> +						     buffer_entry + 3,
> >>> +						     len - 3);
> >>>  			/* Extended data is currently ignored */
> >>>  			break;
> >>>  		case 0x0010: /* Sequence of keys pressed */
> >>>  		case 0x0011: /* Sequence of events occurred */
> >>>  			for (i = 2; i < len; ++i)
> >>> -				dell_wmi_process_key(wdev, buffer_entry[1],
> >>> -						     buffer_entry[i]);
> >>> +				i += dell_wmi_process_key(wdev, buffer_entry[1],
> >>> +							  buffer_entry[i],
> >>> +							  buffer_entry + i,
> >>> +							  len - i - 1);
> >>>  			break;
> >>>  		case 0x0012:
> >>>  			if ((len > 4) && dell_privacy_process_event(buffer_entry[1], buffer_entry[3],
> >>>  								    buffer_entry[4]))
> >>>  				/* dell_privacy_process_event has handled the event */;
> >>>  			else if (len > 2)
> >>> -				dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2]);
> >>> +				dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2],
> >>> +						     buffer_entry + 3, len - 3);
> >>>  			break;
> >>>  		default: /* Unknown event */
> >>>  			pr_info("Unknown WMI event type 0x%x\n",
> >>>
> >>
> > 
>