Re: spinlocks() are severely broken in 2.2.X and 2.4.X for modules

[=?ISO-8859-1?Q?G=E9rard_Roudier?=]

On Sat, 1 Jul 2000, David S. Miller wrote:

>    Date: 	Sat, 1 Jul 2000 10:09:56 +0200 (CEST)
>    From: =?ISO-8859-1?Q?G=E9rard_Roudier?= <[e-mail]>
> 
>    On Fri, 30 Jun 2000, Jeff V. Merkey wrote:
> 
>    [...]
> 
>    > void NWLockLRU(LRU_HANDLE *lru_handle)
>    > {
>    >    spinlock_irqsave(&lru_handle->LRU_spinlock, lru_handle->LRU_flags);
> 
>    Does not seem to me safe to store irq flags in shared data structures.
>    SMP .vs. UP does not seem to matter here.
> 
> In fact this is also a problem in that it encourages people to believe
> that doing irqsave/irqrestore for the same "flags" in different
> functions is ok.  It is not and will crash on some architectures.

I didn't know that the irq_restore() for a given irq_save() must also be
performed in the context of the same function. Thanks for the information.
Is it related to the flags containing informations about function's local
resources ? (registers used, ...)

> For example, this is OK:
> 
> {
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&lock, flags);
> 	...
> 	spin_unlock_irqrestore(&lock, flags);
> }

Indeed. This is the usual construct.
(I am not going to ever use any different construct, by the way).

[ ... ]

  Gerard.

PS:
Note that in my previous ignorance :), I might not have considered copying
the flag manually to data structure to be incorrect. In the above
NWLockLRU() context, this will look like this:

void NWLockLRU(LRU_HANDLE *lru_handle)
{
  unsigned long flags;

  spinlock_irqsave(&lru_handle->LRU_spinlock, flags);
// Make sure no compiler optimization will defeat programmer's assumption here.
  lru_handle->LRU_flags = flags;
}

Yes, this is likely to be broken at least for Sparc given your remark, but
I would be interested in hearing exactly why. :)