On 2012.07.13 01:27, Pete Batard wrote:
What will happen then is I'll submit a proposal to this list with all
the enhancements we talked about sometime next week
Here we go.
I'm still not entirely sure disarming the timerfd in
handle_timerfd_trigger() is that beneficial, but either way, it
shouldn't matter much.
The other item I'm also pondering about is the aborting of the transfer
submission in libusb_submit_transfer(), if an error returned from
add_to_flying_list(itransfer) as the only error currently returned by
this call is if we failed to arm the timer, which isn't consider
critical. Then again, if we alter add_to_flying_list() in the future, we
may return more consequential errors...
Regards,
/Pete
PS: I have now pushed the fix for the possible crash when enumerating
HID devices on Windows, as well as the one to set root hubs with device
number 1.
>From 198bea16c161289f6caba857219677f7be56fe16 Mon Sep 17 00:00:00 2001
From: Pete Batard <p...@akeo.ie>
Date: Tue, 10 Jul 2012 01:59:45 +0100
Subject: [PATCH] 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.
* 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
---
libusb/io.c | 73 ++++++++++++++++++++++++---------------------------
1 file changed, 36 insertions(+), 39 deletions(-)
diff --git a/libusb/io.c b/libusb/io.c
index b8d445c..e034a6e 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1159,7 +1159,7 @@ static int add_to_flying_list(struct usbi_transfer
*transfer)
struct timeval *timeout = &transfer->timeout;
struct libusb_context *ctx = ITRANSFER_CTX(transfer);
int r = 0;
- int first = 1;
+ int first = 0;
usbi_mutex_lock(&ctx->flying_transfers_lock);
@@ -1167,7 +1167,7 @@ static int add_to_flying_list(struct usbi_transfer
*transfer)
if (list_empty(&ctx->flying_transfers)) {
list_add(&transfer->list, &ctx->flying_transfers);
if (timerisset(timeout))
- r = 1;
+ first = 1;
goto out;
}
@@ -1186,15 +1186,32 @@ 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;
+ first = 1;
goto out;
}
- first = 0;
}
/* 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)) {
+ /* 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;
}
@@ -1290,7 +1307,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 +1318,15 @@ 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);
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);
@@ -1402,7 +1406,7 @@ static int arm_timerfd_for_next_timeout(struct
libusb_context *ctx)
/* if we've reached transfers of infinite timeout, then we have
no
* arming to do */
if (!timerisset(cur_tv))
- return 0;
+ goto disarm;
/* act on first transfer that is not already cancelled */
if (!(transfer->flags & USBI_TRANSFER_TIMED_OUT)) {
@@ -1417,14 +1421,10 @@ static int arm_timerfd_for_next_timeout(struct
libusb_context *ctx)
}
}
- return 0;
+disarm:
+ return disarm_timerfd(ctx);
}
#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;
@@ -1456,17 +1456,14 @@ int usbi_handle_transfer_completion(struct
usbi_transfer *itransfer,
usbi_mutex_lock(&ctx->flying_transfers_lock);
list_del(&itransfer->list);
- 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)
+ r = arm_timerfd_for_next_timeout(ctx);
+ if (r < 0) {
+ usbi_mutex_unlock(&ctx->flying_transfers_lock);
return r;
+ }
}
+ usbi_mutex_unlock(&ctx->flying_transfers_lock);
if (status == LIBUSB_TRANSFER_COMPLETED
&& transfer->flags & LIBUSB_TRANSFER_SHORT_NOT_OK) {
@@ -1829,11 +1826,11 @@ static int handle_timerfd_trigger(struct libusb_context
*ctx)
{
int r;
+ usbi_mutex_lock(&ctx->flying_transfers_lock);
+
r = disarm_timerfd(ctx);
if (r < 0)
- return r;
-
- usbi_mutex_lock(&ctx->flying_transfers_lock);
+ goto out;
/* process the timeout that just happened */
r = handle_timeouts_locked(ctx);
--
1.7.11.msysgit.0
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
