From d92e34f3393c82414e58dc571677b60455242c3b Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Wed, 5 Oct 2022 12:30:56 +0100 Subject: [PATCH] Optionally use GUsbSource on Linux to avoid threading issues Having two threads interact is causing difficult to debug deadlocks. Just use libusb_set_pollfd_notifiers() and use the locking in libusb_handle_events(). By default use a thread to avoid breaking other projects like fprintd that expect to be able to manually process the default GMainContext. --- gusb/gusb-context.c | 76 ++++++++++++++++-- gusb/gusb-context.h | 4 +- gusb/gusb-device.c | 9 +++ gusb/gusb-source-private.h | 20 +++++ gusb/gusb-source.c | 157 ++++++++++++++++++++++++++++++++++++- gusb/libgusb.ver | 6 ++ tools/gusb-main.c | 7 +- 7 files changed, 266 insertions(+), 13 deletions(-) create mode 100644 gusb/gusb-source-private.h diff --git a/gusb/gusb-context.c b/gusb/gusb-context.c index f19fd52..edfc8f6 100644 --- a/gusb/gusb-context.c +++ b/gusb/gusb-context.c @@ -19,9 +19,10 @@ #include "gusb-context-private.h" #include "gusb-device-private.h" +#include "gusb-source-private.h" #include "gusb-util.h" -enum { PROP_0, PROP_LIBUSB_CONTEXT, PROP_DEBUG_LEVEL, N_PROPERTIES }; +enum { PROP_0, PROP_LIBUSB_CONTEXT, PROP_DEBUG_LEVEL, PROP_FLAGS, N_PROPERTIES }; enum { DEVICE_ADDED_SIGNAL, DEVICE_REMOVED_SIGNAL, LAST_SIGNAL }; @@ -43,6 +44,8 @@ typedef struct { GThread *thread_event; gboolean done_enumerate; volatile gint thread_event_run; + GMutex source_mutex; + GUsbSource *source; guint hotplug_poll_id; guint hotplug_poll_interval; int debug_level; @@ -119,7 +122,7 @@ g_usb_context_dispose(GObject *object) GUsbContextPrivate *priv = GET_PRIVATE(self); /* this is safe to call even when priv->hotplug_id is unset */ - if (g_atomic_int_dec_and_test(&priv->thread_event_run)) { + if (priv->thread_event != NULL && g_atomic_int_dec_and_test(&priv->thread_event_run)) { libusb_hotplug_deregister_callback(priv->ctx, priv->hotplug_id); g_thread_join(priv->thread_event); } @@ -132,6 +135,10 @@ g_usb_context_dispose(GObject *object) g_source_remove(priv->idle_events_id); priv->idle_events_id = 0; } + if (priv->source != NULL) { + _g_usb_source_destroy(priv->source); + priv->source = NULL; + } g_clear_pointer(&priv->main_ctx, g_main_context_unref); g_clear_pointer(&priv->devices, g_ptr_array_unref); @@ -166,6 +173,9 @@ g_usb_context_get_property(GObject *object, guint prop_id, GValue *value, GParam case PROP_DEBUG_LEVEL: g_value_set_int(value, priv->debug_level); break; + case PROP_FLAGS: + g_value_set_uint64(value, priv->flags); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -187,6 +197,9 @@ g_usb_context_set_property(GObject *object, guint prop_id, const GValue *value, libusb_set_debug(priv->ctx, priv->debug_level); #endif break; + case PROP_FLAGS: + g_usb_context_set_flags(self, g_value_get_uint64(value)); + break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; @@ -215,6 +228,12 @@ g_usb_context_class_init(GUsbContextClass *klass) pspecs[PROP_DEBUG_LEVEL] = g_param_spec_int("debug_level", NULL, NULL, 0, 3, 0, G_PARAM_READWRITE); + /** + * GUsbContext:flags: + */ + pspecs[PROP_FLAGS] = + g_param_spec_uint64("flags", NULL, NULL, 0, G_MAXUINT64, 0, G_PARAM_READWRITE); + g_object_class_install_properties(object_class, N_PROPERTIES, pspecs); /** @@ -767,6 +786,9 @@ g_usb_context_enumerate(GUsbContext *self) g_ptr_array_index(priv->devices, i)); } + /* setup with the default mainloop if not already done */ + g_usb_context_get_source(self, NULL); + /* any queued up hotplug events are queued as idle handlers */ } @@ -784,7 +806,10 @@ void g_usb_context_set_flags(GUsbContext *self, GUsbContextFlags flags) { GUsbContextPrivate *priv = GET_PRIVATE(self); + if (priv->flags == flags) + return; priv->flags = flags; + g_object_notify_by_pspec(G_OBJECT(self), pspecs[PROP_FLAGS]); } /** @@ -831,6 +856,7 @@ g_usb_context_init(GUsbContext *self) priv->devices_removed = g_ptr_array_new_with_free_func((GDestroyNotify)g_object_unref); priv->dict_usb_ids = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, g_free); priv->dict_replug = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL); + g_mutex_init(&priv->source_mutex); /* to escape the thread into the mainloop */ g_rec_mutex_init(&priv->idle_events_mutex); @@ -859,8 +885,13 @@ g_usb_context_initable_init(GInitable *initable, GCancellable *cancellable, GErr priv->main_ctx = g_main_context_ref(g_main_context_default()); priv->ctx = ctx; - priv->thread_event_run = 1; - priv->thread_event = g_thread_new("GUsbEventThread", g_usb_context_event_thread_cb, self); + + /* FreeBSD cannot use libusb_set_pollfd_notifiers(), so use a thread instead */ + if (priv->flags & G_USB_CONTEXT_FLAGS_USE_HOTPLUG_THREAD) { + priv->thread_event_run = 1; + priv->thread_event = + g_thread_new("GUsbEventThread", g_usb_context_event_thread_cb, self); + } /* watch for add/remove */ if (libusb_has_capability(LIBUSB_CAP_HAS_HOTPLUG)) { @@ -910,7 +941,11 @@ _g_usb_context_get_context(GUsbContext *self) * @self: a #GUsbContext * @main_ctx: a #GMainContext, or %NULL * - * This function does nothing. + * Returns a source for this context. The first call actually creates the source and the result + * is returned in all future calls, unless threading is being used. + * + * If the platform does not support libusb_set_pollfd_notifiers() then a thread is being used, + * and this function returns %NULL. * * Return value: (transfer none): the #GUsbSource. * @@ -919,7 +954,16 @@ _g_usb_context_get_context(GUsbContext *self) GUsbSource * g_usb_context_get_source(GUsbContext *self, GMainContext *main_ctx) { - return NULL; + GUsbContextPrivate *priv = GET_PRIVATE(self); + g_autoptr(GMutexLocker) locker = g_mutex_locker_new(&priv->source_mutex); + + g_assert(locker != NULL); + + if (priv->thread_event != NULL) + return NULL; + if (priv->source == NULL) + priv->source = _g_usb_source_new(main_ctx, self); + return priv->source; } /** @@ -1314,5 +1358,23 @@ g_usb_context_wait_for_replug(GUsbContext *self, GUsbContext * g_usb_context_new(GError **error) { - return g_initable_new(G_USB_TYPE_CONTEXT, NULL, error, NULL); + return g_usb_context_new_full(G_USB_CONTEXT_FLAGS_USE_HOTPLUG_THREAD, NULL, error); +} + +/** + * g_usb_context_new_full: + * @flags: a #GUsbContextFlags, e.g. %G_USB_CONTEXT_FLAGS_SAVE_EVENTS + * @cancellable: a #GCancellable, or %NULL + * @error: a #GError, or %NULL + * + * Creates a new context for accessing USB devices. + * + * Return value: a new %GUsbContext object or %NULL on error. + * + * Since: 0.4.2 + **/ +GUsbContext * +g_usb_context_new_full(GUsbContextFlags flags, GCancellable *cancellable, GError **error) +{ + return g_initable_new(G_USB_TYPE_CONTEXT, cancellable, error, "flags", flags, NULL); } diff --git a/gusb/gusb-context.h b/gusb/gusb-context.h index f633520..3c0e898 100644 --- a/gusb/gusb-context.h +++ b/gusb/gusb-context.h @@ -41,6 +41,7 @@ typedef enum { G_USB_CONTEXT_FLAGS_NONE = 0, G_USB_CONTEXT_FLAGS_AUTO_OPEN_DEVICES = 1 << 0, G_USB_CONTEXT_FLAGS_SAVE_EVENTS = 1 << 1, + G_USB_CONTEXT_FLAGS_USE_HOTPLUG_THREAD = 1 << 2, /*< private >*/ G_USB_CONTEXT_FLAGS_LAST } GUsbContextFlags; @@ -50,13 +51,14 @@ g_usb_context_error_quark(void); GUsbContext * g_usb_context_new(GError **error); +GUsbContext * +g_usb_context_new_full(GUsbContextFlags flags, GCancellable *cancellable, GError **error); void g_usb_context_set_flags(GUsbContext *self, GUsbContextFlags flags); GUsbContextFlags g_usb_context_get_flags(GUsbContext *self); -G_DEPRECATED GUsbSource * g_usb_context_get_source(GUsbContext *self, GMainContext *main_ctx); GMainContext * diff --git a/gusb/gusb-device.c b/gusb/gusb-device.c index 601e5d6..45823b4 100644 --- a/gusb/gusb-device.c +++ b/gusb/gusb-device.c @@ -2164,6 +2164,9 @@ g_usb_device_control_transfer_async(GUsbDevice *self, req, NULL); } + + /* setup with the default mainloop */ + g_usb_context_get_source(priv->context, NULL); } /** @@ -2340,6 +2343,9 @@ g_usb_device_bulk_transfer_async(GUsbDevice *self, req, NULL); } + + /* setup with the default mainloop */ + g_usb_context_get_source(priv->context, NULL); } /** @@ -2516,6 +2522,9 @@ g_usb_device_interrupt_transfer_async(GUsbDevice *self, req, NULL); } + + /* setup with the default mainloop */ + g_usb_context_get_source(priv->context, NULL); } /** diff --git a/gusb/gusb-source-private.h b/gusb/gusb-source-private.h new file mode 100644 index 0000000..61f5be0 --- /dev/null +++ b/gusb/gusb-source-private.h @@ -0,0 +1,20 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- + * + * Copyright (C) 2010 Richard Hughes + * Copyright (C) 2011 Debarshi Ray + * + * SPDX-License-Identifier: LGPL-2.1+ + */ + +#pragma once + +#include + +G_BEGIN_DECLS + +GUsbSource * +_g_usb_source_new(GMainContext *main_ctx, GUsbContext *context); +void +_g_usb_source_destroy(GUsbSource *source); + +G_END_DECLS diff --git a/gusb/gusb-source.c b/gusb/gusb-source.c index becb8c8..14755a4 100644 --- a/gusb/gusb-source.c +++ b/gusb/gusb-source.c @@ -10,13 +10,25 @@ * SECTION:gusb-source * @short_description: GSource integration for libusb * - * This object used to integrate libusb into the GLib main loop before we used - * a thread. It's now pretty much unused. + * This object can be used to integrate libusb into the GLib main loop. */ #include "config.h" +#include +#include + +#include "gusb-context-private.h" +#include "gusb-context.h" +#include "gusb-source-private.h" #include "gusb-source.h" +#include "gusb-util.h" + +/* the header is not available on all platforms */ +#ifndef POLLIN +#define POLLIN 0x0001 +#define POLLOUT 0x0004 +#endif /** * g_usb_source_error_quark: @@ -34,6 +46,145 @@ g_usb_source_error_quark(void) return quark; } +struct _GUsbSource { + GSource source; + GSList *pollfds; + libusb_context *ctx; +}; + +static void +g_usb_source_pollfd_add(GUsbSource *self, int fd, short events) +{ + GPollFD *pollfd = g_new0(GPollFD, 1); + + pollfd->fd = fd; + pollfd->events = 0; + pollfd->revents = 0; + if (events & POLLIN) + pollfd->events |= G_IO_IN; + if (events & POLLOUT) + pollfd->events |= G_IO_OUT; + + self->pollfds = g_slist_prepend(self->pollfds, pollfd); + g_source_add_poll((GSource *)self, pollfd); +} + +static void +g_usb_source_pollfd_added_cb(int fd, short events, void *user_data) +{ + GUsbSource *self = user_data; + g_usb_source_pollfd_add(self, fd, events); +} + +static void +g_usb_source_pollfd_removed_cb(int fd, void *user_data) +{ + GUsbSource *self = user_data; + + /* find the pollfd in the list */ + for (GSList *elem = self->pollfds; elem != NULL; elem = elem->next) { + GPollFD *pollfd = elem->data; + if (pollfd->fd == fd) { + g_source_remove_poll((GSource *)self, pollfd); + g_free(pollfd); + self->pollfds = g_slist_delete_link(self->pollfds, elem); + return; + } + } + g_warning("couldn't find fd %d in list", fd); +} + +static gboolean +g_usb_source_prepare(GSource *source, gint *timeout) +{ + *timeout = 0; + return FALSE; +} + +static gboolean +g_usb_source_check(GSource *source) +{ + GUsbSource *self = (GUsbSource *)source; + + for (GSList *elem = self->pollfds; elem != NULL; elem = elem->next) { + GPollFD *pollfd = elem->data; + if (pollfd->revents) + return TRUE; + } + return FALSE; +} + +static gboolean +g_usb_source_dispatch(GSource *source, GSourceFunc callback, gpointer user_data) +{ + GUsbSource *self = (GUsbSource *)source; + struct timeval tv = {0, 0}; + gint rc; + + rc = libusb_handle_events_timeout(self->ctx, &tv); + if (rc < 0) + g_warning("failed to handle event: %s [%i]", g_usb_strerror(rc), rc); + + if (callback != NULL) + callback(user_data); + + return G_SOURCE_CONTINUE; +} + +static void +g_usb_source_finalize(GSource *source) +{ + GUsbSource *self = (GUsbSource *)source; + g_slist_free(self->pollfds); +} + +static GSourceFuncs usb_source_funcs = {g_usb_source_prepare, + g_usb_source_check, + g_usb_source_dispatch, + g_usb_source_finalize}; + +GUsbSource * +_g_usb_source_new(GMainContext *main_ctx, GUsbContext *gusb_ctx) +{ + GUsbSource *self; + const struct libusb_pollfd **pollfds; + + self = (GUsbSource *)g_source_new(&usb_source_funcs, sizeof(GUsbSource)); + self->pollfds = NULL; + self->ctx = _g_usb_context_get_context(gusb_ctx); + + /* watch the fd's already created */ + pollfds = libusb_get_pollfds(self->ctx); + for (guint i = 0; pollfds != NULL && pollfds[i] != NULL; i++) + g_usb_source_pollfd_add(self, pollfds[i]->fd, pollfds[i]->events); + free(pollfds); + + /* watch for PollFD changes */ + g_source_attach((GSource *)self, main_ctx); + libusb_set_pollfd_notifiers(self->ctx, + g_usb_source_pollfd_added_cb, + g_usb_source_pollfd_removed_cb, + self); + return self; +} + +static void +g_usb_source_pollfd_remove_cb(gpointer data, gpointer user_data) +{ + GPollFD *pollfd = (GPollFD *)data; + GSource *source = (GSource *)user_data; + g_source_remove_poll(source, pollfd); +} + +void +_g_usb_source_destroy(GUsbSource *self) +{ + libusb_set_pollfd_notifiers(self->ctx, NULL, NULL, NULL); + g_slist_foreach(self->pollfds, g_usb_source_pollfd_remove_cb, self); + g_slist_free_full(g_steal_pointer(&self->pollfds), g_free); + g_source_destroy((GSource *)self); +} + /** * g_usb_source_set_callback: * @self: a #GUsbSource @@ -41,7 +192,7 @@ g_usb_source_error_quark(void) * @data: data to pass to @func * @notify: a #GDestroyNotify * - * This function does nothing. + * Set a callback to be called when the source is dispatched. * * Since: 0.1.0 **/ diff --git a/gusb/libgusb.ver b/gusb/libgusb.ver index 4a4a731..2e7eccf 100644 --- a/gusb/libgusb.ver +++ b/gusb/libgusb.ver @@ -193,3 +193,9 @@ LIBGUSB_0.4.1 { g_usb_device_add_tag; local: *; } LIBGUSB_0.4.0; + +LIBGUSB_0.4.2 { + global: + g_usb_context_new_full; + local: *; +} LIBGUSB_0.4.1; diff --git a/tools/gusb-main.c b/tools/gusb-main.c index 8037310..51efd15 100644 --- a/tools/gusb-main.c +++ b/tools/gusb-main.c @@ -516,10 +516,13 @@ main(int argc, char *argv[]) } /* GUsbContext */ - priv->usb_ctx = g_usb_context_new(NULL); if (save_events) context_flags |= G_USB_CONTEXT_FLAGS_SAVE_EVENTS; - g_usb_context_set_flags(priv->usb_ctx, context_flags); + priv->usb_ctx = g_usb_context_new_full(context_flags, NULL, &error); + if (priv->usb_ctx == NULL) { + g_printerr("%s\n", error->message); + return 1; + } /* add commands */ priv->cmd_array = g_ptr_array_new_with_free_func((GDestroyNotify)gusb_cmd_item_free);