Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20170427102033.5500-1-matwey@sai.msu.ru>
References: <20170427102033.5500-1-matwey@sai.msu.ru>
From:   "Matwey V. Kornilov" <matwey@sai.msu.ru>
Date:   Sat, 4 Nov 2017 17:05:14 +0300
Message-ID: <CAJs94Ebf0dZ2hxp=vqCrgDtUd0OC2DEY+xeRWLWkE0ojLHmkPA@mail.gmail.com>
Subject: Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
To:     Bin Liu <b-liu@ti.com>, Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        "Matwey V. Kornilov" <matwey@sai.msu.ru>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bin,

I've just checked that the issue is still present in 4.13.10.

2017-04-27 13:20 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
> 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@sai.msu.ru>
> ---
>  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
>


-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

From 1584268667058033944@xxx Thu Nov 16 23:52:18 +0000 2017
X-GM-THRID: 1584263785940925114
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread