[PATCH v9 0/2] HID: multitouch: fix haptic lifetime on delayed input release

[PATCH v9 0/2] HID: multitouch: fix haptic lifetime on delayed input release

[Aldo Ariel Panzardo]

An open evdev fd can keep the input device alive after HID unbind.
The haptic state in ff->private must remain allocated until the input
device is released, and pending work must finish before HID reports
are freed.

Following Dmitry's v7 and v8 review, hid_haptic_init() now clones the
devres-managed mapping state. FF core owns the clone once setup
succeeds and frees it at final input release.

Patch 1 makes input_ff_create() the last fallible initialization step,
uses __module_get(), and drains pending work through ff->stop at input
unregistration. It also removes the extra HID device reference and
propagates haptic setup failures to mt_probe(), including when hidraw
lets hid_connect() succeed after input setup has failed.

Patch 2 allocates the clone in hid_haptic_init() and transfers it with
no_free_ptr(). It keeps one FF instance per HID device because the
touchpad inputs share report fields.

Changes since v8:
  - clone the mapping state in hid_haptic_init() and give FF core
    ownership of the clone
  - remove the devm release action and extra input/HID references
  - use __module_get() after input_ff_create()
  - propagate haptic initialization errors through mt_probe()
  - drain haptic work at input device unregistration

Build checks on x86_64 with GCC 15.2.0 and W=1 passed: the affected
objects with HID built in, a minimal vmlinux and modules build with
HID=m and HID_MULTITOUCH=m, and hid-multitouch.o with HID_HAPTIC=n.
The full build reported kernel-doc warnings in unmodified files.
A QEMU/KVM boot with KASAN ran 10 virtual UHID haptic cycles: upload and
play an effect, destroy the HID device with evdev still open, then close
the fd. FF_HAPTIC was exposed, output reports were observed, and KASAN
reported no errors. Physical hardware was not tested.

Base: 58161ead29e50d70a317ea26b472e0c7508e69da
v8: https://lore.kernel.org/linux-input/20261016173056.3397211-1-qwe.aldo@gmail.com/

Aldo Ariel Panzardo (2):
  HID: haptic: make FF creation the last fallible init step
  HID: multitouch: give FF core ownership of haptic state

 drivers/hid/hid-haptic.c     | 62 +++++++++++++++++++-----------------
 drivers/hid/hid-haptic.h     |  6 ++--
 drivers/hid/hid-multitouch.c | 18 +++++++++--
 3 files changed, 52 insertions(+), 34 deletions(-)

-- 
2.53.0

[PATCH v9 1/2] HID: haptic: make FF creation the last fallible init step

[Aldo Ariel Panzardo]

The caller holds the module while setting up the device. Use
__module_get() after input_ff_create() succeeds, leaving no fallible
step after the FF callbacks are installed.

Drain haptic work through ff->stop when the input device is
unregistered, before it drops its parent reference. The destroy
callback also drains work for devices that were never registered.
After unregistration, that callback only frees local allocations, so
remove the extra HID device reference and its error path.

Clear the FF capability bits if input_ff_create() fails. Return the
error from hid_haptic_input_configured() and save it in the multitouch
driver. Probe must fail even if hidraw lets hid_connect() succeed after
hidinput_connect() has unwound.

Fixes: 8d0bf7908b5a ("HID: multitouch: add haptic multitouch support")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/hid/hid-haptic.c     | 48 ++++++++++++++----------------------
 drivers/hid/hid-multitouch.c | 18 ++++++++++++--
 2 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index 8760eeb..0a6126e 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -96,7 +96,7 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 	if (error) {
 		dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
 			 hdev->name);
-		return 0;
+		return error;
 	}
 
 	return 1;
@@ -377,14 +377,22 @@ static int hid_haptic_erase(struct input_dev *dev, int effect_id)
 	return 0;
 }
 
+static void hid_haptic_stop(struct ff_device *ff)
+{
+	struct hid_haptic_device *haptic = ff->private;
+
+	if (haptic->wq) {
+		destroy_workqueue(haptic->wq);
+		haptic->wq = NULL;
+	}
+}
+
 static void hid_haptic_destroy(struct ff_device *ff)
 {
 	struct hid_haptic_device *haptic = ff->private;
-	struct hid_device *hdev = haptic->hdev;
 	int r;
 
-	if (hdev)
-		put_device(&hdev->dev);
+	hid_haptic_stop(ff);
 
 	kfree(haptic->stop_effect.report_buf);
 	haptic->stop_effect.report_buf = NULL;
@@ -396,9 +404,6 @@ static void hid_haptic_destroy(struct ff_device *ff)
 	}
 	haptic->effect = NULL;
 
