linux-kernel - Re: [PATCH] usb: dwc3: Fix latency of DSTS while receiving wakeup event

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]

Message-ID: <a89b5098-f9d6-4758-52b4-29d24244a09b@quicinc.com>
Date: Tue, 13 Aug 2024 15:23:32 +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: [PATCH] usb: dwc3: Fix latency of DSTS while receiving wakeup
 event



On 08-08-24 05:36 am, Thinh Nguyen wrote:


>> And turns out, retries 1500 to 15000 (worst case), which can range from 3ms
>> to 30ms. By this time, control can reach startXfer, where it tries to
>> perform remote-wakeup even if host just resumed the gadget.
> 
> Polling for 20K time is a bit much, and this will vary depending on
> different setup. This is something that I want to fix in the wakeup()
> ops and keep everything async.
> 
This was done as part of experiment, just to determine the latency in 
DSTS. And it was around 3-30ms. Saw rhis same behaviour when polling 
DSTS in __dwc3_gadget_wakeup(sync)

>>
>> For SS case, this retries count was consistently 1, it was passing in first
>> try itself. But unfortunately doesn't behave the same way in HS.
>>
>>> GUSB2PHYCFG.suspendusb2 turns on the signal required to complete a
>>> command within 50us. This happens within the timeout required for an
>>> endpoint command. As a result, there's no need to perform remote wakeup.
>>>
>>> For usb3 speed, if it's in U3, the gadget is in suspend anyway. There
>>> will be no ep_queue to trigger the Start Transfer command.
>>>
>>> You can just remove the whole Start Transfer check for remote wakeup
>>> completely.
>>>
>> Sorry, i didnt understand your suggestion. The startxfer check is needed as
>> per databook, but we also need to handle the latency seen in DSTS when
>> operating in HS.
>>
> 
> usb_ep_queue should not trigger remote wakeup; it should be done by
> wakeup() ops. The programming guide just noted that the Start Transfer
> command should not be issued while in L1/L2/U3. It suggested to wake up
> the host to bring it out of L1/L2/U3 state so the command can go
> through.
> 
> My suggestion is to remove the L1/L2/U3 check in
> dwc3_send_gadget_ep_cmd(), and it will still work fine with reasons
> noted previously. So, just do this:
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0ea2ca0f0d28..6ef6c4ef2a7b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -411,30 +411,6 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>                          dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>          }
> 
> -       if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
> -               int link_state;
> -
> -               /*
> -                * Initiate remote wakeup if the link state is in U3 when
> -                * operating in SS/SSP or L1/L2 when operating in HS/FS. If the
> -                * link state is in U1/U2, no remote wakeup is needed. The Start
> -                * Transfer command will initiate the link recovery.
> -                */
> -               link_state = dwc3_gadget_get_link_state(dwc);
> -               switch (link_state) {
> -               case DWC3_LINK_STATE_U2:
> -                       if (dwc->gadget->speed >= USB_SPEED_SUPER)
> -                               break;
> -
> -                       fallthrough;
> -               case DWC3_LINK_STATE_U3:
> -                       ret = __dwc3_gadget_wakeup(dwc, false);
> -                       dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
> -                                       ret);
> -                       break;
> -               }
> -       }
> -
>          /*
>           * For some commands such as Update Transfer command, DEPCMDPARn
>           * registers are reserved. Since the driver often sends Update Transfer
> 
> When we receive the wakeup event, then the device is no longer in
> L1/L2/U3. The Start Tranfer command should go through. >
Ok will do this, I hope there won't be any corner cases where the link 
is down when start_xfer happens. I was not really sure about the 
history, thats why tried to incorporate my fix into the above IF check.

> We do have an issue where if the function driver issues remote wakeup,
> the link may not transition before ep_queue() because wakeup() can be
> async. In that case, you probably want to keep the usb_requests in the
> pending_list until the link_state transitions out of low power.
> 
> The other thing that I noted previously is that I want to fix is the
> wakeup() ops. Currently it can be async or synchronous. We should keep
> it consistent and make it async throughout.
> 
Sounds like a good idea, we can move the req to pending list, then issue 
async wakeup, and queue it back once linksts_change interrupt indicates 
L0/U0. Special care is needed in dwc3_gadget_func_wakeup() when making 
it async.

Regards,
Prashanth K