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

[Ben Hutchings <ben@decadent.org.uk>]

[-- 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 --]