Hi,
I thought I'd poke at the "sparse" lock checking stuff too,
and as I half expected, it didn't like the way all the HCDs
temporarily yield their spinlocks while calling out to URB
completion handlers. But a fix to remove the warnings is
simple, just use a construct something like this to annotate
those functions that call usb_hcd_giveback_urb():
#ifdef __CHECKER__
#define __yields(x) __attribute__((context(1,1)))
#else
#define __yields(x)
#endif
Sample patch attached. Should I repackage it so that the
new annotation goes into <linux/compiler.h> with the other
lock annotations? In this case, the reason to watch out
for this lock idiom is that HCDs need to understand that
schedule data structures may have changed. Normally it's
just a case of a USB driver resubmitting that URB, for a
periodic transfer to/from an endpoint; but not always.
Another option of course being to change how all the HCDs
work. But that gets messy ... giveback() could put URBs
into queues so a BH calls the completions, at the cost of
forcing some sort of schedule re-scan in each of the HCDs,
to detect later what's now evident right when giveback()
returns: that particular endpoint queue is empty, so
it's eligible for descheduling. Such a change adds more
complexity to the way the HCDs' DMA queues are managed;
which I'd rather avoid, that being twitchey code. It'd
also mean periodic transfers (including isochronous ones)
would see increased completion latencies...
Other options of course include having such an annotation
just within USB, or ignoring the warnings for now.
Comments?
- Dave
--- 1.64/drivers/usb/host/ehci-q.c 2004-11-04 18:38:12 -08:00
+++ edited/drivers/usb/host/ehci-q.c 2004-11-20 07:48:03 -08:00
@@ -173,8 +173,15 @@
}
}
+#ifdef __CHECKER__
+#define __yields(x) __attribute__((context(1,1)))
+#else
+#define __yields(x)
+#endif
+
static void
ehci_urb_done (struct ehci_hcd *ehci, struct urb *urb, struct pt_regs *regs)
+__yields(ehci->lock)
{
if (likely (urb->hcpriv != 0)) {
struct ehci_qh *qh = (struct ehci_qh *) urb->hcpriv;
--- 1.71/drivers/usb/host/ohci-q.c 2004-11-04 18:38:13 -08:00
+++ edited/drivers/usb/host/ohci-q.c 2004-11-20 07:48:03 -08:00
@@ -28,6 +28,12 @@
/*-------------------------------------------------------------------------*/
+#ifdef __CHECKER__
+#define __yields(x) __attribute__((context(1,1)))
+#else
+#define __yields(x)
+#endif
+
/*
* URB goes back to driver, and isn't reissued.
* It's completely gone from HC data structures.
@@ -35,6 +41,7 @@
*/
static void
finish_urb (struct ohci_hcd *ohci, struct urb *urb, struct pt_regs *regs)
+__yields(ohci->lock)
{
// ASSERT (urb->hcpriv != 0);
--- 1.68/drivers/usb/host/uhci-hcd.c 2004-11-04 18:38:13 -08:00
+++ edited/drivers/usb/host/uhci-hcd.c 2004-11-20 07:48:03 -08:00
@@ -1602,7 +1602,15 @@
}
}
-static void uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs)
+#ifdef __CHECKER__
+#define __yields(x) __attribute__((context(1,1)))
+#else
+#define __yields(x)
+#endif
+
+static void
+uhci_finish_urb(struct usb_hcd *hcd, struct urb *urb, struct pt_regs *regs)
+__yields(uhci->schedule_lock)
{
struct uhci_hcd *uhci = hcd_to_uhci(hcd);
