Is a spinlock necessary in this Linux device driver code?

Question

Is the following Linux device driver code safe, or do I need to protect access to interrupt_flag with a spinlock?

static DECLARE_WAIT_QUEUE_HEAD(wq_head);
static int interrupt_flag = 0;

static ssize_t my_write(struct file* filp, const char* __user buffer, size_t length, loff_t* offset)
{
   interrupt_flag = 0;   
   wait_event_interruptible(wq_head, interrupt_flag != 0);
}

static irqreturn_t handler(int irq, void* dev_id)
{
   interrupt_flag = 1;
   wake_up_interruptible(&wq_head);
   return IRQ_HANDLED;
}

Basically, I kick off some event in my_write() and wait for the interrupt to indicate that it completes.

If so, which form of spin_lock() do I need to use? I thought spin_lock_irq() was appropriate, but when I tried that I got a warning about the IRQ handler enabling interrupts.

Doesn't wait_event_interruptible evaluate the interrupt_flag != 0 condition? That would imply that the lock should be held while it reads the flag, right?

Answer 1

No lock is needed in the example given. Memory barriers are needed after the store of the flag, and before the load -- to ensure visibility to the flag -- but the wait_event_* and wake_up_* functions provide those. See the section entitled "Sleep and wake-up functions" in this document: https://www.kernel.org/doc/Documentation/memory-barriers.txt

Before adding a lock, consider what is being protected. Generally locks are needed if you're setting two or more separate pieces of data and you need to ensure that another cpu/core doesn't see an incomplete intermediate state (after you started but before you finished). In this case, there's no point in protecting the storing / loading of the flag value because stores and loads of a properly aligned integer are always atomic.

So, depending on what else your driver is doing, it's quite possible you do need a lock, but it isn't needed for the snippet you've provided.

Answer 2

Yes you need a lock. With the given example (that uses int and no specific arch is mentioned), the process context may be interrupted while accessing the interrupt_flag. Upon return from the IRQ, it may continue and interrupt_flag may be left in inconsistent state.

Try this:

static DECLARE_WAIT_QUEUE_HEAD(wq_head);
static int interrupt_flag = 0;
DEFINE_SPINLOCK(lock);

static ssize_t my_write(struct file* filp, const char* __user buffer, size_t length, loff_t* offset)
{
   /* spin_lock_irq() or spin_lock_irqsave() is OK here */
   spin_lock_irq(&lock);
   interrupt_flag = 0;
   spin_unlock_irq(&lock);

   wait_event_interruptible(wq_head, interrupt_flag != 0);
}

static irqreturn_t handler(int irq, void* dev_id)
{
   unsigned long flags;

   spin_lock_irqsave(&lock, flags);
   interrupt_flag = 1;
   spin_unlock_irqrestore(&lock, flags);

   wake_up_interruptible(&wq_head);
   return IRQ_HANDLED;
}

IMHO, the code has to be written without making any arch or compiler-related assumptions (like the 'properly aligned integer' in Gil Hamilton answer).

Now if we can change the code and use atomic_t instead of the int flag, then no locks should be needed.