Re: [PATCH] platform/x86: ideapad-laptop: Stop calling i8042_command()

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

Hi Maxim,

On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote:
> On Mon, 05 Aug 2024 at 16:16:08 +0200, Hans de Goede wrote:
>> Commit 07a4a4fc83dd ("ideapad: add Lenovo IdeaPad Z570 support (part 2)")
>> added an i8042_command(..., I8042_CMD_AUX_[EN|DIS]ABLE) call to
>> the ideapad-laptop driver to suppress the touchpad events at the PS/2
>> AUX controller level.
>>
>> Commit c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux
>> port on/off on select models") limited this to only do this by default
>> on the IdeaPad Z570 to replace a growing list of models on which
>> the i8042_command() call was disabled by quirks because it was causing
>> issues.
>>
>> A recent report shows that this is causing issues even on the Z570 for
>> which it was originally added because it can happen on resume before
>> the i8042 controller's own resume() method has run:
>>
>> [   50.241235] ideapad_acpi VPC2004:00: PM: calling acpi_subsys_resume+0x0/0x5d @ 4492, parent: PNP0C09:00
>> [   50.242055] snd_hda_intel 0000:00:0e.0: PM: pci_pm_resume+0x0/0xed returned 0 after 13511 usecs
>> [   50.242120] snd_hda_codec_realtek hdaudioC0D0: PM: calling hda_codec_pm_resume+0x0/0x19 [snd_hda_codec] @ 4518, parent: 0000:00:0e.0
>> [   50.247406] i8042: [49434] a8 -> i8042 (command)
>> [   50.247468] ideapad_acpi VPC2004:00: PM: acpi_subsys_resume+0x0/0x5d returned 0 after 6220 usecs
>> ...
>> [   50.247883] i8042 kbd 00:01: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0
>> [   50.247894] i8042 kbd 00:01: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs
>> [   50.247906] i8042 aux 00:02: PM: calling pnp_bus_resume+0x0/0x9d @ 4492, parent: pnp0
>> [   50.247916] i8042 aux 00:02: PM: pnp_bus_resume+0x0/0x9d returned 0 after 0 usecs
>> ...
>> [   50.248301] i8042 i8042: PM: calling platform_pm_resume+0x0/0x41 @ 4492, parent: platform
>> [   50.248377] i8042: [49434] 55 <- i8042 (flush, kbd)
>> [   50.248407] i8042: [49435] aa -> i8042 (command)
>> [   50.248601] i8042: [49435] 00 <- i8042 (return)
>> [   50.248604] i8042: [49435] i8042 controller selftest: 0x0 != 0x55
> 
> What exactly is the issue? Is it just a few errors in dmesg, or does
> 8042 stop responding completely?

When this issue happens at resume the touchpad stops sending events
completely because the i8042 driver's resume() method fails and exits
early.

> 
> I've seen something similar when I enabled the touchpad while moving the
> cursor, but it was just a matter of a few lines in dmesg and a protocol
> resync, both touchpad and keyboard worked after that.

Right, the problem is that in this case the i8042's resume() method
is failing, which I believe causes the Elan ps/2 driver to not get
re-attached to the aux port on resume.


> 
>> Dmitry (input subsys maintainer) pointed out that just sending
>> KEY_TOUCHPAD_OFF/KEY_TOUCHPAD_ON which the ideapad-laptop driver
>> already does should be sufficient and that it then is up to userspace
>> to filter out touchpad events after having received a KEY_TOUCHPAD_OFF.
> 
> I believe it's not the case (at least it wasn't back then). The whole
> point of my patch in the first place was to make touchpad toggle work
> properly on Z570.
> 
> Userspace (GNOME) supports two variants of hardware:
> 
> 1. Laptops that disable touchpad themselves and send out
> KEY_TOUCHPAD_ON/OFF to report the status. Upon receiving these keycodes,
> GNOME just shows the status pop-up and relies on firmware to disable the
> touchpad.
> 
> 2. Laptops that just send KEY_TOUCHPAD_TOGGLE whenever the key is
> pressed. GNOME maintains its own touchpad state and disables it in
> software (as well as showing the pop-up).

