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

[sashiko-bot@kernel.org]

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