Interrupt handling after sleep

[Morpheus1822]

I came across a bug (?) regarding the interrupt handling after sleeping. My minimal application looks like this:

static volatile uint8_t event = 0;
ISR(INT0_vect) {
    event = 1;
}

void main(void) {
    sleep_enable();
    // interrupt initialization

    while(1) {

        cli();
        while(event == 0) {
            sei();
            sleep_cpu();
            cli();
        }
        sei();

        // handle event
        event = 0;
    }
}

However, sometimes the application is sleeping forever. I looked into it a bit and it seems it occurs, if the compiler decides to put the cli instruction directly after the sleep instruction.

After digging a bit in the simavr source files, I found the following code snippet in the file sim_core.h:

static inline void avr_sreg_set(avr_t * avr, uint8_t flag, uint8_t ival) {
	if (flag == S_I) {
		if (ival) {
			if (!avr->sreg[S_I])
				avr->interrupt_state = -2;
		} else
			avr->interrupt_state = 0;
	}

	avr->sreg[flag] = ival;
}

To my understanding negative values for interrupt_state indicate a latency (in cycles) before the interrupt is handled. Waiting for two cycles leads to the situation that the cli is already executed before the interrupt was handled and handling interrupts with cleared SI bit won't work so the interrupt is never handled and my application stays in it's while loop forever. I never observed this behavior on real hardware.

Did I understand anything wrong? What is not clear to me is, why is there a two cycle delay in handling interrupts?

[buserror]

It is a documented fact about interrupts on the AVR. It is also a feature that is used quite a bit by existing code. For example code that switches the stack pointer use that to make it 'interrupt safe'.

Your code can't work without a 'nop' -- the sei(), sleep_cpu() and cli() are all a single cycle as they are intrinsic on the compiler, so you're missing a cycle to make it safe!

[Morpheus1822]

Oh, I wasn't aware of that. I thought sei is delayed only one instruction. This is how I understood the ISA description of sei.

I would appreciate if you could point me to the right documentation?

[buserror]

I don't have it handy, but from the code, it was made to skip 2 cycles, not "one instruction". I remember merging a patch years ago about this and I haven't reviewed the code since, but it seems to work fine at the minute, I'm reasonably sure it would break quite a few things if this was out of kilter.

[akrasuski1]

Hey there, I've confirmed that the bug is there. Here's the code I tested with:

#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <avr/cpufunc.h>

ISR(TIMER0_OVF_vect) {
	PORTB |= 1<<5;
}

int main() {
	DDRB |= 1<<5;
	TCCR0 = 1<<CS00;
	TIMSK = 1<<TOIE0;
	_delay_ms(100);

	sei();
	_NOP();
	cli();
	while(1);
}

On real hardware (ATmega8), the LED connected to PB5 is turned on, indicating the interrupt fired fine (as it should according to the datasheet, given there is a nop between sei and cli). On simavr the code never goes there, unless one more nop is added.

Fun fact: at this year's GoogleCTF I've made a task related to this behaviour but accidentally made the solution work only on simulated hardware, causing much confusion (Ctrl-F "two instructions").

[vintagepc]

Just want to chime in that I believe we should indeed be setting it to -1 instead of -2.

The Klipper firmware uses a very similar style of IRQ handling and will deadlock on sei() nop() cli() unless either a second nop is inserted after the sei(), or the delay is changed to a single cycle.

Klipper has been proven on many AVR driven printers so I should think that's proof positive the behaviour isn't quite correct.