| Message ID | 1536418986-16609-5-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver | expand |
Hi Anurag, On 9/8/2018 8:03 AM, Anurag Kumar Vulisha wrote: > According to dwc3 databook when streams are used, it may be possible > for the host and device become out of sync, where device may wait for > host to issue prime transcation and host may wait for device to issue > erdy. To avoid such deadlock, timeout needs to be implemented. After > timeout occurs, device will first stop transfer and restart the transfer > again. This patch does the same. > > Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> > Reviewed-by: Thinh Nguyen <thinhn@synopsys.com> > --- > Chnages in v4: > 1. Added description for stream timeout timer as suggested by > "Thinh Nguyen" > > Changes in v3: > 1. Added the changes suggested by "Thinh Nguyen" > > Changes in v2: > 1. Changed STREAM_TIMEOUT to STREAM_TIMEOUT_MS as suggested by > "Andy Shevchenko" > --- > drivers/usb/dwc3/core.h | 7 +++++++ > drivers/usb/dwc3/gadget.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 5bfb625..f62e8c4 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -633,6 +633,11 @@ struct dwc3_event_buffer { > > #define DWC3_TRB_NUM 256 > > +/* > + * Timeout value in msecs used by stream_timeout_timer when streams are enabled > + */ > +#define STREAM_TIMEOUT_MS 50 > + > /** > * struct dwc3_ep - device side endpoint representation > * @endpoint: usb endpoint > @@ -656,6 +661,7 @@ struct dwc3_event_buffer { > * @name: a human readable name e.g. ep1out-bulk > * @direction: true for TX, false for RX > * @stream_capable: true when streams are enabled > + * @stream_timeout_timer: timeout timer used by bulk streams > */ > struct dwc3_ep { > struct usb_ep endpoint; > @@ -705,6 +711,7 @@ struct dwc3_ep { > > unsigned direction:1; > unsigned stream_capable:1; > + struct timer_list stream_timeout_timer; > }; > > enum dwc3_phy { > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 13ea282..306d4c5 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -254,6 +254,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param) > } > > static int __dwc3_gadget_wakeup(struct dwc3 *dwc); > +static void stream_timeout_function(struct timer_list *arg); > > /** > * dwc3_send_gadget_ep_cmd - issue an endpoint command > @@ -574,6 +575,17 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) > | DWC3_DEPCFG_STREAM_EVENT_EN > | DWC3_DEPCFG_XFER_COMPLETE_EN; > dep->stream_capable = true; > + > + /* > + * When BULK streams are enabled it may be possible for the host > + * and device become out of sync, where device may wait for host > + * to issue prime transcation and host may wait for device to > + * issue ERDY. To avoid such deadlock, timeout needs to be > + * implemented. After timeout occurs, device will first stop > + * transfer and restart the transfer again. > + */ > + timer_setup(&dep->stream_timeout_timer, > + stream_timeout_function, 0); > } > > if (!usb_endpoint_xfer_control(desc)) > @@ -730,6 +742,9 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > > trace_dwc3_gadget_ep_disable(dep); > > + if (dep->stream_capable) > + del_timer(&dep->stream_timeout_timer); > + > dwc3_remove_requests(dwc, dep); > > /* make sure HW endpoint isn't stalled */ > @@ -1257,6 +1272,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > return ret; > } > > + if (starting && dep->stream_capable) { > + dep->stream_timeout_timer.expires = jiffies + > + msecs_to_jiffies(STREAM_TIMEOUT_MS); > + add_timer(&dep->stream_timeout_timer); > + } > + > return 0; > } > > @@ -2403,6 +2424,13 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, > stop = true; > } > > + /* > + * Delete the timer that was started in __dwc3_gadget_kick_transfer() > + * for stream capable endpoints. > + */ > + if (dep->stream_capable) > + del_timer(&dep->stream_timeout_timer); > + > dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); > > if (stop) { > @@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > } > break; > case DWC3_DEPEVT_STREAMEVT: > + if (event->status == DEPEVT_STREAMEVT_FOUND) > + del_timer(&dep->stream_timeout_timer); > + else > + dev_dbg(dwc->dev, "unable to find suitable stream"); This debug message is already printed in the driver tracepoint. There's no need to print it here. Also, I don't think Felipe will accept adding new dev_dbg() after removing all of them and converting them to tracepoints. Thinh
Hi Thinh, >> >> @@ -2403,6 +2424,13 @@ static void >dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >> stop = true; >> } >> >> + /* >> + * Delete the timer that was started in __dwc3_gadget_kick_transfer() >> + * for stream capable endpoints. >> + */ >> + if (dep->stream_capable) >> + del_timer(&dep->stream_timeout_timer); >> + >> dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); >> >> if (stop) { >> @@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, >> } >> break; >> case DWC3_DEPEVT_STREAMEVT: >> + if (event->status == DEPEVT_STREAMEVT_FOUND) >> + del_timer(&dep->stream_timeout_timer); >> + else >> + dev_dbg(dwc->dev, "unable to find suitable stream"); > >This debug message is already printed in the driver tracepoint. There's no need to >print it here. Also, I don't think Felipe will accept adding new dev_dbg() after removing >all of them and converting them to tracepoints. > I agree with you, thanks for correcting. Will remove the debug prints and resend the patches Thanks, Anurag Kumar Vulisha
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 5bfb625..f62e8c4 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -633,6 +633,11 @@ struct dwc3_event_buffer { #define DWC3_TRB_NUM 256 +/* + * Timeout value in msecs used by stream_timeout_timer when streams are enabled + */ +#define STREAM_TIMEOUT_MS 50 + /** * struct dwc3_ep - device side endpoint representation * @endpoint: usb endpoint @@ -656,6 +661,7 @@ struct dwc3_event_buffer { * @name: a human readable name e.g. ep1out-bulk * @direction: true for TX, false for RX * @stream_capable: true when streams are enabled + * @stream_timeout_timer: timeout timer used by bulk streams */ struct dwc3_ep { struct usb_ep endpoint; @@ -705,6 +711,7 @@ struct dwc3_ep { unsigned direction:1; unsigned stream_capable:1; + struct timer_list stream_timeout_timer; }; enum dwc3_phy { diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 13ea282..306d4c5 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -254,6 +254,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param) } static int __dwc3_gadget_wakeup(struct dwc3 *dwc); +static void stream_timeout_function(struct timer_list *arg); /** * dwc3_send_gadget_ep_cmd - issue an endpoint command @@ -574,6 +575,17 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) | DWC3_DEPCFG_STREAM_EVENT_EN | DWC3_DEPCFG_XFER_COMPLETE_EN; dep->stream_capable = true; + + /* + * When BULK streams are enabled it may be possible for the host + * and device become out of sync, where device may wait for host + * to issue prime transcation and host may wait for device to + * issue ERDY. To avoid such deadlock, timeout needs to be + * implemented. After timeout occurs, device will first stop + * transfer and restart the transfer again. + */ + timer_setup(&dep->stream_timeout_timer, + stream_timeout_function, 0); } if (!usb_endpoint_xfer_control(desc)) @@ -730,6 +742,9 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) trace_dwc3_gadget_ep_disable(dep); + if (dep->stream_capable) + del_timer(&dep->stream_timeout_timer); + dwc3_remove_requests(dwc, dep); /* make sure HW endpoint isn't stalled */ @@ -1257,6 +1272,12 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) return ret; } + if (starting && dep->stream_capable) { + dep->stream_timeout_timer.expires = jiffies + + msecs_to_jiffies(STREAM_TIMEOUT_MS); + add_timer(&dep->stream_timeout_timer); + } + return 0; } @@ -2403,6 +2424,13 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, stop = true; } + /* + * Delete the timer that was started in __dwc3_gadget_kick_transfer() + * for stream capable endpoints. + */ + if (dep->stream_capable) + del_timer(&dep->stream_timeout_timer); + dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); if (stop) { @@ -2487,6 +2515,11 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, } break; case DWC3_DEPEVT_STREAMEVT: + if (event->status == DEPEVT_STREAMEVT_FOUND) + del_timer(&dep->stream_timeout_timer); + else + dev_dbg(dwc->dev, "unable to find suitable stream"); + break; case DWC3_DEPEVT_RXTXFIFOEVT: break; } @@ -2588,6 +2621,18 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force) } } +static void stream_timeout_function(struct timer_list *arg) +{ + struct dwc3_ep *dep = from_timer(dep, arg, stream_timeout_timer); + struct dwc3 *dwc = dep->dwc; + unsigned long flags; + + spin_lock_irqsave(&dwc->lock, flags); + dwc3_stop_active_transfer(dep, true); + __dwc3_gadget_kick_transfer(dep); + spin_unlock_irqrestore(&dwc->lock, flags); +} + static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) { u32 epnum;