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

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

Hi Dmitry,

On 8/12/24 7:24 PM, Dmitry Torokhov wrote:
> On Mon, Aug 12, 2024 at 04:41:50PM +0200, Hans de Goede wrote:
>> Hi Maxim,
>>
>> On 8/12/24 4:37 PM, Maxim Mikityanskiy wrote:
>>> On Mon, 05 Aug 2024 at 17:45:19 +0200, Hans de Goede wrote:
>>>> On 8/5/24 5:30 PM, Maxim Mikityanskiy wrote:
>>>>> 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 ?
>>>
>>> *Maxim
>>
>> Oops, sorry.
>>
>>> Just a heads-up, my Z570 now belongs to a family member, we'll test what
>>> you asked, but right now there is a btrfs corruption on that laptop that
>>> we need to fix first, it interferes with kernel compilation =/
>>
>> Note as discussed in another part of the thread the original bug report
>> actually was not on a Z570, so the whole usage of i8042_command() on
>> suspend/resume was a bit of a red herring. And the suspend/resume issue
>> has been fixed in another way in the mean time.
>>
>> So there really is no need to test this change anymore. At the moment
>> there are no planned changes to ideapad-laptop related to this.
> 
> I think we still need to stop ideapad-laptop poking into 8042,
> especially ahead of time.

I agree. I think your suggestion of using the new(ish) [un]inhibit
support in the input subsystem for this instead of poking at the i8042
is a good idea.

As I mentioned when you first suggested this, 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?)

> If we do not want to wait for userspace to
> handle this properly, I wonder if we could not create an
> input_handler that would attach to the touchpad device and filter out
> all events coming from the touchpad if touchpad is supposed to be off.

I think using the inhibit stuff would be better no?

The biggest problems with trying to fix this are:

1. Finding time to work on this
2. Finding someone willing to test the patches

Finding the time is going to be an issue for me since the i8042_command()
calls are only still done on a single model laptop (using a DMI quirk)
inside ideapad-laptop now, so this is pretty low priority IMHO. Which
in practice means that I will simply never get around to this, sorry...

Regards,

Hans