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);
