Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

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@xxxxxxxxxx
>> Fixes: 92c08a84b53e ("usb: dwc3: Add function suspend and function wakeup support")
>> Signed-off-by: Prashanth K <prashanth.k@xxxxxxxxxxxxxxxx>
>> ---
>>  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