Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback

[Bin Liu]

Hi Matwey,

On Thu, Apr 27, 2017 at 01:20:33PM +0300, Matwey V. Kornilov wrote:
> This commit changes the order of actions undertaken in
> musb_advance_schedule() in order to overcome issue with broken
> isochronous transfer [1].
> 
> There is no harm to split musb_giveback into two pieces.  The first
> unlinks finished urb, the second givebacks it. The issue here that
> givebacking may be quite time-consuming due to urb->complete() call.
> As it happens in case of pwc-driven web cameras. It may take about 0.5
> ms to call __musb_giveback() that calls urb->callback() internally.
> Under specific circumstances setting MUSB_RXCSR_H_REQPKT in subsequent
> musb_start_urb() for the next urb will be too late to produce physical
> IN packet. Since auto req is not used by this module the exchange
> would be as the following:
> 
> [        ]   7.220456 d=  0.000997 [182   +  3.667] [  3] IN   : 4.5
> [    T   ]   7.220459 d=  0.000003 [182   +  7.000] [800] DATA0: [skipped]
> [        ]   7.222456 d=  0.001997 [184   +  3.667] [  3] IN   : 4.5
> [        ]   7.222459 d=  0.000003 [184   +  7.000] [  3] DATA0: 00 00
> 
> It is known that missed IN in isochronous mode makes some
> perepherial broken. For instance, pwc-driven or uvc-driven
> web cameras.
> In order to workaround this issue we postpone calling
> urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
> next urb if there is the next urb pending in queue.
> 
> [1] https://www.spinics.net/lists/linux-usb/msg145747.html
> 
> Fixes: f551e1352983 ("Revert "usb: musb: musb_host: Enable HCD_BH flag to handle urb return in bottom half"")
> Signed-off-by: Matwey V. Kornilov <matwey@xxxxxxxxxx>

Thanks for the effort of working on this long standing issue, I know you
have spent alot of time on it, but what I am thinking is instead of
workaround the problem we need to understand the root cause - why
uvc-video takes longer to exec the urb callback, why only am335x
reported this issue. This is on my backlog, just seems never got time
for it...

Ideally MUSB driver should be just using HCD_BH flag and let the core to
handle the urb callback, regardless the usb transfer types.

The MUSB drivers are already messy and complicated enough for
maintenance, I'd like to understand the root cause of the delay first
before decide how to solve the issue.

Regards,
-Bin.

> ---
>  drivers/usb/musb/musb_host.c | 54 +++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index ac3a4952abb4..b590c2555dab 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -299,19 +299,24 @@ musb_start_urb(struct musb *musb, int is_in, struct musb_qh *qh)
>  	}
>  }
>  
> -/* Context: caller owns controller lock, IRQs are blocked */
> -static void musb_giveback(struct musb *musb, struct urb *urb, int status)
> +static void __musb_giveback(struct musb *musb, struct urb *urb, int status)
>  __releases(musb->lock)
>  __acquires(musb->lock)
>  {
> -	trace_musb_urb_gb(musb, urb);
> -
> -	usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
>  	spin_unlock(&musb->lock);
>  	usb_hcd_giveback_urb(musb->hcd, urb, status);
>  	spin_lock(&musb->lock);
>  }
>  
> +/* Context: caller owns controller lock, IRQs are blocked */
> +static void musb_giveback(struct musb *musb, struct urb *urb, int status)
> +{
> +	trace_musb_urb_gb(musb, urb);
> +
> +	usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
> +	__musb_giveback(musb, urb, status);
> +}
> +
>  /* For bulk/interrupt endpoints only */
>  static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
>  				    struct urb *urb)
> @@ -346,6 +351,7 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
>  	struct musb_hw_ep	*ep = qh->hw_ep;
>  	int			ready = qh->is_ready;
>  	int			status;
> +	int                     postponed_giveback = 0;
>  
>  	status = (urb->status == -EINPROGRESS) ? 0 : urb->status;
>  
> @@ -361,9 +367,35 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
>  		break;
>  	}
>  
> -	qh->is_ready = 0;
> -	musb_giveback(musb, urb, status);
> -	qh->is_ready = ready;
> +	usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
> +
> +	/* It may take about 0.5 ms to call __musb_giveback() that
> +	 * calls urb->callback() internally. Under specific circumstances
> +	 * setting MUSB_RXCSR_H_REQPKT in subsequent musb_start_urb() for the
> +	 * next urb will be too late to produce physical IN packet. Since
> +	 * auto req is not used by this module the exchange would be as the
> +	 * following:
> +	 *
> +	 * [        ]   7.220456 d=  0.000997 [182   +  3.667] [  3] IN   : 4.5
> +	 * [    T   ]   7.220459 d=  0.000003 [182   +  7.000] [800] DATA0: [skipped]
> +	 * [        ]   7.222456 d=  0.001997 [184   +  3.667] [  3] IN   : 4.5
> +	 * [        ]   7.222459 d=  0.000003 [184   +  7.000] [  3] DATA0: 00 00
> +	 *
> +	 * It is known that missed IN in isochronous mode makes some
> +	 * perepherial broken. For instance, pwc-driven or uvc-driven
> +	 * web cameras.
> +	 * In order to workaround this issue we postpone calling
> +	 * urb->callback() after setting MUSB_RXCSR_H_REQPKT for the
> +	 * next urb if there is the next urb pending in queue.
> +	 */
> +	if (is_in && qh->type == USB_ENDPOINT_XFER_ISOC
> +	    && !list_empty(&qh->hep->urb_list)) {
> +		postponed_giveback = 1;
> +	} else {
> +		qh->is_ready = 0;
> +		__musb_giveback(musb, urb, status);
> +		qh->is_ready = ready;
> +	}
>  
>  	/* reclaim resources (and bandwidth) ASAP; deschedule it, and
>  	 * invalidate qh as soon as list_empty(&hep->urb_list)
> @@ -428,6 +460,12 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
>  		    hw_ep->epnum, is_in ? 'R' : 'T', next_urb(qh));
>  		musb_start_urb(musb, is_in, qh);
>  	}
> +
> +	if (postponed_giveback) {
> +		qh->is_ready = 0;
> +		__musb_giveback(musb, urb, status);
> +		qh->is_ready = ready;
> +	}
>  }
>  
>  static u16 musb_h_flush_rxfifo(struct musb_hw_ep *hw_ep, u16 csr)
> -- 
> 2.12.0
>