Input: i8042 - Add support for platform filter contexts

Currently the platform filter cannot access any driver-specific state
which forces drivers installing a i8042 filter to have at least some
kind of global pointer for their filter.

This however might cause issues should such a driver probe multiple
devices. Fix this by allowing callers of i8042_install_filter() to
submit a context pointer which is then passed to the i8042 filter.

Also introduce a separate type for the i8042 filter (i8042_filter_t)
so that the function definitions can stay compact.

Tested on a Dell Inspiron 3505.

Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Since most of the affected drivers are x86 platform drivers, i would
like to have this changes merged through the platform-drivers-x86
tree. This would also prevent a possible merge conflict should
the samsung-galaxybook driver get merged.
---
 drivers/input/misc/ideapad_slidebar.c   |  4 ++--
 drivers/input/serio/i8042.c             | 17 ++++++++++-------
 drivers/platform/x86/asus-nb-wmi.c      |  3 ++-
 drivers/platform/x86/asus-wmi.c         |  2 +-
 drivers/platform/x86/asus-wmi.h         |  3 +--
 drivers/platform/x86/dell/dell-laptop.c |  6 +++---
 drivers/platform/x86/hp/hp_accel.c      |  4 ++--
 drivers/platform/x86/msi-laptop.c       |  6 +++---
 drivers/platform/x86/panasonic-laptop.c |  4 ++--
 drivers/platform/x86/toshiba_acpi.c     |  4 ++--
 include/linux/i8042.h                   | 15 +++++++--------
 11 files changed, 35 insertions(+), 33 deletions(-)

--
2.39.5

On Mon, 16 Dec 2024, Armin Wolf wrote:

