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

[Maxim Mikityanskiy <maxtram95@gmail.com>]

On Sun, 18 Aug 2024 at 13:30:37 -0700, Dmitry Torokhov wrote:
> On Mon, Aug 12, 2024 at 12:26:47PM -0700, Dmitry Torokhov wrote:
> > 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...
> 
> Maybe something like below can work?

Great patch, thank you, I'll test it and report the results. See some
minor comments below.

> 
> 
> platform/x86: ideapad-laptop: do not poke keyboard controller
> 
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> On Ideapad Z570 the driver tries to disable and reenable data coming
> from the touchpad by poking directly into 8042 keyboard controller.
> This may coincide with the controller resuming and leads to spews in
> dmesg and potentially other instabilities.
> 
> Instead of using i8042_command() to control the touchpad state create a
> input handler that serves as a filter and drop events coming from the
> touchpad when it is supposed to be off.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/platform/x86/ideapad-laptop.c |  171 ++++++++++++++++++++++++++++++++-
>  1 file changed, 168 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index fcf13d88fd6e..2f40feefd5e3 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -17,7 +17,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>
> @@ -157,6 +156,13 @@ struct ideapad_private {
>  		struct led_classdev led;
>  		unsigned int last_brightness;
>  	} fn_lock;
> +	struct {
> +		bool initialized;
> +		bool active;
> +		struct input_handler handler;
> +		struct input_dev *tp_dev;
> +		spinlock_t lock;
> +	} tp_switch;
>  };
>  
>  static bool no_bt_rfkill;
> @@ -1236,6 +1242,158 @@ static void ideapad_check_special_buttons(struct ideapad_private *priv)
>  	}
>  }
>  
> +struct ideapad_tpswitch_handle {
> +	struct input_handle handle;
> +	struct ideapad_private *priv;
> +};
> +
> +#define to_tpswitch_handle(h) \
> +	container_of(h, struct ideapad_tpswitch_handle, handle);
> +
> +static int ideapad_tpswitch_connect(struct input_handler *handler,
> +				    struct input_dev *dev,
> +				    const struct input_device_id *id)
> +{
> +	struct ideapad_private *priv =
> +		container_of(handler, struct ideapad_private, tp_switch.handler);
> +	struct ideapad_tpswitch_handle *h;
> +	int error;
> +
> +	h = kzalloc(sizeof(*h), GFP_KERNEL);
> +	if (!h)
> +		return -ENOMEM;
> +
> +	h->priv = priv;
> +	h->handle.dev = dev;
> +	h->handle.handler = handler;
> +	h->handle.name = "ideapad-tpswitch";
> +
> +	error = input_register_handle(&h->handle);
> +	if (error)
> +		goto err_free_handle;
> +
> +	/*
> +	 * FIXME: ideally we do not want to open the input device here
> +	 * if there are no other users. We need a notion of "observer"
> +	 * handlers in the input core.
> +	 */
> +	error = input_open_device(&h->handle);
> +	if (error)
> +		goto err_unregister_handle;
> +
> +	scoped_guard(spinlock_irq, &priv->tp_switch.lock)
> +		priv->tp_switch.tp_dev = dev;
> +
> +	return 0;
> +
> + err_unregister_handle:
> +	input_unregister_handle(&h->handle);
> +err_free_handle:
> +	kfree(h);
> +	return error;
> +}
> +
> +static void ideapad_tpswitch_disconnect(struct input_handle *handle)
> +{
> +	struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle);
> +	struct ideapad_private *priv = h->priv;
> +
> +	scoped_guard(spinlock_irq, &priv->tp_switch.lock)

Nice syntax, I didn't know about it before.

