Messages in this thread | | | Date | Mon, 23 Dec 2013 19:23:23 +0100 | From | Ingo Molnar <> | Subject | Re: spinlock_irqsave() && flags (Was: pm80xx: Spinlock fix) |
| |
* Oleg Nesterov <oleg@redhat.com> wrote:
> 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.
I don't think that's true: if it was then the lock would not be irqsave, a hardware-irq could come in after the lock has been taken and before flags are saved+disabled.
So AFAICS this is an unsafe pattern, beyond being ugly as hell.
Thanks,
Ingo
| |