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

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/12/26 09:07, Curtis Vogt wrote:
> On Thu, Sep 10, 2026 at 03:58:27PM -0500, Mario Limonciello wrote:
>>
>>
>> On 9/10/26 15:52, Michal Pecio wrote:
>>> On Thu, 10 Sep 2026 15:43:18 -0500, Mario Limonciello wrote:
>>>>>> +/*
>>>>>> + * 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.
> 
>>> 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.
> 
>>> 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,
>   };

Thanks for sharing that.  If we do stick to a kernel quirk we should do 
it at suspend instead of resume.

But to this audience, Curtis had some other findings that libddcutil is 
causing part of the problem on Linux.  There is still some more 
investigation to be done why (for example is it a concurrency issue for 
a shared AUX resource?).

If there is still kernel patches to be neeed, they'll be posted in a v2.