proxmox-mirrors/fwupd
1
0
Fork 0
mirror of https://git.proxmox.com/git/fwupd synced 2025-07-27 15:03:59 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Simplify fu_device_list_wait_for_replug() for sanity

Browse Source 
Rather than using a recursive GMainLoop, just run a single iteration of the
mainloop every 1ms while we're waiting.

This simplifies the device lifecycle dramatically as we don't have to worry
about reentrant removals. It's also a lot simpler to debug as we know the only
way to return is the timeout elapsing or FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG being
unset.

If multiple devices got removed and we're waiting for one to re-appear, also
wait for all devices to come back before returning to the main loop.

In the worst case, this causes the device list to wait the full removal_delay
for the device rather than returning when the first device matches. This allows
us to simplify the hub update when we reboot the entire device and various
devices from different plugins come back in an unpredictable order.

Fixes the other half of https://github.com/fwupd/fwupd/issues/2376
This commit is contained in:
Richard Hughes 2020-10-16 16:54:15 +01:00
parent ae5858ff58
commit f2a3e01f96
2 changed files with 35 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

78
src/fu-device-list.c
View File

@ -38,8 +38,6 @@ struct _FuDeviceList
GObject parent_instance; GObject parent_instance;
GPtrArray *devices; /* of FuDeviceItem */ GPtrArray *devices; /* of FuDeviceItem */
GRWLock devices_mutex; GRWLock devices_mutex;
GMainLoop *replug_loop; /* block waiting for replug */
guint replug_id; /* timeout the loop */
}; };
enum { enum {
@ -567,15 +565,6 @@ fu_device_list_item_set_device (FuDeviceItem *item, FuDevice *device)
g_set_object (&item->device, device); g_set_object (&item->device, device);
} }
static gboolean
fu_device_list_replug_loop_quit_cb (gpointer user_data)
{
FuDeviceList *self = FU_DEVICE_LIST (user_data);
g_debug ("quitting replug loop");
g_main_loop_quit (self->replug_loop);
return G_SOURCE_REMOVE;
}
static void static void
fu_device_list_replace (FuDeviceList *self, FuDeviceItem *item, FuDevice *device) fu_device_list_replace (FuDeviceList *self, FuDeviceItem *item, FuDevice *device)
{ {
@ -651,11 +640,9 @@ fu_device_list_replace (FuDeviceList *self, FuDeviceItem *item, FuDevice *device
fu_device_list_emit_device_changed (self, device); fu_device_list_emit_device_changed (self, device);
/* we were waiting for this... */ /* we were waiting for this... */
if (fu_device_has_flag (item->device_old, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG) && if (fu_device_has_flag (item->device_old, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG)) {
g_main_loop_is_running (self->replug_loop)) { g_debug ("device came back, clearing flag");
g_debug ("schedule quit replug loop in idle");
fu_device_remove_flag (item->device_old, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG); fu_device_remove_flag (item->device_old, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG);
g_idle_add (fu_device_list_replug_loop_quit_cb, self);
} }
} }
@ -691,20 +678,13 @@ fu_device_list_add (FuDeviceList *self, FuDevice *device)
/* is the device waiting to be replugged? */ /* is the device waiting to be replugged? */
item = fu_device_list_find_by_id (self, fu_device_get_id (device), NULL); item = fu_device_list_find_by_id (self, fu_device_get_id (device), NULL);
if (item != NULL && item->remove_id != 0) { if (item != NULL) {
g_debug ("found existing device %s, reusing item", g_debug ("found existing device %s, reusing item",
fu_device_get_id (item->device)); fu_device_get_id (item->device));
fu_device_list_replace (self, item, device); fu_device_list_replace (self, item, device);
return; return;
} }
/* verify the device does not already exist */
if (item != NULL) {
g_debug ("device %s already exists, ignoring",
fu_device_get_id (item->device));
return;
}
/* verify a device with same connection does not already exist */ /* verify a device with same connection does not already exist */
item = fu_device_list_find_by_connection (self, item = fu_device_list_find_by_connection (self,
fu_device_get_physical_id (device), fu_device_get_physical_id (device),
@ -818,18 +798,19 @@ fu_device_list_get_by_guid (FuDeviceList *self, const gchar *guid, GError **erro
return NULL; return NULL;
} }
static gboolean /* count devices that are disconnected and are waiting to be replugged */
fu_device_list_replug_cb (gpointer user_data) static guint
fu_device_list_devices_wait_removed (FuDeviceList *self)
{ {
FuDeviceList *self = FU_DEVICE_LIST (user_data); guint cnt = 0;
g_rw_lock_reader_lock (&self->devices_mutex);
/* no longer valid */ for (guint i = 0; i < self->devices->len; i++) {
self->replug_id = 0; FuDeviceItem *item = g_ptr_array_index (self->devices, i);
if (item->remove_id != 0)
/* quit loop */ cnt++;
g_debug ("device did not replug"); }
g_main_loop_quit (self->replug_loop); g_rw_lock_reader_unlock (&self->devices_mutex);
return FALSE; return cnt;
} }
/** /**
@ -852,11 +833,13 @@ fu_device_list_wait_for_replug (FuDeviceList *self, FuDevice *device, GError **e
{ {
FuDeviceItem *item; FuDeviceItem *item;
guint remove_delay; guint remove_delay;
guint wait_removed;
guint wait_removed_old = 0;
g_autoptr(GTimer) timer = g_timer_new ();
g_return_val_if_fail (FU_IS_DEVICE_LIST (self), FALSE); 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 (FU_IS_DEVICE (device), FALSE);
g_return_val_if_fail (error == NULL || *error == NULL, FALSE); g_return_val_if_fail (error == NULL || *error == NULL, FALSE);
g_return_val_if_fail (self->replug_id == 0, FALSE);
/* not found */ /* not found */
item = fu_device_list_find_by_device (self, device); item = fu_device_list_find_by_device (self, device);
@ -895,14 +878,20 @@ fu_device_list_wait_for_replug (FuDeviceList *self, FuDevice *device, GError **e
} }
/* time to unplug and then re-plug */ /* time to unplug and then re-plug */
self->replug_id = g_timeout_add (remove_delay, fu_device_list_replug_cb, self); do {
g_main_loop_run (self->replug_loop); /* count how many devices are in the remove waiting state */
wait_removed = fu_device_list_devices_wait_removed (self);
/* cancel timeout if still pending */ if (wait_removed != wait_removed_old) {
if (self->replug_id != 0) { g_debug ("devices in wait_removed: %u -> %u",
g_source_remove (self->replug_id); wait_removed_old, wait_removed);
self->replug_id = 0; wait_removed_old = wait_removed;
} }
g_usleep (1000);
g_main_context_iteration (NULL, FALSE);
if (!fu_device_has_flag (item->device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG) &&
wait_removed == 0)
break;
} while (g_timer_elapsed (timer, NULL) * 1000.f < remove_delay);
/* device was not added back to the device list */ /* device was not added back to the device list */
if (fu_device_has_flag (item->device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG)) { if (fu_device_has_flag (item->device, FWUPD_DEVICE_FLAG_WAIT_FOR_REPLUG)) {
@ -1016,7 +1005,6 @@ static void
fu_device_list_init (FuDeviceList *self) fu_device_list_init (FuDeviceList *self)
{ {
self->devices = g_ptr_array_new_with_free_func ((GDestroyNotify) fu_device_list_item_free); self->devices = g_ptr_array_new_with_free_func ((GDestroyNotify) fu_device_list_item_free);
self->replug_loop = g_main_loop_new (NULL, FALSE);
g_rw_lock_init (&self->devices_mutex); g_rw_lock_init (&self->devices_mutex);
} }
@ -1026,11 +1014,7 @@ fu_device_list_finalize (GObject *obj)
FuDeviceList *self = FU_DEVICE_LIST (obj); FuDeviceList *self = FU_DEVICE_LIST (obj);
g_rw_lock_clear (&self->devices_mutex); g_rw_lock_clear (&self->devices_mutex);
if (self->replug_id != 0)
g_source_remove (self->replug_id);
g_ptr_array_unref (self->devices); g_ptr_array_unref (self->devices);
g_main_loop_unref (self->replug_loop);
G_OBJECT_CLASS (fu_device_list_parent_class)->finalize (obj); G_OBJECT_CLASS (fu_device_list_parent_class)->finalize (obj);
} }

5
src/fu-self-test.c
View File

@ -1975,14 +1975,15 @@ fu_device_list_delay_func (gconstpointer user_data)
fu_device_list_add (device_list, device1); fu_device_list_add (device_list, device1);
g_assert_cmpint (added_cnt, ==, 1); g_assert_cmpint (added_cnt, ==, 1);
g_assert_cmpint (removed_cnt, ==, 0); g_assert_cmpint (removed_cnt, ==, 0);
g_assert_cmpint (changed_cnt, ==, 0); g_assert_cmpint (changed_cnt, ==, 1);
/* add a device with the same ID */ /* add a device with the same ID */
fu_device_set_id (device2, "device1"); fu_device_set_id (device2, "device1");
fu_device_list_add (device_list, device2); fu_device_list_add (device_list, device2);
fu_device_set_remove_delay (device2, 100);
g_assert_cmpint (added_cnt, ==, 1); g_assert_cmpint (added_cnt, ==, 1);
g_assert_cmpint (removed_cnt, ==, 0); g_assert_cmpint (removed_cnt, ==, 0);
g_assert_cmpint (changed_cnt, ==, 0); g_assert_cmpint (changed_cnt, ==, 2);
/* spin a bit */ /* spin a bit */
fu_test_loop_run_with_timeout (10); fu_test_loop_run_with_timeout (10);