Re: [PATCH] HID: logitech-dj: Re-announce connected devices on reset-resume

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: logitech-dj: broken Unifying device enumeration in logi_dj_reset_resume()
--

commit 00da9ba908cab58b8ccc5713c8e413562664314d
Author: Mark Pariente <markpariente@gmail.com>

HID: logitech-dj: Re-announce connected devices on reset-resume

This commit sends a HID++ fake device arrival request in
logi_dj_reset_resume() after switching the receiver back to DJ mode so
that connected devices are re-announced and reconfigured by their child
drivers after a USB reset on resume.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=203421
Link: https://gitlab.freedesktop.org/libinput/libinput/-/issues/528

> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 6e8db01d27ec0..cdb20fdc7b21e 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -2010,6 +2010,16 @@ static int logi_dj_reset_resume(struct hid_device *hdev)
>  		return 0;
>  
>  	logi_dj_recv_switch_to_dj_mode(djrcv_dev, 0);
> +
> +	/*
> +	 * Paired devices may have reconnected and lost their configuration
> +	 * (e.g. hi-res scrolling) while the receiver was unpowered or switched
> +	 * away by a KVM. Have the receiver re-announce connected devices so
> +	 * the child drivers run their connect handling again.
> +	 */
> +	if (djrcv_dev->dj_mode && logi_dj_recv_query_hidpp_devices(djrcv_dev))
> +		hid_err(hdev, "%s: failed to query connected devices\n", __func__);

[Severity: Medium]
Can calling logi_dj_recv_query_hidpp_devices() unconditionally here break
Unifying (recvr_type_dj) device enumeration when a slot in
djrcv_dev->paired_dj_devices[] is still NULL during logi_dj_reset_resume()?

For example, if logi_dj_probe() ran while a KVM switch was focused on
another host (where commit 36dcfa46852533 leaves dj_mode false and
paired_dj_devices[] unpopulated), or if a new device was paired while
switched away during suspend, paired_dj_devices[device_index] is NULL when
logi_dj_reset_resume() runs.

Normally, logi_dj_recv_query_paired_devices() enumerates recvr_type_dj
receivers using REPORT_TYPE_CMD_GET_PAIRED_DEVICES so that devices are
created from DJ REPORT_TYPE_NOTIF_DEVICE_PAIRED notifications containing
the full 32-bit reports_supported bitmask:

drivers/hid/hid-logitech-dj.c:logi_dj_recv_query_paired_devices() {
	if (djrcv_dev->type != recvr_type_dj) {
		retval = logi_dj_recv_query_hidpp_devices(djrcv_dev);
		goto out;
	}

	dj_report = kzalloc_obj(struct dj_report);
	if (!dj_report)
		return -ENOMEM;
	dj_report->report_id = REPORT_ID_DJ_SHORT;
	dj_report->device_index = HIDPP_RECEIVER_INDEX;
	dj_report->report_type = REPORT_TYPE_CMD_GET_PAIRED_DEVICES;
	...
}

When logi_dj_recv_query_hidpp_devices() sends HIDPP_FAKE_DEVICE_ARRIVAL to
a Unifying receiver instead, the receiver replies with a HID++
REPORT_TYPE_NOTIF_DEVICE_CONNECTED (0x41) notification with protocol type
0x04 (eQUAD step 4 DJ). Because dj_dev is NULL, logi_dj_hidpp_event()
routes this to logi_hidpp_recv_queue_notif() rather than queuing unknown
work to run logi_dj_recv_query_paired_devices():

drivers/hid/hid-logitech-dj.c:logi_dj_hidpp_event() {
	if (dj_dev) {
		logi_dj_recv_forward_report(dj_dev, data, size);
	} else {
		if (hidpp_report->sub_id == REPORT_TYPE_NOTIF_DEVICE_CONNECTED)
			logi_hidpp_recv_queue_notif(hdev, hidpp_report);
		else
			logi_dj_recv_queue_unknown_work(djrcv_dev);
	}
}

That schedules delayedwork_callback() -> logi_hidpp_dev_conn_notif_equad()
-> logi_dj_recv_add_djhid_device():

drivers/hid/hid-logitech-dj.c:logi_hidpp_dev_conn_notif_equad() {
	workitem->type = WORKITEM_TYPE_PAIRED;
	workitem->device_type = hidpp_report->params[HIDPP_PARAM_DEVICE_INFO] &
				HIDPP_DEVICE_TYPE_MASK;
	...
	switch (workitem->device_type) {
	case REPORT_TYPE_KEYBOARD:
		workitem->reports_supported |= STD_KEYBOARD | MULTIMEDIA |
					       POWER_KEYS | MEDIA_CENTER |
					       HIDPP;
		...
		break;
	case REPORT_TYPE_MOUSE:
		workitem->reports_supported |= STD_MOUSE | HIDPP | MULTIMEDIA;
		break;
	}
}

Because logi_hidpp_dev_conn_notif_equad() only populates reports_supported
for REPORT_TYPE_KEYBOARD and REPORT_TYPE_MOUSE, Unifying Trackballs,
Touchpads, Numpads, Presenters, and Remote Controls end up with
reports_supported == 0, causing logi_dj_ll_parse() to pass a 0-byte report
descriptor that hid_parse_collections() rejects with -EINVAL, while
Unifying keyboard+touchpad combos such as the K400 lose STD_MOUSE and
KBD_LEDS.

Furthermore, because paired_dj_devices[device_index] is then populated,
logi_dj_recv_add_djhid_device() will ignore any subsequent DJ
REPORT_TYPE_NOTIF_DEVICE_PAIRED notification for that slot until the
receiver is unplugged:

drivers/hid/hid-logitech-dj.c:logi_dj_recv_add_djhid_device() {
	if (djrcv_dev->paired_dj_devices[device_index]) {
		/* The device is already known. No need to reallocate it. */
		dbg_hid("%s: device is already known\n", __func__);
		return;
	}
	...
}

> +
>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018224627.60328-1-markpariente@gmail.com?part=1