Why is disabling interrupts necessary here?

Question

static void RadioReleaseSPI(void) {
    __disable_interrupt();
    spiTxRxByteCount &= ~0x0100;
    __enable_interrupt();
}

I understand that multiple tasks may attempt to use the SPI resource. spiTxRxByteCount is a global variable used to keep track of whether the SPI is currently in use by another task. When a task requires the SPI it can check the status of spiTxRxByteCount to see if the SPI is being used. When a task is done using the SPI it calls this function and clears the bit, to indicate that the SPI is now free. But why disable the interrupts first and then re-enable them after? Just paranoia?

Phinney

If you have an RTOS, why not use a mutex? – Whitton

@ Semaphore operation implemented in this way under OS – Oxendine

Answer 1

The &= will do a read-modify-write operation - it's not atomic. You don't want an interrupt changing things in the middle of that, resulting in the write over-writing with an incorrect value.

Lilybelle

Answer 2

You need to disable interrupts to ensure atomic access. You don't want any other process to access and potentially modify that variable while you're reading it.

From Introduction to Embedded Computing:

The Need for Atomic Access

Imagine this scenario: foreground program, running on an 8-bit uC, needs to examine a 16-bit variable, call it X. So it loads the high byte and then loads the low byte (or the other way around, the order doesn’t matter), and then examines the 16-bit value. Now imagine an interrupt with an associated ISR that modifies that 16-bit variable. Further imagine that the value of the variable happens to be 0x1234 at a given time in the program execution. Here is the Very Bad Thing that can happen:

• foreground loads high byte (0x12)
• ISR occurs, modifies X to 0xABCD
• foreground loads low byte (0xCD)
• foreground program sees a 16-bit value of 0x12CD.

The problem is that a supposedly indivisible piece of data, our variable X, was actually modified in the process of accessing it, because the CPU instructions to access the variable were divisible. And thus our load of variable X has been corrupted. You can see that the order of the variable read does not matter. If the order were reversed in our example, the variable would have been incorrectly read as 0xAB34 instead of 0x12CD. Either way, the value read is neither the old valid value (0x1234) nor the new valid value (0xABCD).

Writing ISR-referenced data is no better. This time assume that the foreground program has written, for the benefit of the ISR, the previous value 0x1234, and then needs to write a new value 0xABCD. In this case, the VBT is as follows:

• foreground stores new high byte (0xAB)
• ISR occurs, reads X as 0xAB34
• foreground stores new low byte (0xCD)

Once again the code (this time the ISR) sees neither the previous valid value of 0x1234, nor the new valid value of 0xABCD, but rather the invalid value of 0xAB34.

While spiTxRxByteCount &= ~0x0100; may look like a single instruction in C, it is actually several instructions to the CPU. Compiled in GCC, the assembly listing looks like so:

  57:atomic.c      ****     spiTxRxByteCount &= ~0x0100;
  68                    .loc 1 57 0
  69 004d A1000000      movl    _spiTxRxByteCount, %eax
  69      00
  70 0052 80E4FE        andb    $254, %ah
  71 0055 A3000000      movl    %eax, _spiTxRxByteCount
  71      00

If an interrupt comes in in-between any of those instructions and modifies the data, your first ISR can potentially read the wrong value. So you need to disable interrupts before you operate on it and also declare the variable volatile.

Cedillo

Answer 3

There are two reasons for why you should be disabling interrupts:

• The &= is a read-modify-write operation which is in nature not atomic. It consists of a read, a bitwise-and, and a write. You don't want this operation to be interrupted by an ISR (interrupt service route). The ISR could modify spiTxRxByteCount after the read and before the write. The write would then be based on an outdated value and you would lose information.
• __disable_interrupt() and __enable_interrupt() serve as software barriers. Even if optimization is enabled, the compiler must not move the read or the write across the two barriers. Also, the compiler must not cache the value of spiTxRxByteCount across the two barriers. If there were no barriers, the compiler would be allowed to hold a copy of spiTxRxByteCount in some CPU register even across multiple invocations of RadioReleaseSPI(). This would typically happen if inlining is enabled and RadioReleaseSPI() is called repeatedly.

That disabling and enabling interrupts serves as barriers is at least as important as avoiding the interruption by an ISR, IMHO. But it seems to be overlooked, sometimes.

Circe