Re: [PATCH] USB: DWC3: Fix missed isoc IN transaction

[Felipe Balbi <balbi@xxxxxx>]

Hi,

On Tue, May 15, 2012 at 07:49:52PM +0530, Pratyush Anand wrote:
> If an IN transfer is missed on isoc endpoint, then driver must insure
> that next ep_queue is properly handled.
> This patch fixes this issue by starting a new transfer for next queued
> request.
> Only limitation is that, gadget will have to submit a request within 4
> uf time, which dwc3 driver considers safe enough for staring a new
> transfer.
> 
> Signed-off-by: Pratyush Anand <pratyush.anand@xxxxxx>
> ---
>  drivers/usb/dwc3/core.h   |    3 +++
>  drivers/usb/dwc3/gadget.c |   36 +++++++++++++++++++++++++++++++-----
>  2 files changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2a920f8..c3f61f6 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -352,6 +352,7 @@ struct dwc3_event_buffer {
>   * @number: endpoint number (1 - 15)
>   * @type: set to bmAttributes & USB_ENDPOINT_XFERTYPE_MASK
>   * @res_trans_idx: Resource transfer index
> + * @event: pointer to event struct received during missed isoc xferinprogress
>   * @interval: the intervall on which the ISOC transfer is started
>   * @name: a human readable name e.g. ep1out-bulk
>   * @direction: true for TX, false for RX
> @@ -376,6 +377,7 @@ struct dwc3_ep {
>  #define DWC3_EP_WEDGE		(1 << 2)
>  #define DWC3_EP_BUSY		(1 << 4)
>  #define DWC3_EP_PENDING_REQUEST	(1 << 5)
> +#define DWC3_EP_MISSED_ISOC	(1 << 6)
>  
>  	/* This last one is specific to EP0 */
>  #define DWC3_EP0_DIR_IN		(1 << 31)
> @@ -385,6 +387,7 @@ struct dwc3_ep {
>  	u8			number;
>  	u8			type;
>  	u8			res_trans_idx;
> +	const struct dwc3_event_depevt *event;

please don't do this... (see more below)

>  	u32			interval;
>  
>  	char			name[20];
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 9affe67..5a9f1f19 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -995,6 +995,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, u16 cmd_param,
>  	return 0;
>  }
>  
> +static void dwc3_gadget_start_isoc(struct dwc3 *dwc,
> +		struct dwc3_ep *dep, const struct dwc3_event_depevt *event);

you can make two versions of this function by re-factoring. One version
depends on event and another depends on direct arguments. Similar to
what we did for ep0 ;-)

All you need from the event to start the isochronous transfer is the
uframe number, and we have dwc3_gadget_get_frame() (which should be
re-factored too not to depend on struct usb_gadget *g, something like
below:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b9cbde5..dbe9b2b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1288,15 +1288,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
 
 /* -------------------------------------------------------------------------- */
 
-static int dwc3_gadget_get_frame(struct usb_gadget *g)
+static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
 {
-	struct dwc3		*dwc = gadget_to_dwc(g);
 	u32			reg;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
+
 	return DWC3_DSTS_SOFFN(reg);
 }
 
+static int dwc3_gadget_get_frame(struct usb_gadget *g)
+{
+	struct dwc3		*dwc = gadget_to_dwc(g);
+
+	return __dwc3_gadget_get_frame(dwc);
+}
+
 static int dwc3_gadget_wakeup(struct usb_gadget *g)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
)

Then you can properly start the isochronous transfer whenever you miss
one. No requirement of doing so on the next 4 uframes.

> @@ -1024,8 +1027,19 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>  
>  	list_add_tail(&req->list, &dep->request_list);
>  
> -	if (usb_endpoint_xfer_isoc(dep->desc) && (dep->flags & DWC3_EP_BUSY))
> -		dep->flags |= DWC3_EP_PENDING_REQUEST;
> +	if (usb_endpoint_xfer_isoc(dep->desc)) {
> +		if (dep->flags & DWC3_EP_BUSY) {
> +			dep->flags |= DWC3_EP_PENDING_REQUEST;
> +		} else if (dep->flags & DWC3_EP_MISSED_ISOC) {
> +			/*
> +			 * FIXME: Only case which I still see failing is
> +			 * if gadget delays submission for more than 4
> +			 * uf time.
> +			 */
> +			dwc3_gadget_start_isoc(dwc, dep, dep->event);
> +			dep->flags &= ~DWC3_EP_MISSED_ISOC;
> +		}
> +	}
>  
>  	/*
>  	 * There are two special cases:
> @@ -1590,6 +1604,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>  	struct dwc3_trb		*trb;
>  	unsigned int		count;
>  	unsigned int		s_pkt = 0;
> +	unsigned int		m_isoc = 0;
>  
>  	do {
>  		req = next_request(&dep->req_queued);
> @@ -1615,9 +1630,18 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>  
>  		if (dep->direction) {
>  			if (count) {
> -				dev_err(dwc->dev, "incomplete IN transfer %s\n",
> -						dep->name);
> -				status = -ECONNRESET;
> +				if (usb_endpoint_xfer_isoc(dep->desc)) {

doesn't look quite right. The TRB itself has a MISSED_ISOC status. Can't
you use that instead ?

> +					dev_dbg(dwc->dev, "incomplete IN \
> +							transfer %s\n",
> +							dep->name);
> +					dep->flags |= DWC3_EP_MISSED_ISOC;
> +					dep->event = event;
> +				} else {
> +					dev_err(dwc->dev, "incomplete IN \
> +							transfer %s\n",
> +							dep->name);
> +					status = -ECONNRESET;
> +				}
>  			}
>  		} else {
>  			if (count && (event->status & DEPEVT_STATUS_SHORT))
> @@ -1635,6 +1659,8 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>  		dwc3_gadget_giveback(dep, req, status);
>  		if (s_pkt)
>  			break;
> +		if (m_isoc)

this is never assigned to anything other than zero. What gives ??

-- 
balbi

Attachment: signature.asc
Description: Digital signature