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

[Maxim Mikityanskiy <maxtram95@gmail.com>]

On Tue, 03 Sep 2024 at 16:55:54 -0700, Dmitry Torokhov wrote:
> On Tue, Aug 20, 2024 at 10:28:24PM -0700, Dmitry Torokhov wrote:
> > On Wed, Aug 21, 2024 at 12:40:34AM +0300, Maxim Mikityanskiy wrote:
> > > 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:
> > > > > 
> > > > > 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.
> > > 
> > > 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.
> > 
> > Great, thank you! Give me a couple of days and I think I will implement
> > observer/passive handler support and we can figure out how to merge
> > this...
> 
> OK, so if you could try the patch below that would be great.
> Don't forget to set ".passive_observer = 1" in the ideapad handler.

Sorry for the slow response. I tested the patches, setting
passive_observer in ideapad_tpswitch_init and inverting the check in
ideapad_tpswitch_filter - all seems to work perfectly!

> Thanks!
> 
> -- 
> Dmitry
> 
> 
> Input: introduce notion of passive observers for input handlers
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/input.c |    7 +++++++
>  include/linux/input.h |    5 +++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 54c57b267b25..60a9445d78d5 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -605,6 +605,9 @@ int input_open_device(struct input_handle *handle)
>  
>  	handle->open++;
>  
> +	if (handle->handler->passive_observer)
> +		goto out;
> +
>  	if (dev->users++ || dev->inhibited) {
>  		/*
>  		 * Device is already opened and/or inhibited,
> @@ -668,6 +671,9 @@ void input_close_device(struct input_handle *handle)
>  
>  	__input_release_device(handle);
>  
> +	if (handle->handler->passive_observer)
> +		goto out;
> +
>  	if (!--dev->users && !dev->inhibited) {
>  		if (dev->poller)
>  			input_dev_poller_stop(dev->poller);
> @@ -684,6 +690,7 @@ void input_close_device(struct input_handle *handle)
>  		synchronize_rcu();
>  	}
>  
> +out:
>  	mutex_unlock(&dev->mutex);
>  }
>  EXPORT_SYMBOL(input_close_device);
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 89a0be6ee0e2..6437c35f0796 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -286,6 +286,10 @@ struct input_handle;
>   * @start: starts handler for given handle. This function is called by
>   *	input core right after connect() method and also when a process
>   *	that "grabbed" a device releases it
> + * @passive_observer: set to %true by drivers only interested in observing
> + *	data stream from devices if there are other users present. Such
> + *	drivers will not result in starting underlying hardware device
> + *	when input_open_device() is called for their handles
>   * @legacy_minors: set to %true by drivers using legacy minor ranges
>   * @minor: beginning of range of 32 legacy minors for devices this driver
>   *	can provide
> @@ -321,6 +325,7 @@ struct input_handler {
>  	void (*disconnect)(struct input_handle *handle);
>  	void (*start)(struct input_handle *handle);
>  
> +	bool passive_observer;
>  	bool legacy_minors;
>  	int minor;
>  	const char *name;