| Message ID | 1532519491-19502-5-git-send-email-anurag.kumar.vulisha@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fix broken BULK stream support to dwc3 gadget driver | expand |
On Wed, Jul 25, 2018 at 2:51 PM, Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> 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. > +/* > + * Timeout value in msecs used by stream_timeout_timer when streams are enabled > + */ > +#define STREAM_TIMEOUT 50 Perhaps, STREAM_TIMEOUT_MS 50 Dunno about this driver, but it's a usual practice to help reader with understanding code on the first glance.
Hi Andy, Thanks for your review comments, please find my comments inline >-----Original Message----- >From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] >Sent: Wednesday, July 25, 2018 8:36 PM >To: Anurag Kumar Vulisha <anuragku@xilinx.com> >Cc: Felipe Balbi <balbi@kernel.org>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; v.anuragkumar@gmail.com; USB <linux- >usb@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> >Subject: Re: [PATCH 4/8] usb: dwc3: implement stream transfer timeout > >On Wed, Jul 25, 2018 at 2:51 PM, Anurag Kumar Vulisha ><anurag.kumar.vulisha@xilinx.com> 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. > >> +/* >> + * Timeout value in msecs used by stream_timeout_timer when streams >> +are enabled */ >> +#define STREAM_TIMEOUT 50 > >Perhaps, STREAM_TIMEOUT_MS 50 > >Dunno about this driver, but it's a usual practice to help reader with understanding >code on the first glance. > Since I have mentioned "msecs" in comment above describing the STREAM_TIMEOUT, thought it would suffice. But if you feel I should change it, I will fix it in v2 Thanks, Anurag Kumar Vulisha >-- >With Best Regards, >Andy Shevchenko
On Wed, Jul 25, 2018 at 6:14 PM, Anurag Kumar Vulisha <anuragku@xilinx.com> wrote: >>> +/* >>> + * Timeout value in msecs used by stream_timeout_timer when streams >>> +are enabled */ >>> +#define STREAM_TIMEOUT 50 >> >>Perhaps, STREAM_TIMEOUT_MS 50 >> >>Dunno about this driver, but it's a usual practice to help reader with understanding >>code on the first glance. >> > > Since I have mentioned "msecs" in comment above describing the STREAM_TIMEOUT, > thought it would suffice. But if you feel I should change it, I will fix it in v2 But you didn't put that comment before each occurrence of this constant in the code, right? That's what I'm talking about. If reader looks into the code, there is no need to understand the order of the value for timeout, since units are part of the name. Of course, you may ignore this comment.
>-----Original Message----- >From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com] >Sent: Wednesday, July 25, 2018 8:55 PM >To: Anurag Kumar Vulisha <anuragku@xilinx.com> >Cc: Felipe Balbi <balbi@kernel.org>; Greg Kroah-Hartman ><gregkh@linuxfoundation.org>; v.anuragkumar@gmail.com; USB <linux- >usb@vger.kernel.org>; Linux Kernel Mailing List <linux-kernel@vger.kernel.org> >Subject: Re: [PATCH 4/8] usb: dwc3: implement stream transfer timeout > >On Wed, Jul 25, 2018 at 6:14 PM, Anurag Kumar Vulisha ><anuragku@xilinx.com> wrote: > >>>> +/* >>>> + * Timeout value in msecs used by stream_timeout_timer when streams >>>> +are enabled */ >>>> +#define STREAM_TIMEOUT 50 >>> >>>Perhaps, STREAM_TIMEOUT_MS 50 >>> >>>Dunno about this driver, but it's a usual practice to help reader with understanding >>>code on the first glance. >>> >> >> Since I have mentioned "msecs" in comment above describing the >STREAM_TIMEOUT, >> thought it would suffice. But if you feel I should change it, I will fix it in v2 > >But you didn't put that comment before each occurrence of this >constant in the code, right? >That's what I'm talking about. If reader looks into the code, there is >no need to understand the order of the value for timeout, since units >are part of the name. > >Of course, you may ignore this comment. > I agree with your comments , will wait for other comments on this patch series and address them altogether in v2 Thanks, Anurag Kumar Vulisha >-- >With Best Regards, >Andy Shevchenko
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 285ce0e..581f619 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -619,6 +619,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 50 + /** * struct dwc3_ep - device side endpoint representation * @endpoint: usb endpoint @@ -642,6 +647,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: timer used to aviod deadlock when streams are used */ struct dwc3_ep { struct usb_ep endpoint; @@ -691,6 +697,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 b3e9e7f..e2ccb55 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,8 @@ 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; + timer_setup(&dep->stream_timeout_timer, + stream_timeout_function, 0); } if (!usb_endpoint_xfer_control(desc)) @@ -730,6 +733,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 +1263,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); + add_timer(&dep->stream_timeout_timer); + } + return 0; } @@ -2403,6 +2415,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) { @@ -2486,6 +2505,14 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, } break; case DWC3_DEPEVT_STREAMEVT: + switch (event->status) { + case DEPEVT_STREAMEVT_FOUND: + del_timer(&dep->stream_timeout_timer); + break; + case DEPEVT_STREAMEVT_NOTFOUND: + default: + dev_err(dwc->dev, "unable to find suitable stream"); + } case DWC3_DEPEVT_RXTXFIFOEVT: break; } @@ -2587,6 +2614,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;
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> --- drivers/usb/dwc3/core.h | 7 +++++++ drivers/usb/dwc3/gadget.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)