* Oleg Nesterov <oleg@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
