[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-usb
Subject:    Re: [REGRESSION][BISECTED] flood of "hid-generic ... control queue full" since v5.14-rc1
From:       Holger_Hoffstätte <holger () applied-asynchrony ! com>
Date:       2021-08-30 19:23:22
Message-ID: 97d5cdb9-0372-7e8b-dc61-610198f8664f () applied-asynchrony ! com
[Download RAW message or body]

On 2021-08-16 21:12, Michal Kubecek wrote:
> On Mon, Aug 16, 2021 at 10:38:56AM -0400, Alan Stern wrote:
>> On Mon, Aug 16, 2021 at 04:13:47PM +0200, Michal Kubecek wrote:
>>> Looking at the code, the primary problem seems to be that the "else"
>>> branch in hid_submit_ctrl() recalculates transfer_buffer_length to
>>> a rounded up value but assigns the original length to wLength.
>>
>> Looks like you found the bug.  Fixing it might be as simple as setting
>> len = padlen in that "else" branch.  You could then combine the
>> transfer_buffer_length assignment with the one in the "if" branch and
>> hoist them out after the entire "if" statement.
> 
> With the patch below, there are no errors and the UPS communication
> works correctly and so do other HID devices. But I would prefere someone
> familiar with HID code to confirm that this is what we want and what
> would be the right way to handle usb_submit_urb() errors.
> 
> Michal
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 06130dc431a0..ef240ef63a66 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -377,27 +377,26 @@ static int hid_submit_ctrl(struct hid_device *hid)
>   	len = hid_report_len(report);
>   	if (dir == USB_DIR_OUT) {
>   		usbhid->urbctrl->pipe = usb_sndctrlpipe(hid_to_usb_dev(hid), 0);
> -		usbhid->urbctrl->transfer_buffer_length = len;
>   		if (raw_report) {
>   			memcpy(usbhid->ctrlbuf, raw_report, len);
>   			kfree(raw_report);
>   			usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
>   		}
>   	} else {
> -		int maxpacket, padlen;
> +		int maxpacket;
>   
>   		usbhid->urbctrl->pipe = usb_rcvctrlpipe(hid_to_usb_dev(hid), 0);
>   		maxpacket = usb_maxpacket(hid_to_usb_dev(hid),
>   					  usbhid->urbctrl->pipe, 0);
>   		if (maxpacket > 0) {
> -			padlen = DIV_ROUND_UP(len, maxpacket);
> -			padlen *= maxpacket;
> -			if (padlen > usbhid->bufsize)
> -				padlen = usbhid->bufsize;
> +			len = DIV_ROUND_UP(len, maxpacket);
> +			len *= maxpacket;
> +			if (len > usbhid->bufsize)
> +				len = usbhid->bufsize;
>   		} else
> -			padlen = 0;
> -		usbhid->urbctrl->transfer_buffer_length = padlen;
> +			len = 0;
>   	}
> +	usbhid->urbctrl->transfer_buffer_length = len;
>   	usbhid->urbctrl->dev = hid_to_usb_dev(hid);
>   
>   	usbhid->cr->bRequestType = USB_TYPE_CLASS | USB_RECIP_INTERFACE | dir;
> 

Got the same flood of messages when trying 5.14.0, found this thread, applied
the patch & rebuilt, problem is gone & UPS still working fine. Yay.

cheers
Holger
[prev in list] [next in list] [prev in thread] [next in thread]