> Currently the platform filter cannot access any driver-specific state
> which forces drivers installing a i8042 filter to have at least some
> kind of global pointer for their filter.
> 
> This however might cause issues should such a driver probe multiple
> devices. Fix this by allowing callers of i8042_install_filter() to
> submit a context pointer which is then passed to the i8042 filter.
> 
> Also introduce a separate type for the i8042 filter (i8042_filter_t)
> so that the function definitions can stay compact.
> 
> Tested on a Dell Inspiron 3505.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Since most of the affected drivers are x86 platform drivers, i would
> like to have this changes merged through the platform-drivers-x86
> tree. This would also prevent a possible merge conflict should
> the samsung-galaxybook driver get merged.
> ---
>  drivers/input/misc/ideapad_slidebar.c   |  4 ++--
>  drivers/input/serio/i8042.c             | 17 ++++++++++-------
>  drivers/platform/x86/asus-nb-wmi.c      |  3 ++-
>  drivers/platform/x86/asus-wmi.c         |  2 +-
>  drivers/platform/x86/asus-wmi.h         |  3 +--
>  drivers/platform/x86/dell/dell-laptop.c |  6 +++---
>  drivers/platform/x86/hp/hp_accel.c      |  4 ++--
>  drivers/platform/x86/msi-laptop.c       |  6 +++---
>  drivers/platform/x86/panasonic-laptop.c |  4 ++--
>  drivers/platform/x86/toshiba_acpi.c     |  4 ++--
>  include/linux/i8042.h                   | 15 +++++++--------
>  11 files changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/input/misc/ideapad_slidebar.c b/drivers/input/misc/ideapad_slidebar.c
> index f6e5fc807b4d..ab2e0a401904 100644
> --- a/drivers/input/misc/ideapad_slidebar.c
> +++ b/drivers/input/misc/ideapad_slidebar.c
> @@ -121,7 +121,7 @@ static void slidebar_mode_set(u8 mode)
>  }
> 
>  static bool slidebar_i8042_filter(unsigned char data, unsigned char str,
> -				  struct serio *port)
> +				  struct serio *port, void *context)
>  {
>  	static bool extended = false;
> 
> @@ -219,7 +219,7 @@ static int __init ideapad_probe(struct platform_device* pdev)
>  	input_set_capability(slidebar_input_dev, EV_ABS, ABS_X);
>  	input_set_abs_params(slidebar_input_dev, ABS_X, 0, 0xff, 0, 0);
> 
> -	err = i8042_install_filter(slidebar_i8042_filter);
> +	err = i8042_install_filter(slidebar_i8042_filter, NULL);
>  	if (err) {
>  		dev_err(&pdev->dev,
>  			"Failed to install i8042 filter: %d\n", err);
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 509330a27880..cab5a4c5baf5 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -179,8 +179,8 @@ static struct platform_device *i8042_platform_device;
>  static struct notifier_block i8042_kbd_bind_notifier_block;
> 
>  static bool i8042_handle_data(int irq);
> -static bool (*i8042_platform_filter)(unsigned char data, unsigned char str,
> -				     struct serio *serio);
> +static i8042_filter_t i8042_platform_filter;
> +static void *i8042_platform_filter_context;
> 
>  void i8042_lock_chip(void)
>  {
> @@ -194,8 +194,7 @@ void i8042_unlock_chip(void)
>  }
>  EXPORT_SYMBOL(i8042_unlock_chip);
> 
> -int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
> -					struct serio *serio))
> +int i8042_install_filter(i8042_filter_t filter, void *context)
>  {
>  	guard(spinlock_irqsave)(&i8042_lock);
> 
> @@ -203,12 +202,12 @@ int i8042_install_filter(bool (*filter)(unsigned char data, unsigned char str,
>  		return -EBUSY;
> 
>  	i8042_platform_filter = filter;
> +	i8042_platform_filter_context = context;
>  	return 0;
>  }
>  EXPORT_SYMBOL(i8042_install_filter);
> 
> -int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,
> -				       struct serio *port))
> +int i8042_remove_filter(i8042_filter_t filter)
>  {
>  	guard(spinlock_irqsave)(&i8042_lock);
> 
> @@ -216,6 +215,7 @@ int i8042_remove_filter(bool (*filter)(unsigned char data, unsigned char str,
>  		return -EINVAL;
> 
>  	i8042_platform_filter = NULL;
> +	i8042_platform_filter_context = NULL;
>  	return 0;
>  }
>  EXPORT_SYMBOL(i8042_remove_filter);
> @@ -480,7 +480,10 @@ static bool i8042_filter(unsigned char data, unsigned char str,
>  		}
>  	}
> 
> -	if (i8042_platform_filter && i8042_platform_filter(data, str, serio)) {
> +	if (!i8042_platform_filter)
> +		return false;
> +
> +	if (i8042_platform_filter(data, str, serio, i8042_platform_filter_context)) {
>  		dbg("Filtered out by platform filter\n");
>  		return true;
>  	}
> diff --git a/drivers/platform/x86/asus-nb-wmi.c b/drivers/platform/x86/asus-nb-wmi.c
> index ef04d396f61c..a3d4b98045f8 100644
> --- a/drivers/platform/x86/asus-nb-wmi.c
> +++ b/drivers/platform/x86/asus-nb-wmi.c
> @@ -50,7 +50,8 @@ MODULE_PARM_DESC(tablet_mode_sw, "Tablet mode detect: -1:auto 0:disable 1:kbd-do
>  static struct quirk_entry *quirks;
>  static bool atkbd_reports_vol_keys;
> 
> -static bool asus_i8042_filter(unsigned char data, unsigned char str, struct serio *port)
> +static bool asus_i8042_filter(unsigned char data, unsigned char str, struct serio *port,
> +			      void *context)
>  {
>  	static bool extended_e0;
>  	static bool extended_e1;
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index fdeebab96fc0..6c674de60ec0 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -4824,7 +4824,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	}
> 
>  	if (asus->driver->i8042_filter) {
> -		err = i8042_install_filter(asus->driver->i8042_filter);
> +		err = i8042_install_filter(asus->driver->i8042_filter, NULL);
>  		if (err)
>  			pr_warn("Unable to install key filter - %d\n", err);
>  	}
> diff --git a/drivers/platform/x86/asus-wmi.h b/drivers/platform/x86/asus-wmi.h
> index d02f15fd3482..018dfde4025e 100644
> --- a/drivers/platform/x86/asus-wmi.h
> +++ b/drivers/platform/x86/asus-wmi.h
> @@ -73,8 +73,7 @@ struct asus_wmi_driver {
>  	void (*key_filter) (struct asus_wmi_driver *driver, int *code,
>  			    unsigned int *value, bool *autorelease);
>  	/* Optional standard i8042 filter */
> -	bool (*i8042_filter)(unsigned char data, unsigned char str,
> -			     struct serio *serio);
> +	i8042_filter_t		i8042_filter;
> 
>  	int (*probe) (struct platform_device *device);
>  	void (*detect_quirks) (struct asus_wmi_driver *driver);
> diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> index 5671bd0deee7..58b860b88fff 100644
> --- a/drivers/platform/x86/dell/dell-laptop.c
> +++ b/drivers/platform/x86/dell/dell-laptop.c
> @@ -725,8 +725,8 @@ static void dell_update_rfkill(struct work_struct *ignored)
>  }
>  static DECLARE_DELAYED_WORK(dell_rfkill_work, dell_update_rfkill);
> 
> -static bool dell_laptop_i8042_filter(unsigned char data, unsigned char str,
> -			      struct serio *port)
> +static bool dell_laptop_i8042_filter(unsigned char data, unsigned char str, struct serio *port,
> +				     void *context)
>  {
>  	static bool extended;
> 
> @@ -884,7 +884,7 @@ static int __init dell_setup_rfkill(void)
>  		pr_warn("Unable to register dell rbtn notifier\n");
>  		goto err_filter;
>  	} else {
> -		ret = i8042_install_filter(dell_laptop_i8042_filter);
> +		ret = i8042_install_filter(dell_laptop_i8042_filter, NULL);
>  		if (ret) {
>  			pr_warn("Unable to install key filter\n");
>  			goto err_filter;
> diff --git a/drivers/platform/x86/hp/hp_accel.c b/drivers/platform/x86/hp/hp_accel.c
> index 39a6530f5072..10d5af18d639 100644
> --- a/drivers/platform/x86/hp/hp_accel.c
> +++ b/drivers/platform/x86/hp/hp_accel.c
> @@ -267,7 +267,7 @@ static struct delayed_led_classdev hpled_led = {
>  };
> 
>  static bool hp_accel_i8042_filter(unsigned char data, unsigned char str,
> -				  struct serio *port)
> +				  struct serio *port, void *context)
>  {
>  	static bool extended;
> 
> @@ -326,7 +326,7 @@ static int lis3lv02d_probe(struct platform_device *device)
>  	/* filter to remove HPQ6000 accelerometer data
>  	 * from keyboard bus stream */
>  	if (strstr(dev_name(&device->dev), "HPQ6000"))
> -		i8042_install_filter(hp_accel_i8042_filter);
> +		i8042_install_filter(hp_accel_i8042_filter, NULL);
> 
>  	INIT_WORK(&hpled_led.work, delayed_set_status_worker);
>  	ret = led_classdev_register(NULL, &hpled_led.led_classdev);
> diff --git a/drivers/platform/x86/msi-laptop.c b/drivers/platform/x86/msi-laptop.c
> index e5391a37014d..c4b150fa093f 100644
> --- a/drivers/platform/x86/msi-laptop.c
> +++ b/drivers/platform/x86/msi-laptop.c
> @@ -806,8 +806,8 @@ static void msi_send_touchpad_key(struct work_struct *ignored)
>  }
>  static DECLARE_DELAYED_WORK(msi_touchpad_dwork, msi_send_touchpad_key);
> 
> -static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str,
> -				struct serio *port)
> +static bool msi_laptop_i8042_filter(unsigned char data, unsigned char str, struct serio *port,
> +				    void *context)
>  {
>  	static bool extended;
> 
> @@ -996,7 +996,7 @@ static int __init load_scm_model_init(struct platform_device *sdev)
>  	if (result)
>  		goto fail_input;
> 
> -	result = i8042_install_filter(msi_laptop_i8042_filter);
> +	result = i8042_install_filter(msi_laptop_i8042_filter, NULL);
>  	if (result) {
>  		pr_err("Unable to install key filter\n");
>  		goto fail_filter;
> diff --git a/drivers/platform/x86/panasonic-laptop.c b/drivers/platform/x86/panasonic-laptop.c
> index 22ca70eb8227..2987b4db6009 100644
> --- a/drivers/platform/x86/panasonic-laptop.c
> +++ b/drivers/platform/x86/panasonic-laptop.c
> @@ -260,7 +260,7 @@ struct pcc_acpi {
>   * keypress events over the PS/2 kbd interface, filter these out.
>   */
>  static bool panasonic_i8042_filter(unsigned char data, unsigned char str,
> -				   struct serio *port)
> +				   struct serio *port, void *context)
>  {
>  	static bool extended;
> 
> @@ -1100,7 +1100,7 @@ static int acpi_pcc_hotkey_add(struct acpi_device *device)
>  		pcc->platform = NULL;
>  	}
> 
> -	i8042_install_filter(panasonic_i8042_filter);
> +	i8042_install_filter(panasonic_i8042_filter, NULL);
>  	return 0;
> 
>  out_platform:
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index 78a5aac2dcfd..5ad3a7183d33 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -2755,7 +2755,7 @@ static int toshiba_acpi_enable_hotkeys(struct toshiba_acpi_dev *dev)
>  }
> 
>  static bool toshiba_acpi_i8042_filter(unsigned char data, unsigned char str,
> -				      struct serio *port)
> +				      struct serio *port, void *context)
>  {
>  	if (str & I8042_STR_AUXDATA)
>  		return false;
> @@ -2915,7 +2915,7 @@ static int toshiba_acpi_setup_keyboard(struct toshiba_acpi_dev *dev)
>  	if (ec_handle && acpi_has_method(ec_handle, "NTFY")) {
>  		INIT_WORK(&dev->hotkey_work, toshiba_acpi_hotkey_work);
> 
> -		error = i8042_install_filter(toshiba_acpi_i8042_filter);
> +		error = i8042_install_filter(toshiba_acpi_i8042_filter, NULL);
>  		if (error) {
>  			pr_err("Error installing key filter\n");
>  			goto err_free_dev;
> diff --git a/include/linux/i8042.h b/include/linux/i8042.h
> index 95b07f8b77fe..be9460c874d6 100644
> --- a/include/linux/i8042.h
> +++ b/include/linux/i8042.h
> @@ -54,15 +54,16 @@
> 
>  struct serio;
> 
> +typedef bool (*i8042_filter_t)(unsigned char data, unsigned char str, struct serio *serio,
> +			       void *context);
> +

Could you please add the kerneldoc for the typedef.

Other than that, the change seems good.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Hi Armin,

On Mon, Dec 16, 2024 at 12:36:28AM +0100, Armin Wolf wrote:
> Currently the platform filter cannot access any driver-specific state
> which forces drivers installing a i8042 filter to have at least some
> kind of global pointer for their filter.
> 
> This however might cause issues should such a driver probe multiple
> devices. Fix this by allowing callers of i8042_install_filter() to
> submit a context pointer which is then passed to the i8042 filter.

Right now i8042 supports only one instance of a filter, so the driver
probing several devices will have to sort out the ownership of the
filter anyways.

Unless you plan on supporting multiple filters I do not see the need of
storing the context in i8042. And if you decide to add support for
multiple filters I would need to better understand the use case.

Thanks.

Am 16.12.24 um 18:21 schrieb Dmitry Torokhov:

> Hi Armin,
>
> On Mon, Dec 16, 2024 at 12:36:28AM +0100, Armin Wolf wrote:
>> Currently the platform filter cannot access any driver-specific state
>> which forces drivers installing a i8042 filter to have at least some
>> kind of global pointer for their filter.
>>
>> This however might cause issues should such a driver probe multiple
>> devices. Fix this by allowing callers of i8042_install_filter() to
>> submit a context pointer which is then passed to the i8042 filter.
> Right now i8042 supports only one instance of a filter, so the driver
> probing several devices will have to sort out the ownership of the
> filter anyways.
>
> Unless you plan on supporting multiple filters I do not see the need of
> storing the context in i8042. And if you decide to add support for
> multiple filters I would need to better understand the use case.
>
> Thanks.
>
I am well aware that the i8042 driver currently supports only a single platform filter.

The reason for introducing a context pointer is that otherwise drivers registering a i8042 filter
would need to provide such a global pointer for their filter themself, together with the necessary
locking since those driver can (theoretically) be instantiated multiple times.

With this patch the i8042 driver takes care of that so those driver can finally get rid of global
data structures which will break as soon as the driver is instantiated multiple times.

Additionally this new API will allow us to add support for multiple handlers later should the need
arise.

Thanks,
Armin Wolf