> On 23 Sep 2015, at 15:08 PM, Christophe Fergeau <cferg...@redhat.com> wrote:
>
> On Sun, Aug 16, 2015 at 03:35:44PM +0300, Dmitry Fleytman wrote:
>> From: Kirill Moizik <kmoi...@redhat.com <mailto:kmoi...@redhat.com>>
>>
>> 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?
The blocking call is libusb_close().
It is safer to wait for device close operation to complete, otherwise
re-connection to the same device may be initiated while close is still in
progress which may bring execution to a different corner cases...
>
> 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 <kmoi...@redhat.com>
>> Signed-off-by: Dmitry Fleytman <dfley...@redhat.com>
>> ---
>> 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@lists.freedesktop.org <mailto:Spice-devel@lists.freedesktop.org>
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>> <http://lists.freedesktop.org/mailman/listinfo/spice-devel>
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
