linux-kernel - Re: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 9 Feb 2023 17:36:23 -0800
From:   Elson Serrao <quic_eserrao@...cinc.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH v3 2/5] usb: dwc3: Add remote wakeup handling



On 2/7/2023 6:11 PM, Thinh Nguyen wrote:
> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>
>>
>> On 2/7/2023 5:10 PM, Thinh Nguyen wrote:
>>> On Tue, Feb 07, 2023, Elson Serrao wrote:
>>>> On 2/6/2023 4:48 PM, Thinh Nguyen wrote:
>>>>>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>>>>>     {
>>>>>>     	int			retries;
>>>>>> @@ -2296,9 +2309,14 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>>>>>>     	link_state = DWC3_DSTS_USBLNKST(reg);
>>>>>>     	switch (link_state) {
>>>>>> +	case DWC3_LINK_STATE_U3:	/* in HS, means SUSPEND */
>>>>>
>>>>> It's also possible to do remote wakeup in L1 for highspeed.
>>>>>
>>>>
>>>> The rw_configured flag here is in context of triggering remote wakeup from
>>>> bus suspend only.
>>>>
>>>> The remote wakeup setting for l1 in HighSpeed is controlled through LPM
>>>> token and overrides/ignores the config desc bmAttributes wakeup bit.
>>>>
>>>> Section 4.1 of USB2_LinkPowerMangement_ECN[final] spec "The host system sets the Remote Wake Flag parameter in this request to
>>>> enable or disable the addressed device
>>>> for remote wake from L1. The value of this flag will temporarily (while in
>>>> L1) override the current setting of the
>>>> Remote Wake feature settable by the standard Set/ClearFeature() commands
>>>> defined in Universal Serial Bus Specification, revision 2.0, Chapter 9."
>>>>
>>>> Please let me know if I am missing something.
>>>>
>>>
>>> It overrides the setting of the SetFeature request, not the device
>>> configuration.
>>>
>>> The rw_configured reflects the user configuration. Whether the host
>>> tries to enable the remote wakeup through SetFeature request or LPM
>>> token, the device should operate within the user configuration
>>> limitation.
>>>
>>> If the configuration indicates that it doesn't support remote wakeup, we
>>> should prevent unexpected behavior from the device. For simplicity, we
>>> can just return failure to wakeup for all states.
>>>
>>> Thanks,
>>> Thinh
>>
>> L1 entry/exit is HW controlled and the remote wakeup is conditional.(Section
>> 7.1/Table7.2 of dwc3 data book). Even though we block it from
>> SW the l1 exit will still happen from HW point of view.
>>
>> To correlate the user configuration with LPM token, I experimented by
>> disabling the wakeup bit in the bmAtrributes, but I still see remote wakeup
>> bit being set in the LPM token. From the observation it seems like there is
> 
> That's because the linux xhci driver enables remote wakeup bit in its
> port without regard for the device configuration.
> 
>> no correlation between the wakeup bit in the bmAtrributes and the wakeup bit
>> in the LPM token.
>>
> 
> The host can bring the device out of L1, that's probably what you saw.
> The controller doesn't initiate remote wakeup by itself.
> 
> Thanks,
> Thinh

Actually it seems the controller is initiating a remote wakeup by itself 
to exit from l1 when we send a STARTTRANSFER command. I did below 
experiment when the device was in HighSpeed

1.) Enabled l1.
2.) Disabled the remote wakeup software path (i.e avoid calling 
__gadget_wakeup() if link is in l1 in the gadget_ep_cmd() path).
3.) Sent an IN packet when the link was in l1.

 From the lecroy logs it looks like the controller initiated a remote 
wakeup and sent the data.

Below are the events and the corresponding lecroy snippet
1.)Packet(55551) ------------> LPM token from Windows Host PC.

2.) Link in l1 for 2.445 secs

3. ) Send a ping data from device to host

4. )Packet(55554) ----------------> Resume

5.) IN data


Packet#
_______|_______________________________________________________________________
Transaction(26584) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time Stamp(27 . 204 671 
632)
_______|_______________________________________________________________________Ch0 

Packet(55550) Dir H(S) EXT(0x0F) ADDR(11) ENDP(0) CRC5(0x04) Pkt Len(8)
_______| Duration(133.330 ns) Idle(200.660 ns) Time Stamp(27 . 204 671 632)
_______|_______________________________________________________________________Ch0 

Packet(55551) Dir H(S) LPM(0xC3) BESL(150 us) Link State(0x1)
_______| Rem Wake(0x1) Rsvd(0x0) CRC5(0x04) Pkt Len(8) Duration(133.330 ns)
_______| Idle(182.660 ns) Time Stamp(27 . 204 671 966)
_______|_______________________________________________________________________Ch0 

Packet(55552) Dir H(S) ACK(0x4B) Pkt Len(6) Duration(100.000 ns)
_______| Idle( 11.450 us) Time Stamp(27 . 204 672 282)
_______|_______________________________________________________________________Ch0 

Packet(55553) Dir(?) Full Speed J (Suspend)( 2.445 sec)
_______| Time Stamp(27 . 204 683 832)
_______|_______________________________________________________________________Ch0 

Packet(55554) Dir(?) Full Speed K (Resume?)( 95.168 us) Time(165.134 us)
_______| Time Stamp(29 . 649 644 482)
_______|_______________________________________________________________________
Transfer(67) H(S) Bulk(IN) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(309.366 us) Time Stamp(29 . 649 809 616)
_______|_______________________________________________________________________
Transfer(68) H(S) Bulk(OUT) ADDR(11) ENDP(1) Bytes Transferred(142)
_______| Time(520.050 us) Time Stamp(29 . 650 118 982)
_______|_______________________________________________________________________
Transaction(26655) H(S) EXT(0x0F) LPM(0xC3) ADDR(11) ENDP(0) BESL(150 us)
_______| Link State(0x1) Rem Wake(0x1) ACK(0x4B) Time( 12.168 us)
_______| Time Stamp(29 . 650 639 032)
_______|_______________________________________________________________________

If software was solely responsible for waking up from l1, then there 
should be no reason why the host would exit l1 in this scenario.
I tried with different ping intervals and saw that the duration for 
which the link was in l1 correlates with the ping interval.

Thanks
Elson