Hi,
while implementing an SL811HS driver that uses the OHCI layer (see
different mail on this list for code patch), I found
some bugs in the original OHCI code:
In the function ohci_hub_resume() in ohci-hub.c a bogus value is
written to the cmdstatus register when restarting the ED queues.
The variable 'status' has the value -EINPROGRESS at this time while
in 'temp' the appropriate flags for the cmdstatus register have been
assembled beforehand.
--- linux-2.6.8.1-karo/drivers/usb/host/ohci-hub.c~ohci-bugfix
+++ linux-2.6.8.1-karo/drivers/usb/host/ohci-hub.c
@@ -289,7 +289,7 @@
ohci->hc_control |= enables;
writel (ohci->hc_control, &ohci->regs->control);
if (temp)
- writel (status, &ohci->regs->cmdstatus);
+ writel (temp, &ohci->regs->cmdstatus);
(void) ohci_readl (&ohci->regs->control);
}
--- end of patch ---
The patch below makes sure that ed->tick has been set to the current
frame number BEFORE enabling the SF interrupt. Otherwise the interrupt
routine might read a bogus value from ed->tick and decide that
the ED cannot yet be removed from the done list.
This might be quite unlikely and won't give any problem if there are
further interrupts, since the next one would find the correct ed->tick
value. BUT, if the ED removal has been triggered by device removal we
won't get any further SF interrupts to finish up clearing the done
queue.
--- linux-2.6.8.1-karo/drivers/usb/host/ohci-q.c~ohci-bugfix
+++ linux-2.6.8.1-karo/drivers/usb/host/ohci-q.c
@@ -477,19 +477,17 @@
ed->ed_prev = NULL;
ohci->ed_rm_list = ed;
- /* enable SOF interrupt */
- writel (OHCI_INTR_SF, &ohci->regs->intrstatus);
- writel (OHCI_INTR_SF, &ohci->regs->intrenable);
- // flush those writes, and get latest HCCA contents
- (void) ohci_readl (&ohci->regs->control);
-
/* SF interrupt might get delayed; record the frame counter value that
* indicates when the HC isn't looking at it, so concurrent unlinks
* behave. frame_no wraps every 2^16 msec, and changes right before
* SF is triggered.
*/
ed->tick = OHCI_FRAME_NO(ohci->hcca) + 1;
-
+ /* enable SOF interrupt */
+ writel (OHCI_INTR_SF, &ohci->regs->intrstatus);
+ writel (OHCI_INTR_SF, &ohci->regs->intrenable);
+ // flush those writes, and get latest HCCA contents
+ (void) ohci_readl (&ohci->regs->control);
}
/*-------------------------------------------------------------------------*
--- end of patch ---
The last patch corrects the comparison between ed->tick and the
current frame number accomplished through the macro tick_before(). The
comment near the macro definition:
| /* wrap-aware logic stolen from <linux/jiffies.h> */
makes us think that there can't be anything wrong with this macro,
because it is used elsewhere in the kernel. But it is used in
different context and thus leads to different results!
The problem is, that tick_before() calculates a difference of two
16bit entities but the comparison of the result is done in 32bit
context. Therefore the result: 0xffff (-1 as a signed short) becomes
0x0000ffff (+65535) in the comparison.
In the worst case this can lead to an ED stalled in the done list for
half a cycle time of the frame counter (~32 sec).
--- linux-2.6.8.1-karo/drivers/usb/host/ohci-q.c~ohci-bugfix
+++ linux-2.6.8.1-karo/drivers/usb/host/ohci-q.c
@@ -907,7 +905,14 @@
/*-------------------------------------------------------------------------*/
/* wrap-aware logic stolen from <linux/jiffies.h> */
-#define tick_before(t1,t2) ((((s16)(t1))-((s16)(t2))) < 0)
+/* NOT QUITE: There we operate on 32 bit operands only, while here the result of
+ * a 16 bit subtraction is being compared in 32 bit context!
+//#define tick_before(t1,t2) ((((s16)(t1))-((s16)(t2))) < 0)
+/*
+ * signed or unsigned subtraction doesn't make any difference, but we need to
+ * cast the result to a signed number of the same size as the original operands!
+ */
+#define tick_before(t1,t2) ((s16)(((s16)(t1)) - ((s16)(t2))) < 0)
/* there are some urbs/eds to unlink; called in_irq(), with HCD locked */
static void
--- end of patch ---
Lothar Wassmann
-------------------------------------------------------
This SF.Net email is sponsored by BEA Weblogic Workshop
FREE Java Enterprise J2EE developer tools!
Get your free copy of BEA WebLogic Workshop 8.1 today.
http://ads.osdn.com/?ad_id=5047&alloc_id=10808&op=click
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
