On Mon, Aug 05, 2024 at 05:45:19PM +0200, Hans de Goede wrote:
> 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.
We actually retry up to 5 times so we usually get the right response
from the controller. Additionally on x86 we do not abort
initialization/resume even if controller selftest still fails after all
the retries.
>
> >
> > 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.
There's a69ce592cbe0 ("Input: elantech - fix touchpad state on resume
for Lenovo N24") that sends disable/enable pair as part of Elan touchpad
resume handling which unwedges the touchpad.
>
>
> >
> >> 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.
Still, poking the touchpad directly at a random time is not something
that we should be doing. The command may come in the middle of touchpad
initialization or in the middle of resuming, or at another inopportune
moment - as you mentioned yourself toggling while using the touchpad
results in a spew in dmesg.
We have "inhibit/uninhibit" sysfs controls that allow suppressing input
events form a device, they should be used instead.
>
> 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(¶m, 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
>
Thanks.
--
Dmitry
