RE: A question about ohci_irq()

["Huang, FrankR" <FrankR.Huang@xxxxxxx>]

Alan,

	Thanks for your preciseness. I should add the comments for each
line. 
	The line 1-3 , I refer the code of linux 2.6.22 source code.
Actually, I think it is not correct to set the ints to
OHCI_INTR_WDH(line 4). But it does so like this in the 2.6.22 kernel
source.
	Line 7 shoud be as below, that is my careless. 
		ints &= ~OHCI_INTR_WDH;
	The Line 8-10, I want to clear the OHCI_INTR_WDH bit in the
intrstatus register. That is based on the code of ohci_irq().
		if(HC_IS_RUNNING(hcd->state)){
			ohci_writel(ohci, ints, &regs->intrstatus);
			...
		} 
	From the ohci spec 6.4.5, I know the bit should bit clear by HC
driver. So I add this part code. Otherwise no clear operation is
existing for this special condition. In your new patch, there is no need
to do so.
	I am testing your patch now.

Thanks,
Frank

-----Original Message-----
From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] 
Sent: Wednesday, December 30, 2009 11:03 PM
To: Huang, FrankR
Cc: USB list
Subject: RE: A question about ohci_irq()

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