| Message ID | 20220713003523.29309-6-quic_wcheng@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix controller halt and endxfer timeout issues | expand |
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); ?
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
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 --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));
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(+)