You're right I had forgotten about this. There is really no reason
why GNOME cannot also suppress events after a TOUCHPAD_OFF event,
but atm it indeed does not do this. We could fix this by patching:
plugins/media-keys/gsd-media-keys-manager.c of gnome-settings-daemon
to also update the TOUCHPAD_ENABLED_KEY setting when receiving
KEY_TOUCHPAD_ON/OFF. Something which I think we should do to,
but that will not help solve this bug since we cannot rely
on users having a fixed g-s-d.

So: self-NACK for this patch. (which is a bummer because I really
liked being able to just remove this)

> That means, userspace is not filtering out events upon receiving
> KEY_TOUCHPAD_OFF. If we wanted to rely on that, we would need to send
> KEY_TOUCHPAD_TOGGLE from the driver, but we actually can't, because Z570
> is weird. It maintains the touchpad state in firmware to light up the
> status LED, but the firmware doesn't do the actual touchpad disablement.
> 
> That is, if we use TOGGLE, the LED will get out of sync. If we use
> ON/OFF, the touchpad won't be disabled, unless we do it in the kernel.

Ack.

So how about this instead:

diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
index 1ace711f7442..b7fa06f793cb 100644
--- a/drivers/platform/x86/ideapad-laptop.c
+++ b/drivers/platform/x86/ideapad-laptop.c
@@ -1574,7 +1574,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
 	 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
 	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
 	 */
