Disabling interrupts to protect data
in [Embedded]
|
Prev: ALP for Real Time Clock using timer unit of HC12
Next: How to create a code in C++ for LM3S811 in IAR Embedded Workbench
From: David Brown on 30 Oct 2009 08:13 ChrisQ wrote: > David Brown wrote: > >>> >> >> Do you mean something like this : >> >> void foo(void) { >> doSomethingUnlocked(); >> memoryBarrier(); >> oldSREG = SREG; // Preserve old interrupt state >> cli(); // Ensure interrupts are disabled now >> doSomethingLocked(); >> memoryBarrier(); >> SREG = oldSREG; // Restore old interrupt flag >> doMoreUnlocked(); >> } >> >> The details vary from processor to processor, but the principle is the >> same. >> >> Some compilers have extensions or extra headers making it easier. For >> example, avr-gcc standard library has ATOMIC_BLOCK macros for this >> purpose, and msp430-gcc has an extra "critical" function __attribute__ >> that wraps a function in an interrupt disable/restore pair. > > Yes, but using an array, rather than a single variable. The code looked > like: > > > static U8 u8PwmInterruptStack [SIZE]; > static U8 u8PwmStackPointer = 0; > > /* Push Pwm Interrupt State and Mask Interrupts */ > /* -------------------------------------------- */ > void vPwmPushMaskInterruptState (void) { > > if (u8PwmStackPointer < PWM_STACKDEPTH) { > u8PwmInterruptStack [u8PwmStackPointer++] = u8PwmReadIntState (); > vPwmMaskInterrupts (); > } > } > > /* Pop Pwm Interrupt State */ > /* ----------------------- */ > void vPwmPopInterruptState (void) { > > if (u8PwmStackPointer > 0) { > u8PwmStackPointer--; > if ((u8PwmInterruptStack [u8PwmStackPointer] & PWM_INTMASK) != 0) { > vPwmUnmaskInterrupts (); > } > } > } > > The range check is just there for safety and in practice, the array is > sized to suit the application. The first function is called before > twiddling the shared data and second after. > > Turned out to be a simple solution, but the first time i've had to use > anything like it... > > Regards, > > Chris > This may sound like a silly question, but why don't you just use a local variable to hold the old interrupt status? It is rare that you would want to disable interrupts in one function, and restore the state in a different function - since you want to keep the disable time to a minimum, and be very clear about when they are disabled or not, your disable and enable are typically done at the same level. Thus you have { uint8_t oldState = u8PwmReadIntState (); vPwmMaskInterrupts () doSomething(); if ((oldState & PWM_INTMASK) != 0) { vPwmUnmaskInterrupts (); } } The code in doSomething() could quite happily have the same sort of sequence. There is no need to have a special stack - normal locals suffice. If you have a complex system with particular interrupts (rather than global ones) enabled and disabled from different threads in a program, then local variables like that won't be sufficient. But I'd first suggest thinking hard about the structure of the program - there are probably better, clearer and therefore safer ways to get the effect you want. However, if you want to do something like that, you don't need a stack - you just need a counter variable tracking the number of times the interrupt has been disabled. Of course, you need to disable global interrupts around access to this counter...
From: Mark Piffer on 30 Oct 2009 11:11 On 30 Okt., 07:45, bastia...(a)yahoo.com (42Bastian Schick) wrote: > On Tue, 27 Oct 2009 22:07:40 +0100, David Brown > > <david.br...(a)hesbynett.removethisbit.no> wrote: > >Reservations are hardware intensive to implement, unless you are willing > >to pay extra memory latency. But they scale well for multi-processor > >systems, making them a good solution for "standard" PPC processors. The > >PPC core in the MPC55xx is more like a microcontroller core than a "big" > >processor core, and Freescale probably saved a fair amount of silicon > >and complexity by dropping reservations so that you must use traditional > >interrupt disabling. > > If FSL only would have placed a big sticker on the manual. :-) > Seriously, the manual states reservation works, and some place later > in the EBI (external bus interface) description they write it does not > work for EBI. > => I learned, I have to read every single word of a manual. > > -- > 42Bastian > Do not email to bastia...(a)yahoo.com, it's a spam-only account :-) > Use <same-name>@monlynx.de instead ! I remember reading somewhere, something like Motorola documentation departments being the only US organizations running with CMMI level 5. Their documents suck with a high standard definitively. :) Mark
From: ChrisQ on 30 Oct 2009 14:35 David Brown wrote: > > This may sound like a silly question, but why don't you just use a local > variable to hold the old interrupt status? It is rare that you would > want to disable interrupts in one function, and restore the state in a > different function - since you want to keep the disable time to a > minimum, and be very clear about when they are disabled or not, your > disable and enable are typically done at the same level. Thus you have The product is typically a 128 x 192 x 2 colour led pixel graphics display, driven via serial comms at 9.6k. The leds are driven from ti led drivers, which handle 8 leds each, and use a set of serial control lines to clock and latch data and switch between data transfer and self test modes. Intensity is done via pwm on an enable line, which itself is one of the control lines. The absolute requirement to avoid any flicker at all on the display means that most control functions and data transfers can only be done in interrupt context, synchronised to the pwm off period. From a programming point of view, it's not ideal but that's the hardware that we had to work with. There's no rtos, rather state driven, with more state machinery in the comms, data transfer and data self test areas. The software was developed over 2 years and now runs on many variants. Problem was that there were many layers of code and an overall lock requirement that was difficult to resolve across the layers. The only way to keep track was to save and restore context at each point. Yes, you could do this with a local variable in each function, but it is more code and more bug prone than a single line function call for disable and enable. The functionality is encapsulated and I don't have to think about the internals, nor remember how many instances there are in the code if it needs to be changed. Run time speed is not an issue either, so an added function call or so is irrelevant. Ideal world rarely maps onto real products in practice :-)... Regards, Chris
From: Stefan Reuther on 30 Oct 2009 15:54 David Brown wrote: > Stefan Reuther wrote: >> All this is totally simple when you can use CLI/STI: >> void atomic_swap(node** a, node** b, node* c) { >> cli(); *a = *b; *b = c; sti(); >> } >> >> // prepend new node to a list (easier than appending...) >> node* list; >> node* newNode; >> atomic_swap(&newNode->next, &list, newNode); >> (some strategically-placed 'volatile' can be appropriate.) > > Here you have missed one important point that many people fail to > understand about interrupt disabling (and the related topic of volatile > accesses) - memory barriers. Unless your cli() and sti() macros / > assembly functions specifically include memory barriers, then your > atomic_swap function is not safe. Yep. The declaration should've been void atomic_swap(node*volatile* a, node*volatile* b, node* c); I was just too lazy to figure out where to place the 'volatile's :-) That, together with cli/sti macros acting as memory barriers (honestly, cli/sti which are not such barriers are useless) fixes the routine for a single processor system. Multiprocessor is a little harder. > In the atomic_swap function above, for example, the compiler will > probably see the cli() and sti() as volatile accesses (depending on the > compiler and the definition of these macros). But the code in the > middle does not use volatile accesses - the compiler can therefore > freely move these operations before or after the cli() and sti() > operations. Actually, our compiler moved the accesses even though I used 'volatile'. Fortunately the vendor fixed it quickly, but in the meantime I've made an asm version to fix it forever. Stefan
From: Hans-Bernhard Bröker on 30 Oct 2009 17:14
David Brown wrote: > { > uint8_t oldState = u8PwmReadIntState (); > vPwmMaskInterrupts () That won't work, because it has the exact same problem that the concept of a critical section is supposed to work against. Interrupt state can change between these two instructions, and that would leave you restoring an out-of-date oldState. Ultimately, you need atomic query-and-change operations to resolve this. |