diff --git a/libusb/core.c b/libusb/core.c index 2e3816c..d1e93c8 100644 --- a/libusb/core.c +++ b/libusb/core.c @@ -1351,10 +1351,10 @@ static void do_close(struct libusb_context *ctx, if (transfer->dev_handle != dev_handle) continue; - if (!(itransfer->flags & USBI_TRANSFER_DEVICE_DISAPPEARED)) { + if (!(itransfer->state_flags & USBI_TRANSFER_DEVICE_DISAPPEARED)) { usbi_err(ctx, "Device handle closed while transfer was still being processed, but the device is still connected as far as we know"); - if (itransfer->flags & USBI_TRANSFER_CANCELLING) + if (itransfer->state_flags & USBI_TRANSFER_CANCELLING) usbi_warn(ctx, "A cancellation for an in-flight transfer hasn't completed but closing the device handle"); else usbi_err(ctx, "A cancellation hasn't even been scheduled on the transfer for which the device is closing"); diff --git a/libusb/io.c b/libusb/io.c index 3bd1675..3757f44 100644 --- a/libusb/io.c +++ b/libusb/io.c @@ -1269,7 +1269,6 @@ struct libusb_transfer * LIBUSB_CALL libusb_alloc_transfer( itransfer->num_iso_packets = iso_packets; usbi_mutex_init(&itransfer->lock); - usbi_mutex_init(&itransfer->flags_lock); transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); usbi_dbg("transfer %p", transfer); return transfer; @@ -1304,7 +1303,6 @@ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer) itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); usbi_mutex_destroy(&itransfer->lock); - usbi_mutex_destroy(&itransfer->flags_lock); free(itransfer); } @@ -1339,8 +1337,8 @@ static int arm_timerfd_for_next_timeout(struct libusb_context *ctx) if (!timerisset(cur_tv)) goto disarm; - /* act on first transfer that is not already cancelled */ - if (!(transfer->flags & USBI_TRANSFER_TIMEOUT_HANDLED)) { + /* act on first transfer that has not already been handled */ + if (!(transfer->timeout_flags & USBI_TRANSFER_TIMEOUT_HANDLED)) { int r; const struct itimerspec it = { {0, 0}, { cur_tv->tv_sec, cur_tv->tv_usec * 1000 } }; @@ -1374,8 +1372,6 @@ static int add_to_flying_list(struct usbi_transfer *transfer) int r = 0; int first = 1; - usbi_mutex_lock(&ctx->flying_transfers_lock); - /* if we have no other flying transfers, start the list with this one */ if (list_empty(&ctx->flying_transfers)) { list_add(&transfer->list, &ctx->flying_transfers); @@ -1428,7 +1424,6 @@ out: if (r) list_del(&transfer->list); - usbi_mutex_unlock(&ctx->flying_transfers_lock); return r; } @@ -1471,62 +1466,79 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer) { struct usbi_transfer *itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer); - int remove = 0; + struct libusb_context *ctx = TRANSFER_CTX(transfer); int r; usbi_dbg("transfer %p", transfer); + + /* + * Important note on locking, this function takes / releases locks + * in the following order: + * take flying_transfers_lock + * take itransfer->lock + * clear transfer + * add to flying_transfers list + * release flying_transfers_lock + * submit transfer + * release itransfer->lock + * if submit failed: + * take flying_transfers_lock + * remove from flying_transfers list + * release flying_transfers_lock + * + * Note that it takes locks in the order a-b and then releases them + * in the same order a-b. This is somewhat unusual but not wrong, + * release order is not important as long as *all* locks are released + * before re-acquiring any locks. + * + * This means that the ordering of first releasing itransfer->lock + * and then re-acquiring the flying_transfers_list on error is + * important and must not be changed! + * + * This is done this way because when we take both locks we must always + * take flying_transfers_lock first to avoid ab-ba style deadlocks with + * the timeout handling and usbi_handle_disconnect paths. + * + * And we cannot release itransfer->lock before the submission is + * complete otherwise timeout handling for transfers with short + * timeouts may run before submission. + */ + usbi_mutex_lock(&ctx->flying_transfers_lock); usbi_mutex_lock(&itransfer->lock); - usbi_mutex_lock(&itransfer->flags_lock); - if (itransfer->flags & USBI_TRANSFER_IN_FLIGHT) { - r = LIBUSB_ERROR_BUSY; - goto out; + if (itransfer->state_flags & USBI_TRANSFER_IN_FLIGHT) { + usbi_mutex_unlock(&ctx->flying_transfers_lock); + usbi_mutex_unlock(&itransfer->lock); + return LIBUSB_ERROR_BUSY; } itransfer->transferred = 0; - itransfer->flags = 0; + itransfer->state_flags = 0; + itransfer->timeout_flags = 0; r = calculate_timeout(itransfer); if (r < 0) { - r = LIBUSB_ERROR_OTHER; - goto out; + usbi_mutex_unlock(&ctx->flying_transfers_lock); + usbi_mutex_unlock(&itransfer->lock); + return LIBUSB_ERROR_OTHER; } - itransfer->flags |= USBI_TRANSFER_SUBMITTING; - usbi_mutex_unlock(&itransfer->flags_lock); r = add_to_flying_list(itransfer); if (r) { - usbi_mutex_lock(&itransfer->flags_lock); - itransfer->flags = 0; - goto out; + usbi_mutex_unlock(&ctx->flying_transfers_lock); + usbi_mutex_unlock(&itransfer->lock); + return r; } + usbi_mutex_unlock(&ctx->flying_transfers_lock); - /* keep a reference to this device */ - libusb_ref_device(transfer->dev_handle->dev); r = usbi_backend->submit_transfer(itransfer); - - usbi_mutex_lock(&itransfer->flags_lock); - itransfer->flags &= ~USBI_TRANSFER_SUBMITTING; if (r == LIBUSB_SUCCESS) { - /* check for two possible special conditions: - * 1) device disconnect occurred immediately after submission - * 2) transfer completed before we got here to update the flags - */ - if (itransfer->flags & USBI_TRANSFER_DEVICE_DISAPPEARED) { - usbi_backend->clear_transfer_priv(itransfer); - remove = 1; - r = LIBUSB_ERROR_NO_DEVICE; - } - else if (!(itransfer->flags & USBI_TRANSFER_COMPLETED)) { - itransfer->flags |= USBI_TRANSFER_IN_FLIGHT; - } - } else { - remove = 1; - } -out: - usbi_mutex_unlock(&itransfer->flags_lock); - if (remove) { - libusb_unref_device(transfer->dev_handle->dev); - remove_from_flying_list(itransfer); + itransfer->state_flags |= USBI_TRANSFER_IN_FLIGHT; + /* keep a reference to this device */ + libusb_ref_device(transfer->dev_handle->dev); } usbi_mutex_unlock(&itransfer->lock); + + if (r != LIBUSB_SUCCESS) + remove_from_flying_list(itransfer); + return r; } @@ -1552,9 +1564,8 @@ int API_EXPORTED libusb_cancel_transfer(struct libusb_transfer *transfer) usbi_dbg("transfer %p", transfer ); usbi_mutex_lock(&itransfer->lock); - usbi_mutex_lock(&itransfer->flags_lock); - if (!(itransfer->flags & USBI_TRANSFER_IN_FLIGHT) - || (itransfer->flags & USBI_TRANSFER_CANCELLING)) { + if (!(itransfer->state_flags & USBI_TRANSFER_IN_FLIGHT) + || (itransfer->state_flags & USBI_TRANSFER_CANCELLING)) { r = LIBUSB_ERROR_NOT_FOUND; goto out; } @@ -1568,13 +1579,12 @@ int API_EXPORTED libusb_cancel_transfer(struct libusb_transfer *transfer) usbi_dbg("cancel transfer failed error %d", r); if (r == LIBUSB_ERROR_NO_DEVICE) - itransfer->flags |= USBI_TRANSFER_DEVICE_DISAPPEARED; + itransfer->state_flags |= USBI_TRANSFER_DEVICE_DISAPPEARED; } - itransfer->flags |= USBI_TRANSFER_CANCELLING; + itransfer->state_flags |= USBI_TRANSFER_CANCELLING; out: - usbi_mutex_unlock(&itransfer->flags_lock); usbi_mutex_unlock(&itransfer->lock); return r; } @@ -1637,10 +1647,9 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer, if (r < 0) usbi_err(ITRANSFER_CTX(itransfer), "failed to set timer for next timeout, errno=%d", errno); - usbi_mutex_lock(&itransfer->flags_lock); - itransfer->flags &= ~USBI_TRANSFER_IN_FLIGHT; - itransfer->flags |= USBI_TRANSFER_COMPLETED; - usbi_mutex_unlock(&itransfer->flags_lock); + usbi_mutex_lock(&itransfer->lock); + itransfer->state_flags &= ~USBI_TRANSFER_IN_FLIGHT; + usbi_mutex_unlock(&itransfer->lock); if (status == LIBUSB_TRANSFER_COMPLETED && transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) { @@ -1676,7 +1685,7 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer, int usbi_handle_transfer_cancellation(struct usbi_transfer *transfer) { /* if the URB was cancelled due to timeout, report timeout to the user */ - if (transfer->flags & USBI_TRANSFER_TIMED_OUT) { + if (transfer->timeout_flags & USBI_TRANSFER_TIMED_OUT) { usbi_dbg("detected timeout cancellation"); return usbi_handle_transfer_completion(transfer, LIBUSB_TRANSFER_TIMED_OUT); } @@ -1967,10 +1976,10 @@ static void handle_timeout(struct usbi_transfer *itransfer) USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); int r; - itransfer->flags |= USBI_TRANSFER_TIMEOUT_HANDLED; + itransfer->timeout_flags |= USBI_TRANSFER_TIMEOUT_HANDLED; r = libusb_cancel_transfer(transfer); if (r == 0) - itransfer->flags |= USBI_TRANSFER_TIMED_OUT; + itransfer->timeout_flags |= USBI_TRANSFER_TIMED_OUT; else usbi_warn(TRANSFER_CTX(transfer), "async cancel failed %d errno=%d", r, errno); @@ -2003,7 +2012,7 @@ static int handle_timeouts_locked(struct libusb_context *ctx) return 0; /* ignore timeouts we've already handled */ - if (transfer->flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT)) + if (transfer->timeout_flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT)) continue; /* if transfer has non-expired timeout, nothing more to do */ @@ -2549,7 +2558,7 @@ int API_EXPORTED libusb_get_next_timeout(libusb_context *ctx, /* find next transfer which hasn't already been processed as timed out */ list_for_each_entry(transfer, &ctx->flying_transfers, list, struct usbi_transfer) { - if (transfer->flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT)) + if (transfer->timeout_flags & (USBI_TRANSFER_TIMEOUT_HANDLED | USBI_TRANSFER_OS_HANDLES_TIMEOUT)) continue; /* if we've reached transfers of infinte timeout, we're done looking */ @@ -2766,9 +2775,10 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle) * possible scenarios: * 1. the transfer is currently in-flight, in which case we terminate the * transfer here - * 2. the transfer is not in-flight (or is but hasn't been marked as such), - * in which case we record that the device disappeared and this will be - * handled by libusb_submit_transfer() + * 2. the transfer has been added to the flying transfer list by + * libusb_submit_transfer, has failed to submit and + * libusb_submit_transfer is waiting for us to release the + * flying_transfers_lock to remove it, so we ignore it */ while (1) { @@ -2776,12 +2786,10 @@ void usbi_handle_disconnect(struct libusb_device_handle *dev_handle) usbi_mutex_lock(&HANDLE_CTX(dev_handle)->flying_transfers_lock); list_for_each_entry(cur, &HANDLE_CTX(dev_handle)->flying_transfers, list, struct usbi_transfer) if (USBI_TRANSFER_TO_LIBUSB_TRANSFER(cur)->dev_handle == dev_handle) { - usbi_mutex_lock(&cur->flags_lock); - if (cur->flags & USBI_TRANSFER_IN_FLIGHT) + usbi_mutex_lock(&cur->lock); + if (cur->state_flags & USBI_TRANSFER_IN_FLIGHT) to_cancel = cur; - else - cur->flags |= USBI_TRANSFER_DEVICE_DISAPPEARED; - usbi_mutex_unlock(&cur->flags_lock); + usbi_mutex_unlock(&cur->lock); if (to_cancel) break; diff --git a/libusb/libusbi.h b/libusb/libusbi.h index ca54a46..cc0906c 100644 --- a/libusb/libusbi.h +++ b/libusb/libusbi.h @@ -277,6 +277,8 @@ struct libusb_context { * the list, URBs that will time out later are placed after, and urbs with * infinite timeout are always placed at the very end. */ struct list_head flying_transfers; + /* Note paths taking both this and usbi_transfer->lock must always + * take this lock first */ usbi_mutex_t flying_transfers_lock; /* user callbacks for pollfd changes */ @@ -440,7 +442,8 @@ struct usbi_transfer { struct timeval timeout; int transferred; uint32_t stream_id; - uint8_t flags; + uint8_t state_flags; /* Protected by usbi_transfer->lock */ + uint8_t timeout_flags; /* Protected by the flying_stransfers_lock */ /* this lock is held during libusb_submit_transfer() and * libusb_cancel_transfer() (allowing the OS backend to prevent duplicate @@ -448,38 +451,32 @@ struct usbi_transfer { * should also take this lock in the handle_events path, to prevent the user * cancelling the transfer from another thread while you are processing * its completion (presumably there would be races within your OS backend - * if this were possible). */ + * if this were possible). + * Note paths taking both this and the flying_transfers_lock must + * always take the flying_transfers_lock first */ usbi_mutex_t lock; - - /* this lock should be held whenever viewing or modifying flags - * relating to the transfer state */ - usbi_mutex_t flags_lock; }; -enum usbi_transfer_flags { - /* The transfer has timed out */ - USBI_TRANSFER_TIMED_OUT = 1 << 0, - - /* Set by backend submit_transfer() if the OS handles timeout */ - USBI_TRANSFER_OS_HANDLES_TIMEOUT = 1 << 1, +enum usbi_transfer_state_flags { + /* Transfer successfully submitted by backend */ + USBI_TRANSFER_IN_FLIGHT = 1 << 0, /* Cancellation was requested via libusb_cancel_transfer() */ - USBI_TRANSFER_CANCELLING = 1 << 2, + USBI_TRANSFER_CANCELLING = 1 << 1, /* Operation on the transfer failed because the device disappeared */ - USBI_TRANSFER_DEVICE_DISAPPEARED = 1 << 3, + USBI_TRANSFER_DEVICE_DISAPPEARED = 1 << 2, +}; - /* Transfer is currently being submitted */ - USBI_TRANSFER_SUBMITTING = 1 << 4, - - /* Transfer successfully submitted by backend */ - USBI_TRANSFER_IN_FLIGHT = 1 << 5, - - /* Completion handler has run */ - USBI_TRANSFER_COMPLETED = 1 << 6, +enum usbi_transfer_timeout_flags { + /* Set by backend submit_transfer() if the OS handles timeout */ + USBI_TRANSFER_OS_HANDLES_TIMEOUT = 1 << 0, /* The transfer timeout has been handled */ - USBI_TRANSFER_TIMEOUT_HANDLED = 1 << 7, + USBI_TRANSFER_TIMEOUT_HANDLED = 1 << 1, + + /* The transfer timeout was successfully processed */ + USBI_TRANSFER_TIMED_OUT = 1 << 2, }; #define USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer) \ diff --git a/libusb/os/darwin_usb.c b/libusb/os/darwin_usb.c index 4e4ab04..7912282 100644 --- a/libusb/os/darwin_usb.c +++ b/libusb/os/darwin_usb.c @@ -1561,7 +1561,7 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer) { ret = (*(cInterface->interface))->WritePipeAsync(cInterface->interface, pipeRef, transfer->buffer, transfer->length, darwin_async_io_callback, itransfer); } else { - itransfer->flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; + itransfer->timeout_flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; if (IS_XFERIN(transfer)) ret = (*(cInterface->interface))->ReadPipeAsyncTO(cInterface->interface, pipeRef, transfer->buffer, @@ -1593,7 +1593,7 @@ static int submit_stream_transfer(struct usbi_transfer *itransfer) { return LIBUSB_ERROR_NOT_FOUND; } - itransfer->flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; + itransfer->timeout_flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; if (IS_XFERIN(transfer)) ret = (*(cInterface->interface))->ReadStreamsPipeAsyncTO(cInterface->interface, pipeRef, itransfer->stream_id, @@ -1721,7 +1721,7 @@ static int submit_control_transfer(struct usbi_transfer *itransfer) { tpriv->req.completionTimeout = transfer->timeout; tpriv->req.noDataTimeout = transfer->timeout; - itransfer->flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; + itransfer->timeout_flags |= USBI_TRANSFER_OS_HANDLES_TIMEOUT; /* all transfers in libusb-1.0 are async */ @@ -1870,7 +1870,7 @@ static void darwin_async_io_callback (void *refcon, IOReturn result, void *arg0) } static int darwin_transfer_status (struct usbi_transfer *itransfer, kern_return_t result) { - if (itransfer->flags & USBI_TRANSFER_TIMED_OUT) + if (itransfer->timeout_flags & USBI_TRANSFER_TIMED_OUT) result = kIOUSBTransactionTimeout; switch (result) { @@ -1887,7 +1887,7 @@ static int darwin_transfer_status (struct usbi_transfer *itransfer, kern_return_ return LIBUSB_TRANSFER_OVERFLOW; case kIOUSBTransactionTimeout: usbi_warn (ITRANSFER_CTX (itransfer), "transfer error: timed out"); - itransfer->flags |= USBI_TRANSFER_TIMED_OUT; + itransfer->timeout_flags |= USBI_TRANSFER_TIMED_OUT; return LIBUSB_TRANSFER_TIMED_OUT; default: usbi_warn (ITRANSFER_CTX (itransfer), "transfer error: %s (value = 0x%08x)", darwin_error_str (result), result); diff --git a/libusb/version_nano.h b/libusb/version_nano.h index 3fdb625..de73e86 100644 --- a/libusb/version_nano.h +++ b/libusb/version_nano.h @@ -1 +1 @@ -#define LIBUSB_NANO 11131 +#define LIBUSB_NANO 11132