On Wed, 5 Dec 2007, Pete Zaitcev wrote: > On Wed, 5 Dec 2007 16:35:26 -0500 (EST), Alan Stern <[EMAIL PROTECTED]> wrote: > > > Host controller IRQs are supposed to be serviced with interrupts > > disabled. This patch (as1025) adds an IRQF_DISABLED flag to all the > > controller drivers that lack it. > > OK, if you prefer to use a crew-fired weapon in case of a menacing > rodent.
Why do you say that? In fact every HCD _must_ keep interrupts disabled while doing most of its IRQ processing. This is because an interrupt could cause some random driver to submit an URB -- and URB submission can't be done concurrently with IRQ processing since they both alter the same data structures. > It's a matter of taste, but the following is wrong IMHO: > > @@ -415,16 +414,16 @@ static irqreturn_t uhci_irq(struct usb_h > > * pending unlinks */ > > mod_timer(&hcd->rh_timer, jiffies); > > } > > - spin_unlock_irqrestore(&uhci->lock, flags); > > + spin_unlock(&uhci->lock); > > } > > } > > If anything, this bug is a good example when not to do that. > > Clearly uhci, taken by in reasonable isolation, makes use of timers, > and so its atomic code can be re-entered. This is why it uses irqsave, > to keep its local invariants safe _in the face of changing externals_ > (e.g. one day you want a monstrous IRQF_DISABLED, another day you don't, > the code remains correct). No -- it uses irqsave because the IRQ handler wasn't registered with IRQF_DISABLED. That was a conscious decision on my part. If there had been a guarantee that interrupts would be disabled while uhci_irq() ran, I would have used spin_lock() from the beginning. With the new patch, now there is such a guarantee. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
