On Fri, 7 Sep 2012, Pavankumar Kondeti wrote:
> There is a possibility of QH overlay region having reference to a stale
> qTD pointer during unlink.
>
> Consider an endpoint having two pending qTD before unlink process begins.
> The endpoint's QH queue looks like this.
>
> qTD1 --> qTD2 --> Dummy
>
> To unlink qTD2, QH is removed from asynchronous list and Asynchronous
> Advance Doorbell is programmed. The qTD1's next qTD pointer is set to
> qTD2'2 next qTD pointer and qTD2 is retired upon controller's doorbell
> interrupt. If QH's current qTD pointer points to qTD1, transfer overlay
> region still have reference to qTD2. But qtD2 is just unlinked and freed.
> This may cause EHCI system error. Fix this by updating qTD next pointer
> in QH overlay region with the qTD next pointer of the current qTD.
Pavan:
Your patch got me thinking. The test we use is this:
if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
But it's possible (although highly unlikely) that this test could
succeed when it shouldn't. That is, it might happen purely by
coincidence that the qTD at the start of the list had been reused, so
it's really a new TD at the same old address, and the old address is
still in the overlay region.
(To make this happen, you'd have to unlink an URB while it is running,
and the completion handler would have to submit a new URB and
immediately unlink it. Then the next URB submitted might reuse the qTD
that was running originally.)
It would be better to keep track of whether or not the contents of the
overlay area in an idle QH are active and valid, since that's what we
really want to test here. Can you read through this patch and verify
that it will do what it's supposed to do?
Thanks,
Alan Stern
Index: usb-3.6/drivers/usb/host/ehci.h
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci.h
+++ usb-3.6/drivers/usb/host/ehci.h
@@ -401,6 +401,7 @@ struct ehci_qh {
struct usb_device *dev; /* access to TT */
unsigned is_out:1; /* bulk or intr OUT */
unsigned clearing_tt:1; /* Clear-TT-Buf in progress */
+ unsigned overlay_active:1; /* no update needed */
};
/*-------------------------------------------------------------------------*/
Index: usb-3.6/drivers/usb/host/ehci-q.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-q.c
+++ usb-3.6/drivers/usb/host/ehci-q.c
@@ -135,7 +135,7 @@ qh_refresh (struct ehci_hcd *ehci, struc
* qtd is updated in qh_completions(). Update the QH
* overlay here.
*/
- if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
+ if (qh->overlay_active) {
qh->hw->hw_qtd_next = qtd->hw_next;
qtd = NULL;
}
@@ -456,9 +456,7 @@ qh_completions (struct ehci_hcd *ehci, s
continue;
/* qh unlinked; token in overlay may be most current */
- if (state == QH_STATE_IDLE
- && cpu_to_hc32(ehci, qtd->qtd_dma)
- == hw->hw_current) {
+ if (state == QH_STATE_IDLE && qh->overlay_active) {
token = hc32_to_cpu(ehci, hw->hw_token);
/* An unlink may leave an incomplete
@@ -513,6 +511,15 @@ qh_completions (struct ehci_hcd *ehci, s
last->hw_next = qtd->hw_next;
}
+ /*
+ * Conversely, if we're removing something at the head
+ * of the queue then the QH's overlay area can no longer
+ * be active and valid.
+ */
+ else {
+ qh->overlay_active = 0;
+ }
+
/* remove qtd; it's recycled after possible urb completion */
list_del (&qtd->qtd_list);
last = qtd;
@@ -1243,6 +1250,8 @@ static void end_unlink_async(struct ehci
qh->unlink_next = NULL;
qh->qh_state = QH_STATE_IDLE;
+ qh->overlay_active = !!(qh->hw->hw_token &
+ cpu_to_hc32(ehci, QTD_STS_ACTIVE));
qh->qh_next.qh = NULL;
qh_completions(ehci, qh);
Index: usb-3.6/drivers/usb/host/ehci-sched.c
===================================================================
--- usb-3.6.orig/drivers/usb/host/ehci-sched.c
+++ usb-3.6/drivers/usb/host/ehci-sched.c
@@ -670,6 +670,8 @@ static void end_unlink_intr(struct ehci_
int rc;
qh->qh_state = QH_STATE_IDLE;
+ qh->overlay_active = !!(hw->hw_token &
+ cpu_to_hc32(ehci, QTD_STS_ACTIVE));
hw->hw_next = EHCI_LIST_END(ehci);
qh_completions(ehci, qh);
--
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
