| 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
| ||
|
Message-ID: <20250407233757.jmtohzgm4xebjndn@synopsys.com>
Date: Mon, 7 Apr 2025 23:38:28 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Prashanth K <prashanth.k@....qualcomm.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Kees Bakker <kees@...erbout.nl>,
William McVicker <willmcvicker@...gle.com>,
Marek Szyprowski <m.szyprowski@...sung.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"stable@...nel.org" <stable@...nel.org>
Subject: Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous
On Thu, Apr 03, 2025, Prashanth K wrote:
> Currently gadget_wakeup() waits for U0 synchronously if it was
> called from func_wakeup(), this is because we need to send the
> function wakeup command soon after the link is active. And the
> call is made synchronous by polling DSTS continuosly for 20000
> times in __dwc3_gadget_wakeup(). But it observed that sometimes
> the link is not active even after polling 20K times, leading to
> remote wakeup failures. Adding a small delay between each poll
> helps, but that won't guarantee resolution in future. Hence make
> the gadget_wakeup completely asynchronous.
>
> Since multiple interfaces can issue a function wakeup at once,
> add a new variable func_wakeup_pending which will indicate the
> functions that has issued func_wakup, this is represented in a
> bitmap format. If the link is in U3, dwc3_gadget_func_wakeup()
> will set the bit corresponding to interface_id and bail out.
> Once link comes back to U0, linksts_change irq is triggered,
> where the function wakeup command is sent based on bitmap.
>
> Cc: stable@...nel.org
> Fixes: 92c08a84b53e ("usb: dwc3: Add function suspend and function wakeup support")
> Signed-off-by: Prashanth K <prashanth.k@....qualcomm.com>
> ---
> drivers/usb/dwc3/core.h | 4 +++
> drivers/usb/dwc3/gadget.c | 60 ++++++++++++++++-----------------------
> 2 files changed, 28 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index aaa39e663f60..2cdbbd3236d7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1164,6 +1164,9 @@ struct dwc3_scratchpad_array {
> * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO,
> * DATWRREQINFO, and DESWRREQINFO value passed from
> * glue driver.
> + * @func_wakeup_pending: Indicates whether any interface has requested for
> + * function wakeup. Also represents the interface_id
> + * using bitmap.
> */
> struct dwc3 {
> struct work_struct drd_work;
> @@ -1394,6 +1397,7 @@ struct dwc3 {
> int num_ep_resized;
> struct dentry *debug_root;
> u32 gsbuscfg0_reqinfo;
> + u32 func_wakeup_pending;
Can we rename this to wakeup_pending_funcs to not be mixed with bitmap
vs boolean?
> };
>
> #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89a4dc8ebf94..3289e57471f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -276,8 +276,6 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> return ret;
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
> -
> /**
> * dwc3_send_gadget_ep_cmd - issue an endpoint command
> * @dep: the endpoint to which the command is going to be issued
> @@ -2351,10 +2349,8 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
> return __dwc3_gadget_get_frame(dwc);
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> {
> - int retries;
> -
> int ret;
> u32 reg;
>
> @@ -2382,8 +2378,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> return -EINVAL;
> }
>
> - if (async)
> - dwc3_gadget_enable_linksts_evts(dwc, true);
> + dwc3_gadget_enable_linksts_evts(dwc, true);
>
> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
> if (ret < 0) {
> @@ -2404,25 +2399,6 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> * Since link status change events are enabled we will receive
> * an U0 event when wakeup is successful. So bail out.
> */
> - if (async)
> - return 0;
> -
> - /* poll until Link State changes to ON */
> - retries = 20000;
> -
> - while (retries--) {
> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> -
> - /* in HS, means ON */
> - if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
> - break;
> - }
> -
> - if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
> - dev_err(dwc->dev, "failed to send remote wakeup\n");
> - return -EINVAL;
> - }
> -
> return 0;
> }
>
> @@ -2443,7 +2419,7 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> spin_unlock_irqrestore(&dwc->lock, flags);
> return -EINVAL;
> }
> - ret = __dwc3_gadget_wakeup(dwc, true);
> + ret = __dwc3_gadget_wakeup(dwc);
>
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> @@ -2471,14 +2447,10 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> */
> link_state = dwc3_gadget_get_link_state(dwc);
> if (link_state == DWC3_LINK_STATE_U3) {
> - ret = __dwc3_gadget_wakeup(dwc, false);
> - if (ret) {
> - spin_unlock_irqrestore(&dwc->lock, flags);
> - return -EINVAL;
> - }
> - dwc3_resume_gadget(dwc);
> - dwc->suspended = false;
> - dwc->link_state = DWC3_LINK_STATE_U0;
> + dwc->func_wakeup_pending |= BIT(intf_id);
> + ret = __dwc3_gadget_wakeup(dwc);
> + spin_unlock_irqrestore(&dwc->lock, flags);
> + return ret;
> }
>
> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> @@ -4300,6 +4272,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> {
> enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
> unsigned int pwropt;
> + int ret, intf_id = 0;
Can we keep declarations in separate lines?
>
> /*
> * WORKAROUND: DWC3 < 2.50a have an issue when configured without
> @@ -4375,7 +4348,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>
> switch (next) {
> case DWC3_LINK_STATE_U0:
> - if (dwc->gadget->wakeup_armed) {
> + if (dwc->gadget->wakeup_armed || dwc->func_wakeup_pending) {
> dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_resume_gadget(dwc);
> dwc->suspended = false;
> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> }
>
> dwc->link_state = next;
> +
> + /* Proceed with func wakeup if any interfaces that has requested */
> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> + if (dwc->func_wakeup_pending & BIT(0)) {
> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
> + if (ret)
> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> + intf_id, ret);
> + }
> + dwc->func_wakeup_pending >>= 1;
This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
use ffs(x) to properly find and clear the interface ID from the bitmap
one at a time.
> + intf_id++;
> + }
> +
> }
>
> static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
> --
> 2.25.1
>
Thanks,
Thinh
Powered by blists - more mailing lists