Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S939067AbdD0Pfa (ORCPT ); Thu, 27 Apr 2017 11:35:30 -0400 Received: from lelnx193.ext.ti.com ([198.47.27.77]:49137 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S968262AbdD0PfV (ORCPT ); Thu, 27 Apr 2017 11:35:21 -0400 Date: Thu, 27 Apr 2017 10:35:06 -0500 From: Bin Liu To: "Matwey V. Kornilov" CC: , , Subject: Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback Message-ID: <20170427153506.GA25403@uda0271908> Mail-Followup-To: Bin Liu , "Matwey V. Kornilov" , gregkh@linuxfoundation.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org References: <20170427102033.5500-1-matwey@sai.msu.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline In-Reply-To: <20170427102033.5500-1-matwey@sai.msu.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6063 Lines: 152 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 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 >