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

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

Hi,

On 8/5/24 7:00 PM, Dmitry Torokhov wrote:
> 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.

Using those indeed would be better, I guess this requires 2 things:

1. Some helper to find the struct input_dev for the input_dev related
   to the ps/2 aux port
2. In kernel API / functions to do inhibit/uninhibit
   (maybe these already exist?)

Anyways I have to focus on camera stuff for the rest of this week,
so lets continue this discussion next week.

Regards,

Hans