Re: [PATCH 2/3] HID: valve-index: Reboot headset on system power transitions

On Thu, Sep 10, 2026 at 03:58:27PM -0500, Mario Limonciello wrote:

On 9/10/26 15:52, Michal Pecio wrote:

quoted

On Thu, 10 Sep 2026 15:43:18 -0500, Mario Limonciello wrote:

quoted

quoted

quoted

+/*
+ * The headset's EDID service is lost when the host disables the DisplayPort
+ * PHY during system suspend, so it needs the reboot on the way out of
+ * suspend.  Doing it on the way in does not work: the headset dropping off
+ * USB is a remote-wakeup event from its hub and aborts the suspend.
+ */

The internal hub which will be quirked by the next patch, or its parent?

It has to be the internal hub if quirking it works, no?

The parent needs to be quirked: the breakout box's own hub, 28de:2613,
sits above Microchip USB2744 (0424:2744). Only the 28de:2613 hub ever
registered wakeup events, and quirking it alone is enough.

I also tested the quirk using a stock kernel (7.2.3) using the kernel
param `usbcore.quirks=28de:2613:j` which stopped the headset from waking
the host. Without that param I found that the headset would wake the
host when healthy but not when wedged.

quoted

I believe there are two separate problems here:

1. resetting the device at suspend causes instant wakeup
2. a few seconds later the system wakes up anyway

1. is solved by resetting on resume rather than suspend
2. is solved by the quirk

Questions:

Any chance that 2 also solves 1?

It does. With the quirk from 3/3 and the driver rebooting from the suspend
hook instead of resume the headset reboots during suspend entry and the
host stays asleep.

quoted

Would resetting on suspend be preferable, as the comment suggests?
Maybe it would, if the reset can race with DP seeing empty EDID?

Yes, and the log bears out the ordering concern. With the reboot on
suspend the headset is already back and so the EDID reads cleanly the
first time. With the headset reboot on resume a re-enumeration occurs.
There was no "EDID err" upon resume with either variant. We can go back to
Mario's implementation of headset reboot on suspend.

I do think that resetting on suspend makes a lot more sense for that exact
reason.  That's why my original PoC did it that way.

That's a very good idea to see if the quirk + moving it back to suspend
works.

I've validated this approach works. Happy to return to it.

Diff against 2/3 moving the headset reboot back to the suspend hook:

---8<---
 drivers/hid/hid-valve-index.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-valve-index.c b/drivers/hid/hid-valve-index.c
index 43c1142b7215..f6941dd04910 100644
--- a/drivers/hid/hid-valve-index.c
+++ b/drivers/hid/hid-valve-index.c

@@ -103,14 +103,18 @@ static struct attribute *valve_index_attrs[] = {
 ATTRIBUTE_GROUPS(valve_index);
 
 /*
- * The headset's EDID service is lost when the host disables the DisplayPort
- * PHY during system suspend, so it needs the reboot on the way out of
- * suspend.  Doing it on the way in does not work: the headset dropping off
- * USB is a remote-wakeup event from its hub and aborts the suspend.
+ * Disabling the DisplayPort PHY during system suspend leaves the headset
+ * unable to serve its EDID until it is rebooted, so send the reboot from
+ * the suspend hook.  The headset drops off USB about a second later; that
+ * does not abort the suspend because its breakout box hub is quirked to
+ * not be a wakeup source.  On resume the hub finds the rebooted headset on
+ * the same port and resets it in place, and the connector detection reads
+ * a fresh EDID.  Runtime autosuspend is left alone.
  */
-static int valve_index_resume(struct hid_device *hdev)
+static int valve_index_suspend(struct hid_device *hdev, pm_message_t message)
 {
-	valve_index_reboot(hdev, false);
+	if (!PMSG_IS_AUTO(message))
+		valve_index_reboot(hdev, false);
 
 	return 0;
 }

@@ -130,8 +134,7 @@ MODULE_DEVICE_TABLE(hid, valve_index_devices);
 static struct hid_driver valve_index_driver = {
 	.name = "valve-index",
 	.id_table = valve_index_devices,
-	.resume = valve_index_resume,
-	.reset_resume = valve_index_resume,
+	.suspend = valve_index_suspend,
 	.shutdown = valve_index_shutdown,
 	.driver.dev_groups = valve_index_groups,
 };