linux-kernel - Re: [v3] usb: dwc3: Avoid waking up gadget during startxfer

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] [day] [month] [year] [list]

Message-ID: <e31f0796-7163-a36c-486f-0f8e0a613661@quicinc.com>
Date: Wed, 21 Aug 2024 18:08:11 +0530
From: Prashanth K <quic_prashk@...cinc.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [v3] usb: dwc3: Avoid waking up gadget during startxfer



On 21-08-24 04:08 am, Thinh Nguyen wrote:
> On Tue, Aug 20, 2024, Prashanth K wrote:
>> When operating in High-Speed, it is observed that DSTS[USBLNKST] doesn't
>> update link state immediately after receiving the wakeup interrupt. Since
>> wakeup event handler calls the resume callbacks, there is a chance that
>> function drivers can perform an ep queue, which in turn tries to perform
>> remote wakeup from send_gadget_ep_cmd(STARTXFER). This happens because
>> DSTS[[21:18] wasn't updated to U0 yet, it's observed that the latency of
>> DSTS can be in order of milli-seconds. Hence avoid calling gadget_wakeup
>> during startxfer to prevent unnecessarily issuing remote wakeup to host.
>>
>> Fixes: c36d8e947a56 ("usb: dwc3: gadget: put link to U0 before Start Transfer")
>> Cc: <stable@...r.kernel.org>
>> Suggested-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
>> Signed-off-by: Prashanth K <quic_prashk@...cinc.com>
>> ---
>> v3: Added notes on top the function definition.
>> v2: Refactored the patch as suggested in v1 discussion.
>>
>>  drivers/usb/dwc3/gadget.c | 31 +++++++------------------------
>>  1 file changed, 7 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 89fc690fdf34..d4f2f0e1f031 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -287,6 +287,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>>   *
>>   * Caller should handle locking. This function will issue @cmd with given
>>   * @params to @dep and wait for its completion.
>> + *
>> + * According to databook, if the link is in L1/L2/U3 while issuing StartXfer command,
>> + * software must bring the link back to L0/U0 by performing remote wakeup. But we don't
> 
> Change "L0" -> "On" state
> 
>> + * expect ep_queue to trigger a remote wakeup; instead it should be done by wakeup ops.
>> + *
>> + * After receiving wakeup event, device should no longer be in U3, and any link
>> + * transition afterwards needs to be adressed with wakeup ops.
>>   */
> 
> You're missing the explanation for the case of L1. Please incorporate
> this snippet (reword as necessary to fit in the rest of your comment):
> 
> While operating in usb2 speed, if the device is in low power link state
> (L1/L2), the Start Transfer command may not complete and timeout. The
> programming guide suggested to initiate remote wakeup to bring the
> device to ON state, allowing the command to go through. However, since
> issuing a command in usb2 speed requires the clearing of
> GUSB2PHYCFG.suspendusb2, this turns on the signal required (in 50us) to
> complete a command. This should happen within the command timeout set by
> the driver. No extra handling is needed.
> 
> Special note: if wakeup() ops is triggered for remote wakeup, care
> should be taken should the Start Transfer command needs to be sent soon
> after. The wakeup() ops is asynchronous and the link state may not
> transition to U0 link state yet.
> 
> 

Does this sound good? (Didnt want to spam with new patches)

"According to databook, while issuing StartXfer command if the link is
in L1/L2/U3,
then the command may not complete and timeout, hence software must bring
the link
back to ON state by performing remote wakeup. However, since issuing a
command in
USB2 speeds requires the clearing of GUSB2PHYCFG.suspendusb2, which
turns on the
signal required to complete the given command (usually within 50us).
This should
happen within the command timeout set by driver. Hence we don't expect
to trigger
a remote wakeup from here; instead it should be done by wakeup ops.

Special note: If wakeup() ops is triggered for remote wakeup, care
should be taken
if StartXfer command needs to be sent soon after. The wakeup() ops is
asynchronous
and the link state may not transition to U0 link state yet. After
receiving wakeup
event, device would no longer be in U3, and any link transition
afterwards needs
to be adressed with wakeup ops."

Thanks,
Prashanth K