| 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: <20200421133019.27167-7-matwey.kornilov@gmail.com>
Date: Tue, 21 Apr 2020 16:30:19 +0300
From: matwey.kornilov@...il.com
To: stern@...land.harvard.edu, Bin Liu <b-liu@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-usb@...r.kernel.org (open list:MUSB MULTIPOINT HIGH SPEED
DUAL-ROLE CONTROLLER), linux-kernel@...r.kernel.org (open list)
Cc: "Matwey V. Kornilov" <matwey@....msu.ru>,
linux-usb@...r.kernel.org (open list:MUSB MULTIPOINT HIGH SPEED
DUAL-ROLE CONTROLLER), linux-kernel@...r.kernel.org (open list)
Subject: [PATCH v3 6/6] usb: musb: Decrease URB starting latency in musb_advance_schedule()
From: "Matwey V. Kornilov" <matwey@....msu.ru>
Previously, the algorithm was the following:
1. giveback current URB
2. if current qh is not empty
then start next URB
3. if current qh is empty
then dispose the qh, find next qh if any, and start URB.
It may take a while to run urb->callback inside URB giveback which is
run synchronously in musb. In order to improve the latency we rearrange
the function behaviour for the case when qh is not empty: next URB is
started before URB giveback. When qh is empty or current URB has an
error then the behaviour is intentionally kept in order
a) not to break existing inter qh scheduling: URB giveback could
potentionally enqueue other URB to the empty qh preventing it from being
disposed;
b) allow the class driver to cancel outstanding URBs in the queue.
Correct URB giveback order is guaranteed as the following. For each
qh there can be at most three ready URBs processing by the driver.
Indeed, every ready URB can send at most one URB in
musb_advance_schedule(), and in the worst case scenario we have the
following ready URBs:
1) URB in the giveback lock protected section inside musb_giveback()
2) URB waiting at the giveback lock acqusition in musb_giveback()
3) URB waiting at the controller lock acqusition in the glue layer
interrput handler
Here URB #2 and URB #3 are triggered by URB #1 and URB #2
correspondingly when they passed through musb_advance_schedule().
Since URB #3 is waiting before musb_advance_schedule(), no other new
URBs will be sent until URB#1 is finished, URB#2 goes to the giveback
lock protected section, and URB#3 goes to the controller lock protected
musb_advance_schedule().
Before this patch, time spent in urb->callback led to the following
glitches between the host and a hub during isoc transfer (line 4):
11.624492 d= 0.000124 [130.6 + 1.050] [ 4] SPLIT
11.624492 d= 0.000000 [130.6 + 1.467] [ 3] IN : 3.5
11.624493 d= 0.000000 [130.6 + 1.967] [ 37] DATA0: aa 08 [skipped...]
11.625617 d= 0.001124 [131.7 + 1.050] [ 4] SPLIT
11.625617 d= 0.000000 [131.7 + 1.467] [ 3] IN : 3.5
11.625867 d= 0.000250 [132.1 + 1.050] [ 4] SPLIT
11.625867 d= 0.000000 [132.1 + 1.467] [ 3] IN : 3.5
11.625868 d= 0.000001 [132.1 + 1.983] [ 3] DATA0: 00 00
11.626617 d= 0.000749 [132.7 + 1.050] [ 4] SPLIT
11.626617 d= 0.000000 [132.7 + 1.467] [ 3] IN : 3.5
11.626867 d= 0.000250 [133.1 + 1.050] [ 4] SPLIT
11.626867 d= 0.000000 [133.1 + 1.467] [ 3] IN : 3.5
11.626868 d= 0.000000 [133.1 + 1.967] [ 3] DATA0: 00 00
After the hub, they look as the following and may lead to broken
perepherial transfer (as in case of PWC based webcam):
11.332004 d= 0.000997 [ 30.0 + 3.417] [ 3] IN : 5.5
11.332007 d= 0.000003 [ 30.0 + 6.833] [800] DATA0: 8a 1c [skipped...]
11.334004 d= 0.001997 [ 32.0 + 3.417] [ 3] IN : 5.5
11.334007 d= 0.000003 [ 32.0 + 6.750] [ 3] DATA0: 00 00
11.335004 d= 0.000997 [ 33 + 3.417] [ 3] IN : 5.5
11.335007 d= 0.000003 [ 33 + 6.750] [ 3] DATA0: 00 00
Removing this glitches makes us able to successfully run 10fps
video stream from the webcam attached via USB hub. That was
previously impossible.
Signed-off-by: Matwey V. Kornilov <matwey@....msu.ru>
---
drivers/usb/musb/musb_host.c | 36 ++++++++++++++++++++++++++++++++----
drivers/usb/musb/musb_host.h | 1 +
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index 46d9dd7d6f67..863f55907ddc 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -85,6 +85,11 @@ static bool musb_qh_empty(struct musb_qh *qh)
return list_empty(&qh->hep->urb_list);
}
+static bool musb_qh_singular(struct musb_qh *qh)
+{
+ return list_is_singular(&qh->hep->urb_list);
+}
+
static void musb_qh_unlink_hep(struct musb_qh *qh)
{
if (!qh->hep)
@@ -301,15 +306,24 @@ musb_start_next_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 musb_qh *qh, struct urb *urb, int status)
__releases(musb->lock)
__acquires(musb->lock)
{
trace_musb_urb_gb(musb, urb);
+ /*
+ * This line is protected by the controller lock: at most
+ * one thread waiting on the giveback lock.
+ */
+ spin_lock(&qh->giveback_lock);
usb_hcd_unlink_urb_from_ep(musb->hcd, urb);
+
spin_unlock(&musb->lock);
+
usb_hcd_giveback_urb(musb->hcd, urb, status);
+ spin_unlock(&qh->giveback_lock);
+
spin_lock(&musb->lock);
}
@@ -344,8 +358,21 @@ static void musb_advance_schedule(struct musb *musb, struct urb *urb,
break;
}
+ if (ready && !musb_qh_singular(qh) && !status) {
+ struct urb *next_urb = list_next_entry(urb, urb_list);
+
+ musb_dbg(musb, "... next ep%d %cX urb %p", hw_ep->epnum, is_in ? 'R' : 'T', next_urb);
+ musb_start_urb(musb, is_in, qh, next_urb);
+
+ qh->is_ready = 0;
+ musb_giveback(musb, qh, urb, status);
+ qh->is_ready = ready;
+
+ return;
+ }
+
qh->is_ready = 0;
- musb_giveback(musb, urb, status);
+ musb_giveback(musb, qh, urb, status);
qh->is_ready = ready;
/* reclaim resources (and bandwidth) ASAP; deschedule it, and
@@ -2174,6 +2201,7 @@ static int musb_urb_enqueue(
qh->hep = hep;
qh->dev = urb->dev;
INIT_LIST_HEAD(&qh->ring);
+ spin_lock_init(&qh->giveback_lock);
qh->is_ready = 1;
qh->maxpacket = usb_endpoint_maxp(epd);
@@ -2406,7 +2434,7 @@ static int musb_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
int ready = qh->is_ready;
qh->is_ready = 0;
- musb_giveback(musb, urb, 0);
+ musb_giveback(musb, qh, urb, 0);
qh->is_ready = ready;
/* If nothing else (usually musb_giveback) is using it
@@ -2465,7 +2493,7 @@ musb_h_disable(struct usb_hcd *hcd, struct usb_host_endpoint *hep)
* will activate any of these as it advances.
*/
while (!musb_qh_empty(qh))
- musb_giveback(musb, next_urb(qh), -ESHUTDOWN);
+ musb_giveback(musb, qh, next_urb(qh), -ESHUTDOWN);
musb_qh_free(qh);
}
diff --git a/drivers/usb/musb/musb_host.h b/drivers/usb/musb/musb_host.h
index 2999845632ce..6223b0177c68 100644
--- a/drivers/usb/musb/musb_host.h
+++ b/drivers/usb/musb/musb_host.h
@@ -19,6 +19,7 @@ struct musb_qh {
struct musb_hw_ep *hw_ep; /* current binding */
struct list_head ring; /* of musb_qh */
+ spinlock_t giveback_lock; /* to keep URB giveback order */
/* struct musb_qh *next; */ /* for periodic tree */
u8 mux; /* qh multiplexed to hw_ep */
--
2.25.0
Powered by blists - more mailing lists