From 02a882c89076a626e689c7dd127076fa5ede36d9 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 8 Jul 2013 10:59:10 +0200 Subject: [PATCH 1/4] libusb: Properly wait for transfer cancellation on read_thread() exit While triaging libusb bugs, I took an indepth look at: https://github.com/libusbx/libusbx/issues/25 This has lead me to the conclusion that there are 2 issues with hidapi's libusb code wrt waiting for the transfer cancellation on read_thread() exit: 1) There is a race where hid_close() can successfully cancel the transfer after a read_callback() has submitted it but before read_thread() checks shutdown_thread. If this race is hit, then the libusb_cancel_transfer() in read_thread() will fail, causing read_thread() to not call libusb_handle_events() to complete the cancelled transfer. hid_close() will then free the transfer, and if later on libusb_handle_events() gets called on the same context, it will try to complete the now freed transfer. This is what I believe leads to the segfault described in https://github.com/libusbx/libusbx/issues/25 2) hidapi uses one read_thread() per hid_device, so if there are multiple hid_devices then there are multiple threads calling libusb_handle_events(), in this case there is no guarantee that a single libusb_handle_events() call will successfully lead to the cancelled transfer being completed. If the transfer completion is already handled by another read_thread() and there are no other events, then the libusb_handle_events() call will hang, and thus the pthread_join() and thus hidapi_close() will hang. As number 2 is a generic problem found in more libusb apps, libusb has gotten a new API called libusb_handle_events_completed(), which takes an extra pointer to an int, whose contents must be set to nonzero on completion by the callback, which allows waiting for the completion of a specific transfer in a race-free manner. This patch switches the waiting for the transfer's final completion to using libusb_handle_events_completed(), thereby fixing both issues. Note the while is necessary since libusb_handle_events_completed(), like libusb_handle_events(), will return as soon as it has handled *any* event. The difference with libusb_handle_events_completed() is that once it has all the necessary internal libusb locks, it checks the contents of the completed parameter, and will bail if that has become nonzero without waiting for further events. Signed-off-by: Hans de Goede --- configure.ac | 2 +- libusb/hid.c | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/configure.ac b/configure.ac index 3533bef7..8449b401 100644 --- a/configure.ac +++ b/configure.ac @@ -68,7 +68,7 @@ case $host in # HIDAPI/libusb libs AC_CHECK_LIB([rt], [clock_gettime], [LIBS_LIBUSB_PRIVATE+=" -lrt"], [hidapi_lib_error librt]) - PKG_CHECK_MODULES([libusb], [libusb-1.0], true, [hidapi_lib_error libusb-1.0]) + PKG_CHECK_MODULES([libusb], [libusb-1.0 >= 1.0.9], true, [hidapi_lib_error libusb-1.0]) LIBS_LIBUSB_PRIVATE+=" $libusb_LIBS" CFLAGS_LIBUSB+=" $libusb_CFLAGS" ;; diff --git a/libusb/hid.c b/libusb/hid.c index 94c1b3b6..dcf9d80e 100644 --- a/libusb/hid.c +++ b/libusb/hid.c @@ -105,6 +105,7 @@ struct hid_device_ { pthread_cond_t condition; pthread_barrier_t barrier; /* Ensures correct startup sequence */ int shutdown_thread; + int cancelled; struct libusb_transfer *transfer; /* List of received input reports. */ @@ -683,10 +684,12 @@ static void read_callback(struct libusb_transfer *transfer) } else if (transfer->status == LIBUSB_TRANSFER_CANCELLED) { dev->shutdown_thread = 1; + dev->cancelled = 1; return; } else if (transfer->status == LIBUSB_TRANSFER_NO_DEVICE) { dev->shutdown_thread = 1; + dev->cancelled = 1; return; } else if (transfer->status == LIBUSB_TRANSFER_TIMED_OUT) { @@ -701,6 +704,7 @@ static void read_callback(struct libusb_transfer *transfer) if (res != 0) { LOG("Unable to submit URB. libusb error code: %d\n", res); dev->shutdown_thread = 1; + dev->cancelled = 1; } } @@ -750,10 +754,10 @@ static void *read_thread(void *param) /* Cancel any transfer that may be pending. This call will fail if no transfers are pending, but that's OK. */ - if (libusb_cancel_transfer(dev->transfer) == 0) { - /* The transfer was cancelled, so wait for its completion. */ - libusb_handle_events(usb_context); - } + libusb_cancel_transfer(dev->transfer); + + while (!dev->cancelled) + libusb_handle_events_completed(usb_context, &dev->cancelled); /* Now that the read thread is stopping, Wake any threads which are waiting on data (in hid_read_timeout()). Do this under a mutex to From 4e6e9ffe864e1e7446296294c2a329bd91076529 Mon Sep 17 00:00:00 2001 From: Spencer Oliver Date: Tue, 6 Aug 2013 21:05:17 +0100 Subject: [PATCH 2/4] gitignore: add config.* to .gitignore This makes HIDAPI less noisy when used a git submodule. Signed-off-by: Spencer Oliver --- .gitignore | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index bc0410f0..b43ca58c 100644 --- a/.gitignore +++ b/.gitignore @@ -3,9 +3,7 @@ Makefile.in aclocal.m4 autom4te.cache/ -config.guess -config.h.in -config.sub +config.* configure depcomp install-sh @@ -18,8 +16,5 @@ testgui/Makefile.in windows/Makefile.in Makefile -config.h -config.log -config.status stamp-h1 libtool From 1ec76ad9bda77574fc276d40e4f978c898de1e89 Mon Sep 17 00:00:00 2001 From: Alan Ott Date: Mon, 9 Sep 2013 00:18:00 -0400 Subject: [PATCH 3/4] README: Add build dependencies Some build system dependencies were missing from the documentation. --- README.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.txt b/README.txt index 9297fb6e..b4e7c0e0 100644 --- a/README.txt +++ b/README.txt @@ -154,7 +154,7 @@ Prerequisites: If you downloaded the source directly from the git repository (using git clone), you'll need Autotools: - sudo apt-get install autotools-dev + sudo apt-get install autotools-dev autoconf automake libtool FreeBSD: --------- From a88c7244d632ed238b829968be9b765605b53c34 Mon Sep 17 00:00:00 2001 From: Alan Ott Date: Mon, 9 Sep 2013 09:39:31 -0400 Subject: [PATCH 4/4] mac: Fix incorrect device list after device removal On Mac OS X 10.8.x (Mountain Lion), if a device is connected, opened, then closed, then unplugged, subsequent calls to hid_enumerate() will still list the device (even though it's been unplugged). If the device is plugged in again, a second instance will show up in the list and often it will be impossible to open either. Github user TamToucan figured out that in hid_enumerate() if a call is made to IOHIDManagerSetDeviceMatching() before the call to IOHIDManagerCopyDevices() that this will clear the problem. Thanks to TamToucan for figuring this out. --- mac/hid.c | 1 + 1 file changed, 1 insertion(+) diff --git a/mac/hid.c b/mac/hid.c index 13109239..6768f9c2 100644 --- a/mac/hid.c +++ b/mac/hid.c @@ -420,6 +420,7 @@ struct hid_device_info HID_API_EXPORT *hid_enumerate(unsigned short vendor_id, process_pending_events(); /* Get a list of the Devices */ + IOHIDManagerSetDeviceMatching(hid_mgr, NULL); CFSetRef device_set = IOHIDManagerCopyDevices(hid_mgr); /* Convert the list into a C array so we can iterate easily. */