From 3a8d532855f7f44786250dd1b863fb8cfa60daa2 Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Thu, 30 Aug 2018 11:51:10 +0100 Subject: [PATCH] Allow the device list to take care of waiting for the device replug This means that individual plugins do not have to manage thier own GUsbDevice lifecycle and no longer have to call g_usb_context_wait_for_replug(). --- libfwupd/fwupd-enums.c | 4 + libfwupd/fwupd-enums.h | 2 + plugins/colorhug/fu-colorhug-device.c | 2 + plugins/colorhug/fu-plugin-colorhug.c | 34 +------ plugins/dfu/fu-plugin-dfu.c | 10 +- plugins/ebitdo/fu-plugin-ebitdo.c | 12 +-- src/fu-device-list.c | 126 +++++++++++++++++++++++++- src/fu-device-list.h | 3 + src/fu-engine.c | 15 +++ src/fu-self-test.c | 122 +++++++++++++++++++++++++ 10 files changed, 281 insertions(+), 49 deletions(-) diff --git a/libfwupd/fwupd-enums.c b/libfwupd/fwupd-enums.c index 74eb39714..975e0c06b 100644 --- a/libfwupd/fwupd-enums.c +++ b/libfwupd/fwupd-enums.c @@ -145,6 +145,8 @@ fwupd_device_flag_to_string (FwupdDeviceFlags device_flag) return "install-parent-first"; if (device_flag == FWUPD_DEVICE_FLAG_IS_BOOTLOADER) return "is-bootloader"; + if (device_flag == FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG) + return "wait-for-replug"; if (device_flag == FWUPD_DEVICE_FLAG_UNKNOWN) return "unknown"; return NULL; @@ -195,6 +197,8 @@ fwupd_device_flag_from_string (const gchar *device_flag) return FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST; if (g_strcmp0 (device_flag, "is-bootloader") == 0) return FWUPD_DEVICE_FLAG_IS_BOOTLOADER; + if (g_strcmp0 (device_flag, "wait-for-replug") == 0) + return FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG; return FWUPD_DEVICE_FLAG_UNKNOWN; } diff --git a/libfwupd/fwupd-enums.h b/libfwupd/fwupd-enums.h index 6b21b0fdf..f83728f3e 100644 --- a/libfwupd/fwupd-enums.h +++ b/libfwupd/fwupd-enums.h @@ -78,6 +78,7 @@ typedef enum { * @FWUPD_DEVICE_FLAG_USE_RUNTIME_VERSION: Always use the runtime version rather than the bootloader * @FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST: Install composite firmware on the parent before the child * @FWUPD_DEVICE_FLAG_IS_BOOTLOADER: Is currently in bootloader mode + * @FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG: The hardware is waiting to be replugged * * The device flags. **/ @@ -96,6 +97,7 @@ typedef enum { #define FWUPD_DEVICE_FLAG_USE_RUNTIME_VERSION (1u << 11) /* Since: 1.0.6 */ #define FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST (1u << 12) /* Since: 1.0.8 */ #define FWUPD_DEVICE_FLAG_IS_BOOTLOADER (1u << 13) /* Since: 1.0.8 */ +#define FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG (1u << 14) /* Since: 1.1.2 */ #define FWUPD_DEVICE_FLAG_UNKNOWN G_MAXUINT64 /* Since: 0.7.3 */ typedef guint64 FwupdDeviceFlags; diff --git a/plugins/colorhug/fu-colorhug-device.c b/plugins/colorhug/fu-colorhug-device.c index dbef7b975..88db807cb 100644 --- a/plugins/colorhug/fu-colorhug-device.c +++ b/plugins/colorhug/fu-colorhug-device.c @@ -413,6 +413,8 @@ fu_colorhug_device_init (FuColorhugDevice *self) { /* this is the application code */ self->start_addr = CH_EEPROM_ADDR_RUNCODE; + fu_device_set_remove_delay (FU_DEVICE (self), + FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE); } static void diff --git a/plugins/colorhug/fu-plugin-colorhug.c b/plugins/colorhug/fu-plugin-colorhug.c index 5c533a7d6..afe9aa27c 100644 --- a/plugins/colorhug/fu-plugin-colorhug.c +++ b/plugins/colorhug/fu-plugin-colorhug.c @@ -20,10 +20,7 @@ fu_plugin_init (FuPlugin *plugin) gboolean fu_plugin_update_detach (FuPlugin *plugin, FuDevice *device, GError **error) { - FuColorhugDevice *self = FU_COLORHUG_DEVICE (device); - GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE (device)); g_autoptr(FuDeviceLocker) locker = NULL; - g_autoptr(GUsbDevice) usb_device2 = NULL; /* open device */ locker = fu_device_locker_new (device, error); @@ -41,29 +38,14 @@ fu_plugin_update_detach (FuPlugin *plugin, FuDevice *device, GError **error) return FALSE; /* wait for replug */ - g_clear_object (&locker); - usb_device2 = g_usb_context_wait_for_replug (fu_plugin_get_usb_context (plugin), - usb_device, - 10000, error); - if (usb_device2 == NULL) { - g_prefix_error (error, "device did not come back: "); - return FALSE; - } - - /* set the new device until we can use a new FuDevice */ - fu_usb_device_set_dev (FU_USB_DEVICE (self), usb_device2); - - /* success */ + fu_device_add_flag (device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); return TRUE; } gboolean fu_plugin_update_attach (FuPlugin *plugin, FuDevice *device, GError **error) { - FuColorhugDevice *self = FU_COLORHUG_DEVICE (device); - GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE (device)); g_autoptr(FuDeviceLocker) locker = NULL; - g_autoptr(GUsbDevice) usb_device2 = NULL; /* open device */ locker = fu_device_locker_new (device, error); @@ -81,19 +63,7 @@ fu_plugin_update_attach (FuPlugin *plugin, FuDevice *device, GError **error) return FALSE; /* wait for replug */ - g_clear_object (&locker); - usb_device2 = g_usb_context_wait_for_replug (fu_plugin_get_usb_context (plugin), - usb_device, - 10000, error); - if (usb_device2 == NULL) { - g_prefix_error (error, "device did not come back: "); - return FALSE; - } - - /* set the new device until we can use a new FuDevice */ - fu_usb_device_set_dev (FU_USB_DEVICE (self), usb_device2); - - /* success */ + fu_device_add_flag (device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); return TRUE; } diff --git a/plugins/dfu/fu-plugin-dfu.c b/plugins/dfu/fu-plugin-dfu.c index 16ef30ca6..4b2f196a1 100644 --- a/plugins/dfu/fu-plugin-dfu.c +++ b/plugins/dfu/fu-plugin-dfu.c @@ -84,10 +84,9 @@ fu_plugin_update_detach (FuPlugin *plugin, FuDevice *dev, GError **error) /* detach and USB reset */ if (!dfu_device_detach (device, error)) return FALSE; - if (!dfu_device_wait_for_replug (device, FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE, error)) - return FALSE; - /* success */ + /* wait for replug */ + fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); return TRUE; } @@ -112,10 +111,9 @@ fu_plugin_update_attach (FuPlugin *plugin, FuDevice *dev, GError **error) /* attach it */ if (!dfu_device_attach (device, error)) return FALSE; - if (!dfu_device_wait_for_replug (device, FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE, error)) - return FALSE; - /* success */ + /* wait for replug */ + fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); return TRUE; } diff --git a/plugins/ebitdo/fu-plugin-ebitdo.c b/plugins/ebitdo/fu-plugin-ebitdo.c index b97ff74e0..23fed9210 100644 --- a/plugins/ebitdo/fu-plugin-ebitdo.c +++ b/plugins/ebitdo/fu-plugin-ebitdo.c @@ -45,7 +45,6 @@ fu_plugin_update (FuPlugin *plugin, GUsbDevice *usb_device = fu_usb_device_get_dev (FU_USB_DEVICE (dev)); FuEbitdoDevice *ebitdo_dev = FU_EBITDO_DEVICE (dev); g_autoptr(FuDeviceLocker) locker = NULL; - g_autoptr(GUsbDevice) usb_device2 = NULL; /* get version */ if (!fu_device_has_flag (dev, FWUPD_DEVICE_FLAG_IS_BOOTLOADER)) { @@ -70,16 +69,9 @@ fu_plugin_update (FuPlugin *plugin, g_prefix_error (error, "failed to force-reset device: "); return FALSE; } - g_clear_object (&locker); - usb_device2 = g_usb_context_wait_for_replug (fu_plugin_get_usb_context (plugin), - usb_device, 10000, error); - if (usb_device2 == NULL) { - g_prefix_error (error, "device did not come back: "); - return FALSE; - } - fu_usb_device_set_dev (FU_USB_DEVICE (ebitdo_dev), usb_device2); - /* success */ + /* wait for replug */ + fu_device_add_flag (dev, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); return TRUE; } diff --git a/src/fu-device-list.c b/src/fu-device-list.c index 0b4c4186a..5986d8837 100644 --- a/src/fu-device-list.c +++ b/src/fu-device-list.c @@ -49,6 +49,8 @@ typedef struct { FuDevice *device; FuDevice *device_old; FuDeviceList *self; /* no ref */ + GMainLoop *replug_loop; /* block waiting for replug */ + guint replug_id; /* timeout the loop */ guint remove_id; } FuDeviceItem; @@ -191,6 +193,26 @@ fu_device_list_find_by_guid (FuDeviceList *self, const gchar *guid) return NULL; } +static FuDeviceItem * +fu_device_list_find_by_platform_id (FuDeviceList *self, const gchar *platform_id) +{ + for (guint i = 0; i < self->devices->len; i++) { + FuDeviceItem *item_tmp = g_ptr_array_index (self->devices, i); + FuDevice *device = item_tmp->device; + if (device != NULL && + g_strcmp0 (fu_device_get_platform_id (device), platform_id) == 0) + return item_tmp; + } + for (guint i = 0; i < self->devices->len; i++) { + FuDeviceItem *item_tmp = g_ptr_array_index (self->devices, i); + FuDevice *device = item_tmp->device_old; + if (device != NULL && + g_strcmp0 (fu_device_get_platform_id (device), platform_id) == 0) + return item_tmp; + } + return NULL; +} + static FuDeviceItem * fu_device_list_find_by_id (FuDeviceList *self, const gchar *device_id, @@ -215,6 +237,10 @@ fu_device_list_find_by_id (FuDeviceList *self, } } } + if (item != NULL) + return item; + + /* only search old devices if we didn't find the active device */ for (guint i = 0; i < self->devices->len; i++) { FuDeviceItem *item_tmp = g_ptr_array_index (self->devices, i); const gchar *ids[3] = { NULL }; @@ -404,6 +430,12 @@ fu_device_list_replace (FuDeviceList *self, FuDeviceItem *item, FuDevice *device g_set_object (&item->device_old, item->device); g_set_object (&item->device, device); fu_device_list_emit_device_changed (self, device); + + /* we were waiting for this... */ + if (g_main_loop_is_running (item->replug_loop)) { + g_debug ("quitting replug loop"); + g_main_loop_quit (item->replug_loop); + } } /** @@ -445,7 +477,7 @@ fu_device_list_add (FuDeviceList *self, FuDevice *device) g_source_remove (item->remove_id); item->remove_id = 0; } - fu_device_list_emit_device_changed (self, device); + fu_device_list_replace (self, item, device); return; } @@ -458,6 +490,10 @@ fu_device_list_add (FuDeviceList *self, FuDevice *device) /* verify a compatible device does not already exist */ item = fu_device_list_get_by_guids (self, fu_device_get_guids (device)); + if (item == NULL) { + item = fu_device_list_find_by_platform_id (self, + fu_device_get_platform_id (device)); + } if (item != NULL && item->remove_id != 0) { g_debug ("found compatible device %s recently removed, reusing " "item from plugin %s for plugin %s", @@ -499,6 +535,7 @@ fu_device_list_add (FuDeviceList *self, FuDevice *device) item = g_new0 (FuDeviceItem, 1); item->self = self; /* no ref */ item->device = g_object_ref (device); + item->replug_loop = g_main_loop_new (NULL, FALSE); g_ptr_array_add (self->devices, item); fu_device_list_emit_device_added (self, device); } @@ -533,6 +570,90 @@ fu_device_list_get_by_guid (FuDeviceList *self, const gchar *guid, GError **erro return NULL; } +static gboolean +fu_device_list_replug_cb (gpointer user_data) +{ + FuDeviceItem *item = (FuDeviceItem *) user_data; + + /* no longer valid */ + item->replug_id = 0; + + /* quit loop */ + g_debug ("device did not replug"); + g_main_loop_quit (item->replug_loop); + return FALSE; +} + +/** + * fu_device_list_wait_for_replug: + * @self: A #FuDeviceList + * @guid: A device GUID + * @error: A #GError, or %NULL + * + * Waits for a specific devic to replug if %FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG + * is set. + * + * If the device does not exist this function returns without an error. + * + * Returns: %TRUE for success + * + * Since: 1.1.2 + **/ +gboolean +fu_device_list_wait_for_replug (FuDeviceList *self, FuDevice *device, GError **error) +{ + FuDeviceItem *item; + guint remove_delay; + + g_return_val_if_fail (FU_IS_DEVICE_LIST (self), FALSE); + g_return_val_if_fail (FU_IS_DEVICE (device), FALSE); + g_return_val_if_fail (error == NULL || *error == NULL, FALSE); + + /* not found */ + item = fu_device_list_find_by_device (self, device); + if (item == NULL) + return TRUE; + + /* not required, or possibly literally just happened */ + if (!fu_device_has_flag (item->device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG)) { + g_debug ("no replug or re-enumerate required"); + return TRUE; + } + + /* plugin did not specify */ + remove_delay = fu_device_get_remove_delay (device); + if (remove_delay == 0) { + remove_delay = FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE; + g_warning ("plugin %s did not specify a remove delay for %s, " + "so guessing we should wait %ums for replug", + fu_device_get_plugin (device), + fu_device_get_id (device), + remove_delay); + } else { + g_debug ("waiting %ums for replug", remove_delay); + } + + /* time to unplug and then re-plug */ + item->replug_id = g_timeout_add (remove_delay, fu_device_list_replug_cb, item); + g_main_loop_run (item->replug_loop); + + /* the loop was quit without the timer */ + if (item->replug_id != 0) { + g_debug ("waited for replug"); + g_source_remove (item->replug_id); + item->replug_id = 0; + return TRUE; + } + + /* device was not added back to the device list */ + g_set_error (error, + FWUPD_ERROR, + FWUPD_ERROR_NOT_FOUND, + "device %s did not come back", + fu_device_get_id (device)); + return FALSE; +} + /** * fu_device_list_get_by_id: * @self: A #FuDeviceList @@ -586,8 +707,11 @@ fu_device_list_item_free (FuDeviceItem *item) { if (item->remove_id != 0) g_source_remove (item->remove_id); + if (item->replug_id != 0) + g_source_remove (item->replug_id); if (item->device_old != NULL) g_object_unref (item->device_old); + g_main_loop_unref (item->replug_loop); g_object_unref (item->device); g_free (item); } diff --git a/src/fu-device-list.h b/src/fu-device-list.h index 04e635bcf..9e30b4b97 100644 --- a/src/fu-device-list.h +++ b/src/fu-device-list.h @@ -33,6 +33,9 @@ FuDevice *fu_device_list_get_by_id (FuDeviceList *self, FuDevice *fu_device_list_get_by_guid (FuDeviceList *self, const gchar *guid, GError **error); +gboolean fu_device_list_wait_for_replug (FuDeviceList *self, + FuDevice *device, + GError **error); G_END_DECLS diff --git a/src/fu-engine.c b/src/fu-engine.c index 81011df50..e9bf54bbd 100644 --- a/src/fu-engine.c +++ b/src/fu-engine.c @@ -1434,6 +1434,11 @@ fu_engine_install_blob (FuEngine *self, return FALSE; } device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); + if (device == NULL) + return FALSE; + if (!fu_device_list_wait_for_replug (self->device_list, device, error)) + return FALSE; + device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); if (device == NULL) return FALSE; if (!fu_plugin_runner_update (plugin, @@ -1477,6 +1482,11 @@ fu_engine_install_blob (FuEngine *self, return FALSE; } device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); + if (device == NULL) + return FALSE; + if (!fu_device_list_wait_for_replug (self->device_list, device, error)) + return FALSE; + device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); if (device == NULL) return FALSE; if (!fu_plugin_runner_update_attach (plugin, device, &error_local)) { @@ -1491,6 +1501,11 @@ fu_engine_install_blob (FuEngine *self, g_propagate_error (error, g_steal_pointer (&error_local)); return FALSE; } + device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); + if (device == NULL) + return FALSE; + if (!fu_device_list_wait_for_replug (self->device_list, device, error)) + return FALSE; /* get the new version number */ device = fu_device_list_get_by_id (self->device_list, device_id_orig, error); diff --git a/src/fu-self-test.c b/src/fu-self-test.c index d2439b0ef..4ee0ab66e 100644 --- a/src/fu-self-test.c +++ b/src/fu-self-test.c @@ -854,6 +854,126 @@ fu_device_list_delay_func (void) g_assert_cmpint (changed_cnt, ==, 1); } +typedef struct { + FuDevice *device_new; + FuDevice *device_old; + FuDeviceList *device_list; +} FuDeviceListReplugHelper; + +static gboolean +fu_device_list_remove_cb (gpointer user_data) +{ + FuDeviceListReplugHelper *helper = (FuDeviceListReplugHelper *) user_data; + fu_device_list_remove (helper->device_list, helper->device_old); + return FALSE; +} + +static gboolean +fu_device_list_add_cb (gpointer user_data) +{ + FuDeviceListReplugHelper *helper = (FuDeviceListReplugHelper *) user_data; + fu_device_list_add (helper->device_list, helper->device_new); + return FALSE; +} + +static void +fu_device_list_replug_auto_func (void) +{ + gboolean ret; + g_autoptr(FuDevice) device1 = fu_device_new (); + g_autoptr(FuDevice) device2 = fu_device_new (); + g_autoptr(FuDeviceList) device_list = fu_device_list_new (); + g_autoptr(GError) error = NULL; + FuDeviceListReplugHelper helper; + + /* fake devices */ + fu_device_set_id (device1, "device1"); + fu_device_set_platform_id (device1, "ID"); + fu_device_set_plugin (device1, "self-test"); + fu_device_set_remove_delay (device1, FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE); + fu_device_set_id (device2, "device2"); + fu_device_set_platform_id (device2, "ID"); /* matches */ + fu_device_set_plugin (device2, "self-test"); + fu_device_set_remove_delay (device2, FU_DEVICE_REMOVE_DELAY_RE_ENUMERATE); + + /* not yet added */ + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); + + /* add device */ + fu_device_list_add (device_list, device1); + + /* not waiting */ + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); + + /* waiting */ + helper.device_old = device1; + helper.device_new = device2; + helper.device_list = device_list; + g_timeout_add (100, fu_device_list_remove_cb, &helper); + g_timeout_add (200, fu_device_list_add_cb, &helper); + fu_device_add_flag (device1, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); + + /* waiting, failed */ + fu_device_add_flag (device2, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); + ret = fu_device_list_wait_for_replug (device_list, device2, &error); + g_assert_error (error, FWUPD_ERROR, FWUPD_ERROR_NOT_FOUND); + g_assert (!ret); +} + +static void +fu_device_list_replug_user_func (void) +{ + gboolean ret; + g_autoptr(FuDevice) device1 = fu_device_new (); + g_autoptr(FuDevice) device2 = fu_device_new (); + g_autoptr(FuDeviceList) device_list = fu_device_list_new (); + g_autoptr(GError) error = NULL; + FuDeviceListReplugHelper helper; + + /* fake devices */ + fu_device_set_id (device1, "device1"); + fu_device_add_guid (device1, "foo"); + fu_device_add_guid (device1, "bar"); + fu_device_set_plugin (device1, "self-test"); + fu_device_set_remove_delay (device1, FU_DEVICE_REMOVE_DELAY_USER_REPLUG); + fu_device_set_id (device2, "device2"); + fu_device_add_guid (device2, "baz"); + fu_device_add_guid (device2, "bar"); /* matches */ + fu_device_set_plugin (device2, "self-test"); + fu_device_set_remove_delay (device2, FU_DEVICE_REMOVE_DELAY_USER_REPLUG); + + /* not yet added */ + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); + + /* add device */ + fu_device_list_add (device_list, device1); + + /* not waiting */ + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); + + /* waiting */ + helper.device_old = device1; + helper.device_new = device2; + helper.device_list = device_list; + g_timeout_add (100, fu_device_list_remove_cb, &helper); + g_timeout_add (200, fu_device_list_add_cb, &helper); + fu_device_add_flag (device1, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); + ret = fu_device_list_wait_for_replug (device_list, device1, &error); + g_assert_no_error (error); + g_assert (ret); +} + static void fu_device_list_compatible_func (void) { @@ -2456,6 +2576,8 @@ main (int argc, char **argv) g_test_add_func ("/fwupd/engine{device-unlock}", fu_engine_device_unlock_func); g_test_add_func ("/fwupd/engine{history-success}", fu_engine_history_func); g_test_add_func ("/fwupd/engine{history-error}", fu_engine_history_error_func); + g_test_add_func ("/fwupd/device-list{replug-auto}", fu_device_list_replug_auto_func); + g_test_add_func ("/fwupd/device-list{replug-user}", fu_device_list_replug_user_func); g_test_add_func ("/fwupd/engine{require-hwid}", fu_engine_require_hwid_func); g_test_add_func ("/fwupd/engine{partial-hash}", fu_engine_partial_hash_func); g_test_add_func ("/fwupd/engine{downgrade}", fu_engine_downgrade_func);