dell-dock: add support for k2 dock #7355

CragW · 2024-06-17T15:42:09Z

Type of pull request:

New plugin (Please include new plugin checklist)
Code fix
Feature
Documentation

CragW · 2024-06-17T15:47:09Z

I'm seeing an error with WD19 that the child device is somewhat early finalized than the parent, I have no idea how to make the order. This issue isn't reproducible with origin/main, @superm1 any suggestion?

15:42:40.544 Fwupd                FuDevice child 0x5711be414510 was finalized while still having parent WD19TBS [570867f4ddda9915445da59bd85cd0e0e507270e]!
15:42:40.544 GLib-GObject         invalid unclassed pointer in cast to 'GObject'
15:42:40.545 GLib-GObject         g_object_weak_unref: assertion 'G_IS_OBJECT (object)' failed
15:42:40.545 GLib-GObject         g_object_unref: assertion 'G_IS_OBJECT (object)' failed

wd19.log

plugins/dell-dock/fu-dell-dock-ec-v2.c

superm1

As a general comment; it would be a lot easier to review if you can split this into individual commits.

It will also be a lot easier to bisect when/how problems are introduced if you have individual commits.

superm1 · 2024-06-18T02:51:08Z

plugins/dell-dock/meson.build

I think it's worth a commit before you add this to sort the list and put them in the right place alphabetically

Created #7369 to rebuild the base.

superm1 · 2024-06-18T02:51:24Z

plugins/dell-dock/fu-dell-dock-usb-pd.h

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2018 Dell Inc.


Surely 2024 for a new file?

superm1 · 2024-06-18T02:52:12Z

plugins/dell-dock/fu-dell-dock-usb-dpmux.h

@@ -0,0 +1,24 @@
+/*
+ * Copyright 2018 Dell Inc.


Surely 2024 for a new file?

superm1 · 2024-06-18T02:53:50Z

plugins/dell-dock/fu-dell-dock-hid-v2.h

@@ -0,0 +1,34 @@
+/*
+ * Copyright 2018 Realtek Semiconductor Corporation
+ * Copyright 2018 Dell Inc.


2024 right?

superm1 · 2024-06-18T02:55:41Z

plugins/dell-dock/fu-dell-dock-plugin.c

-	g_autoptr(FuDeviceLocker) locker = NULL;
-
-	locker = fu_device_locker_new(device, error);
-	if (locker == NULL)
+	if (!fu_device_open(device, error))
 		return FALSE;
-
 	fu_plugin_device_add(plugin, device);
-


isn't this a device lifecycle problem? You'll leave it open after it's created

superm1 · 2024-06-18T03:51:42Z

plugins/dell-dock/fu-dell-dock-ec-v2.c

+	name = g_string_new(self->dock_data->marketing_name);
+	if (name->len > 0)
+		fu_device_set_name(device, name->str);


has this happened in practice? I would think you should just be able to do fu_device_set_name(device, self->dock_data->marketing_name

superm1 · 2024-06-18T03:53:11Z

plugins/dell-dock/fu-dell-dock-ec-v2.h

all the enums in this file should be rustgen

Yes, -- and also structs like FuDellDockVer2DockFWVersion

superm1 · 2024-06-18T03:53:50Z

plugins/dell-dock/fu-dell-dock-hid-v2.c

+	/* header */
+	fu_byte_array_append_uint8(fwbuf, HID_v2_CMD_WRITE_DATA);
+	fu_byte_array_append_uint8(fwbuf, HID_v2_EXT_WRITE_DATA);
+	fu_byte_array_append_uint32(fwbuf, 7 + fw_size, G_BIG_ENDIAN); // 7 = sizeof(command)


if 7 = sizeof(command), how about just use that instead?

the command here means total bytes from below section, i.e.: 1 + 1 + 1 + 4. I haven't found a way to streamline hardcoded 7 into the sum of bytes.

superm1 · 2024-06-18T16:12:00Z

As a general comment; it would be a lot easier to review if you can split this into individual commits.

And you can do your generic "get ready" changes that move lines around and rename stuff and definitely don't break things immediately in another PR too to make this one a lot smaller.

CragW · 2024-06-19T16:11:59Z

plugins/dell-dock/fu-dell-dock-plugin.c

