Re: [ 45/82] USB: EHCI: dont check DMA values in QH overlays

Ben Hutchings
2013-03-19T03:09:12Z

[-- Attachment #1: Type: text/plain, Size: 3656 bytes --]

On Mon, 2013-03-18 at 04:22 +0000, Ben Hutchings wrote:
> 3.2-stable review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Alan Stern <stern@rowland.harvard.edu>
>
> commit feca7746d5d9e84b105a613b7f3b6ad00d327372 upstream.
>
> This patch (as1661) fixes a rather obscure bug in ehci-hcd. In a
> couple of places, the driver compares the DMA address stored in a QH's
> overlay region with the address of a particular qTD, in order to see
> whether that qTD is the one currently being processed by the hardware.
> (If it is then the status in the QH's overlay region is more
> up-to-date than the status in the qTD, and if it isn't then the
> overlay's value needs to be adjusted when the QH is added back to the
> active schedule.)
>
> However, DMA address in the overlay region isn't always valid. It
> sometimes will contain a stale value, which may happen by coincidence
> to be equal to a qTD's DMA address. Instead of checking the DMA
> address, we should check whether the overlay region is active and
> valid. The patch tests the ACTIVE bit in the overlay, and clears this
> bit when the overlay becomes invalid (which happens when the
> currently-executing URB is unlinked).
>
> This is the second part of a fix for the regression reported at:
>
> https://bugs.launchpad.net/bugs/1088733

Alan, the first part (commit 6402c796d3b4 aka as1660) didn't apply and I
couldn't see how to adapt it for 3.2. Does this second part have any
value without the first? Or, if you could provide a backport of the
first part, that would be very much appreciated.

Ben.

> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Joseph Salisbury <joseph.salisbury@canonical.com>
> Reported-and-tested-by: Stephen Thirlwall <sdt@dr.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> drivers/usb/host/ehci-q.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> --- a/drivers/usb/host/ehci-q.c
> +++ b/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->hw->hw_token & ACTIVE_BIT(ehci)) {
> qh->hw->hw_qtd_next = qtd->hw_next;
> qtd = NULL;
> }
> @@ -444,11 +444,19 @@ qh_completions (struct ehci_hcd *ehci, s
> else if (last_status == -EINPROGRESS && !urb->unlinked)
> 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 this was the active qtd when the qh was unlinked
> + * and the overlay's token is active, then the overlay
> + * hasn't been written back to the qtd yet so use its
> + * token instead of the qtd's. After the qtd is
> + * processed and removed, the overlay won't be valid
> + * any more.
> + */
> + if (state == QH_STATE_IDLE &&
> + qh->qtd_list.next == &qtd->qtd_list &&
> + (hw->hw_token & ACTIVE_BIT(ehci))) {
> token = hc32_to_cpu(ehci, hw->hw_token);
> + hw->hw_token &= ~ACTIVE_BIT(ehci);
>
> /* An unlink may leave an incomplete
> * async transaction in the TT buffer.
>

--
Ben Hutchings
When you say `I wrote a program that crashed Windows', people just stare ...
and say `Hey, I got those with the system, *for free*'. - Linus Torvalds

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]