[En-Nut-Discussion] Fwd: Not fixed!!!: Re: confirmed!!! Re: NutOS 4.4.0 on ARM7: possibly bug in NutEnterCritical / NutExitCritical

[Nathan Moore]

On Sun, Feb 24, 2008 at 11:24 AM, duane ellis <ethernut at duaneellis.com>
wrote:

> Harald Kipp wrote:
> > duane ellis schrieb:
> >
> >> Look at how Linux does this.
> >>
> >> http://www.promethos.org/lxr/http/ident?v=2.6.14;i=local_irq_save
> >>
> >> Those guys are smart. Learn from them.
> >>
> >
> >
> > Well, I tried. But there are things I do not understand. For example,
> > why do they use the "memory" clobber? This forces the compiler to forget
> > about all values currently stored in registers.
> >
> I believe (90% sure) it has to do with volatility and optimization of
> variables. For example, if reg0 has the value of global X, and you call
> some function, upon return, the global variable X may have changed.
> Likewise, when you re-enable interrupts using inline assembly, the odds
> are, you might have an IRQ pending which will immediately occur, and
> thus, the complier must forget what it knows about optimized values.
>  From an optimization point of view - the inline code must act like the
> inline assembly was a function call.
>

I don't think that this really makes sense even if it were the reason that
"memory" is in the clobber list since an IRQ could pop in and change
stuff in memory at any time when interrupts aren't disabled, not just
right after interrupts have been reenabled.  Casting variables to volatile
and use the variables that might change within the critical section rather
than after it would be the right thing to do.


>
>
> > Some posters do not understand, why we do not simply take the approach
> > of other operating systems (not just Linux btw.), passing a local
> > variable to the Enter/ExitCritical macros.
> > (Maintainer hat on)
> >  From the view of the developer this might not hurt much. From my view,
> > this will cause a lot of trouble, because it will break almost all
> > existing applications, which make use of critical sections. I definitely
> > know, that many organizations and companies out there are using Nut/OS
> > based applications, while the original developers are no longer
> > available. I'd really hate to tell them, that there is a bug fix for
> > their strange crashes, but they need to rewrite some device driver code.
> > (Maintainer hat off)
> >
>
The counter argument to this is that correctness trumps backwards API
compatibility.


>
> If your goal is to keep backward compatibility - then there is an easy
> way of solving it.
>
> (1) Create NEW funcs/macros that do it the right way.
> (2) Change OLD funcs/macros to use the workaround [see below]
> (3) Deprecate OLD, give X time frame warning.
> (4) OLD users have 2 choices, stay with the old version, or move to the
> new.
>
> >> Why don't we simply use a global counter
> >> variable to keep track of the enter/exit level?
>
>
>   >> My suggestion is, to fix the existing NutEnter/ExitCritical using a
>   >>global counter variable and, ....

I've seen code (including in the message passing code) that is wrong, but
would
totally break a lot of stuff with this because they sleep or do IO within
critical sections.
I'm not really sure why these things still work at all, but if you made it a
thread local
varialbe it would work better:
    CurrentThread->CriticalDepth

If you do change the API, I'd suggest going with the absolute best
solution(s) rather
than trying to make it similar to the current way of doing things.  It's not
something
that you want to change again and if everyone has to go through their apps
to fix
stuff already then might as well really fix it.

Nathan