Core: Fix unconditional disarming of timerfd

* Existing code appears disarms the timerfd always, which cancels
  pending timeouts as soon as one packet completes.
* This fix moves the disarming of the timerfd to
  arm_timerfd_for_next_timeout(), where it is now done conditionally.
  It also avoids calling disarm outside of the above call.
* This patch also ensures that all handling of the timerfd is done
  under the flying transfers lock.
* Issue reported by Hans de Goede. For more info, see:
  https://sourceforge.net/mailarchive/message.php?msg_id=29442693

diff --git a/libusb/io.c b/libusb/io.c
index b8d445c..e6d4132 100644 (file)
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1166,14 +1166,13 @@ static int add_to_flying_list(struct usbi_transfer *transfer)
        /* 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);
-               if (timerisset(timeout))
-                       r = 1;
                goto out;
        }
 
        /* if we have infinite timeout, append to end of list */
        if (!timerisset(timeout)) {
                list_add_tail(&transfer->list, &ctx->flying_transfers);
+               /* first is irrelevant in this case */
                goto out;
        }
 
@@ -1186,15 +1185,33 @@ static int add_to_flying_list(struct usbi_transfer *transfer)
                                (cur_tv->tv_sec == timeout->tv_sec &&
                                        cur_tv->tv_usec > timeout->tv_usec)) {
                        list_add_tail(&transfer->list, &cur->list);
-                       r = first;
                        goto out;
                }
                first = 0;
        }
+       /* first is 0 at this stage (list not empty) */
 
        /* otherwise we need to be inserted at the end */
        list_add_tail(&transfer->list, &ctx->flying_transfers);
 out:
+#ifdef USBI_TIMERFD_AVAILABLE
+       if (first && usbi_using_timerfd(ctx) && timerisset(timeout)) {
+               /* if this transfer has the lowest timeout of all active transfers,
+                * rearm the timerfd with this transfer's timeout */
+               const struct itimerspec it = { {0, 0},
+                       { transfer->timeout.tv_sec, transfer->timeout.tv_usec * 1000 } };
+               usbi_dbg("arm timerfd for timeout in %dms (first in line)",
+                       USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
+               r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
+               if (r < 0) {
+                       usbi_warn(ctx, "failed to arm first timerfd (error %d)", r);
+                       r = LIBUSB_ERROR_OTHER;
+               }
+       }
+#else
+       UNUSED(first);
+#endif
+
        usbi_mutex_unlock(&ctx->flying_transfers_lock);
        return r;
 }
@@ -1272,6 +1289,62 @@ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer)
        free(itransfer);
 }
 
+#ifdef USBI_TIMERFD_AVAILABLE
+static int disarm_timerfd(struct libusb_context *ctx)
+{
+       const struct itimerspec disarm_timer = { { 0, 0 }, { 0, 0 } };
+       int r;
+
+       usbi_dbg("");
+       r = timerfd_settime(ctx->timerfd, 0, &disarm_timer, NULL);
+       if (r < 0)
+               return LIBUSB_ERROR_OTHER;
+       else
+               return 0;
+}
+
+/* iterates through the flying transfers, and rearms the timerfd based on the
+ * next upcoming timeout.
+ * must be called with flying_list locked.
+ * returns 0 if there was no timeout to arm, 1 if the next timeout was armed,
+ * or a LIBUSB_ERROR code on failure.
+ */
+static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
+{
+       struct usbi_transfer *transfer;
+
+       list_for_each_entry(transfer, &ctx->flying_transfers, list, struct usbi_transfer) {
+               struct timeval *cur_tv = &transfer->timeout;
+
+               /* if we've reached transfers of infinite timeout, then we have no
+                * arming to do */
+               if (!timerisset(cur_tv))
+                       goto disarm;
+
+               /* act on first transfer that is not already cancelled */
+               if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
+                       int r;
+                       const struct itimerspec it = { {0, 0},
+                               { cur_tv->tv_sec, cur_tv->tv_usec * 1000 } };
+                       usbi_dbg("next timeout originally %dms", USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
+                       r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
+                       if (r < 0)
+                               return LIBUSB_ERROR_OTHER;
+                       return 1;
+               }
+       }
+
+disarm:
+       return disarm_timerfd(ctx);
+}
+#else
+static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
+{
+       (void)ctx;
+       return 0;
+}
+#endif
+
 /** \ingroup asyncio
  * Submit a transfer. This function will fire off the USB transfer and then
  * return immediately.
@@ -1290,7 +1363,6 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
        struct usbi_transfer *itransfer =
                LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
        int r;
-       int first;
        int updated_fds;
 
        usbi_mutex_lock(&itransfer->lock);
@@ -1302,27 +1374,16 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
                goto out;
        }
 
-       first = add_to_flying_list(itransfer);
+       r = add_to_flying_list(itransfer);
+       if (r)
+               goto out;
        r = usbi_backend->submit_transfer(itransfer);
        if (r) {
                usbi_mutex_lock(&ctx->flying_transfers_lock);
                list_del(&itransfer->list);
+               arm_timerfd_for_next_timeout(ctx);
                usbi_mutex_unlock(&ctx->flying_transfers_lock);
        }
-#ifdef USBI_TIMERFD_AVAILABLE
-       else if (first && usbi_using_timerfd(ctx)) {
-               /* if this transfer has the lowest timeout of all active transfers,
-                * rearm the timerfd with this transfer's timeout */
-               const struct itimerspec it = { {0, 0},
-                       { itransfer->timeout.tv_sec, itransfer->timeout.tv_usec * 1000 } };
-               usbi_dbg("arm timerfd for timeout in %dms (first in line)", transfer->timeout);
-               r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
-               if (r < 0)
-                       r = LIBUSB_ERROR_OTHER;
-       }
-#else
-       (void)first;
-#endif
 
 out:
        updated_fds = (itransfer->flags & USBI_TRANSFER_UPDATED_FDS);
