lkml.org 
[lkml]   [2013]   [Dec]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
Subjectspinlock_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.



\
 
 \ /
  Last update: 2013-12-23 18:41    [W:0.084 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site