On Tue, 20 Aug 2024 at 13:46:53 +0300, Maxim Mikityanskiy wrote: > 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(¶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 ? > > > > >>> > > > > >>> *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@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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. I tested the patch on Z570 (with this check inverted), and it seems to work great. Also tested what happens on resume from suspend: the laptop reenables the touchpad (the LED turns off on suspend and blinks briefly on resume), and the driver handles it properly. > > > + 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(¶m, 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);