@@ -1372,66 +1433,6 @@ int API_EXPORTED libusb_cancel_transfer(struct libusb_transfer *transfer)
        return r;
 }
 
-#ifdef USBI_TIMERFD_AVAILABLE
-static int disarm_timerfd(struct libusb_context *ctx)
-{
-       const struct itimerspec disarm_timer = { { 0, 0 }, { 0, 0 } };
-       int r;
-
-       usbi_dbg("");
-       r = timerfd_settime(ctx->timerfd, 0, &disarm_timer, NULL);
-       if (r < 0)
-               return LIBUSB_ERROR_OTHER;
-       else
-               return 0;
-}
-
-/* iterates through the flying transfers, and rearms the timerfd based on the
- * next upcoming timeout.
- * must be called with flying_list locked.
- * returns 0 if there was no timeout to arm, 1 if the next timeout was armed,
- * or a LIBUSB_ERROR code on failure.
- */
-static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
-{
-       struct usbi_transfer *transfer;
-
-       list_for_each_entry(transfer, &ctx->flying_transfers, list, struct usbi_transfer) {
-               struct timeval *cur_tv = &transfer->timeout;
-
-               /* if we've reached transfers of infinite timeout, then we have no
-                * arming to do */
-               if (!timerisset(cur_tv))
-                       return 0;
-
-               /* act on first transfer that is not already cancelled */
-               if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
-                       int r;
-                       const struct itimerspec it = { {0, 0},
-                               { cur_tv->tv_sec, cur_tv->tv_usec * 1000 } };
-                       usbi_dbg("next timeout originally %dms", USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer)->timeout);
-                       r = timerfd_settime(ctx->timerfd, TFD_TIMER_ABSTIME, &it, NULL);
-                       if (r < 0)
-                               return LIBUSB_ERROR_OTHER;
-                       return 1;
-               }
-       }
-
-       return 0;
-}
-#else
-static int disarm_timerfd(struct libusb_context *ctx)
-{
-       (void)ctx;
-       return 0;
-}
-static int arm_timerfd_for_next_timeout(struct libusb_context *ctx)
-{
-       (void)ctx;
-       return 0;
-}
-#endif
-
 /* Handle completion of a transfer (completion might be an error condition).
  * This will invoke the user-supplied callback function, which may end up
  * freeing the transfer. Therefore you cannot use the transfer structure
@@ -1459,14 +1460,8 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
        if (usbi_using_timerfd(ctx))
                r = arm_timerfd_for_next_timeout(ctx);
        usbi_mutex_unlock(&ctx->flying_transfers_lock);
-
-       if (usbi_using_timerfd(ctx)) {
-               if (r < 0)
-                       return r;
-               r = disarm_timerfd(ctx);
-               if (r < 0)
-                       return r;
-       }
+       if (usbi_using_timerfd(ctx) && (r < 0))
+               return r;
 
        if (status == LIBUSB_TRANSFER_COMPLETED
                        && transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
@@ -1829,10 +1824,6 @@ static int handle_timerfd_trigger(struct libusb_context *ctx)
 {
        int r;
 
-       r = disarm_timerfd(ctx);
-       if (r < 0)
-               return r;
-
        usbi_mutex_lock(&ctx->flying_transfers_lock);
 
        /* process the timeout that just happened */

diff --git a/libusb/version_nano.h b/libusb/version_nano.h
index fbf5397..a43c750 100644 (file)
--- a/libusb/version_nano.h
+++ b/libusb/version_nano.h
@@ -1 +1 @@
-#define LIBUSB_NANO 10543
+#define LIBUSB_NANO 10544