> +		priv->tp_switch.tp_dev = NULL;
> +
> +	input_close_device(handle);
> +	input_unregister_handle(handle);
> +	kfree(h);
> +}
> +
> +static bool ideapad_tpswitch_filter(struct input_handle *handle,
> +				    unsigned int type, unsigned int code,
> +				    int value)
> +{
> +	struct ideapad_tpswitch_handle *h = to_tpswitch_handle(handle);
> +	struct ideapad_private *priv = h->priv;
> +
> +	if (!priv->tp_switch.active)

This check seems inverted. ideapad_tpswitch_toggle assigns true when the
touchpad is enabled.

> +		return false;
> +
> +	/* Allow passing button release events, drop everything else */
> +	return !(type == EV_KEY && value == 0) &&
> +	       !(type == EV_SYN && code == SYN_REPORT);
> +
> +}
> +
> +static const struct input_device_id ideapad_tpswitch_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT |
> +				INPUT_DEVICE_ID_MATCH_KEYBIT |
> +				INPUT_DEVICE_ID_MATCH_ABSBIT,
> +		.bustype = BUS_I8042,
> +		.vendor = 0x0002,
> +		.evbit = { BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS) },
> +		.keybit = { [BIT_WORD(BTN_TOOL_FINGER)] =
> +				BIT_MASK(BTN_TOOL_FINGER) },
> +		.absbit = { BIT_MASK(ABS_X) | BIT_MASK(ABS_Y) |
> +				BIT_MASK(ABS_PRESSURE) |
> +				BIT_MASK(ABS_TOOL_WIDTH) },
> +	},
> +	{ }
> +};
> +
> +static int ideapad_tpswitch_init(struct ideapad_private *priv)
> +{
> +	int error;
> +
> +	if (!priv->features.ctrl_ps2_aux_port)

Nit: the comment above ctrl_ps2_aux_port and the MODULE_PARAM_DESC
should be altered, because it no longer disables PS/2 AUX, but just
filters the events on software level.

Not sure whether we want to keep the old name for the module parameter.
I think it's better to keep it, because it essentially serves the same
purpose, but the implementation is better.

> +		return 0;
> +
> +	spin_lock_init(&priv->tp_switch.lock);
> +
> +	priv->tp_switch.handler.name = "ideapad-tpswitch";
> +	priv->tp_switch.handler.id_table = ideapad_tpswitch_ids;
> +	priv->tp_switch.handler.filter = ideapad_tpswitch_filter;
> +	priv->tp_switch.handler.connect = ideapad_tpswitch_connect;
> +	priv->tp_switch.handler.disconnect = ideapad_tpswitch_disconnect;
> +
> +	error = input_register_handler(&priv->tp_switch.handler);
> +	if (error) {
> +		dev_err(&priv->platform_device->dev,
> +			"failed to register touchpad switch handler: %d",
> +			error);
> +		return error;
> +	}
> +
> +	priv->tp_switch.initialized = true;
> +	return 0;
> +}
> +
> +static void ideapad_tpswitch_exit(struct ideapad_private *priv)
> +{
> +	if (priv->tp_switch.initialized) {
> +		input_unregister_handler(&priv->tp_switch.handler);
> +		priv->tp_switch.initialized = false;
> +	}
> +}
> +
> +static void ideapad_tpswitch_toggle(struct ideapad_private *priv, bool on)
> +{
> +	guard(spinlock_irq)(&priv->tp_switch.lock);
> +
> +	priv->tp_switch.active = on;
> +	if (on) {
> +		struct input_dev *tp_dev = priv->tp_switch.tp_dev;
> +		if (tp_dev) {
> +			input_report_key(tp_dev, BTN_TOUCH, 0);
> +			input_report_key(tp_dev, BTN_TOOL_FINGER, 0);
> +			input_report_key(tp_dev, BTN_TOOL_DOUBLETAP, 0);
> +			input_report_key(tp_dev, BTN_TOOL_TRIPLETAP, 0);
> +			input_report_key(tp_dev, BTN_LEFT, 0);
> +			input_report_key(tp_dev, BTN_RIGHT, 0);
> +			input_report_key(tp_dev, BTN_MIDDLE, 0);
> +			input_sync(tp_dev);
> +		}
> +	}
> +}
> +
>  /*
>   * backlight
>   */
> @@ -1567,7 +1725,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 */
> @@ -1582,7 +1739,7 @@ static void ideapad_sync_touchpad_state(struct ideapad_private *priv, bool send_
>  	 * 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);
> +		ideapad_tpswitch_toggle(priv, value);
>  
>  	/*
>  	 * On older models the EC controls the touchpad and toggles it on/off
> @@ -1927,6 +2084,10 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  	if (err)
>  		goto input_failed;
>  
> +	err = ideapad_tpswitch_init(priv);
> +	if (err)
> +		goto tpswitch_failed;
> +
>  	err = ideapad_kbd_bl_init(priv);
>  	if (err) {
>  		if (err != -ENODEV)
> @@ -2001,6 +2162,9 @@ static int ideapad_acpi_add(struct platform_device *pdev)
>  
>  	ideapad_fn_lock_led_exit(priv);
>  	ideapad_kbd_bl_exit(priv);
> +	ideapad_tpswitch_exit(priv);
> +
> +tpswitch_failed:
>  	ideapad_input_exit(priv);
>  
>  input_failed:
> @@ -2029,6 +2193,7 @@ static void ideapad_acpi_remove(struct platform_device *pdev)
>  
>  	ideapad_fn_lock_led_exit(priv);
>  	ideapad_kbd_bl_exit(priv);
> +	ideapad_tpswitch_exit(priv);
>  	ideapad_input_exit(priv);
>  	ideapad_debugfs_exit(priv);
>  	ideapad_sysfs_exit(priv);