linux-kernel - Re: [PATCH] hid/usbhid: add reset device for EPROTO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250918112128.05c21d6b.michal.pecio@gmail.com>
Date: Thu, 18 Sep 2025 11:21:28 +0200
From: Michal Pecio <michal.pecio@...il.com>
To: zhangjinpeng <zhangjinpeng@...inos.cn>
Cc: jikos@...nel.org, benjamin.tissoires@...hat.com,
 linux-usb@...r.kernel.org, linux-input@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] hid/usbhid: add reset device for EPROTO

Hi,

I think this patch may deserve more explanation than just a log snippet.

Is this for case when the mouse is actually being unplugged? Why bother?

Or is the mouse disconnecting itself in response to missing clear halt
request or other misbehavior of host system and the patch fixes it?

On Thu, 18 Sep 2025 13:55:27 +0800, zhangjinpeng wrote:
> [  792.354988] input: PixArt USB Optical Mouse as /devices/platform/PHYT0039:03/usb7/7-1/7-1.2/7-1.2:1.0/0003:093A:2510.0028/input/input53
> [  792.355081] hid-generic 0003:093A:2510.0028: input,hidraw1: USB HID v1.11 Mouse [PixArt USB Optical Mouse] on usb-PHYT0039:03-1.2/input0
> [  792.355137] hub 7-1:1.0: state 7 ports 4 chg 0000 evt 0004
> ： xhci-hcd PHYT0039:03: Transfer error for slot 4 ep 2 on endpoint

No timestamp. Log corruption? Missing fragment? Copy-paste mistake?

> [  794.579339] xhci-hcd PHYT0039:03: Giveback URB 00000000ab6c1cac, len = 0, expected = 4, status = -71

This looks wrong, shouldn't there be some noise about cancellation of 
this URB and Set TR Deq before it is given back with -EPROTO status?

If not a missing log fragment, this may be another case of HW failing
to update EP Context state. What sort of xHCI controller is this?

Is this condition reproducible?

> [  794.596152] xhci-hcd PHYT0039:03: WARN halted endpoint, queueing URB anyway.

Either the dreadful resubmission - async giveback race, or indeed
a halt has not been recognized by xhci_hcd.

Is this log noise the actual thing which you wanted to fix?

> [  917.451251] hub 7-1:1.0: state 7 ports 4 chg 0000 evt 0004
> [  917.451323] usb 7-1-port2: status 0100, change 0001, 12 Mb/s
> [  917.451362] usb 7-1-port2: indicator auto status 0
> [  917.451365] usb 7-1.2: USB disconnect, device number 45
> [  917.451367] usb 7-1.2: unregistering device
> [  917.451369] usb 7-1.2: unregistering interface 7-1.2:1.0
> [  917.451429] xhci-hcd PHYT0039:03: Cancel URB 00000000ab6c1cac, dev 1.2, ep 0x81, starting at offset 0x2361ea6280
> [  917.451432] xhci-hcd PHYT0039:03: // Ding dong!
> [  917.451436] xhci-hcd PHYT0039:03: shutdown urb ffffffa2ebc8e400 ep1in-intr
> [  917.451440] xhci-hcd PHYT0039:03: Removing canceled TD starting at 0x2361ea6280 (dma).
> [  917.500303] usb 7-1.2: usb_disable_device nuking all URBs
> [  917.500310] xhci-hcd PHYT0039:03: xhci_drop_endpoint called for udev 00000000e00ae900
> [  917.500324] xhci-hcd PHYT0039:03: drop ep 0x81, slot id 4, new drop flags = 0x8, new add flags = 0x0
> [  917.500326] xhci-hcd PHYT0039:03: xhci_check_bandwidth called for udev 00000000e00ae900
> [  917.500330] xhci-hcd PHYT0039:03: // Ding dong!
> [  917.500351] xhci-hcd PHYT0039:03: Successful Endpoint Configure command
> [  917.500579] xhci-hcd PHYT0039:03: // Ding dong!
> [  917.656189] usb 7-1-port2: debounce total 100ms stable 100ms status 0x100
> 
> Signed-off-by: zhangjinpeng <zhangjinpeng@...inos.cn>

I guess if I won't do it then Greg will: you should spell your
name properly, with spaces (if applicable) and capitalization.

> ---
>  drivers/hid/usbhid/hid-core.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 257dd73e37bf..253f82f33b08 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -306,8 +306,13 @@ static void hid_irq_in(struct urb *urb)
>  	case -ESHUTDOWN:	/* unplug */
>  		clear_bit(HID_IN_RUNNING, &usbhid->iofl);
>  		return;
> -	case -EILSEQ:		/* protocol error or unplug */
>  	case -EPROTO:		/* protocol error or unplug */
> +		usbhid_mark_busy(usbhid);
> +		clear_bit(HID_IN_RUNNING, &usbhid->iofl);
> +		set_bit(HID_CLEAR_HALT, &usbhid->iofl);
> +		usb_queue_reset_device(usbhid->intf);

Isn't usb_clear_halt() on the affected endpoint enough?

> +		return;
> +	case -EILSEQ:		/* protocol error or unplug */
>  	case -ETIME:		/* protocol error or unplug */

Documentation/driver-api/usb/error-codes.rst is clear as mud, but
these cases seem roughly equivalent to -EPROTO and all seem to imply
a halted endpoint. Why handle them differently?

On xhci_hcd it's true that -EPROTO is the only one you normally get,
and if you ever see -EILSEQ you have a serious driver problem which
no reset is likely to fix, but other HCDs could be different.

>  	case -ETIMEDOUT:	/* Should never happen, but... */
>  		usbhid_mark_busy(usbhid);
> 2.25.1
> 

Regards,
Michal