On Sun, Aug 16, 2015 at 03:35:44PM +0300, Dmitry Fleytman wrote:
> From: Kirill Moizik <kmoizik@xxxxxxxxxx>
>
> On Windows when using usbdk, opening and closing USB device handle,
> i.e. calling libusb_open()/libusb_unref_device() can block for a few
> seconds (3-5 second more specifically on patch author's HW).
>
> libusb_open() is called by spice_usbredir_channel_open_device().
>
> libusb_unref_device() is called implicitely via
> usbredirhost_set_device(..., NULL) from
> spice_usbredir_channel_disconnect_device().
Is this libusb_unref_device() or libusb_close() which can block?
spice_usbredir_channel_disconnect_device() does:
/* This also closes the libusb handle we passed from open_device */
usbredirhost_set_device(priv->host, NULL);
libusb_unref_device(priv->device);
so the libusb_close() call is done by usbredirhost_set_device() while
the libusb_unref_device() call is done explicitly in
spice_usbredir_channel_disconnect_device()
If what is blocking is libusb_unref_device(), I would not bother making
disconnect() async by running in a thread, but I'd just spawn a thread
to do the libusb_unref_device() call. I don't think it's important that
we wait until the call completes, so _disconnect() would not need to
change apart from this delayed unref.
If the blocking call is libusb_close(), I don't know if the same
approach can be used with usbredirhost_set_device? Has this been tried?
Christophe
>
> Both these calls happen on the main thread. If it blocks,
> this causes the UI to freeze.
>
> This commit makes sure libusb_open() is called from a different
> thread to avoid blocking the mainloop freezes when usbdk is used.
>
> Following commits also move usbredirhost_set_device(..., NULL) call
> to the different threads.
>
> Since this commit introduces additional execution contexts running in
> parallell to the main thread threre are thread safety concerns to be secured.
> Mainly there are 3 types of objects accessed by newly introduced threads:
> 1. libusb contexts
> 2. usbredir context
> 3. redirection channels
>
> Fortunately libusb accesses either thread safe of already performed by
> a separate thread and protected by locks as needed.
>
> As for channels and usbredir, in order to achieve thread safety this patch introduces
> additional locks and references, namely:
>
> 1. To make sure channel objects are not disposed while redirection in progress,
> they are referenced on flow start and unreferenced on flow completion;
> 2. Channel objects data accesses from different threads protected with a
> new lock (flows_mutex);
> 3. Handling usbredir messages protected by the same new lock in order to
> ensure there are no messages processing flows in progres when device gets
> connected or disconneced.
>
> Signed-off-by: Kirill Moizik <kmoizik@xxxxxxxxxx>
> Signed-off-by: Dmitry Fleytman <dfleytma@xxxxxxxxxx>
> ---
> src/channel-usbredir.c | 41 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index bbfd2b0..44da3ce 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -315,6 +315,27 @@ static void spice_usbredir_channel_open_acl_cb(
> }
> #endif
>
> +#ifndef USE_POLKIT
> +static void
> +_open_device_async_cb(GSimpleAsyncResult *simple,
> + GObject *object,
> + GCancellable *cancellable)
> +{
> + GError *err = NULL;
> + SpiceUsbredirChannel *channel = SPICE_USBREDIR_CHANNEL(object);
> + SpiceUsbredirChannelPrivate *priv = channel->priv;
> + g_mutex_lock(priv->flows_mutex);
> + if (!spice_usbredir_channel_open_device(channel, &err)) {
> + g_simple_async_result_take_error(simple, err);
> + libusb_unref_device(priv->device);
> + priv->device = NULL;
> + g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> + priv->spice_device = NULL;
> + }
> + g_mutex_unlock(priv->flows_mutex);
> +}
> +#endif
> +
> G_GNUC_INTERNAL
> void spice_usbredir_channel_connect_device_async(
> SpiceUsbredirChannel *channel,
> @@ -324,15 +345,14 @@ void spice_usbredir_channel_connect_device_async(
> GAsyncReadyCallback callback,
> gpointer user_data)
> {
> - SpiceUsbredirChannelPrivate *priv = channel->priv;
> + SpiceUsbredirChannelPrivate *priv;
> GSimpleAsyncResult *result;
> -#if ! USE_POLKIT
> - GError *err = NULL;
> -#endif
>
> g_return_if_fail(SPICE_IS_USBREDIR_CHANNEL(channel));
> g_return_if_fail(device != NULL);
>
> + priv = channel->priv;
> +
> CHANNEL_DEBUG(channel, "connecting usb channel %p", channel);
>
> result = g_simple_async_result_new(G_OBJECT(channel), callback, user_data,
> @@ -369,13 +389,12 @@ void spice_usbredir_channel_connect_device_async(
> channel);
> return;
> #else
> - if (!spice_usbredir_channel_open_device(channel, &err)) {
> - g_simple_async_result_take_error(result, err);
> - libusb_unref_device(priv->device);
> - priv->device = NULL;
> - g_boxed_free(spice_usb_device_get_type(), priv->spice_device);
> - priv->spice_device = NULL;
> - }
> + g_simple_async_result_run_in_thread(result,
> + _open_device_async_cb,
> + G_PRIORITY_DEFAULT,
> + cancellable);
> + g_object_unref(result);
> + return;
> #endif
>
> done:
> --
> 2.4.3
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgp4Ni0xYyEjV.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel