| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJs94EaxQMnCCFjndHuqAaMt95nwLu=WMwwjMsyXmmVMf0iw+g@mail.gmail.com>
Date: Thu, 27 Apr 2017 19:26:31 +0300
From: "Matwey V. Kornilov" <matwey@....msu.ru>
To: Bin Liu <b-liu@...com>, "Matwey V. Kornilov" <matwey@....msu.ru>,
gregkh@...uxfoundation.org, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: musb: musb_host: Introduce postponed URB giveback
2017-04-27 18:35 GMT+03:00 Bin Liu <b-liu@...com>:
> 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@....msu.ru>
>
> 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...
Have you tried other SoCs with Invetra MUSB?
>
> Ideally MUSB driver should be just using HCD_BH flag and let the core to
> handle the urb callback, regardless the usb transfer types.
I think the only reason why everything worked before with HCD_BH is
that execution of urb->callback() was placed after musb_start(). The
order of operations matters.
However, you said that something was also wrong with HCD_BH and other
peripherals.
>
> 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.
>
I feel from playing with OpenVizsla that REQPKT should be set well in
advance. So, time window to set the flag is actually smaller than 1
ms.
urb->callback() is never takes longer than 0.4 ms for pwc driver, but
INs are skipped.
At the same time musb_host doesn't utilize AutoReq here. I think many
other USB host controllers (OHCI?) just rely on hardware to send IN
packets in time.
> 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
>>
>
--
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119234, Moscow, Universitetsky pr-k 13, +7 (495) 9392382
Powered by blists - more mailing lists