diff mbox series

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

Message ID 20220713003523.29309-6-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Fix controller halt and endxfer timeout issues | expand

Commit Message

Wesley Cheng July 13, 2022, 12:35 a.m. UTC
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@synopsys.com>
Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Link:
  https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/

 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Jack Pham July 13, 2022, 2:56 a.m. UTC | #1
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@synopsys.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Link:
>   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/
> 
>  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
John Keeping July 13, 2022, 11:40 a.m. UTC | #2
On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
> 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@synopsys.com>
> > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > ---
> > Link:
> >   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/
> > 
> >  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));

Does it make sense to convert this loop to use read_poll_timeout() and
make the timeout explicit, something like:

	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);

?
Jack Pham July 13, 2022, 9:53 p.m. UTC | #3
On Wed, Jul 13, 2022 at 12:40:53PM +0100, John Keeping wrote:
> On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
> > 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@synopsys.com>
> > > Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> > > ---
> > > Link:
> > >   https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/
> > > 
> > >  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));
> 
> Does it make sense to convert this loop to use read_poll_timeout() and
> make the timeout explicit, something like:
> 
> 	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
> 				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);
> 
> ?

Yeah I think it would make sense.  Might even be worthwhile to revisit
similar loops being performed in dwc3_send_gadget_generic_command() and
dwc3_send_gadget_ep_cmd() which are currently spinning delay-lessly for a
fixed number of iterations.

Jack
Thinh Nguyen July 14, 2022, 12:56 a.m. UTC | #4
On 7/13/2022, Jack Pham wrote:
> On Wed, Jul 13, 2022 at 12:40:53PM +0100, John Keeping wrote:
>> On Tue, Jul 12, 2022 at 07:56:43PM -0700, Jack Pham wrote:
>>> 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@synopsys.com>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>>>> ---
>>>> Link:
>>>>    https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/4988ed34-04a4-060a-ccef-f57790f76a2b@synopsys.com/__;!!A4F2R9G_pg!eidgBYKrTCOm9XSLhpRDscGcM5pkmRIG-XDwBbOYmdcEWUM2MhWJLeeJHhTm8TPNNs9hOgaK1yT8W-0zeZ51Pip-VA$
>>>>
>>>>   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));
>> Does it make sense to convert this loop to use read_poll_timeout() and
>> make the timeout explicit, something like:
>>
>> 	ret = read_poll_timeout(dwc3_readl, reg, !(!is_on ^ !(reg & DWC3_DSTS_DEVCTRLHLT)),
>> 				100, timeout * USEC_PER_MSEC, true, dwc->regs, DWC3_DSTS);
>>
>> ?
> Yeah I think it would make sense.  Might even be worthwhile to revisit
> similar loops being performed in dwc3_send_gadget_generic_command() and
> dwc3_send_gadget_ep_cmd() which are currently spinning delay-lessly for a
> fixed number of iterations.
>

++ Jun

BTW, Jun started on this awhile ago. You can review his patch for 
reference if anyone wants to take on this:

https://patchwork.kernel.org/project/linux-usb/patch/1588928985-1585-1-git-send-email-jun.li@nxp.com/

Thanks,
Thinh
diff mbox series

Patch

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);
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 		reg &= DWC3_DSTS_DEVCTRLHLT;
 	} while (--timeout && !(!is_on ^ !reg));