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

Message-ID: <20240820223800.zt52jaxedijbvskt@synopsys.com>
Date: Tue, 20 Aug 2024 22:38:02 +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: [v3] usb: dwc3: Avoid waking up gadget during startxfer

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.


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

Your $subject is missing "[Patch]" for some reason. Can you look into
that?

Thanks,
Thinh