Use the parent physical ID to autoset the device parent

Of course, this only works if the parent and child both export a
FuDevice of the same physical kind, e.g. USB.

libfwupdplugin/fu-device-private.h

@@ -17,6 +17,9 @@ FuDeviceInternalFlags fu_device_internal_flag_from_string (const gchar	*flag);
 GPtrArray	*fu_device_get_parent_guids		(FuDevice	*self);
 gboolean	 fu_device_has_parent_guid		(FuDevice	*self,
 							 const gchar	*guid);
+GPtrArray	*fu_device_get_parent_physical_ids	(FuDevice	*self);
+gboolean	 fu_device_has_parent_physical_id	(FuDevice	*self,
+							 const gchar	*physical_id);
 void		 fu_device_set_parent			(FuDevice	*self,
 							 FuDevice	*parent);
 gint		 fu_device_get_order			(FuDevice	*self);

libfwupdplugin/fu-device.c

@@ -51,6 +51,7 @@ typedef struct {
 	GRWLock				 metadata_mutex;
 	GPtrArray			*parent_guids;
 	GRWLock				 parent_guids_mutex;
+	GPtrArray			*parent_physical_ids;	/* (nullable) */
 	guint				 remove_delay;	/* ms */
 	guint				 progress;
 	guint				 battery_level;
@@ -210,6 +211,8 @@ fu_device_internal_flag_to_string (FuDeviceInternalFlags flag)
 		return "is-open";
 	if (flag == FU_DEVICE_INTERNAL_FLAG_NO_SERIAL_NUMBER)
 		return "no-serial-number";
+	if (flag == FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN)
+		return "auto-parent-children";
 	return NULL;
 }
 
@@ -250,6 +253,8 @@ fu_device_internal_flag_from_string (const gchar *flag)
 		return FU_DEVICE_INTERNAL_FLAG_IS_OPEN;
 	if (g_strcmp0 (flag, "no-serial-number") == 0)
 		return FU_DEVICE_INTERNAL_FLAG_NO_SERIAL_NUMBER;
+	if (g_strcmp0 (flag, "auto-parent-children") == 0)
+		return FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN;
 	return FU_DEVICE_INTERNAL_FLAG_UNKNOWN;
 }
 
@@ -883,6 +888,13 @@ fu_device_set_parent (FuDevice *self, FuDevice *parent)
 {
 	g_return_if_fail (FU_IS_DEVICE (self));
 
+	/* debug */
+	if (parent != NULL) {
+		g_debug ("setting parent of %s [%s] to be %s [%s]",
+			 fu_device_get_name (self), fu_device_get_id (self),
+			 fu_device_get_name (parent), fu_device_get_id (parent));
+	}
+
 	/* set the composite ID on the children and grandchildren */
 	if (parent != NULL)
 		fu_device_set_composite_id (self, fu_device_get_composite_id (parent));
@@ -1120,6 +1132,86 @@ fu_device_add_parent_guid (FuDevice *self, const gchar *guid)
 	g_ptr_array_add (priv->parent_guids, g_strdup (guid));
 }
 
