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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: multitouch: mt_input_configured() validates is_haptic_touchpad too late
--

commit a6c433e1de88f40d35d53fd68c994b47885232f5
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 to ff->private so the input core frees it on final
input device release. It also restricts FF initialization to the first
input device containing a multitouch touchpad report.

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 154dc948b156e..e53389e28ad53 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1954,25 +1954,9 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  	struct mt_report_data *rdata;
>  	struct mt_application *mt_application = NULL;
>  	struct hid_report *report;
> +	bool has_mt_touchpad = false;
>  	int ret;
>  
> -	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 (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;
> -	}
> -
>  	list_for_each_entry(report, &hi->reports, hidinput_list) {
>  		rdata = mt_find_report_data(td, report);
>  		if (!rdata) {
> @@ -1987,9 +1971,29 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  							mt_application);
>  			if (ret)
>  				return ret;
> +			if (hi->application == HID_DG_TOUCHPAD)
> +				has_mt_touchpad = true;
>  		}
>  	}
>  
> +	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 (has_mt_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;
> +	}

[Severity: Medium]
Does moving the td->is_haptic_touchpad validation after the hi->reports loop
cause mt_touch_input_configured() to observe an unvalidated
td->is_haptic_touchpad state?

During descriptor mapping, mt_touch_input_mapping() sets
td->is_haptic_touchpad = true whenever hid_haptic_check_pressure_unit() sees
a HID_DG_TIPPRESSURE field with gram or newton units, without checking
td->mtclass.name or whether Simple Haptic Controller trigger reports exist:

drivers/hid/hid-multitouch.c:mt_touch_input_mapping()
	case HID_DG_TIPPRESSURE:
		set_abs(hi->input, ABS_MT_PRESSURE, field,
			cls->sn_pressure);
		td->is_haptic_touchpad =
			hid_haptic_check_pressure_unit(td->haptic,
						       hi, field);

Inside the hi->reports loop, mt_touch_input_configured() checks
td->is_haptic_touchpad and passes INPUT_MT_TOTAL_FORCE to
input_mt_init_slots():

drivers/hid/hid-multitouch.c:mt_touch_input_configured()
	if (td->is_haptic_touchpad)
		app->mt_flags |= INPUT_MT_TOTAL_FORCE;
	...
	ret = input_mt_init_slots(input, td->maxcontacts, app->mt_flags);

Because this now runs before mt_input_configured() checks td->mtclass.name
or calls hid_haptic_input_configured(), INPUT_MT_TOTAL_FORCE is already set in
input->mt->flags even if td->is_haptic_touchpad is cleared to false right
after the loop (or on non-haptic MT inputs of a multi-input device), causing
input_mt_report_pointer_emulation() to sum ABS_MT_PRESSURE across contacts.

Also, if a Win8-class device sets td->is_haptic_touchpad on a non-touchpad
MT collection and has no HID_DG_TOUCHPAD MT collection, has_mt_touchpad will
be false for all inputs, leaving td->is_haptic_touchpad true so mt_probe()
keeps td->haptic and mt_process_slot() / mt_sync_frame() update
td->haptic->pressure_sum on every frame.

>  
>  	switch (hi->application) {
>  	case HID_GD_KEYBOARD:
>  	case HID_GD_KEYPAD:

[ ... ]

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