[SeaBIOS] [PATCH 0/3] Additional USB fixes
Kevin O'Connor
kevin at koconnor.net
Sat Nov 19 20:44:06 CET 2011
On Thu, Nov 17, 2011 at 10:22:58PM -0500, Kevin O'Connor wrote:
> This series ports Paolo's uhci fixes to the ehci code. Unfortunately,
> I'm still seeing an occasional hang with my flash drives on my e350m1
> (ehci controller), so there's still some other bug lurking in the ehci
> code.
I think I found the issue on EHCI. The EHCI controller actually has
the opposite requirement from UHCI - the TD is updated before the QH.
So, I've created a new patch which always checks the TDs to verify
transfer completion. (Control transfers used to just check the QH,
and it actually only checked that the last TD was in progress.)
This has been very stable on my e350m1 with several different USB
drives. I have seen a few timeouts, but they're so sporadic now that
I haven't been able to track them down. (The e350m1 also has a known
usb init bug which could be causing the remaining failures.)
So, please ignore the previous patch 2/3 and use the patch below.
(I've committed the unrelated patch 1 of the previous series.)
-Kevin
-------------- next part --------------
>From 49ecf6933af0b803defcfe10f5786e5f1eee7f46 Mon Sep 17 00:00:00 2001
From: Kevin O'Connor <kevin at koconnor.net>
Date: Sat, 19 Nov 2011 14:21:51 -0500
Subject: [PATCH] usb-ehci: Fix races with controller on updates to QH.
To: seabios at seabios.org
The EHCI controller writes to the TD after writing to the QH, so the
driver must wait for all the TDs to be complete before considering the
transfer completed. (The previous implementation was particularly bad
as it only checked that the last TD was in progress before considering
the transfer complete.)
Also, avoid writing to the qh.token field when starting a transfer to
eliminate a potential race. Place the qh.token in an available state
by default - that way only the qtd_next field needs to be updated to
start the transfer.
Signed-off-by: Kevin O'Connor <kevin at koconnor.net>
---
src/usb-ehci.c | 130 +++++++++++++++++++++++++-------------------------------
1 files changed, 58 insertions(+), 72 deletions(-)
diff --git a/src/usb-ehci.c b/src/usb-ehci.c
index a60c607..9bdd638 100644
--- a/src/usb-ehci.c
+++ b/src/usb-ehci.c
@@ -303,22 +303,12 @@ ehci_init(struct pci_device *pci, int busid, struct pci_device *comppci)
* End point communication
****************************************************************/
-static int
-ehci_wait_qh(struct usb_ehci_s *cntl, struct ehci_qh *qh)
-{
- // XXX - 500ms just a guess
- u64 end = calc_future_tsc(500);
- for (;;) {
- if (qh->qtd_next & EHCI_PTR_TERM)
- // XXX - confirm
- return 0;
- if (check_tsc(end)) {
- warn_timeout();
- return -1;
- }
- yield();
- }
-}
+struct ehci_pipe {
+ struct ehci_qh qh;
+ struct ehci_qtd *next_td, *tds;
+ void *data;
+ struct usb_pipe pipe;
+};
// Wait for next USB async frame to start - for ensuring safe memory release.
static void
@@ -362,12 +352,46 @@ ehci_waittick(struct usb_ehci_s *cntl)
writel(&cntl->regs->usbsts, STS_IAA);
}
-struct ehci_pipe {
- struct ehci_qh qh;
- struct ehci_qtd *next_td, *tds;
- void *data;
- struct usb_pipe pipe;
-};
+static void
+ehci_reset_pipe(struct ehci_pipe *pipe)
+{
+ SET_FLATPTR(pipe->qh.qtd_next, EHCI_PTR_TERM);
+ SET_FLATPTR(pipe->qh.alt_next, EHCI_PTR_TERM);
+ barrier();
+ SET_FLATPTR(pipe->qh.token, GET_FLATPTR(pipe->qh.token) & QTD_TOGGLE);
+}
+
+static int
+ehci_wait_td(struct ehci_pipe *pipe, struct ehci_qtd *td, int timeout)
+{
+ u64 end = calc_future_tsc(timeout);
+ u32 status;
+ for (;;) {
+ status = td->token;
+ if (!(status & QTD_STS_ACTIVE))
+ break;
+ if (check_tsc(end)) {
+ u32 cur = GET_FLATPTR(pipe->qh.current);
+ u32 tok = GET_FLATPTR(pipe->qh.token);
+ u32 next = GET_FLATPTR(pipe->qh.qtd_next);
+ warn_timeout();
+ dprintf(1, "ehci pipe=%p cur=%08x tok=%08x next=%x td=%p status=%x\n"
+ , pipe, cur, tok, next, td, status);
+ ehci_reset_pipe(pipe);
+ struct usb_ehci_s *cntl = container_of(
+ GET_FLATPTR(pipe->pipe.cntl), struct usb_ehci_s, usb);
+ ehci_waittick(cntl);
+ return -1;
+ }
+ yield();
+ }
+ if (status & QTD_STS_HALT) {
+ dprintf(1, "ehci_wait_td error - status=%x\n", status);
+ ehci_reset_pipe(pipe);
+ return -2;
+ }
+ return 0;
+}
void
ehci_free_pipe(struct usb_pipe *p)
@@ -416,7 +440,6 @@ ehci_alloc_control_pipe(struct usb_pipe *dummy)
memset(pipe, 0, sizeof(*pipe));
memcpy(&pipe->pipe, dummy, sizeof(pipe->pipe));
pipe->qh.qtd_next = pipe->qh.alt_next = EHCI_PTR_TERM;
- pipe->qh.token = QTD_STS_HALT;
// Add queue head to controller list.
struct ehci_qh *async_qh = cntl->async_qh;
@@ -455,15 +478,14 @@ ehci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize
ASSERT32FLAT();
if (! CONFIG_USB_EHCI)
return -1;
- dprintf(5, "ehci_control %p\n", p);
+ dprintf(5, "ehci_control %p (dir=%d cmd=%d data=%d)\n"
+ , p, dir, cmdsize, datasize);
if (datasize > 4*4096 || cmdsize > 4*4096) {
// XXX - should support larger sizes.
warn_noalloc();
return -1;
}
struct ehci_pipe *pipe = container_of(p, struct ehci_pipe, pipe);
- struct usb_ehci_s *cntl = container_of(
- pipe->pipe.cntl, struct usb_ehci_s, usb);
u16 maxpacket = pipe->pipe.maxpacket;
int speed = pipe->pipe.speed;
@@ -513,14 +535,12 @@ ehci_control(struct usb_pipe *p, int dir, const void *cmd, int cmdsize
// Transfer data
barrier();
pipe->qh.qtd_next = (u32)tds;
- barrier();
- pipe->qh.token = 0;
- int ret = ehci_wait_qh(cntl, &pipe->qh);
- pipe->qh.token = QTD_STS_HALT;
- if (ret) {
- pipe->qh.qtd_next = pipe->qh.alt_next = EHCI_PTR_TERM;
- // XXX - halt qh?
- ehci_waittick(cntl);
+ int i, ret=0;
+ for (i=0; i<3; i++) {
+ struct ehci_qtd *td = &tds[i];
+ ret = ehci_wait_td(pipe, td, 500);
+ if (ret)
+ break;
}
free(tds);
return ret;
@@ -545,7 +565,6 @@ ehci_alloc_bulk_pipe(struct usb_pipe *dummy)
memset(pipe, 0, sizeof(*pipe));
memcpy(&pipe->pipe, dummy, sizeof(pipe->pipe));
pipe->qh.qtd_next = pipe->qh.alt_next = EHCI_PTR_TERM;
- pipe->qh.token = QTD_STS_HALT;
// Add queue head to controller list.
struct ehci_qh *async_qh = cntl->async_qh;
@@ -555,28 +574,6 @@ ehci_alloc_bulk_pipe(struct usb_pipe *dummy)
return &pipe->pipe;
}
-static int
-ehci_wait_td(struct ehci_qtd *td)
-{
- u64 end = calc_future_tsc(5000); // XXX - lookup real time.
- u32 status;
- for (;;) {
- status = td->token;
- if (!(status & QTD_STS_ACTIVE))
- break;
- if (check_tsc(end)) {
- warn_timeout();
- return -1;
- }
- yield();
- }
- if (status & QTD_STS_HALT) {
- dprintf(1, "ehci_wait_td error - status=%x\n", status);
- return -2;
- }
- return 0;
-}
-
#define STACKQTDS 4
int
@@ -607,15 +604,13 @@ ehci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize)
| (GET_FLATPTR(pipe->pipe.tt_devaddr) << QH_HUBADDR_SHIFT)));
barrier();
SET_FLATPTR(pipe->qh.qtd_next, (u32)MAKE_FLATPTR(GET_SEG(SS), tds));
- barrier();
- SET_FLATPTR(pipe->qh.token, GET_FLATPTR(pipe->qh.token) & QTD_TOGGLE);
int tdpos = 0;
while (datasize) {
struct ehci_qtd *td = &tds[tdpos++ % STACKQTDS];
- int ret = ehci_wait_td(td);
+ int ret = ehci_wait_td(pipe, td, 5000);
if (ret)
- goto fail;
+ return -1;
struct ehci_qtd *nexttd_fl = MAKE_FLATPTR(GET_SEG(SS)
, &tds[tdpos % STACKQTDS]);
@@ -633,21 +628,12 @@ ehci_send_bulk(struct usb_pipe *p, int dir, void *data, int datasize)
int i;
for (i=0; i<STACKQTDS; i++) {
struct ehci_qtd *td = &tds[tdpos++ % STACKQTDS];
- int ret = ehci_wait_td(td);
+ int ret = ehci_wait_td(pipe, td, 5000);
if (ret)
- goto fail;
+ return -1;
}
return 0;
-fail:
- dprintf(1, "ehci_send_bulk failed\n");
- SET_FLATPTR(pipe->qh.qtd_next, EHCI_PTR_TERM);
- SET_FLATPTR(pipe->qh.alt_next, EHCI_PTR_TERM);
- // XXX - halt qh?
- struct usb_ehci_s *cntl = container_of(
- GET_FLATPTR(pipe->pipe.cntl), struct usb_ehci_s, usb);
- ehci_waittick(cntl);
- return -1;
}
struct usb_pipe *
--
1.7.6.4
More information about the SeaBIOS
mailing list