+/**
+ * fu_device_get_parent_physical_ids:
+ * @self: a #FuDevice
+ *
+ * Gets any parent device IDs. If a device is added to the daemon that matches
+ * the physical ID added from fu_device_add_parent_physical_id() then this
+ * device is marked the parent of @self.
+ *
+ * Returns: (transfer none) (element-type utf8) (nullable): a list of IDs
+ *
+ * Since: 1.6.2
+ **/
+GPtrArray *
+fu_device_get_parent_physical_ids (FuDevice *self)
+{
+	FuDevicePrivate *priv = GET_PRIVATE (self);
+	g_return_val_if_fail (FU_IS_DEVICE (self), NULL);
+	return priv->parent_physical_ids;
+}
+
+/**
+ * fu_device_has_parent_physical_id:
+ * @self: a #FuDevice
+ * @physical_id: a device physical ID
+ *
+ * Searches the list of parent IDs for a string match.
+ *
+ * Returns: %TRUE if the parent ID exists
+ *
+ * Since: 1.6.2
+ **/
+gboolean
+fu_device_has_parent_physical_id (FuDevice *self, const gchar *physical_id)
+{
+	FuDevicePrivate *priv = GET_PRIVATE (self);
+
+	g_return_val_if_fail (FU_IS_DEVICE (self), FALSE);
+	g_return_val_if_fail (physical_id != NULL, FALSE);
+
+	if (priv->parent_physical_ids == NULL)
+		return FALSE;
+	for (guint i = 0; i < priv->parent_physical_ids->len; i++) {
+		const gchar *tmp = g_ptr_array_index (priv->parent_physical_ids, i);
+		if (g_strcmp0 (tmp, physical_id) == 0)
+			return TRUE;
+	}
+	return FALSE;
+}
+
+/**
+ * fu_device_add_parent_physical_id:
+ * @self: a #FuDevice
+ * @physical_id: a device physical ID
+ *
+ * Sets any parent device using the physical ID. An parent device is logically
+ * linked to the primary device in some way and can be added before or after @self.
+ *
+ * The IDs are searched in order, and so the order of adding IDs may be
+ * important if more than one parent device might match.
+ *
+ * Since: 1.6.2
+ **/
+void
+fu_device_add_parent_physical_id (FuDevice *self, const gchar *physical_id)
+{
+	FuDevicePrivate *priv = GET_PRIVATE (self);
+
+	g_return_if_fail (FU_IS_DEVICE (self));
+	g_return_if_fail (physical_id != NULL);
+
+	/* ensure exists */
+	if (priv->parent_physical_ids == NULL)
+		priv->parent_physical_ids = g_ptr_array_new_with_free_func (g_free);
+
+	/* already present */
+	if (fu_device_has_parent_physical_id (self, physical_id))
+		return;
+	g_ptr_array_add (priv->parent_physical_ids, g_strdup (physical_id));
+}
+
 static gboolean
 fu_device_add_child_by_type_guid (FuDevice *self,
 				  GType type,
@@ -2897,6 +2989,10 @@ fu_device_add_string (FuDevice *self, guint idt, GString *str)
 		const gchar *name = g_ptr_array_index (priv->possible_plugins, i);
 		fu_common_string_append_kv (str, idt + 1, "PossiblePlugin", name);
 	}
