Messages in this thread | | | Date | Mon, 23 Dec 2013 18:27:44 +0100 | From | Oleg Nesterov <> | Subject | spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix) |
| |
On 12/23, Oleg Nesterov wrote: > > Perhaps we should ask the maintainers upstream? Even if this works, I am > not sure this is _supposed_ to work. I mean, in theory spin_lock_irqave() > can be changed as, say > > #define spin_lock_irqsave(lock, flags) \ > do { \ > local_irq_save(flags); \ > spin_lock(lock); \ > } while (0) > > (and iirc it was defined this way a long ago). In this case "flags" is > obviously not protected.
Yes, lets ask the maintainers.
In short, is this code
spinlock_t LOCK; unsigned long FLAGS;
void my_lock(void) { spin_lock_irqsave(&LOCK, FLAGS); }
void my_unlock(void) { spin_unlock_irqrestore(&LOCK, FLAGS); }
correct or not?
Initially I thought that this is obviously wrong, irqsave/irqrestore assume that "flags" is owned by the caller, not by the lock. And iirc this was certainly wrong in the past.
But when I look at spinlock.c it seems that this code can actually work. _irqsave() writes to FLAGS after it takes the lock, and _irqrestore() has a copy of FLAGS before it drops this lock.
And it turns out, some users assume this should work, for example
arch/arm/mach-omap2/powerdomain.c: pwrdm_lock() and pwrdm_unlock()
drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c: brcmf_fws_lock() and brcmf_fws_unlock()
seem to do exactly this. Plus the pending patch for drivers/scsi/pm8001/.
So is it documented somewhere that this sequence is correct, or the code above should be changed even if it happens to work?
Oleg.
| |