On 07/23/2013 03:26 AM, Jiri Kosina wrote:
> Hi,
> 
> as Greg mentioned already -- normally there shouldn't be any reason for 
> private e-mail, Ccing proper mailinglists is always a good idea to get as 
> much feedback as possible; feel free to drop me an e-mail.
> 

Hello,

Ahh, apologies to both of you...  I posted a response to this thread earlier
and it looks like I somehow didn't cc: either of you.  :(  I'll post again
what I posted just earlier:

----

Hello,

I'd like to post the incomplete patch I've written so far and see if
anybody has anything to say about it, and perhaps see if anyone can help
me with a couple little problems I'm having trouble with.

It's taken me a while to come back to this because I've been busy trying
to make sure that the code I've written so far isn't a complete mess.

I decided to post it in this thread since it already discusses the
problem - but for anyone who hasn't read the prior posts, here is a
summary:

On some OHCI controllers, the usbhid driver needs a second input URB to
prevent data being lost when one URB is being read on a frame boundary
and incoming interrupt data is learned about on the next frame.  I'm
completely new to programming anything for the kernel and I'm trying to
write a patch to fix this.

Here's some thoughts about what I've coded so far:

It occurs to me that this only happens on "some" controllers.  So, I
thought it'd be nice if I could find a way to test for the necessity of
a second (or maybe even a third? who knows) input URB instead of just
giving every usbhid device two of them.

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.

Please let me know if I've made even the slightest mistake so I can
learn from this!

Regards,
Josef

--

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);
+               if (!(usbhid->inurbs)) {
+                       ret = -ENOMEM;
+                       goto fail;
+               }
+
                if (usb_endpoint_dir_in(endpoint)) {
-                       if (usbhid->urbin)
-                               continue;
-                       if (!(usbhid->urbin = usb_alloc_urb(0, GFP_KERNEL)))
-                               goto fail;
                        pipe = usb_rcvintpipe(dev, endpoint->bEndpointAddress);
-                       usb_fill_int_urb(usbhid->urbin, dev, pipe, 
usbhid->inbuf, insize,
-                                        hid_irq_in, hid, interval);
-                       usbhid->urbin->transfer_dma = usbhid->inbuf_dma;
-                       usbhid->urbin->transfer_flags |= 
URB_NO_TRANSFER_DMA_MAP;
+
+                       for (i = 0; i < usbhid->n_inurbs; i++) {
+                               if (!(usbhid->inurbs[i] = usb_alloc_urb(0, 
GFP_KERNEL)))
+                                       goto fail;
+                               usb_fill_int_urb(usbhid->inurbs[i], dev, pipe, 
usbhid->inbuf, insize,
+                                                hid_irq_in, hid, interval);
+                               usbhid->inurbs[i]->transfer_dma = 
usbhid->inbuf_dma;
+                               usbhid->inurbs[i]->transfer_flags |= 
URB_NO_TRANSFER_DMA_MAP;
+                       }
                } else {
                        if (usbhid->urbout)
                                continue;
@@ -1187,10 +1215,16 @@ static int usbhid_start(struct hid_devic
        return 0;
 
 fail:
-       usb_free_urb(usbhid->urbin);
+       if (usbhid->inurbs) {
+               for (i = 0; i < usbhid->n_inurbs; i++)
+                       usb_free_urb(usbhid->inurbs[i]);
+
+               kfree(usbhid->inurbs);
+               usbhid->inurbs = NULL;
+       }
+
        usb_free_urb(usbhid->urbout);
        usb_free_urb(usbhid->urbctrl);
-       usbhid->urbin = NULL;
        usbhid->urbout = NULL;
        usbhid->urbctrl = NULL;
        hid_free_buffers(dev, hid);
@@ -1200,6 +1234,7 @@ fail:
 static void usbhid_stop(struct hid_device *hid)
 {
        struct usbhid_device *usbhid = hid->driver_data;
+       int i;
 
        if (WARN_ON(!usbhid))
                return;
@@ -1208,7 +1243,15 @@ static void usbhid_stop(struct hid_devic
        spin_lock_irq(&usbhid->lock);   /* Sync with error and led handlers */
        set_bit(HID_DISCONNECTED, &usbhid->iofl);
        spin_unlock_irq(&usbhid->lock);
-       usb_kill_urb(usbhid->urbin);
+
+       for (i = 0; i < usbhid->n_inurbs; i++) {
+               usb_kill_urb(usbhid->inurbs[i]);
+               usb_free_urb(usbhid->inurbs[i]);
+       }
+
+       kfree(usbhid->inurbs);
+       usbhid->inurbs = NULL;
+
        usb_kill_urb(usbhid->urbout);
        usb_kill_urb(usbhid->urbctrl);
 
@@ -1216,10 +1259,8 @@ static void usbhid_stop(struct hid_devic
 
        hid->claimed = 0;
 
-       usb_free_urb(usbhid->urbin);
        usb_free_urb(usbhid->urbctrl);
        usb_free_urb(usbhid->urbout);
-       usbhid->urbin = NULL; /* don't mess up next start */
        usbhid->urbctrl = NULL;
        usbhid->urbout = NULL;
 
@@ -1407,8 +1448,13 @@ static void hid_cancel_delayed_stuff(str
 
 static void hid_cease_io(struct usbhid_device *usbhid)
 {
+       int i;
+
        del_timer_sync(&usbhid->io_retry);
-       usb_kill_urb(usbhid->urbin);
+
+       for (i = 0; i < usbhid->n_inurbs; i++)
+               usb_kill_urb(usbhid->inurbs[i]);
+
        usb_kill_urb(usbhid->urbctrl);
        usb_kill_urb(usbhid->urbout);
 }
diff -uprN -X linux-3.10-vanilla/Documentation/dontdiff 
linux-3.10-vanilla/drivers/hid/usbhid/usbhid.h 
linux-3.10-dev/drivers/hid/usbhid/usbhid.h
--- linux-3.10-vanilla/drivers/hid/usbhid/usbhid.h      2013-06-30 
17:13:29.000000000 -0500
+++ linux-3.10-dev/drivers/hid/usbhid/usbhid.h  2013-07-22 06:17:24.413184441 
-0500
@@ -66,7 +66,8 @@ struct usbhid_device {
 
        unsigned int bufsize;                                           /* URB 
buffer size */
 
-       struct urb *urbin;                                              /* 
Input URB */
+       int n_inurbs;                                                   /* 
Number of input URBs */
+       struct urb **inurbs;                                            /* 
Input URBs */
        char *inbuf;                                                    /* 
Input buffer */
        dma_addr_t inbuf_dma;                                           /* 
Input buffer dma */

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to