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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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(&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@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(&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);




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux