Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Tue, 23 Jul 2013, Josef Schimke wrote:
> @@ -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 submitting the second URB fails, you probably want to cancel the
first URB.
> @@ -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);
> }
Before clearing the halt, you should make sure that neither URB is
running. Probably the best way is to call usb_unlink_urb() for the
other URB whenever one gets an error.
Alan Stern
--
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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Wed, Jul 24, 2013 at 12:38:13AM -0500, Josef Schimke wrote: > On 07/23/2013 09:57 PM, Greg KH wrote: > > 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. > > Sounds good to me. :) Do you think I should re-write the patch to save > a kmalloc(), an int in the usbhid_device struct, and some complexity by > swapping my "struct urb **inurbs" to something like > "struct urb urbin[NUM_INPUT_URBS]" instead? Yes I do. > The SubmittingPatches doc confuses me a bit, so can you tell me if I > should submit patches to linux-usb when they relate to things like this, > or should I submit them straight to the main list, or...? The tool, scripts/get_maintainer.pl should be able to answer this one. Run it on your patch and you will get a list of people, and mailing lists, to cc: your patch on. I don't think it will list the linux-usb mailing list, so feel free to also add that one if you want, that's not a problem. Hope this helps, greg k-h -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On 07/23/2013 09:57 PM, Greg KH wrote: > 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. Sounds good to me. :) Do you think I should re-write the patch to save a kmalloc(), an int in the usbhid_device struct, and some complexity by swapping my "struct urb **inurbs" to something like "struct urb urbin[NUM_INPUT_URBS]" instead? I'm not trying to nitpick about my own patch, but being clear about stuff like that will definitely help me learn what's expected of my code. :) I wrote my current patch the way I did thinking that it might be useful to dynamically figure out how many URBs are needed (which is the part I couldn't do :p) - but I simply don't have the knowledge needed to be able to say for certain if that was a good idea. But I'm leaning toward the thought that >2 URBs might never be needed for this driver. Jiri, will you also give your advice to me about this too please, along with any comments about my current patch? >> ***snipped my patch here*** > > A hint, run your patch through scripts/checkpatch.pl so that people > don't complain about coding style issues next time :) Will do. :) > > 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?) I appreciate every bit of help and advice you, Alan, and (soon, I think!) Jiri have given me. :) I'd like to be as helpful as I can in the future, and you guys are setting me on the right track. I have a couple more ropes to jump over: The SubmittingPatches doc confuses me a bit, so can you tell me if I should submit patches to linux-usb when they relate to things like this, or should I submit them straight to the main list, or...? > > thanks, > > greg k-h > Regards, Josef/sgtcapslock -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
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.0 -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
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.c2013-06-30
17:13:29.0 -0500
+++ linux-3.10-dev/drivers/hid/usbhid/hid-core.c2013-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_wake
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Sun, 21 Jul 2013, sgtcapslock wrote: > I took Alan's excellent advice and read a good bit of that book last > night. Definitely some good authors there! > > After pondering Alan's diagnosis for a bit, I went to inspect the usbhid > driver code, and wound up creating a patch which works! I've tested > three different gaming mice, and they now all poll properly using the > ohci_hcd driver when the usbhid driver uses two URBs to receive input data: > > (output from the evhz utility) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > > I probably need a lot more time to research things and make sure that I > did this the "proper" way. I'm a bit scared to submit a patch for the > first time, so I'd like it to be right. > > Jiri, can I send you some private e-mail to ask for your advice about > all that? 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. Looking forward to your patches, -- Jiri Kosina SUSE Labs -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
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.c2013-06-30
17:13:29.0 -0500
+++ linux-3.10-dev/drivers/hid/usbhid/hid-core.c2013-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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Sun, Jul 21, 2013 at 02:59:41PM -0500, sgtcapslock wrote: > I took Alan's excellent advice and read a good bit of that book last > night. Definitely some good authors there! > > After pondering Alan's diagnosis for a bit, I went to inspect the usbhid > driver code, and wound up creating a patch which works! I've tested > three different gaming mice, and they now all poll properly using the > ohci_hcd driver when the usbhid driver uses two URBs to receive input data: > > (output from the evhz utility) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > > I probably need a lot more time to research things and make sure that I > did this the "proper" way. I'm a bit scared to submit a patch for the > first time, so I'd like it to be right. > > Jiri, can I send you some private e-mail to ask for your advice about > all that? Private email does not scale for kernel maintainers: http://www.arm.linux.org.uk/news/?newsitem=11 http://www.eyrie.org/~eagle/faqs/questions.html Please just submit the patch here and we will be glad to review it and see what, if anything, needs to be done to get it merged. You might want to read the file, Documentation/SubmittingPatches and run the tool, scripts/checkpatch.pl on your patch before sending it. Other than that, don't worry about public patch review, it's how the kernel is developed, not in private :) thanks, greg k-h -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
I took Alan's excellent advice and read a good bit of that book last night. Definitely some good authors there! After pondering Alan's diagnosis for a bit, I went to inspect the usbhid driver code, and wound up creating a patch which works! I've tested three different gaming mice, and they now all poll properly using the ohci_hcd driver when the usbhid driver uses two URBs to receive input data: (output from the evhz utility) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) event3: latest hz = 1000 (average hz = 1000) I probably need a lot more time to research things and make sure that I did this the "proper" way. I'm a bit scared to submit a patch for the first time, so I'd like it to be right. Jiri, can I send you some private e-mail to ask for your advice about all that? -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Sat, 20 Jul 2013, sgtcapslock wrote: > > This could be considered a bug in the usbhid driver. As far as I can > > see, the only way to fix it is to use two interrupts URBs rather than > > one. > > > > Alan Stern > > > > Thanks a lot for taking time to diagnose this, I appreciate it! I think > I understand the general gist of what you've said. > > I'd love to be able to try and solve this or to write a patch, but I'm > afraid I've never really done any kernel-related hacking before, and I'm > not too familiar with how USB works... I'm extremely interested in > writing patches for Linux in the future, so I'll be researching this. > > So if anybody wants to write a patch, or even give some tips on what I > might need to research to do this myself in the future (I don't know > what an URB is, tbh), I'd appreciate it in either case. If you don't know anything about USB programming in the Linux kernel, you've got a ways to go before you can write patches. There are resources online that can help. For example, there's a chapter on USB programming in the Linux Device Drivers book (3rd edition), which is freely available. There are also some files in the Documentation directory of the kernel source that explain how to format and submit patches. I suggest you get in touch with Jiri Kosina, the maintainer of the usbhid driver about this. He ought to be able to help you get going and explain what needs to be changed. That's why I CC'ed him on the previous email. Alan Stern -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On 07/20/2013 01:27 PM, Alan Stern wrote: > > I think I see what's going on. > > The usbhid driver uses a single interrupt URB to receive data from the > device. When the URB completes, it gets resubmitted to receive the > next packet of data. > > The OHCI controller issues interrupts at frame boundaries, and it > probably gets the information for upcoming interrupt transfers at the > beginning of a frame. Thus, suppose some data is received during frame > N. An interrupt occurs and the URB completes at the end of frame N. > The usbhid driver processes the data and then resubmits the URB. By > that time, however, it is already too late for the controller to handle > it during frame N+1. It doesn't get handled until frame N+2. Since > each frame is one millisecond, the maximum polling rate ends up being > 500 Hz. > > With EHCI controllers, interrupts occur at the boundaries of > microframes rather than frames. Thus, the usbhid driver gets the data > and processes it after only 1/8 ms has elapsed, which leaves enough > time to resubmit the URB before frame N+1 begins. > > This could be considered a bug in the usbhid driver. As far as I can > see, the only way to fix it is to use two interrupts URBs rather than > one. > > Alan Stern > Thanks a lot for taking time to diagnose this, I appreciate it! I think I understand the general gist of what you've said. I'd love to be able to try and solve this or to write a patch, but I'm afraid I've never really done any kernel-related hacking before, and I'm not too familiar with how USB works... I'm extremely interested in writing patches for Linux in the future, so I'll be researching this. So if anybody wants to write a patch, or even give some tips on what I might need to research to do this myself in the future (I don't know what an URB is, tbh), I'd appreciate it in either case. Regards! -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Fri, 19 Jul 2013 sgtcapsl...@lavabit.com wrote: > > Can you provide usbmon traces showing what happens when the mouse is > > plugged in to an OHCI controller and when it is plugged in to an EHCI > > controller? Instructions are in Documentation/usb/usbmon.txt. > > > > Alan Stern > > > > > > Absolutely. Ahh, I'm not really sure if I should send them as attachments > to this message or not, so I've decided to put them up on a paste site. > Please let me know if you want them here instead. > > Here's that same mouse (Zowie EC1 eVo) plugged into a [native ehci] port > on the board which hands it off to ohci: > > http://pastebin.com/auM1MavY > > And here it is when plugged into an external USB hub on one of the same > ports, which causes it to use the ehci driver instead: > > http://pastebin.com/1r7BQSBw I think I see what's going on. The usbhid driver uses a single interrupt URB to receive data from the device. When the URB completes, it gets resubmitted to receive the next packet of data. The OHCI controller issues interrupts at frame boundaries, and it probably gets the information for upcoming interrupt transfers at the beginning of a frame. Thus, suppose some data is received during frame N. An interrupt occurs and the URB completes at the end of frame N. The usbhid driver processes the data and then resubmits the URB. By that time, however, it is already too late for the controller to handle it during frame N+1. It doesn't get handled until frame N+2. Since each frame is one millisecond, the maximum polling rate ends up being 500 Hz. With EHCI controllers, interrupts occur at the boundaries of microframes rather than frames. Thus, the usbhid driver gets the data and processes it after only 1/8 ms has elapsed, which leaves enough time to resubmit the URB before frame N+1 begins. This could be considered a bug in the usbhid driver. As far as I can see, the only way to fix it is to use two interrupts URBs rather than one. Alan Stern -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
> Can you provide usbmon traces showing what happens when the mouse is > plugged in to an OHCI controller and when it is plugged in to an EHCI > controller? Instructions are in Documentation/usb/usbmon.txt. > > Alan Stern > > Absolutely. Ahh, I'm not really sure if I should send them as attachments to this message or not, so I've decided to put them up on a paste site. Please let me know if you want them here instead. Here's that same mouse (Zowie EC1 eVo) plugged into a [native ehci] port on the board which hands it off to ohci: http://pastebin.com/auM1MavY And here it is when plugged into an external USB hub on one of the same ports, which causes it to use the ehci driver instead: http://pastebin.com/1r7BQSBw -- 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
Re: [PROBLEM/BUG] mouse polling at only half rate on ohci_hcd
On Fri, 19 Jul 2013 sgtcapsl...@lavabit.com wrote: > This is https://bugzilla.kernel.org/show_bug.cgi?id=60586 - I'm posting it > here as directed by Greg Kroah-Hartman. > > -- > > usbhid's mousepoll parameter is setting all of the gaming mice I've tested > to exactly 1/2 of the expected polling rate that I specify, but only if > the gaming mice are using the ohci-hcd driver. > > To test polling rates, I use the evhz utility: > http://web.archive.org/web/20060623094750/http://homepages.nildram.co.uk/~kial/evhz.c > > Here is an example using a Zowie EC1 eVo gaming mouse with > usbhid.mousepoll=1: > > [ohci] > From dmesg: > [ 9998.795207] usb 7-3: new full-speed USB device number 2 using ohci_hcd > > From evhz: > event3: latest hz = 500 (average hz = 500) > event3: latest hz = 500 (average hz = 500) > event3: latest hz = 500 (average hz = 500) > event3: latest hz = 500 (average hz = 500) > > [ehci] > From dmesg: > [10217.625617] usb 2-1.1: new full-speed USB device number 5 using ehci-pci > > From evhz: > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > event3: latest hz = 1000 (average hz = 1000) > > My motherboard is an Asus Sabertooth 990FX/GEN3 R2.0, which uses the SB950 > south bridge chip to provide USB. I've also tested a Gigabyte > GA-990FXA-UD3 which uses the same south bridge chip and have the exact > same problem. Can you provide usbmon traces showing what happens when the mouse is plugged in to an OHCI controller and when it is plugged in to an EHCI controller? Instructions are in Documentation/usb/usbmon.txt. Alan Stern -- 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
