CODEWITHSUNDEEP
linux-kernellinux-device-driverinterrupt-handlingspinlock
Ben Voigt
Ben Voigt

Reputation: 283674

Is returning while holding a spinlock automatically unsafe?

The venerated book Linux Driver Development says that

The flags argument passed to spin_unlock_irqrestore must be the same variable passed to spin_lock_irqsave. You must also call spin_lock_irqsave and spin_unlock_irqrestore in the same function; otherwise your code may break on some architectures.

Yet I can't find any such restriction required by the official documentation bundled with the kernel code itself. And I find driver code that violates this guidance.

Obviously it isn't a good idea to call spin_lock_irqsave and spin_unlock_irqrestore from separate functions, because you're supposed to minimize the work done while holding a lock (with interrupts disabled, no less!). But have changes to the kernel made it possible if done with care, was it never actually against the API contract, or is it still verboten to do so?

If the restriction has been removed at some point, did it apply to version 3.10.17?

Upvotes: 3

Views: 413

Answers (2)

Kaz
Kaz

Reputation: 58578

This is just a guess, but the might be unclearly referring to a potential bug which could happen if you try to use a nonlocal variable or storage location for flags.

Basically, flags has to be private to the current execution context, which is why spin_lock_irqsave is a macro which takes the name of the flags. While flags is being saved, you don't have the spinlock yet.

How this is related to locking and unlocking in a different function:

Consider two functions that some driver developer might write:

void my_lock(my_object *ctx)
{
  spin_lock_irqsave(&ctx->mylock, ctx->myflags);  /* BUG */
}

void my_unlock(my_object *ctx)
{
  spin_unlock_irqrestore(&ctx->mylock, ctx->myflags);
}

This is a bug because at the time ctx->myflags is written, the lock is not yet held, and it is a shared variable visible to other contexts and processors. The local flags must be saved to a private location on the stack. Then when the lock is owned, by the caller, a copy of the flags can be saved into the exclusively owned object. In other words, it can be fixed like this:

void my_lock(my_object *ctx)
{
  unsigned long flags;
  spin_lock_irqsave(&ctx->mylock, flag);
  ctx->myflags = flags;
}

void my_unlock(my_object *ctx)
{
  unsigned long flags = ctx->myflags; /* probably unnecessary */
  spin_unlock_irqrestore(&ctx->mylock, flags); 
}

If it couldn't be fixed like that, it would be very difficult to implement higher level primitives which need to wrap IRQ spinlocks.

How it could be arch-dependent:

Suppose that spin_lock_irqsave expands into machine code which saves the current flags in some register, then acquires the lock, and then saves that register into specified flags destination. In that case, the buggy code is actually safe. If the expanded code saves the flags into the actual flags object designated by the caller and then tries to acquire the lock, then it's broken.

Upvotes: 6

Tsyvarev
Tsyvarev

Reputation: 65928

I have never see that constraint aside from the book. Probably, given information in the book is just outdated, or .. simply wrong.

In the current kernel(and at least since 2.6.32, which I start to work with) actual locking is done through many level of nested calls from spin_lock_irqsave(see, e.g. __raw_spin_lock_irqsave, which is called in the middle). So different function's context for lock and unlock may hardly be a reason for misfunction.

Upvotes: 1

Related Questions