Potential issue with interrupts in AudioStream.cpp

[MSzymanek]

Hi, I'm new to embedded development so please correct me if I'm wrong here.

Currently AudioStream.cpp uses allocate() and release() functions to allocate memory to audio blocks. These functions call __disable_irq() and __enable_irq(), which can lead to problems if the user has disabled interrupts beforehand, since allocating or releasing memory will re-enable interrupts. I discovered it when working on some issues with usb audio transfers, and replacing __disable_irq() and __enable_irq() inside of allocate/release functions with ATOMIC_BLOCK(ATOMIC_RESTORESTATE) from avr-libc util/atomic.h seems to solve this problem. By this I mean

void AudioStream::release(audio_block_t *block)
{
	//if (block == NULL) return;
	uint32_t mask = (0x80000000 >> (31 - (block->memory_pool_index & 0x1F)));
	uint32_t index = block->memory_pool_index >> 5;
	//__disable_irq();
	ATOMIC_BLOCK(ATOMIC_RESTORESTATE){
		if (block->ref_count > 1) {
			block->ref_count--;
		} else {
			//Serial.print("reles:");
			//Serial.println((uint32_t)block, HEX);
			memory_pool_available_mask[index] |= mask;
			if (index < memory_pool_first_mask) memory_pool_first_mask = index;
			memory_used--;
		}
	}
	//__enable_irq();
}

As far as I understood atomic block macro checks global interrupt state when entering its scope, disables interrupts and enables them again only if they were set when entering its scope.

 

[LenSamuelson]

Disabling/enabling interrupts instead of save/restore: subtly dangerous

You make a good observation. The ATOMIC_BLOCK code (that I'm seeing for the first time so take this with a grain of salt and subject to further study) seems like a good solution, though it's a strict boolean disable/re-enable setup and depends on compiler facilities.

It's well known in the embedded community that "disable_interrupts()" should return current state and the re-enable should be "restore_interrupts(original_state)" for the reasons you have noted in your post. (It's been true in my life from PDP-8 onward...)

More generically, and the way I write embedded code, is to use IRQ priority controls. In several projects I have worked on, there is a priority level for interrupts that absolutely must always be serviced, with a matching rule that such service functions cannot ever affect the resources controlled by normal critical sections. If there's no need for a "super high priority ISR" then the "newLevel" priority can be the maximum priority the hardware offers, in with case it's effectively an IRQ disable.

    int irqlevel = set_new_priority(newLevel);
   {
        critical section;
   }
   restore_priority(irqlevel);

With that paradigm irq_enable() is used only for operating system start, and there's no need for irq_disable().

Using disable/enable without that save/restore capability requires very VERY careful review of the entire codebase in which it's used.

Finally, thanks for the reference to the atomic code, I didn't know about until now and learned something new today.