-	destroy_workqueue(haptic->wq);
-	haptic->wq = NULL;
-
 	kfree(haptic->duration_map);
 	haptic->duration_map = NULL;
 
@@ -419,8 +424,6 @@ int hid_haptic_init(struct hid_device *hdev,
 	};
 	const char *prefix = "hid-haptic";
 	char *name;
-	int (*flush)(struct input_dev *dev, struct file *file);
-	int (*event)(struct input_dev *dev, unsigned int type, unsigned int code, int value);
 
 	haptic->hdev = hdev;
 	haptic->max_waveform_id = max(2u, haptic->max_waveform_id);
@@ -501,38 +504,25 @@ int hid_haptic_init(struct hid_device *hdev,
 
 	input_set_capability(dev, EV_FF, FF_HAPTIC);
 
-	flush = dev->flush;
-	event = dev->event;
 	ret = input_ff_create(dev, FF_MAX_EFFECTS);
 	if (ret) {
 		dev_err(&hdev->dev, "Failed to create ff device.\n");
-		goto stop_buffer_free;
+		goto ff_cap_clear;
 	}
 
 	ff = dev->ff;
-	ff->private = haptic;
 	ff->upload = hid_haptic_upload_effect;
 	ff->playback = hid_haptic_playback;
 	ff->erase = hid_haptic_erase;
+	ff->stop = hid_haptic_stop;
 	ff->destroy = hid_haptic_destroy;
-	if (!try_module_get(THIS_MODULE)) {
-		dev_err(&hdev->dev, "Failed to increase module count.\n");
-		goto input_free;
-	}
-	if (!get_device(&hdev->dev)) {
-		dev_err(&hdev->dev, "Failed to get hdev device.\n");
-		module_put(THIS_MODULE);
-		goto input_free;
-	}
+	__module_get(THIS_MODULE);
+	ff->private = haptic;
 	return 0;
 
-input_free:
-	input_ff_destroy(dev);
-	/* Restore dev flush and event */
-	dev->flush = flush;
-	dev->event = event;
-	return ret;
-stop_buffer_free:
+ff_cap_clear:
+	__clear_bit(FF_HAPTIC, dev->ffbit);
+	__clear_bit(EV_FF, dev->evbit);
 	kfree(haptic->stop_effect.report_buf);
 	haptic->stop_effect.report_buf = NULL;
 buffer_free:
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 4e19a0c..154dc94 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -174,6 +174,7 @@ struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	struct timer_list release_timer;	/* to release sticky fingers */
 	struct hid_haptic_device *haptic;	/* haptic related configuration */
+	int haptic_init_error;
 	struct hid_device *hdev;	/* hid_device we're attached to */
 	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_RUNNING) */
 	unsigned long *active_slots;	/* bitmap of slots with an active
@@ -1958,8 +1959,16 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	if (td->is_haptic_touchpad && (td->mtclass.name == MT_CLS_WIN_8 ||
 	    td->mtclass.name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
 	    td->mtclass.name == MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE)) {
-		if (hid_haptic_input_configured(hdev, td->haptic, hi) == 0)
-			td->is_haptic_touchpad = false;
+		if (hi->application == HID_DG_TOUCHPAD) {
+			ret = hid_haptic_input_configured(hdev, td->haptic, hi);
+			if (ret < 0) {
+				td->haptic_init_error = ret;
+				td->is_haptic_touchpad = false;
+				return ret;
+			}
+			if (ret == 0)
+				td->is_haptic_touchpad = false;
+		}
 	} else {
 		td->is_haptic_touchpad = false;
 	}
@@ -2197,6 +2206,11 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret)
 		return ret;
+	if (td->haptic_init_error) {
+		ret = td->haptic_init_error;
+		hid_hw_stop(hdev);
+		return ret;
+	}
 
 	ret = sysfs_create_group(&hdev->dev.kobj, &mt_attribute_group);
 	if (ret)
-- 
2.53.0

[PATCH v9 2/2] HID: multitouch: give FF core ownership of haptic state

[Aldo Ariel Panzardo]

An open evdev fd can delay input_ff_destroy() until after HID unbind.
The descriptor mapping state is devres managed and cannot serve as
ff->private for that lifetime.

Clone the mapped state in hid_haptic_init(). Transfer the allocation
to ff->private only after input_ff_create() succeeds; the input core
then frees it at final input device release. Scope cleanup frees the
clone on initialization failure. The multitouch driver keeps its
mapping allocation and needs no ownership tracking for the FF state.

Record the first successfully configured input in the mapping state
and skip FF initialization for later touchpad inputs. They share HID
reports, so separate FF instances would write the same report fields
under different mutexes.

Fixes: 8d0bf7908b5a ("HID: multitouch: add haptic multitouch support")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/hid/hid-haptic.c | 16 ++++++++++++++--
 drivers/hid/hid-haptic.h |  6 ++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index 0a6126e..b0fb94f 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -5,6 +5,7 @@
  *  Copyright (c) 2021 Angela Czubak <acz@semihalf.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/input/mt.h>
 #include <linux/module.h>
 
@@ -92,6 +93,10 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 
 	__set_bit(INPUT_PROP_PRESSUREPAD, hi->input->propbit);
 
+	/* The HID reports are shared; create only one FF instance. */
+	if (haptic->input_dev)
+		return 1;
+
 	error = hid_haptic_init(hdev, haptic, hi->input);
 	if (error) {
 		dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
@@ -99,6 +104,8 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 		return error;
 	}
 
+	haptic->input_dev = hi->input;
+
 	return 1;
 }
 EXPORT_SYMBOL_GPL(hid_haptic_input_configured);
@@ -414,9 +421,11 @@ static void hid_haptic_destroy(struct ff_device *ff)
 }
 
 int hid_haptic_init(struct hid_device *hdev,
-		    struct hid_haptic_device *haptic,
+		    const struct hid_haptic_device *config,
 		    struct input_dev *dev)
 {
+	struct hid_haptic_device *haptic __free(kfree) =
+		kmemdup(config, sizeof(*config), GFP_KERNEL);
 	struct ff_device *ff;
 	int ret = 0, r;
 	struct ff_haptic_effect stop_effect = {
@@ -425,6 +434,9 @@ int hid_haptic_init(struct hid_device *hdev,
 	const char *prefix = "hid-haptic";
 	char *name;
 
+	if (!haptic)
+		return -ENOMEM;
+
 	haptic->hdev = hdev;
 	haptic->max_waveform_id = max(2u, haptic->max_waveform_id);
 	haptic->max_duration_id = max(2u, haptic->max_duration_id);
@@ -517,7 +529,7 @@ int hid_haptic_init(struct hid_device *hdev,
 	ff->stop = hid_haptic_stop;
 	ff->destroy = hid_haptic_destroy;
 	__module_get(THIS_MODULE);
-	ff->private = haptic;
+	ff->private = no_free_ptr(haptic);
 	return 0;
 
 ff_cap_clear:
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
index 6332991..661b605 100644
--- a/drivers/hid/hid-haptic.h
+++ b/drivers/hid/hid-haptic.h
@@ -69,7 +69,8 @@ int hid_haptic_input_mapping(struct hid_device *hdev,
 int hid_haptic_input_configured(struct hid_device *hdev,
 				struct hid_haptic_device *haptic,
 				struct hid_input *hi);
-int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device *haptic,
+int hid_haptic_init(struct hid_device *hdev,
+		    const struct hid_haptic_device *config,
 		    struct input_dev *dev);
 void hid_haptic_handle_press_release(struct hid_haptic_device *haptic);
 void hid_haptic_pressure_reset(struct hid_haptic_device *haptic);
@@ -108,7 +109,8 @@ static inline
 void hid_haptic_reset(struct hid_device *hdev, struct hid_haptic_device *haptic)
 {}
 static inline
-int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device *haptic,
+int hid_haptic_init(struct hid_device *hdev,
+		    const struct hid_haptic_device *config,
 		    struct input_dev *dev)
 {
 	return 0;
-- 
2.53.0

Re: [PATCH v9 2/2] HID: multitouch: give FF core ownership of haptic state

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] HID: haptic: hid_haptic_input_configured() configures wrong or skipped touchpad inputs
- [Low] HID: haptic: hid_haptic_init() mixes __free() cleanup with goto unwind labels
--

commit 460ed664066c1c242ed1db3ec867440a01166da3
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

HID: multitouch: give FF core ownership of haptic state

This commit clones the mapped haptic state in hid_haptic_init() and
transfers ownership of the clone to ff->private so the input core frees it
at final input device release. It also records the first configured input
device to avoid creating multiple force-feedback instances sharing the same
HID reports.

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> index 0a6126e8de5e9..b0fb94f759a37 100644
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
> @@ -5,6 +5,7 @@
>   *  Copyright (c) 2021 Angela Czubak <acz@semihalf.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/input/mt.h>
>  #include <linux/module.h>
>  
> @@ -92,6 +93,10 @@ int hid_haptic_input_configured(struct hid_device *hdev,
>  
>  	__set_bit(INPUT_PROP_PRESSUREPAD, hi->input->propbit);
>  
> +	/* The HID reports are shared; create only one FF instance. */
> +	if (haptic->input_dev)
> +		return 1;
> +
>  	error = hid_haptic_init(hdev, haptic, hi->input);
>  	if (error) {
>  		dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
> @@ -99,6 +104,8 @@ int hid_haptic_input_configured(struct hid_device *hdev,
>  		return error;
>  	}
>  
> +	haptic->input_dev = hi->input;
> +

[Severity: Medium]
Can this attach the single force-feedback instance to the wrong input
device or mark skipped touchpad inputs with INPUT_PROP_PRESSUREPAD?

On a multi-input haptic touchpad (for example with
MT_CLS_WIN_8_FORCE_MULTI_INPUT, which sets HID_QUIRK_MULTI_INPUT),
hidinput_connect() invokes mt_input_configured() on each hid_input in
descriptor order before checking hidinput_has_been_populated():

drivers/hid/hid-input.c:hidinput_connect() {
	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
		if (drv->input_configured &&
		    drv->input_configured(hid, hidinput))
			goto out_unwind;

		if (!hidinput_has_been_populated(hidinput)) {
			/* no need to register an input device not populated */
			hidinput_cleanup_hidinput(hid, hidinput);
			continue;
		}
}

In mt_input_configured(), hid_haptic_input_configured() is called for any
hid_input with hi->application == HID_DG_TOUCHPAD before checking
rdata->is_mt_collection:

drivers/hid/hid-multitouch.c:mt_input_configured() {
	if (td->is_haptic_touchpad && (td->mtclass.name == MT_CLS_WIN_8 ||
	    td->mtclass.name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
	    td->mtclass.name == MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE)) {
		if (hi->application == HID_DG_TOUCHPAD) {
			ret = hid_haptic_input_configured(hdev, td->haptic, hi);
}

If a non-multitouch or unpopulated HID_DG_TOUCHPAD report precedes the
multitouch report, hid_haptic_input_configured() initializes EV_FF on that
first hid_input (causing hidinput_has_been_populated() to keep and register
an otherwise empty input_dev), sets haptic->input_dev, and then skips the
actual multitouch touchpad input_dev.

In addition, because __set_bit(INPUT_PROP_PRESSUREPAD, hi->input->propbit)
runs before checking haptic->input_dev, subsequent HID_DG_TOUCHPAD inputs
that skip hid_haptic_init() still advertise INPUT_PROP_PRESSUREPAD.

>  	return 1;
>  }
>  EXPORT_SYMBOL_GPL(hid_haptic_input_configured);
> @@ -414,9 +421,11 @@ static void hid_haptic_destroy(struct ff_device *ff)
>  }
>  
>  int hid_haptic_init(struct hid_device *hdev,
> -		    struct hid_haptic_device *haptic,
> +		    const struct hid_haptic_device *config,
>  		    struct input_dev *dev)
>  {
> +	struct hid_haptic_device *haptic __free(kfree) =
> +		kmemdup(config, sizeof(*config), GFP_KERNEL);
>  	struct ff_device *ff;
>  	int ret = 0, r;
>  	struct ff_haptic_effect stop_effect = {
> @@ -425,6 +434,9 @@ int hid_haptic_init(struct hid_device *hdev,
>  	const char *prefix = "hid-haptic";
>  	char *name;
>  
> +	if (!haptic)
> +		return -ENOMEM;
> +
>  	haptic->hdev = hdev;
>  	haptic->max_waveform_id = max(2u, haptic->max_waveform_id);
>  	haptic->max_duration_id = max(2u, haptic->max_duration_id);
> @@ -517,7 +529,7 @@ int hid_haptic_init(struct hid_device *hdev,
>  	ff->stop = hid_haptic_stop;
>  	ff->destroy = hid_haptic_destroy;
>  	__module_get(THIS_MODULE);
> -	ff->private = haptic;
> +	ff->private = no_free_ptr(haptic);
>  	return 0;
>  
>  ff_cap_clear:

[Severity: Low]
Should hid_haptic_init() avoid mixing __free(kfree) with goto-based cleanup
labels?

In hid_haptic_init(), haptic is declared with __free(kfree) and transferred
with no_free_ptr(haptic), while the rest of the function uses a goto unwind
ladder (ff_cap_clear, buffer_free, output_queue, duration_map, usage_map,
and exit) that dereferences haptic to free its sub-allocations before scope
exit runs.

Per include/linux/cleanup.h, goto cleanup labels and __free() helpers
should not be mixed in the same function. Could haptic be freed explicitly
at the exit label instead, or all resources in hid_haptic_init() converted
to scope-based cleanup?

[ ... ]

>
[::-1]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017140332.1480375-1-qwe.aldo@gmail.com?part=2