* Peter Zijlstra <[EMAIL PROTECTED]> wrote: > --- linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c~ 2005-08-15 > 21:23:45.000000000 +0200 > +++ linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c 2005-08-15 > 22:03:33.000000000 +0200 > @@ -506,13 +506,11 @@ error: > } > > /* any errors get returned through the urb completion */ > - local_irq_save (flags); > + local_irq_save_nort (flags); > spin_lock (&urb->lock); > if (urb->status == -EINPROGRESS) > urb->status = status; > spin_unlock (&urb->lock); > usb_hcd_giveback_urb (hcd, urb, NULL); > - local_irq_restore (flags); > + local_irq_restore_nort (flags); > return 0; > }
i'm wondering whether we could/should also fix this upstream - and whether this [making the IRQ flags disabling a NOP on -RT] is the right fix. Why does the USB hcd.c code do this in the first place? Disabling interrupts during usb_hcd_giveback_urb() [but not holding the urb->lock] might serialize on UP, but it has no serialization effect on SMP and is hence potentially buggy. Is there something i'm missing about this code? the normal way of using urb->lock would be spin_lock_irqsave() and spin_lock_irqrestore(), not the 'detached' method seen above. > similar fix, completions need not have irqs disabled on PREEMPT_RT > right? correct, PREEMPT_RT is very strict about the use of the interrupt flags. A fair portion of the now-illegal API uses are also SMP bugs on upstream, so these details are worth pursuing. > --- linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c~ 2005-08-15 > 22:03:33.000000000 +0200 > +++ linux-2.6.13-rc6-git7-RT-V0.7.53-11/drivers/usb/core/hcd.c 2005-08-15 > 22:32:54.000000000 +0200 > @@ -538,7 +538,7 @@ void usb_hcd_poll_rh_status(struct usb_h > if (length > 0) { > > /* try to complete the status urb */ > - local_irq_save (flags); > + local_irq_save_nort (flags); > spin_lock(&hcd_root_hub_lock); > urb = hcd->status_urb; > if (urb) { > @@ -562,7 +562,7 @@ void usb_hcd_poll_rh_status(struct usb_h > usb_hcd_giveback_urb (hcd, urb, NULL); > else > hcd->poll_pending = 1; > - local_irq_restore (flags); > + local_irq_restore_nort (flags); same question: why are interrupts being kept disabled longer, and why is usb_hcd_giveback_urb() called with interrupts disabled? (I tried to use spin_lock_irqsave/irqrestore() in earlier -RT versions, but people reported hangs and USB misbehavior, which might be related. I'm worried that your _nort patch could cause similar misbehavior.) how about (naively) extending the urb->lock to cover usb_hcd_giveback_urb() calls too - does that cause a deadlock or is it unsafe in some other way? Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/