On 2012.05.03 11:25, Toby Gray wrote:
> Without this change the Windows backend needed to call usbi_fd_notification()
> from within the backend's submit_transfer. This could cause deadlock
> when attempting to lock the event lock if another thread was processing
> events on the just-submitted transfer.
>
> The deadlock comes about as the thread calling libusb_submit_transfer acquires
> the transfer mutex before trying to acquire the event lock; this is the other
> order of lock acquisition from an event thread handling activity on the just
> submitted transfer. This could lead to one of two deadlocks:
>
> 1) If the transfer completes while usbi_fd_notification() is waiting for
> the event lock and the callback attempts to resubmit the transfer.
>
> 2) If the transfer timeout is hit while usbi_fd_notification() is waiting
> for the event lock then the attempt to cancel the transfer will deadlock.
>
> This patch fixes both of these deadlocks by having libusb_submit_transfer()
> only call usbi_fd_notification() after having released the transfer mutex.
Looks good to me. Thanks for sending the updated version and good call
on making sure we get a copy of the flags before we release the lock.
On 2012.05.03 11:28, Toby Gray wrote:
> Looking into it it seemed most sensible to just add another flag to
> the transfer flags; there is already at least one flag,
> USBI_TRANSFER_OS_HANDLES_TIMEOUT, which the backend uses to
> communicate some OS specific information. I'm happy to change it to
> an entirely new member if that's a better choice than polluting the
> flags.
Looks sensible to me too. Didn't realize we already had a bunch of flags
there - of course we want to extend on those while we can.
Unless there are comments, I'll integrate your patch later on today.
Thanks again,
/Pete
> Signed-off-by: Toby Gray<toby.g...@realvnc.com>
> ---
> libusb/io.c | 4 ++++
> libusb/libusbi.h | 3 +++
> libusb/os/windows_usb.c | 6 +++---
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libusb/io.c b/libusb/io.c
> index d7fae7e..ab32e70 100644
> --- a/libusb/io.c
> +++ b/libusb/io.c
> @@ -1292,6 +1292,7 @@ int API_EXPORTED libusb_submit_transfer(struct
> libusb_transfer *transfer)
> LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
> int r;
> int first;
> + int updated_fds;
>
> usbi_mutex_lock(&itransfer->lock);
> itransfer->transferred = 0;
> @@ -1325,7 +1326,10 @@ int API_EXPORTED libusb_submit_transfer(struct
> libusb_transfer *transfer)
> #endif
>
> out:
> + updated_fds = (itransfer->flags& USBI_TRANSFER_UPDATED_FDS);
> usbi_mutex_unlock(&itransfer->lock);
> + if (updated_fds)
> + usbi_fd_notification(ctx);
> return r;
> }
>
> diff --git a/libusb/libusbi.h b/libusb/libusbi.h
> index c3d2158..0aa6bf9 100644
> --- a/libusb/libusbi.h
> +++ b/libusb/libusbi.h
> @@ -347,6 +347,9 @@ enum usbi_transfer_flags {
>
> /* Operation on the transfer failed because the device disappeared */
> USBI_TRANSFER_DEVICE_DISAPPEARED = 1<< 3,
> +
> + /* Set by backend submit_transfer() if the fds in use have been updated
> */
> + USBI_TRANSFER_UPDATED_FDS = 1<< 4,
> };
>
> #define USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer) \
> diff --git a/libusb/os/windows_usb.c b/libusb/os/windows_usb.c
> index 1957964..f29f84c 100644
> --- a/libusb/os/windows_usb.c
> +++ b/libusb/os/windows_usb.c
> @@ -1770,7 +1770,7 @@ static int submit_bulk_transfer(struct usbi_transfer
> *itransfer)
> usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd,
> (short)(IS_XFERIN(transfer) ? POLLIN : POLLOUT));
>
> - usbi_fd_notification(ctx);
> + itransfer->flags |= USBI_TRANSFER_UPDATED_FDS;
> return LIBUSB_SUCCESS;
> }
>
> @@ -1790,7 +1790,7 @@ static int submit_iso_transfer(struct usbi_transfer
> *itransfer)
> usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd,
> (short)(IS_XFERIN(transfer) ? POLLIN : POLLOUT));
>
> - usbi_fd_notification(ctx);
> + itransfer->flags |= USBI_TRANSFER_UPDATED_FDS;
> return LIBUSB_SUCCESS;
> }
>
> @@ -1809,7 +1809,7 @@ static int submit_control_transfer(struct usbi_transfer
> *itransfer)
>
> usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, POLLIN);
>
> - usbi_fd_notification(ctx);
> + itransfer->flags |= USBI_TRANSFER_UPDATED_FDS;
> return LIBUSB_SUCCESS;
>
> }
------------------------------------------------------------------------------
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
