[linux-usb-devel] hiddev's HIDIOCSREPORT fixed (was: [PATCH] Fixes for force feedback)]

[John Cheevers]

From: Johann Deneux <[EMAIL PROTECTED]>
Date: Thu, 08 Jan 2004 16:42:07 +0000
Subject: [linux-usb-devel] [PATCH] Fixes for force feedback support 
of logitech hid devices
Here is a patch against 2.6.1-rc1 for hid force feedback devices.

hid-core.c:
- Use INT out urbs instead of bulk out urbs. That's the way it 
should be.

Original message: 
http://www.mail-archive.com/[EMAIL PROTECTED]/msg18641.html

The original subject of this email is a terrible understatement, and
I hope other users of hiddev (and people who gave up on hiddev) won't
overlook this patch.  What is fixed here is one-third of the hiddev
API (the HIDIOCSREPORT out of HIDIOCSREPORT, HIDIOGREPORT, and events). 
This is something that appears to have been broken since hiddev was
born 3? years ago.
It's bigger than force feedback :-)         
Thanks for submitting this, Johann.

You beat me by a few days - I had fixed the same bug but hadn't
generated a patch yet.  While the fix above means most devices
will now work, there are still some improvements that hid-core needs.
The above bug (bulk urbs instead of int urbs) is wrapped by another 
nasty bug.  The bad urbs were submitted in this code from hid-core.c,
in hid_submit_report():

              if (!test_and_set_bit(HID_OUT_RUNNING, &hid->iofl))
                      hid_submit_out(hid);
              spin_unlock_irqrestore(&hid->outlock, flags);
              return;
The corresponding clear_bit(HID_OUT_RUNNING, &hid->iofl) call is made
later, in the urb's callback, hid_irq_out().  However, if hid_submit_out()
fails [which it always did because it was BULK instead of INT] then the
callback is never registered, and the bit is never cleared.  You get one
single shot, and then all further hid-out urbs are silently(!) discarded
until you reboot.
At a minimum it needs     

              if (!test_and_set_bit(HID_OUT_RUNNING, &hid->iofl)) {
                      if (hid_submit_out(hid))
                              clear_bit(HID_OUT_RUNNING, &hid->iofl);
              }
              spin_unlock_irqrestore(&hid->outlock, flags);
              return;
but that still silently discards urbs if they come in fast.  And it
it doesn't let the user know of errors if they occur.  Addressing
that requires changing the hid_submit_report() return type from void
to int, and something like
              if (test_and_set_bit(HID_OUT_RUNNING, &hid->iofl))
                      return -EBUSY;
              ret = hid_submit_out(hid));
              spin_unlock_irqrestore(&hid->outlock, flags);
              if (ret < 0) {
                      clear_bit(HID_OUT_RUNNING, &hid->iofl);
                      return ret;
              }
              else 
                      return 0;

In fact, the whole bubbling up of errors in hid could be better.
Currently the tree is:
      usb_submit_urb() returns 0 (success) or -EINVAL,ENODEV,EPIP,EMSGSIZE
    hid_submit_out retuns 0 (success) or -1
  hid_submit_report eats errors, returns void
ioctl returns 0 (success) or -EINVAL
Perhaps it should be:

      usb_submit_urb() returns 0 (success) or -EINVAL,ENODEV,EPIPE,EMSGSIZE
    hid_submit_out returns 0 (success) or -EINVAL,ENODEV,EPIPE,EMSGSIZE,EBUSY
  hid_submit_report returns 0 or -EINVAL,ENODEV,EPIPE,EMSGSIZE,EBUSY
hiddev_ioctl returns 0 or -EINVAL,ENODEV,EPIPE,EMSGSIZE,EBUSY
Why don't I just submit a completed, tested patch?  Well, I only code about
1hr per week so it may be a while.
John Cheevers
[EMAIL PROTECTED]             





-------------------------------------------------------
This SF.net email is sponsored by: Perforce Software.
Perforce is the Fast Software Configuration Management System offering
advanced branching capabilities and atomic changes on 50+ platforms.
Free Eval! http://www.perforce.com/perforce/loadprog.html
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel