On Tue, Jul 23, 2013 at 04:30:29AM -0500, Josef Schimke wrote:
> wrt that:
> 1. Am I on the right track thinking like that?
> 2. Does the USB stuff in the kernel already have a way to test this?
> 3. If not, I'm really stumped about how I can do this, so any advice
> would be great. :p
>
> Aside from that - in hid_start_in(), would it have been better if I'd
> used goto and a label instead of breaking from the for loop? I wouldn't
> have to test if (rc == 0) to clear that HID_NO_BANDWIDTH bit that way,
> but it seemed more messy.
I'd recommend just always using 2 interrupt urbs, it can't hurt other
devices / host controllers if it's always there.
> diff -uprN -X linux-3.10-vanilla/Documentation/dontdiff linux-3.10-vanilla/drivers/hid/usbhid/hid-core.c linux-3.10-dev/drivers/hid/usbhid/hid-core.c
> --- linux-3.10-vanilla/drivers/hid/usbhid/hid-core.c 2013-06-30 17:13:29.000000000 -0500
> +++ linux-3.10-dev/drivers/hid/usbhid/hid-core.c 2013-07-22 16:00:24.785950521 -0500
> @@ -80,20 +80,26 @@ static int hid_start_in(struct hid_devic
> unsigned long flags;
> int rc = 0;
> struct usbhid_device *usbhid = hid->driver_data;
> + int i;
>
> spin_lock_irqsave(&usbhid->lock, flags);
> if (hid->open > 0 &&
> !test_bit(HID_DISCONNECTED, &usbhid->iofl) &&
> !test_bit(HID_SUSPENDED, &usbhid->iofl) &&
> !test_and_set_bit(HID_IN_RUNNING, &usbhid->iofl)) {
> - rc = usb_submit_urb(usbhid->urbin, GFP_ATOMIC);
> - if (rc != 0) {
> - clear_bit(HID_IN_RUNNING, &usbhid->iofl);
> - if (rc == -ENOSPC)
> - set_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> - } else {
> - clear_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> + for (i = 0; i < usbhid->n_inurbs; i++) {
> + rc = usb_submit_urb(usbhid->inurbs[i], GFP_ATOMIC);
> + if (rc != 0) {
> + clear_bit(HID_IN_RUNNING, &usbhid->iofl);
> + if (rc == -ENOSPC)
> + set_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> +
> + break;
> + }
> }
> +
> + if (rc == 0)
> + clear_bit(HID_NO_BANDWIDTH, &usbhid->iofl);
> }
> spin_unlock_irqrestore(&usbhid->lock, flags);
> return rc;
> @@ -120,7 +126,7 @@ static void hid_reset(struct work_struct
>
> if (test_bit(HID_CLEAR_HALT, &usbhid->iofl)) {
> dev_dbg(&usbhid->intf->dev, "clear halt\n");
> - rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->urbin->pipe);
> + rc = usb_clear_halt(hid_to_usb_dev(hid), usbhid->inurbs[0]->pipe);
> clear_bit(HID_CLEAR_HALT, &usbhid->iofl);
> hid_start_in(hid);
> }
> @@ -780,6 +786,7 @@ done:
> void usbhid_close(struct hid_device *hid)
> {
> struct usbhid_device *usbhid = hid->driver_data;
> + int i;
>
> mutex_lock(&hid_open_mut);
>
> @@ -791,7 +798,10 @@ void usbhid_close(struct hid_device *hid
> if (!--hid->open) {
> spin_unlock_irq(&usbhid->lock);
> hid_cancel_delayed_stuff(usbhid);
> - usb_kill_urb(usbhid->urbin);
> +
> + for (i = 0; i < usbhid->n_inurbs; i++)
> + usb_kill_urb(usbhid->inurbs[i]);
> +
> usbhid->intf->needs_remote_wakeup = 0;
> } else {
> spin_unlock_irq(&usbhid->lock);
> @@ -1087,6 +1097,7 @@ static int usbhid_start(struct hid_devic
> struct usb_device *dev = interface_to_usbdev(intf);
> struct usbhid_device *usbhid = hid->driver_data;
> unsigned int n, insize = 0;
> + int i;
> int ret;
>
> clear_bit(HID_DISCONNECTED, &usbhid->iofl);
> @@ -1133,16 +1144,33 @@ static int usbhid_start(struct hid_devic
> interval = hid_mousepoll_interval;
>
> ret = -ENOMEM;
> +
> + /*
> + * Some controllers need more than one input URB to prevent data from being lost
> + * while processing a prior completed URB.
> + */
> + usbhid->n_inurbs = 2;
> +
> + if (usbhid->inurbs)
> + continue;
> +
> + usbhid->inurbs = kmalloc(usbhid->n_inurbs * sizeof(struct urb *), GFP_KERNEL);
A hint, run your patch through scripts/checkpatch.pl so that people
don't complain about coding style issues next time :)
I can't see anything really wrong with this, care to resend it with a
signed-off-by: line so that Jiri can apply it (if he agrees with it?)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
