Menu
▾
▴
[libusb] [PATCH] core: Change the process of interrupting event handlers
|
From: Chris D. <chr...@gm...> - 2014-08-30 00:17:38
|
No longer take the events lock during usbi_fd_notification, as it
is unnecessary and can lead to deadlock in certain situations, such
as a transfer callback resubmitting the transfer on a system where
the backend uses an fd for each transfer. It is sufficient to simply
write to the control pipe and let the event handler clear it.
With these changes, it is now only necessary to interrupt and block
event handlers during a device close. Thus the pollfd_modify counter
and its lock are renamed accordingly to device_close.
Signed-off-by: Chris Dickens <chr...@gm...>
---
libusb/core.c | 59 ++++++++++++---------------------------------------
libusb/io.c | 57 ++++++++++++++++++++++++++++++++-----------------
libusb/libusbi.h | 6 +++---
libusb/version_nano.h | 2 +-
4 files changed, 54 insertions(+), 70 deletions(-)
diff --git a/libusb/core.c b/libusb/core.c
index 4561740..772c219 100644
--- a/libusb/core.c
+++ b/libusb/core.c
@@ -1029,39 +1029,10 @@ void usbi_fd_notification(struct libusb_context *ctx)
unsigned char dummy = 1;
ssize_t r;
- if (ctx == NULL)
- return;
-
- /* record that we are messing with poll fds */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify++;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
-
/* write some data on control pipe to interrupt event handlers */
r = usbi_write(ctx->ctrl_pipe[1], &dummy, sizeof(dummy));
- if (r <= 0) {
+ if (r != sizeof(dummy))
usbi_warn(ctx, "internal signalling write failed");
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify--;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
- return;
- }
-
- /* take event handling lock */
- libusb_lock_events(ctx);
-
- /* read the dummy data */
- r = usbi_read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
- if (r <= 0)
- usbi_warn(ctx, "internal signalling read failed");
-
- /* we're done with modifying poll fds */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify--;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
-
- /* Release event handling lock and wake up event waiters */
- libusb_unlock_events(ctx);
}
/** \ingroup dev
@@ -1196,8 +1167,6 @@ static void do_close(struct libusb_context *ctx,
struct usbi_transfer *itransfer;
struct usbi_transfer *tmp;
- libusb_lock_events(ctx);
-
/* remove any transfers in flight that are for this device */
usbi_mutex_lock(&ctx->flying_transfers_lock);
@@ -1236,8 +1205,6 @@ static void do_close(struct libusb_context *ctx,
}
usbi_mutex_unlock(&ctx->flying_transfers_lock);
- libusb_unlock_events(ctx);
-
usbi_mutex_lock(&ctx->open_devs_lock);
list_del(&dev_handle->list);
usbi_mutex_unlock(&ctx->open_devs_lock);
@@ -1277,19 +1244,19 @@ void API_EXPORTED libusb_close(libusb_device_handle *dev_handle)
* thread from doing event handling) because we will be removing a file
* descriptor from the polling loop. */
- /* record that we are messing with poll fds */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify++;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ /* record that we are closing a device */
+ usbi_mutex_lock(&ctx->device_close_lock);
+ ctx->device_close++;
+ usbi_mutex_unlock(&ctx->device_close_lock);
/* write some data on control pipe to interrupt event handlers */
r = usbi_write(ctx->ctrl_pipe[1], &dummy, sizeof(dummy));
if (r <= 0) {
usbi_warn(ctx, "internal signalling write failed, closing anyway");
do_close(ctx, dev_handle);
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify--;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ usbi_mutex_lock(&ctx->device_close_lock);
+ ctx->device_close--;
+ usbi_mutex_unlock(&ctx->device_close_lock);
return;
}
@@ -1298,16 +1265,16 @@ void API_EXPORTED libusb_close(libusb_device_handle *dev_handle)
/* read the dummy data */
r = usbi_read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
- if (r <= 0)
+ if (r != sizeof(dummy))
usbi_warn(ctx, "internal signalling read failed, closing anyway");
/* Close the device */
do_close(ctx, dev_handle);
- /* we're done with modifying poll fds */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ctx->pollfd_modify--;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ /* we're done with closing the device */
+ usbi_mutex_lock(&ctx->device_close_lock);
+ ctx->device_close--;
+ usbi_mutex_unlock(&ctx->device_close_lock);
/* Release event handling lock and wake up event waiters */
libusb_unlock_events(ctx);
diff --git a/libusb/io.c b/libusb/io.c
index b4c630f..f84b71a 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1111,7 +1111,7 @@ int usbi_io_init(struct libusb_context *ctx)
usbi_mutex_init(&ctx->flying_transfers_lock, NULL);
usbi_mutex_init(&ctx->pollfds_lock, NULL);
- usbi_mutex_init(&ctx->pollfd_modify_lock, NULL);
+ usbi_mutex_init(&ctx->device_close_lock, NULL);
usbi_mutex_init_recursive(&ctx->events_lock, NULL);
usbi_mutex_init(&ctx->event_waiters_lock, NULL);
usbi_cond_init(&ctx->event_waiters_cond, NULL);
@@ -1168,7 +1168,7 @@ err_close_pipe:
err:
usbi_mutex_destroy(&ctx->flying_transfers_lock);
usbi_mutex_destroy(&ctx->pollfds_lock);
- usbi_mutex_destroy(&ctx->pollfd_modify_lock);
+ usbi_mutex_destroy(&ctx->device_close_lock);
usbi_mutex_destroy(&ctx->events_lock);
usbi_mutex_destroy(&ctx->event_waiters_lock);
usbi_cond_destroy(&ctx->event_waiters_cond);
@@ -1191,7 +1191,7 @@ void usbi_io_exit(struct libusb_context *ctx)
#endif
usbi_mutex_destroy(&ctx->flying_transfers_lock);
usbi_mutex_destroy(&ctx->pollfds_lock);
- usbi_mutex_destroy(&ctx->pollfd_modify_lock);
+ usbi_mutex_destroy(&ctx->device_close_lock);
usbi_mutex_destroy(&ctx->events_lock);
usbi_mutex_destroy(&ctx->event_waiters_lock);
usbi_cond_destroy(&ctx->event_waiters_cond);
@@ -1649,13 +1649,13 @@ int API_EXPORTED libusb_try_lock_events(libusb_context *ctx)
unsigned int ru;
USBI_GET_CONTEXT(ctx);
- /* is someone else waiting to modify poll fds? if so, don't let this thread
+ /* is someone else waiting to close a device? if so, don't let this thread
* start event handling */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- ru = ctx->pollfd_modify;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ usbi_mutex_lock(&ctx->device_close_lock);
+ ru = ctx->device_close;
+ usbi_mutex_unlock(&ctx->device_close_lock);
if (ru) {
- usbi_dbg("someone else is modifying poll fds");
+ usbi_dbg("someone else is closing a device");
return 1;
}
@@ -1707,8 +1707,8 @@ void API_EXPORTED libusb_unlock_events(libusb_context *ctx)
usbi_mutex_unlock(&ctx->events_lock);
/* FIXME: perhaps we should be a bit more efficient by not broadcasting
- * the availability of the events lock when we are modifying pollfds
- * (check ctx->pollfd_modify)? */
+ * the availability of the events lock when we are closing a device
+ * (check ctx->device_close)? */
usbi_mutex_lock(&ctx->event_waiters_lock);
usbi_cond_broadcast(&ctx->event_waiters_cond);
usbi_mutex_unlock(&ctx->event_waiters_lock);
@@ -1740,13 +1740,13 @@ int API_EXPORTED libusb_event_handling_ok(libusb_context *ctx)
unsigned int r;
USBI_GET_CONTEXT(ctx);
- /* is someone else waiting to modify poll fds? if so, don't let this thread
+ /* is someone else waiting to close a device? if so, don't let this thread
* continue event handling */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- r = ctx->pollfd_modify;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ usbi_mutex_lock(&ctx->device_close_lock);
+ r = ctx->device_close;
+ usbi_mutex_unlock(&ctx->device_close_lock);
if (r) {
- usbi_dbg("someone else is modifying poll fds");
+ usbi_dbg("someone else is closing a device");
return 0;
}
@@ -1768,13 +1768,13 @@ int API_EXPORTED libusb_event_handler_active(libusb_context *ctx)
unsigned int r;
USBI_GET_CONTEXT(ctx);
- /* is someone else waiting to modify poll fds? if so, don't let this thread
+ /* is someone else waiting to close a device? if so, don't let this thread
* start event handling -- indicate that event handling is happening */
- usbi_mutex_lock(&ctx->pollfd_modify_lock);
- r = ctx->pollfd_modify;
- usbi_mutex_unlock(&ctx->pollfd_modify_lock);
+ usbi_mutex_lock(&ctx->device_close_lock);
+ r = ctx->device_close;
+ usbi_mutex_unlock(&ctx->device_close_lock);
if (r) {
- usbi_dbg("someone else is modifying poll fds");
+ usbi_dbg("someone else is closing a device");
return 1;
}
@@ -2046,8 +2046,25 @@ redo_poll:
/* another thread wanted to interrupt event handling, and it succeeded!
* handle any other events that cropped up at the same time, and
* simply return */
+ ssize_t ret;
+ unsigned char dummy;
+
usbi_dbg("caught a fish on the control pipe");
+ /* clear the control pipe unless someone is closing a device.
+ * note that it is okay to read device_close outside of its lock
+ * because if this control pipe event is from the thread that is
+ * closing the device, it set this value *before* writing to the
+ * pipe */
+ if (!ctx->device_close) {
+ ret = usbi_read(ctx->ctrl_pipe[0], &dummy, sizeof(dummy));
+ if (ret != sizeof(dummy)) {
+ usbi_err(ctx, "control pipe read error %d err=%d", ret, errno);
+ r = LIBUSB_ERROR_OTHER;
+ goto handled;
+ }
+ }
+
if (0 == --r)
goto handled;
}
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
index 620ce7e..165333b 100644
--- a/libusb/libusbi.h
+++ b/libusb/libusbi.h
@@ -275,9 +275,9 @@ struct libusb_context {
usbi_mutex_t pollfds_lock;
/* a counter that is set when we want to interrupt event handling, in order
- * to modify the poll fd set. and a lock to protect it. */
- unsigned int pollfd_modify;
- usbi_mutex_t pollfd_modify_lock;
+ * to safely close a device, and a lock to protect it. */
+ unsigned int device_close;
+ usbi_mutex_t device_close_lock;
/* user callbacks for pollfd changes */
libusb_pollfd_added_cb fd_added_cb;
diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index ada73a8..79784e2 100644
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 10911
+#define LIBUSB_NANO 10913
--
1.9.0
|