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