[PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink

[PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink

[Pavankumar Kondeti]

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.

Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
---
 drivers/usb/host/ehci-q.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
index 9bc39ca..4b66374 100644
--- a/drivers/usb/host/ehci-q.c
+++ b/drivers/usb/host/ehci-q.c
@@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
 	else {
 		qtd = list_entry (qh->qtd_list.next,
 				struct ehci_qtd, qtd_list);
-		/* first qtd may already be partially processed */
-		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
+		/*
+		 * first qtd may already be partially processed.
+		 * If we come here during unlink, the QH overlay region
+		 * might have reference to the just unlinked qtd. The
+		 * qtd is updated in qh_completions(). Update the QH
+		 * overlay here.
+		 */
+		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
+			qh->hw->hw_qtd_next = qtd->hw_next;
 			qtd = NULL;
+		}
 	}
 
 	if (qtd)
-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation.

Re: [PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink

[Alan Stern]

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.
> 
> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
> ---
>  drivers/usb/host/ehci-q.c |   12 ++++++++++--
>  1 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> index 9bc39ca..4b66374 100644
> --- a/drivers/usb/host/ehci-q.c
> +++ b/drivers/usb/host/ehci-q.c
> @@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
>  	else {
>  		qtd = list_entry (qh->qtd_list.next,
>  				struct ehci_qtd, qtd_list);
> -		/* first qtd may already be partially processed */
> -		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
> +		/*
> +		 * first qtd may already be partially processed.
> +		 * If we come here during unlink, the QH overlay region
> +		 * might have reference to the just unlinked qtd. The
> +		 * qtd is updated in qh_completions(). Update the QH
> +		 * overlay here.
> +		 */
> +		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
> +			qh->hw->hw_qtd_next = qtd->hw_next;
>  			qtd = NULL;
> +		}
>  	}
>  
>  	if (qtd)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Have you been able to determine that this eliminates your host system
errors?

Alan Stern

Re: [PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink

[Pavan Kondeti]

On 9/7/2012 9:22 PM, Alan Stern wrote:
> 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.
>>
>> Signed-off-by: Pavankumar Kondeti <pkondeti@codeaurora.org>
>> ---
>>  drivers/usb/host/ehci-q.c |   12 ++++++++++--
>>  1 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
>> index 9bc39ca..4b66374 100644
>> --- a/drivers/usb/host/ehci-q.c
>> +++ b/drivers/usb/host/ehci-q.c
>> @@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
>>  	else {
>>  		qtd = list_entry (qh->qtd_list.next,
>>  				struct ehci_qtd, qtd_list);
>> -		/* first qtd may already be partially processed */
>> -		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
>> +		/*
>> +		 * first qtd may already be partially processed.
>> +		 * If we come here during unlink, the QH overlay region
>> +		 * might have reference to the just unlinked qtd. The
>> +		 * qtd is updated in qh_completions(). Update the QH
>> +		 * overlay here.
>> +		 */
>> +		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
>> +			qh->hw->hw_qtd_next = qtd->hw_next;
>>  			qtd = NULL;
>> +		}
>>  	}
>>  
>>  	if (qtd)
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>
> 

Thanks Alan for reviewing the patch.

> Have you been able to determine that this eliminates your host system
> errors?

Yes. We are able to determine that this patch is fixing the EHCI system
error.

-- 
Sent by a consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation.

Re: [PATCH] EHCI: Update qTD next pointer in QH overlay region during unlink

[Alan Stern]

On Fri, 7 Sep 2012, Alan Stern wrote:

> 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.
> > 
> > Signed-off-by: Pavankumar Kondeti <pkondeti-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
> > ---
> >  drivers/usb/host/ehci-q.c |   12 ++++++++++--
> >  1 files changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci-q.c b/drivers/usb/host/ehci-q.c
> > index 9bc39ca..4b66374 100644
> > --- a/drivers/usb/host/ehci-q.c
> > +++ b/drivers/usb/host/ehci-q.c
> > @@ -128,9 +128,17 @@ qh_refresh (struct ehci_hcd *ehci, struct ehci_qh *qh)
> >  	else {
> >  		qtd = list_entry (qh->qtd_list.next,
> >  				struct ehci_qtd, qtd_list);
> > -		/* first qtd may already be partially processed */
> > -		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current)
> > +		/*
> > +		 * first qtd may already be partially processed.
> > +		 * If we come here during unlink, the QH overlay region
> > +		 * might have reference to the just unlinked qtd. The
> > +		 * qtd is updated in qh_completions(). Update the QH
> > +		 * overlay here.
> > +		 */
> > +		if (cpu_to_hc32(ehci, qtd->qtd_dma) == qh->hw->hw_current) {
> > +			qh->hw->hw_qtd_next = qtd->hw_next;
> >  			qtd = NULL;
> > +		}
> >  	}
> >  
> >  	if (qtd)
> 
> Acked-by: Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org>

I forgot to mention: This patch should be included in the next 3.6-rc 
release and marked for -stable.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html