+			if (!fu_device_open(FU_DEVICE(hub_device), error))
+				return FALSE;
+		} else {
+			hub_device = FU_DELL_DOCK_HUB(device);


I've just realized that this assignment (or even with g_steal_pointer) will fall into a situation that a ghost device in the plugin. I don't have better way to do this but currently it is https://github.com/fwupd/fwupd/pull/7369/files#diff-cf3e2d3d7e960a425b0276b4438c42ef41d1d841fddafa0d256331654f303844R202.

plugins/dell-dock/fu-dell-dock-status.c

superm1 · 2024-07-02T16:13:20Z

plugins/dell-dock/fu-dell-dock-status.c

+	} else {
+		g_set_error_literal(error,
+				    FWUPD_ERROR,
+				    FWUPD_ERROR_NOT_SUPPORTED,
+				    "no valid proxy for package update.");
+	}


you should return FALSE in this branch

superm1 · 2024-07-08T17:26:18Z

plugins/dell-dock/fu-dell-dock-status.c

-	fu_device_add_flag(FU_DEVICE(self), FWUPD_DEVICE_FLAG_UNSIGNED_PAYLOAD);
+	fu_device_add_flag(FU_DEVICE(self), FWUPD_DEVICE_FLAG_SIGNED_PAYLOAD);


Is this really right? The package is being signed now? It's different from WD19/WD22 then. There should be special casing between docks for this flag it means.

I've just reconfirmed the status package itself is unsigned, will fix it.

superm1 · 2024-07-09T00:19:16Z

plugins/dell-dock/fu-dell-dock-i2c-pd-firmware.h

+#define FU_TYPE_DELL_DOCK_PD_FIRMWARE (fu_dell_dock_pd_firmware_get_type())
+G_DECLARE_FINAL_TYPE(FuDellDockPdFirmware,
+		     fu_dell_dock_pd_firmware,
+		     FU,
+		     DELL_DOCK_PD_FIRMWARE,
+		     FuFirmware)
+
+FuFirmware *
+fu_dell_dock_pd_firmware_new(void);


You're missing a call to fu_plugin_add_firmware_gtype to register this. Look at when/how other plugins do it.

CragW force-pushed the wip/cragw/k2 branch from 5d10b34 to ea729be Compare June 17, 2024 15:49

github-advanced-security bot found potential problems Jun 17, 2024

View reviewed changes

plugins/dell-dock/fu-dell-dock-ec-v2.c Fixed Show resolved Hide resolved

superm1 reviewed Jun 18, 2024

View reviewed changes

CragW commented Jun 19, 2024

View reviewed changes

CragW force-pushed the wip/cragw/k2 branch from ea729be to e02581a Compare July 2, 2024 16:01

superm1 reviewed Jul 2, 2024

View reviewed changes

plugins/dell-dock/fu-dell-dock-status.c Show resolved Hide resolved

superm1 reviewed Jul 2, 2024

View reviewed changes

CragW force-pushed the wip/cragw/k2 branch from e02581a to 5013d04 Compare July 8, 2024 17:15

superm1 reviewed Jul 8, 2024

View reviewed changes

superm1 reviewed Jul 9, 2024

View reviewed changes

CragW force-pushed the wip/cragw/k2 branch from 5013d04 to 70e172c Compare July 9, 2024 07:21

dell-dock: add support for k2 dock

ae013c5

CragW force-pushed the wip/cragw/k2 branch from 828f923 to ae013c5 Compare July 10, 2024 07:30

CragW closed this Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dell-dock: add support for k2 dock #7355

dell-dock: add support for k2 dock #7355

CragW commented Jun 17, 2024

CragW commented Jun 17, 2024

superm1 left a comment

superm1 Jun 18, 2024

CragW Jun 19, 2024

superm1 Jun 18, 2024

superm1 Jun 18, 2024

superm1 Jun 18, 2024

superm1 Jun 18, 2024

superm1 Jun 18, 2024

superm1 Jun 18, 2024

hughsie Jun 18, 2024

superm1 Jun 18, 2024

CragW Jul 2, 2024

superm1 commented Jun 18, 2024

CragW Jun 19, 2024

superm1 Jul 2, 2024

superm1 Jul 8, 2024

CragW Jul 8, 2024

superm1 Jul 9, 2024

		fu_device_add_flag(FU_DEVICE(self), FWUPD_DEVICE_FLAG_UNSIGNED_PAYLOAD);
		fu_device_add_flag(FU_DEVICE(self), FWUPD_DEVICE_FLAG_SIGNED_PAYLOAD);

dell-dock: add support for k2 dock #7355

dell-dock: add support for k2 dock #7355

Conversation

CragW commented Jun 17, 2024

CragW commented Jun 17, 2024

superm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superm1 commented Jun 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment