RE: A question about ohci_irq()

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

On Wed, 30 Dec 2009, Huang, FrankR wrote:

> See the ohci_irq() and found that at the last part of this function,
> there is a sentence " ohci_writel(ohci, ints, &regs->intrstatus); " 
> It will clear the WDH bit in order for another interrupt.
> 
> So my opinion is that whether I need to add this sentence to my patch ,
> looks like below:
> 		-- ints = ohci_readl(ohci, &regs->intrstatus);
> 1		++ if((ohci->hcca->done_head!=0)
> 2		++		&&!(hc32_to_cpup(ohci,
> &ohci->hcca->done_head)
> 3		++			&0x01)){
> 4		++		ints = OHCI_INTR_WDH;
> 5		++ }else{
> 6		++		ints = ohci_readl(ohci,
> &regs->intrstatus);
> 7		++		ints = 0xfffffffd;
> 8		++		if(ints == 0)
> 9		++			ohci_writel(ohci, OHCI_INTR_WDH,
> &regs->interrupts);
> 10		++ }
> This will be better I think. What's your opinion?

I don't like your patch for several reasons, such as:

	You added code at the wrong spot.  The new code belongs lower
	down, where a test for OHCI_INTR_WDH already exists.

	You should not set ints to OHCI_INTR_WDH.  You must always
	read regs->intrstatus; otherwise you could miss important
	information during a real interrupt.

	You should not use numeric constants like 0xfffffffd or 0x01.  
	Use symbolic constants instead.

	Line 7 in your patch makes line 6 unnecessary, because it
	overwrites the value stored in ints.

	You didn't add any comments explaining the reason for the
	new code.

> Although right now I do not clear OHCI_INTR_WDH(without line 8 and line
> 9), but when the real OHCI_INTR_WDH is happening and the done_head is
> not 0, the code can also work.

Here is my patch.  Compare it to yours.  Notice that it adds only 2 
lines of executable code.

Alan Stern


Index: usb-2.6/drivers/usb/host/ohci-hcd.c
===================================================================
--- usb-2.6.orig/drivers/usb/host/ohci-hcd.c
+++ usb-2.6/drivers/usb/host/ohci-hcd.c
@@ -831,10 +831,19 @@ static irqreturn_t ohci_irq (struct usb_
 			usb_hcd_resume_root_hub(hcd);
 	}
 
+	/* Some controllers violate the OHCI spec by setting OHCI_INTR_WDH
+	 * before writing ohci->hcca->done_head.  They don't generate an
+	 * early interrupt, but if the IRQ line is shared then we might see
+	 * the status bit prematurely.
+	 */
 	if (ints & OHCI_INTR_WDH) {
-		spin_lock (&ohci->lock);
-		dl_done_list (ohci);
-		spin_unlock (&ohci->lock);
+		if (ohci->hcca->done_head == 0) {
+			ints &= ~OHCI_INTR_WDH;
+		} else {
+			spin_lock(&ohci->lock);
+			dl_done_list(ohci);
+			spin_unlock(&ohci->lock);
+		}
 	}
 
 	if (quirk_zfmicro(ohci) && (ints & OHCI_INTR_SF)) {

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