(no subject)

[Peter W. Morreale]

In a driver I am reviewing I found the following locking constructs.
Notice how 'foo" is being called while we have suspended interrupts.

This seems wrong since we've mixed locking primitives.

Is it?

Thanks in advance.

-PWM

---------------------snip--------------------------------------
spin_lock_irqsave(global_lock, &flags);
....
foo()
{
    unsigned long lflags;

    spin_unlock(global_lock);
    ...
    {
        spin_lock_irqsave(global_lock, &lflags);
                .
                .
        spin_unlock_irqrestore(global_lock, &lflags);
    }

    spin_lock_irq(global_lock);
}

spin_unlock_irqrestore(global_lock, &flags);

Re:

[Ross Biro]

On Tue, 08 Mar 2005 09:32:48 -0700, Peter W. Morreale
<peter_w_morreale@hotmail.com> wrote:
> 
> This seems wrong since we've mixed locking primitives.
> 
> Is it?

It's not really wrong, it just wastes time turning interrupts off over
and over again.
> ....
> foo()
> {
>     unsigned long lflags;

At this point, interrupts are off, the lock is held.
> 
>     spin_unlock(global_lock);

Interrupts are still off, the lock is no longer held.
>     ...
>     {
>         spin_lock_irqsave(global_lock, &lflags);

Interrupts are still off, and just to be sure we turned them off
again.  The lock is held.
>                 .
>                 .
>         spin_unlock_irqrestore(global_lock, &lflags);
Interrupts are still off, but we restored them to the off state they
were in before
we grabbed the lock the last time.  The lock is no longer held.
>     }
> 
>     spin_lock_irq(global_lock);
Turn off interrupts again just to be extra sure they are off.  The
lock is held again.

>From the looks of this code, the locking will work.  But it's not what
it should be.

If you know foo is only called with interrupts off, then there is no
reason to turn them off over and over again.  Just use the standard
spin_lock and spin_unlock and comment that interrupts are already off.

You should also question if interrupts need to be disabled at all.  If
the spin lock is never grabbed at interrupt time (and probably won't
be in the near future), then there is no point in turning interrupts
on and off at all.

    Ross