Hello.
On pondělí 16. srpna 2021 21:12:49 CEST 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;
Any luck with moving this forward please? I've got a similar issue with APC
UPS and v5.14 kernel, and this patch also solves the connectivity issue for
me.
Thanks.
--
Oleksandr Natalenko (post-factum)
