Re: [PATCH v2 5/5] usb: dwc3: gadget: Increase DWC3 controller halt timeout

[Jack Pham]

Hi Wesley,

On Tue, Jul 12, 2022 at 05:35:23PM -0700, Wesley Cheng wrote:
> Since EP0 transactions need to be completed before the controller halt
> sequence is finished, this may take some time depending on the host and the
> enabled functions.  Increase the controller halt timeout, so that we give
> the controller sufficient time to handle EP0 transfers.
> 
> Fixes: 861c010a2ee1 ("usb: dwc3: gadget: Refactor pullup()")
> Suggested-by: Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>
> Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> ---
> Link:
>   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@xxxxxxxxxxxx/
> 
>  drivers/usb/dwc3/gadget.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 41b7007358de..e32d7293c447 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2476,6 +2476,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	dwc3_gadget_dctl_write_safe(dwc, reg);
>  
>  	do {
> +		msleep(1);

Be aware that this probably won't sleep for *just* 1ms.  From
Documentation/timers/timers-howto.rst:

	msleep(1~20) may not do what the caller intends, and
	will often sleep longer (~20 ms actual sleep for any
	value given in the 1~20ms range). In many cases this
	is not the desired behavior.

So with timeout==500 this loop could very well end up iterating for up
to 10 seconds.  Granted this shouldn't be called from any atomic context
but just wanted to make sure that the effective increase in timeout as
$SUBJECT intends is made clear here and that it's not overly generous.

>  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>  		reg &= DWC3_DSTS_DEVCTRLHLT;
>  	} while (--timeout && !(!is_on ^ !reg));

Jack