| 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: <10bf04c1-f040-4646-9484-70827db36d27@oss.qualcomm.com>
Date: Tue, 8 Apr 2025 11:04:03 +0530
From: Prashanth K <prashanth.k@....qualcomm.com>
To: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
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 08-04-25 05:08 am, Thinh Nguyen wrote:
> 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?
>
ACK
>> };
>>
>> #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?
>
OK
>>
>> /*
>> * 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.
>
Yes, we can use ffs(x). But I didn't understand how this would break
bitmap of dwc->func_wakeup_pending.
Regards,
Prashanth K
>> + intf_id++;
>> + }
>> +
>> }
>>
>> static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>> --
>> 2.25.1
>>
>
> Thanks,
> Thinh
Powered by blists - more mailing lists