On Thu, 2015-10-29 at 17:27 +0200, 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().
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;
I don't see this anywhere in this patch2. Channel objects data accesses from different threads protected with a
new lock (flows_mutex);
This refers to the earlier patches.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.
is this also referring to a previous patch?
Indeed, this commit message is referring to an earlier series and became outdated. Fixed in the next version.
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 c88322a..302a2aa 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;
+
I would just drop this hunk.
Dropped, thanks. CHANNEL_DEBUG(channel, "connecting device %04x:%04x (%p) to channel %p",
spice_usb_device_get_vid(spice_device),
spice_usb_device_get_pid(spice_device),
@@ -372,13 +392,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:
The code looks fine, but I think the commit log needs to be revised a little bitdue to (presumably) patches being split
Done.
|