linux-kernel - Re: [PATCH v4] 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: <20240828020348.z6575s5yowcf5bdx@synopsys.com>
Date: Wed, 28 Aug 2024 02:03:53 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <quic_prashk@...cinc.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "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 v4] usb: dwc3: Avoid waking up gadget during startxfer

On Tue, Aug 27, 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>
> ---
> v4: Rewording the comment in function definition.
> v3: Added notes on top the function definition.
> v2: Refactored the patch as suggested in v1 discussion.
> 
>  drivers/usb/dwc3/gadget.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89fc690fdf34..ea583d24aa37 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -287,6 +287,20 @@ 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, 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 ON state yet. And after receiving wakeup
> + * event, device would no longer be in U3, and any link transition afterwards needs
> + * to be adressed with wakeup ops.
>   */

Please reword as below:

According to the programming guide, if the link state is in L1/L2/U3,
then sending the Start Transfer command may not complete. The
programming guide suggested to bring the link state back to ON/U0 by
performing remote wakeup prior to sending the command. However, don't
initiate remote wakeup when the user/function does not send wakeup
request via wakeup ops. Send the command when it's allowed.

Notes:
For L1 link state, issuing a command 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. No additional step is needed.

For L2 or U3 link state, the gadget is in USB suspend. Care should be
taken when sending Start Transfer command to ensure that it's done after
USB resume.

>  int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
>  		struct dwc3_gadget_ep_cmd_params *params)
> @@ -327,30 +341,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
> -- 
> 2.25.1
> 

For the handling of some corner cases such as when restarting the
endpoint in the middle of the suspend or restarting the endpoint before
a remote wakeup, that's to be a separate fix.

With the current implemenation of dwc3 (and of its use of update
transfer command) and how the function drivers are currently
implemented, it's an unlikely scenario. Typical use case should not
encounter an issue.

Thanks,
Thinh