+	if (priv->parent_physical_ids != NULL && priv->parent_physical_ids->len > 0) {
+		g_autofree gchar *flags = fu_common_strjoin_array (",", priv->parent_physical_ids);
+		fu_common_string_append_kv (str, idt + 1, "ParentPhysicalIds", flags);
+	}
 	if (priv->internal_flags != FU_DEVICE_INTERNAL_FLAG_NONE) {
 		g_autoptr(GString) tmp2 = g_string_new ("");
 		for (guint i = 0; i < 64; i++) {
@@ -3853,6 +3949,7 @@ fu_device_incorporate (FuDevice *self, FuDevice *donor)
 	FuDevicePrivate *priv_donor = GET_PRIVATE (donor);
 	GPtrArray *instance_ids = fu_device_get_instance_ids (donor);
 	GPtrArray *parent_guids = fu_device_get_parent_guids (donor);
+	GPtrArray *parent_physical_ids = fu_device_get_parent_physical_ids (donor);
 
 	g_return_if_fail (FU_IS_DEVICE (self));
 	g_return_if_fail (FU_IS_DEVICE (donor));
@@ -3878,6 +3975,12 @@ fu_device_incorporate (FuDevice *self, FuDevice *donor)
 	for (guint i = 0; i < parent_guids->len; i++)
 		fu_device_add_parent_guid (self, g_ptr_array_index (parent_guids, i));
 	g_rw_lock_reader_unlock (&priv_donor->parent_guids_mutex);
+	if (parent_physical_ids != NULL) {
+		for (guint i = 0; i < parent_physical_ids->len; i++) {
+			const gchar *tmp = g_ptr_array_index (parent_physical_ids, i);
+			fu_device_add_parent_physical_id (self, tmp);
+		}
+	}
 	g_rw_lock_reader_lock (&priv_donor->metadata_mutex);
 	if (priv->metadata != NULL) {
 		g_autoptr(GList) keys = g_hash_table_get_keys (priv_donor->metadata);
@@ -4057,6 +4160,8 @@ fu_device_finalize (GObject *object)
 		g_hash_table_unref (priv->metadata);
 	if (priv->inhibits != NULL)
 		g_hash_table_unref (priv->inhibits);
+	if (priv->parent_physical_ids != NULL)
+		g_ptr_array_unref (priv->parent_physical_ids);
 	g_ptr_array_unref (priv->parent_guids);
 	g_ptr_array_unref (priv->possible_plugins);
 	g_ptr_array_unref (priv->retry_recs);

libfwupdplugin/fu-device.h

@@ -229,6 +229,7 @@ FuDevice	*fu_device_new				(void);
  * @FU_DEVICE_INTERNAL_FLAG_INHERIT_ACTIVATION:		Inherit activation status from the history database on startup
  * @FU_DEVICE_INTERNAL_FLAG_IS_OPEN:			The device opened successfully and ready to use
  * @FU_DEVICE_INTERNAL_FLAG_NO_SERIAL_NUMBER:		Do not attempt to read the device serial number
+ * @FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN:	Automatically assign the parent for children of this device
  *
  * The device internal flags.
  **/
@@ -246,6 +247,7 @@ typedef enum {
 	FU_DEVICE_INTERNAL_FLAG_INHERIT_ACTIVATION	= (1llu << 9),  /* Since: 1.5.9 */
 	FU_DEVICE_INTERNAL_FLAG_IS_OPEN			= (1llu << 10),	/* Since: 1.6.1 */
 	FU_DEVICE_INTERNAL_FLAG_NO_SERIAL_NUMBER	= (1llu << 11),	/* Since: 1.6.2 */
+	FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN	= (1llu << 12),	/* Since: 1.6.2 */
 	/*< private >*/
 	FU_DEVICE_INTERNAL_FLAG_UNKNOWN			= G_MAXUINT64,
 } FuDeviceInternalFlags;
@@ -275,6 +277,8 @@ void		 fu_device_add_child			(FuDevice	*self,
 							 FuDevice	*child);
 void		 fu_device_add_parent_guid		(FuDevice	*self,
 							 const gchar	*guid);
+void		 fu_device_add_parent_physical_id	(FuDevice	*self,
+							 const gchar	*physical_id);
 void		 fu_device_add_counterpart_guid		(FuDevice	*self,
 							 const gchar	*guid);
 FuDevice	*fu_device_get_proxy			(FuDevice	*self);

libfwupdplugin/fu-usb-device.c

@@ -287,6 +287,7 @@ fu_usb_device_probe (FuDevice *device, GError **error)
 	g_autofree gchar *devid0 = NULL;
 	g_autofree gchar *devid1 = NULL;
 	g_autofree gchar *devid2 = NULL;
+	g_autofree gchar *platform_id = NULL;
 	g_autofree gchar *vendor_id = NULL;
 	g_autoptr(GPtrArray) intfs = NULL;
 
@@ -344,6 +345,18 @@ fu_usb_device_probe (FuDevice *device, GError **error)
 		fu_device_add_instance_id_full (device, intid3,
 						FU_DEVICE_INSTANCE_FLAG_ONLY_QUIRKS);
 	}
+
+	/* add 2 levels of parent IDs */
+	platform_id = g_strdup (g_usb_device_get_platform_id (priv->usb_device));
+	for (guint i = 0; i < 2; i++) {
+		gchar *tok = g_strrstr (platform_id, ":");
+		if (tok == NULL)
+			break;
+		*tok = '\0';
+		if (g_strcmp0 (platform_id, "usb") == 0)
+			break;
+		fu_device_add_parent_physical_id (device, platform_id);
+	}
 #endif
 
 	/* success */

libfwupdplugin/fwupdplugin.map

@@ -819,5 +819,8 @@ LIBFWUPDPLUGIN_1.6.1 {
 LIBFWUPDPLUGIN_1.6.2 {
   global:
     fu_common_check_kernel_version;
+    fu_device_add_parent_physical_id;
+    fu_device_get_parent_physical_ids;
+    fu_device_has_parent_physical_id;
   local: *;
 } LIBFWUPDPLUGIN_1.6.1;

src/fu-engine.c

@@ -5350,27 +5350,49 @@ fu_engine_adopt_children (FuEngine *self, FuDevice *device)
 	GPtrArray *guids;
 	g_autoptr(GPtrArray) devices = fu_device_list_get_active (self->device_list);
 
-	/* find the parent GUID in any existing device */
+	/* find the parent in any existing device */
-	guids = fu_device_get_parent_guids (device);
+	if (fu_device_get_parent (device) == NULL) {
-	for (guint j = 0; j < guids->len; j++) {
-		const gchar *guid = g_ptr_array_index (guids, j);
 		for (guint i = 0; i < devices->len; i++) {
 			FuDevice *device_tmp = g_ptr_array_index (devices, i);
-			if (fu_device_get_parent (device) != NULL)
+			if (!fu_device_has_internal_flag (device_tmp, FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN))
 				continue;
-			if (fu_device_has_guid (device_tmp, guid)) {
+			if (fu_device_get_physical_id (device_tmp) == NULL)
-				g_debug ("setting parent of %s [%s] to be %s [%s]",
+				continue;
-					 fu_device_get_name (device),
+			if (fu_device_has_parent_physical_id (device, fu_device_get_physical_id (device_tmp))) {
-					 fu_device_get_id (device),
-					 fu_device_get_name (device_tmp),
-					 fu_device_get_id (device_tmp));
 				fu_device_set_parent (device, device_tmp);
 				break;
 			}
 		}
 	}
+	if (fu_device_get_parent (device) == NULL) {
+		guids = fu_device_get_parent_guids (device);
+		for (guint j = 0; j < guids->len; j++) {
+			const gchar *guid = g_ptr_array_index (guids, j);
+			for (guint i = 0; i < devices->len; i++) {
+				FuDevice *device_tmp = g_ptr_array_index (devices, i);
+				if (fu_device_has_guid (device_tmp, guid)) {
+					fu_device_set_parent (device, device_tmp);
+					break;
+				}
+			}
+		}
+	}
 
 	/* the new device is the parent to an existing child */
+	for (guint j = 0; j < devices->len; j++) {
+		GPtrArray *parent_physical_ids = NULL;
+		FuDevice *device_tmp = g_ptr_array_index (devices, j);
+		if (fu_device_get_parent (device_tmp) != NULL)
+			continue;
+		parent_physical_ids = fu_device_get_parent_physical_ids (device_tmp);
+		if (parent_physical_ids == NULL)
+			continue;
+		for (guint i = 0; i < parent_physical_ids->len; i++) {
+			const gchar *parent_physical_id = g_ptr_array_index (parent_physical_ids, i);
+			if (g_strcmp0 (parent_physical_id, fu_device_get_physical_id (device)) == 0)
+				fu_device_set_parent (device_tmp, device);
+		}
+	}
 	guids = fu_device_get_guids (device);
 	for (guint j = 0; j < guids->len; j++) {
 		const gchar *guid = g_ptr_array_index (guids, j);
@@ -5378,16 +5400,10 @@ fu_engine_adopt_children (FuEngine *self, FuDevice *device)
 			FuDevice *device_tmp = g_ptr_array_index (devices, i);
 			if (fu_device_get_parent (device_tmp) != NULL)
 				continue;
-			if (fu_device_has_parent_guid (device_tmp, guid)) {
+			if (fu_device_has_parent_guid (device_tmp, guid))
-				g_debug ("setting parent of %s [%s] to be %s [%s]",
-					 fu_device_get_name (device_tmp),
-					 fu_device_get_id (device_tmp),
-					 fu_device_get_name (device),
-					 fu_device_get_id (device));
 				fu_device_set_parent (device_tmp, device);
 		}
 	}
-	}
 }
 
 static void

src/fu-self-test.c

@@ -1046,7 +1046,7 @@ fu_engine_requirements_parent_device_func (gconstpointer user_data)
 }
 
 static void
-fu_engine_device_parent_func (gconstpointer user_data)
+fu_engine_device_parent_guid_func (gconstpointer user_data)
 {
 	g_autoptr(FuDevice) device1 = fu_device_new ();
 	g_autoptr(FuDevice) device2 = fu_device_new ();
@@ -1098,6 +1098,75 @@ fu_engine_device_parent_func (gconstpointer user_data)
 	g_assert_cmpint (fu_device_get_order (device3), ==, -1);
 }
 
+static void
+fu_engine_device_parent_id_func (gconstpointer user_data)
+{
+	g_autoptr(FuDevice) device1 = fu_device_new ();
+	g_autoptr(FuDevice) device2 = fu_device_new ();
+	g_autoptr(FuDevice) device3 = fu_device_new ();
+	g_autoptr(FuDevice) device4 = fu_device_new ();
+	g_autoptr(FuEngine) engine = fu_engine_new (FU_APP_FLAGS_NONE);
+	g_autoptr(XbSilo) silo_empty = xb_silo_new ();
+
+	/* no metadata in daemon */
+	fu_engine_set_silo (engine, silo_empty);
+
+	/* add child */
+	fu_device_set_id (device1, "child1");
+	fu_device_set_name (device1, "Child1");
+	fu_device_set_physical_id (device1, "child-ID1");
+	fu_device_add_vendor_id (device1, "USB:FFFF");
+	fu_device_add_protocol (device1, "com.acme");
+	fu_device_add_instance_id (device1, "child-GUID-1");
+	fu_device_add_parent_physical_id (device1, "parent-ID-notfound");
+	fu_device_add_parent_physical_id (device1, "parent-ID");
+	fu_device_convert_instance_ids (device1);
+	fu_engine_add_device (engine, device1);
+
+	/* parent */
+	fu_device_set_id (device2, "parent");
+	fu_device_set_name (device2, "Parent");
+	fu_device_set_physical_id (device2, "parent-ID");
+	fu_device_add_vendor_id (device2, "USB:FFFF");
+	fu_device_add_protocol (device2, "com.acme");
+	fu_device_add_instance_id (device2, "parent-GUID");
+	fu_device_set_vendor (device2, "oem");
+	fu_device_add_internal_flag (device2, FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN);
+	fu_device_convert_instance_ids (device2);
+
+	/* add another child */
+	fu_device_set_id (device3, "child2");
+	fu_device_set_name (device3, "Child2");
+	fu_device_set_physical_id (device3, "child-ID2");
+	fu_device_add_instance_id (device3, "child-GUID-2");
+	fu_device_add_parent_physical_id (device3, "parent-ID");
+	fu_device_convert_instance_ids (device3);
+	fu_device_add_child (device2, device3);
+
+	/* add two together */
+	fu_engine_add_device (engine, device2);
+
+	/* add non-child */
+	fu_device_set_id (device4, "child4");
+	fu_device_set_name (device4, "Child4");
+	fu_device_set_physical_id (device4, "child-ID4");
+	fu_device_add_vendor_id (device4, "USB:FFFF");
+	fu_device_add_protocol (device4, "com.acme");
+	fu_device_add_instance_id (device4, "child-GUID-4");
+	fu_device_add_parent_physical_id (device4, "parent-ID");
+	fu_device_convert_instance_ids (device4);
+	fu_engine_add_device (engine, device4);
+
+	/* this is normally done by fu_plugin_device_add() */
+	fu_engine_add_device (engine, device4);
+
+	/* verify both children were adopted */
+	g_assert (fu_device_get_parent (device3) == device2);
+	g_assert (fu_device_get_parent (device4) == device2);
+	g_assert (fu_device_get_parent (device1) == device2);
+	g_assert_cmpstr (fu_device_get_vendor (device3), ==, "oem");
+}
+
 static void
 fu_engine_partial_hash_func (gconstpointer user_data)
 {
@@ -3246,8 +3315,10 @@ main (int argc, char **argv)
 			      fu_engine_requirements_device_plain_func);
 	g_test_add_data_func ("/fwupd/engine{requirements-version-format}", self,
 			      fu_engine_requirements_version_format_func);
-	g_test_add_data_func ("/fwupd/engine{device-auto-parent}", self,
+	g_test_add_data_func ("/fwupd/engine{device-auto-parent-id}", self,
-			      fu_engine_device_parent_func);
+			      fu_engine_device_parent_id_func);
+	g_test_add_data_func ("/fwupd/engine{device-auto-parent-guid}", self,
+			      fu_engine_device_parent_guid_func);
 	g_test_add_data_func ("/fwupd/engine{install-duration}", self,
 			      fu_engine_install_duration_func);
 	g_test_add_data_func ("/fwupd/engine{generate-md}", self,