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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Mon, Aug 12, 2024 at 08:18:24PM +0200, Hans de Goede wrote:
> 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 issue with inhibit/uninhibit is that they are only exposed to
userpsace via sysfs. And as you mentioned we need to locate the input
device corresponding to the touchpad.

With input handler we are essentially getting both - psmouse does not do
anything special in inhibit so it is just the input core dropping
events, the same as with the filter handler, and we can use hanlder's
match table to limit it to the touchpad and input core will find the
device for us.

> 
> 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...

Yeah, I can see that ;) Maybe I will find a couple of hours to waste...

Thanks.

-- 
Dmitry