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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
