On Thu, 23 Jul 2009, David Brownell wrote:
> On Thursday 23 July 2009, Alan Stern wrote:
> > BTW, qh_link_async does an apparently unnecessary check that
> > qh->qh_state == QH_STATE_IDLE. Is there any reason for this? It would
> > be a bug if the state weren't IDLE.
>
> /* clear halt and/or toggle; and maybe recover from silicon quirk */
> if (qh->qh_state == QH_STATE_IDLE)
> qh_refresh (ehci, qh);
>
> I don't recall just now. Probably it's left over from
> early paranoia.
Okay, I'll assume that's the case.
Here's the proposed patch. I haven't tested it yet; first I'd like to
know whether you think it is suitable and sane.
Alan Stern
Index: usb-2.6/drivers/usb/host/ehci.h
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci.h
+++ usb-2.6/drivers/usb/host/ehci.h
@@ -357,6 +357,7 @@ struct ehci_qh {
struct usb_device *dev; /* access to TT */
unsigned clearing_tt:1; /* Clear-TT-Buf in progress */
+ unsigned needs_rescan:1; /* Dequeue during giveback */
} __attribute__ ((aligned (32)));
/*-------------------------------------------------------------------------*/
Index: usb-2.6/drivers/usb/host/ehci-hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-hcd.c
+++ usb-2.6/drivers/usb/host/ehci-hcd.c
@@ -850,9 +850,14 @@ static void unlink_async (struct ehci_hc
if (!HC_IS_RUNNING(ehci_to_hcd(ehci)->state) && ehci->reclaim)
end_unlink_async(ehci);
- /* if it's not linked then there's nothing to do */
- if (qh->qh_state != QH_STATE_LINKED)
- ;
+ /* If the QH isn't linked then there's nothing we can do
+ * unless we were called during a giveback, in which case
+ * qh_completions() has to deal with it.
+ */
+ if (qh->qh_state != QH_STATE_LINKED) {
+ if (qh->qh_state == QH_STATE_COMPLETING)
+ qh->needs_rescan = 1;
+ }
/* defer till later if busy */
else if (ehci->reclaim) {
@@ -988,6 +993,7 @@ rescan:
qh->qh_state = QH_STATE_IDLE;
switch (qh->qh_state) {
case QH_STATE_LINKED:
+ case QH_STATE_COMPLETING:
for (tmp = ehci->async->qh_next.qh;
tmp && tmp != qh;
tmp = tmp->qh_next.qh)
@@ -1052,7 +1058,8 @@ ehci_endpoint_reset(struct usb_hcd *hcd,
usb_settoggle(qh->dev, epnum, is_out, 0);
if (!list_empty(&qh->qtd_list)) {
WARN_ONCE(1, "clear_halt for a busy endpoint\n");
- } else if (qh->qh_state == QH_STATE_LINKED) {
+ } else if (qh->qh_state == QH_STATE_LINKED ||
+ qh->qh_state == QH_STATE_COMPLETING) {
/* The toggle value in the QH can't be updated
* while the QH is active. Unlink it now;
Index: usb-2.6/drivers/usb/host/ehci-q.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ehci-q.c
+++ usb-2.6/drivers/usb/host/ehci-q.c
@@ -308,13 +308,13 @@ static int qh_schedule (struct ehci_hcd
static unsigned
qh_completions (struct ehci_hcd *ehci, struct ehci_qh *qh)
{
- struct ehci_qtd *last = NULL, *end = qh->dummy;
+ struct ehci_qtd *last, *end = qh->dummy;
struct list_head *entry, *tmp;
- int last_status = -EINPROGRESS;
+ int last_status;
int stopped;
unsigned count = 0;
u8 state;
- __le32 halt = HALT_BIT(ehci);
+ const __le32 halt = HALT_BIT(ehci);
if (unlikely (list_empty (&qh->qtd_list)))
return count;
@@ -324,11 +324,20 @@ qh_completions (struct ehci_hcd *ehci, s
* they add urbs to this qh's queue or mark them for unlinking.
*
* NOTE: unlinking expects to be done in queue order.
+ *
+ * It's a bug for qh->qh_state to be anything other than
+ * QH_STATE_IDLE, unless our caller is scan_async() or
+ * scan_periodic().
*/
state = qh->qh_state;
qh->qh_state = QH_STATE_COMPLETING;
stopped = (state == QH_STATE_IDLE);
+ rescan:
+ last = NULL;
+ last_status = -EINPROGRESS;
+ qh->needs_rescan = 0;
+
/* remove de-activated QTDs from front of queue.
* after faults (including short reads), cleanup this urb
* then let the queue advance.
@@ -504,6 +513,21 @@ halt:
ehci_qtd_free (ehci, last);
}
+ /* Do we need to rescan for URBs dequeued during a giveback? */
+ if (unlikely(qh->needs_rescan)) {
+ /* If the QH is already unlinked, do the rescan now. */
+ if (state == QH_STATE_IDLE)
+ goto rescan;
+
+ /* Otherwise we have to wait until the QH is fully unlinked.
+ * Our caller will start an unlink if qh->needs_rescan is
+ * set. But if an unlink has already started, nothing needs
+ * to be done.
+ */
+ if (state != QH_STATE_LINKED)
+ qh->needs_rescan = 0;
+ }
+
/* restore original state; caller must unlink or relink */
qh->qh_state = state;
@@ -532,8 +556,10 @@ halt:
& qh->hw_info2) != 0) {
intr_deschedule (ehci, qh);
(void) qh_schedule (ehci, qh);
- } else
- unlink_async (ehci, qh);
+ } else {
+ /* Tell the caller to start an unlink */
+ qh->needs_rescan = 1;
+ }
break;
/* otherwise, unlink already started */
}
@@ -911,6 +937,8 @@ static void qh_link_async (struct ehci_h
if (unlikely(qh->clearing_tt))
return;
+ WARN_ON(qh->qh_state != QH_STATE_IDLE);
+
/* (re)start the async schedule? */
head = ehci->async;
timer_action_done (ehci, TIMER_ASYNC_OFF);
@@ -929,8 +957,7 @@ static void qh_link_async (struct ehci_h
}
/* clear halt and/or toggle; and maybe recover from silicon quirk */
- if (qh->qh_state == QH_STATE_IDLE)
- qh_refresh (ehci, qh);
+ qh_refresh(ehci, qh);
/* splice right after start */
qh->qh_next = head->qh_next;
@@ -1215,6 +1242,8 @@ rescan:
qh = qh_get (qh);
qh->stamp = ehci->stamp;
temp = qh_completions (ehci, qh);
+ if (qh->needs_rescan)
+ unlink_async(ehci, qh);
qh_put (qh);
if (temp != 0) {
goto rescan;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