-	if (priv->features.ctrl_ps2_aux_port)
+	if (send_events && priv->features.ctrl_ps2_aux_port)
 		i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
 
 	/*

Maxmime, if you still have your Z570 can you check if the touchpad state after a suspend/resume
correctly reflects the state before suspend/resume in both touchpad on / off states ?

Jonathan, as the reporter of the original suspend/resume issue, can you check if
a kernel with this patch + ideapad-laptop re-enabled no longer has the suspend/resume
issue you were seeing ?

Regards,

Hans





> 
>> Given all the problems the i8042_command() call has been causing just
>> removing it indeed seems best, so this removes it completely. Note that
>> this only impacts the Ideapad Z570 since it was already disabled by
>> default on all other models.
> 
> While I agree that i8042_command() is not a perfect solution, I don't
> like the idea of breaking the touchpad toggle, even if "only one" laptop
> model is affected. Can we suppress input events from the touchpad in
> some other way, purely in software? I.e. don't call i8042_command(),
> don't disrupt the PS/2 protocol, but instead let ideapad-laptop tell
> psmouse to stop generating input events for a while?
> 
>> Fixes: c69e7d843d2c ("platform/x86: ideapad-laptop: Only toggle ps2 aux port on/off on select models")
>> Reported-by: Jonathan Denose <jdenose@chromium.org>
>> Closes: https://lore.kernel.org/linux-input/20231102075243.1.Idb37ff8043a29f607beab6440c32b9ae52525825@changeid/
>> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Maxim Mikityanskiy <maxtram95@gmail.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/ideapad-laptop.c | 37 ---------------------------
>>  1 file changed, 37 deletions(-)
>>
>> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
>> index 1ace711f7442..255fb56ec9ee 100644
>> --- a/drivers/platform/x86/ideapad-laptop.c
>> +++ b/drivers/platform/x86/ideapad-laptop.c
>> @@ -18,7 +18,6 @@
>>  #include <linux/device.h>
>>  #include <linux/dmi.h>
>>  #include <linux/fb.h>
>> -#include <linux/i8042.h>
>>  #include <linux/init.h>
>>  #include <linux/input.h>
>>  #include <linux/input/sparse-keymap.h>
>> @@ -144,7 +143,6 @@ struct ideapad_private {
>>  		bool hw_rfkill_switch     : 1;
>>  		bool kbd_bl               : 1;
>>  		bool touchpad_ctrl_via_ec : 1;
>> -		bool ctrl_ps2_aux_port    : 1;
>>  		bool usb_charging         : 1;
>>  	} features;
>>  	struct {
>> @@ -182,12 +180,6 @@ MODULE_PARM_DESC(set_fn_lock_led,
>>  	"Enable driver based updates of the fn-lock LED on fn-lock changes. "
>>  	"If you need this please report this to: platform-driver-x86@vger.kernel.org");
>>  
>> -static bool ctrl_ps2_aux_port;
>> -module_param(ctrl_ps2_aux_port, bool, 0444);
>> -MODULE_PARM_DESC(ctrl_ps2_aux_port,
>> -	"Enable driver based PS/2 aux port en-/dis-abling on touchpad on/off toggle. "
>> -	"If you need this please report this to: platform-driver-x86@vger.kernel.org");
>> -
>>  static bool touchpad_ctrl_via_ec;
>>  module_param(touchpad_ctrl_via_ec, bool, 0444);
>>  MODULE_PARM_DESC(touchpad_ctrl_via_ec,
>> @@ -1560,7 +1552,6 @@ static void ideapad_fn_lock_led_exit(struct ideapad_private *priv)
>>  static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_events)
>>  {
>>  	unsigned long value;
>> -	unsigned char param;
>>  	int ret;
>>  
>>  	/* Without reading from EC touchpad LED doesn't switch state */
>> @@ -1568,15 +1559,6 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
>>  	if (ret)
>>  		return;
>>  
>> -	/*
>> -	 * Some IdeaPads don't really turn off touchpad - they only
>> -	 * switch the LED state. We (de)activate KBC AUX port to turn
>> -	 * touchpad off and on. We send KEY_TOUCHPAD_OFF and
>> -	 * KEY_TOUCHPAD_ON to not to get out of sync with LED
>> -	 */
>> -	if (priv->features.ctrl_ps2_aux_port)
>> -		i8042_command(&param, value ? I8042_CMD_AUX_ENABLE : I8042_CMD_AUX_DISABLE);
>> -
>>  	/*
>>  	 * On older models the EC controls the touchpad and toggles it on/off
>>  	 * itself, in this case we report KEY_TOUCHPAD_ON/_OFF. Some models do
>> @@ -1699,23 +1681,6 @@ static const struct dmi_system_id hw_rfkill_list[] = {
>>  	{}
>>  };
>>  
>> -/*
>> - * On some models the EC toggles the touchpad muted LED on touchpad toggle
>> - * hotkey presses, but the EC does not actually disable the touchpad itself.
>> - * On these models the driver needs to explicitly enable/disable the i8042
>> - * (PS/2) aux port.
>> - */
>> -static const struct dmi_system_id ctrl_ps2_aux_port_list[] = {
>> -	{
>> -	/* Lenovo Ideapad Z570 */
>> -	.matches = {
>> -		DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
>> -		DMI_MATCH(DMI_PRODUCT_VERSION, "Ideapad Z570"),
>> -		},
>> -	},
>> -	{}
>> -};
>> -
>>  static void ideapad_check_features(struct ideapad_private *priv)
>>  {
>>  	acpi_handle handle = priv->adev->handle;
>> @@ -1725,8 +1690,6 @@ static void ideapad_check_features(struct ideapad_private *priv)
>>  		set_fn_lock_led || dmi_check_system(set_fn_lock_led_list);
>>  	priv->features.hw_rfkill_switch =
>>  		hw_rfkill_switch || dmi_check_system(hw_rfkill_list);
>> -	priv->features.ctrl_ps2_aux_port =
>> -		ctrl_ps2_aux_port || dmi_check_system(ctrl_ps2_aux_port_list);
>>  	priv->features.touchpad_ctrl_via_ec = touchpad_ctrl_via_ec;
>>  
>>  	if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
>> -- 
>> 2.45.2
>>
>