Re: [patch] Real-Time Preemption, -RT-2.6.13-rc6-V0.7.53-11

[Alan Stern <stern@rowland.harvard.edu>]

On Tue, 16 Aug 2005, Ingo Molnar wrote:

> * Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > Interrupts are disabled during usb_hcd_giveback_urb because that's how 
> > it was done originally and nobody has made an effort to remove this 
> > assumption from the USB device drivers.  There's no real reason for it 
> > other than historical inertia.  It's not done for serialization -- 
> > there's no need for serialization since an URB can't be resubmitted 
> > before the previous callback occurs (unless a driver is badly broken).  
> > The "detached" method is used simply to avoid an extra pair of 
> > enable/disable instructions.
> 
> so we can remove it altogether, via the patch below? (If there's any 
> unsafe driver, it should already be unsafe on SMP, and with the 
> proliferation of HT and dual-core CPUs, SMP will be the norm within a 
> year or so - so the sooner we trigger any breakages, the better i 
> guess.)
> 
> i'll give it a whirl in the -RT tree.

In general yes, the patch should be okay.  But there are a few things to
check for.  Perhaps most of the USB drivers don't care whether interrupts
are enabled or not in their completion routines.  However I know of at
least one where it does matter.  The current code _is_ SMP-safe, but it
won't remain UP-safe unless you add this patch in addition to your own.

Alan Stern

P.S.: Another routine worth examining is async_completed() in 
drivers/usb/core/devio.c.



Index: usb-2.6/drivers/usb/core/message.c
===================================================================
--- usb-2.6.orig/drivers/usb/core/message.c
+++ usb-2.6/drivers/usb/core/message.c
@@ -224,8 +224,9 @@ static void sg_clean (struct usb_sg_requ
 static void sg_complete (struct urb *urb, struct pt_regs *regs)
 {
 	struct usb_sg_request	*io = (struct usb_sg_request *) urb->context;
+	unsigned long flags;
 
-	spin_lock (&io->lock);
+	spin_lock_irqsave (&io->lock, flags);
 
 	/* In 2.5 we require hcds' endpoint queues not to progress after fault
 	 * reports, until the completion callback (this!) returns.  That lets
@@ -259,7 +260,7 @@ static void sg_complete (struct urb *urb
 		 * unlink pending urbs so they won't rx/tx bad data.
 		 * careful: unlink can sometimes be synchronous...
 		 */
-		spin_unlock (&io->lock);
+		spin_unlock_irqrestore (&io->lock, flags);
 		for (i = 0, found = 0; i < io->entries; i++) {
 			if (!io->urbs [i] || !io->urbs [i]->dev)
 				continue;
@@ -272,7 +273,7 @@ static void sg_complete (struct urb *urb
 			} else if (urb == io->urbs [i])
 				found = 1;
 		}
-		spin_lock (&io->lock);
+		spin_lock_irqsave (&io->lock, flags);
 	}
 	urb->dev = NULL;
 
@@ -282,7 +283,7 @@ static void sg_complete (struct urb *urb
 	if (!io->count)
 		complete (&io->complete);
 
-	spin_unlock (&io->lock);
+	spin_unlock_irqrestore (&io->lock, flags);
 }
 
 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/