firewire: core: no need to track irq flags in bm_work
in [Kernel]
|
Prev: [PATCH] VIDEO: ivtvfb, remove unneeded NULL test
Next: kvm, Fix a race condition for usage of is_hwpoison_address
From: Stefan Richter on 22 Jun 2010 07:50 Philippe De Muyter wrote: > On Mon, Jun 21, 2010 at 11:23:52PM +0200, Stefan Richter wrote: >> This is a workqueue job and always entered with IRQs enabled. > > did you mean 'disabled' ? I meant enabled. [...] >> @@ -247,10 +246,10 @@ static void fw_card_bm_work(struct work_ >> bool root_device_is_cmc; >> bool irm_is_1394_1995_only; >> >> - spin_lock_irqsave(&card->lock, flags); >> + spin_lock_irq(&card->lock); - spin_lock + spin_unlock don't influence whether IRQs on the current CPU are on or off. - spin_lock_irq + spin_unlock_irq always switch IRQs on the current CPU off and back on. This is necessary if the lock could also be taken by an IRQ handler. (Well, card->lock is actually only taken by process contexts and by tasklets. Seems we could switch to spin_lock_bh + spin_unlock_bh for card->lock everywhere in the firewire stack.) - spin_lock_irqsave + spin_unlock_irqrestore switch IRQs on the current CPU off and back on only if used while IRQs are enabled; if used while local IRQs are already disabled they leave them disabled. http://lwn.net/images/pdf/LDD3/ch05.pdf#page=14 Therefore some people prefer to use the safer spin_lock_irqsave()/ spin_unlock_irqrestore() everywhere. However, their downsides are the need to track IRQ state flags, and --- subjectively --- that their appearance in the code could create an impression to a casual reader that this code was meant to be able to run in IRQs-on context as well as in IRQs-off context. fw_card_bm_work() however definitely requires to be called with IRQs on, notably to be able to wait for IEEE 1394 transactions to complete. -- Stefan Richter -=====-==-=- -==- =-==- http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Stefan Richter on 22 Jun 2010 14:40
Philippe De Muyter wrote: > Thanks for the clear explanation, and sorry for your wasted time. Oh, no problem. It helps that somebody actually looks at these patches and asks if something looks illogical. I for one am on record to have posted patches that got things backwards. :-) -- Stefan Richter -=====-==-=- -==- =-==- http://arcgraph.de/sr/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |