From 81f9552095d1da63b50b1376bab6a8b74b79db5b Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Sun, 13 Sep 2020 15:12:57 +0100 Subject: [PATCH] Correctly order devices when using logical parents If the device parent is added using fu_device_add_parent_guid() or ParentGuid from a quirk file then the child is not returned in fu_device_get_children() by design as the physical ID will be likely different. This means we cannot reliably 'depsolve' the order in FuDevice as we need the full list of devices that might be parents. The existing algorithm had several logical problems when dealing with more than a single parent and child. The FuDeviceList object is the right place to do this as it knows about all added devices on the system. This means the addition of a logical device causes the root logical device (and all it's children, and grandchildren) to be re-ordered. This allows firmware on deeply nested composite devices like hubs to be installed in the correct order. --- libfwupdplugin/fu-device.c | 16 ----------- src/fu-device-list.c | 54 ++++++++++++++++++++++++++++++++++++++ src/fu-device-list.h | 2 ++ src/fu-engine.c | 3 +++ src/fu-self-test.c | 9 ++++--- 5 files changed, 65 insertions(+), 19 deletions(-) diff --git a/libfwupdplugin/fu-device.c b/libfwupdplugin/fu-device.c index 8b29de013..5720a9ec1 100644 --- a/libfwupdplugin/fu-device.c +++ b/libfwupdplugin/fu-device.c @@ -653,13 +653,6 @@ fu_device_set_parent (FuDevice *self, FuDevice *parent) g_return_if_fail (FU_IS_DEVICE (self)); - /* if unspecified, always child before parent */ - if (parent != NULL && - fu_device_get_order (parent) == fu_device_get_order (self)) { - g_debug ("auto-setting %s order", fu_device_get_id (parent)); - fu_device_set_order (parent, fu_device_get_order (self) + 1); - } - /* if the parent has quirks, make the child inherit it */ if (parent != NULL) { if (fu_device_get_quirks (self) == NULL && @@ -802,15 +795,6 @@ fu_device_add_child (FuDevice *self, FuDevice *child) /* ensure the parent is also set on the child */ fu_device_set_parent (child, self); - - /* order devices so they are updated in the correct sequence */ - if (fu_device_has_flag (child, FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST)) { - if (priv->order >= fu_device_get_order (child)) - fu_device_set_order (child, priv->order + 1); - } else { - if (priv->order <= fu_device_get_order (child)) - priv->order = fu_device_get_order (child) + 1; - } } /** diff --git a/src/fu-device-list.c b/src/fu-device-list.c index fc154442d..de25ed3be 100644 --- a/src/fu-device-list.c +++ b/src/fu-device-list.c @@ -81,6 +81,60 @@ fu_device_list_emit_device_changed (FuDeviceList *self, FuDevice *device) g_signal_emit (self, signals[SIGNAL_CHANGED], 0, device); } +/* we cannot use fu_device_get_children() as this will not find "parent-only" + * logical relationships added using fu_device_add_parent_guid() */ +static GPtrArray * +fu_device_list_get_children (FuDeviceList *self, FuDevice *device) +{ + GPtrArray *devices = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); + g_rw_lock_reader_lock (&self->devices_mutex); + for (guint i = 0; i < self->devices->len; i++) { + FuDeviceItem *item = g_ptr_array_index (self->devices, i); + if (device == fu_device_get_parent (item->device)) + g_ptr_array_add (devices, g_object_ref (item->device)); + } + g_rw_lock_reader_unlock (&self->devices_mutex); + return devices; +} + +static void +fu_device_list_depsolve_order_full (FuDeviceList *self, FuDevice *device, guint depth) +{ + g_autoptr(GPtrArray) children = NULL; + + /* ourself */ + fu_device_set_order (device, depth); + + /* optional children */ + children = fu_device_list_get_children (self, device); + for (guint i = 0; i < children->len; i++) { + FuDevice *child = g_ptr_array_index (children, i); + if (fu_device_has_flag (child, FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST)) { + fu_device_list_depsolve_order_full (self, child, depth + 1); + } else { + fu_device_list_depsolve_order_full (self, child, depth - 1); + } + } +} + +/** + * fu_device_list_depsolve_order: + * @self: A #FuDeviceList + * @device: A #FuDevice + * + * Sets the device order using the logical parent->child relationships -- by default + * the child is updated first, unless the device has set flag + * %FWUPD_DEVICE_FLAG_INSTALL_PARENT_FIRST. + * + * Since: 1.5.0 + **/ +void +fu_device_list_depsolve_order (FuDeviceList *self, FuDevice *device) +{ + g_autoptr(FuDevice) root = fu_device_get_root (device); + fu_device_list_depsolve_order_full (self, root, 0); +} + /** * fu_device_list_get_all: * @self: A #FuDeviceList diff --git a/src/fu-device-list.h b/src/fu-device-list.h index 783b9d765..eaf711a6b 100644 --- a/src/fu-device-list.h +++ b/src/fu-device-list.h @@ -31,3 +31,5 @@ FuDevice *fu_device_list_get_by_guid (FuDeviceList *self, gboolean fu_device_list_wait_for_replug (FuDeviceList *self, FuDevice *device, GError **error); +void fu_device_list_depsolve_order (FuDeviceList *self, + FuDevice *device); diff --git a/src/fu-engine.c b/src/fu-engine.c index 72dff9b9a..f0c543ce3 100644 --- a/src/fu-engine.c +++ b/src/fu-engine.c @@ -5208,6 +5208,9 @@ fu_engine_add_device (FuEngine *self, FuDevice *device) /* create new device */ fu_device_list_add (self->device_list, device); + /* fix order */ + fu_device_list_depsolve_order (self->device_list, device); + /* fixup the name and format as needed from cached metadata */ if (component != NULL) fu_engine_md_refresh_device_from_component (self, device, component); diff --git a/src/fu-self-test.c b/src/fu-self-test.c index 1f9abbabf..903edba41 100644 --- a/src/fu-self-test.c +++ b/src/fu-self-test.c @@ -1076,15 +1076,18 @@ fu_engine_device_parent_func (gconstpointer user_data) /* add two together */ fu_engine_add_device (engine, device2); + /* this is normally done by fu_plugin_device_add() */ + fu_engine_add_device (engine, device3); + /* verify both children were adopted */ g_assert (fu_device_get_parent (device3) == device2); g_assert (fu_device_get_parent (device1) == device2); g_assert_cmpstr (fu_device_get_vendor (device3), ==, "oem"); /* verify order */ - g_assert_cmpint (fu_device_get_order (device1), ==, 0); - g_assert_cmpint (fu_device_get_order (device2), ==, 1); - g_assert_cmpint (fu_device_get_order (device3), ==, 0); + g_assert_cmpint (fu_device_get_order (device1), ==, -1); + g_assert_cmpint (fu_device_get_order (device2), ==, 0); + g_assert_cmpint (fu_device_get_order (device3), ==, -1); } static void