CODEWITHSUNDEEP
cembeddedinterruptc99
Maestro
Maestro

Reputation: 9508

Prevent nested calls

I have a function to disable interrupts, but the problem is that if I disable them and I call a function which also disables/enables them, they get re-enabled too early. Is the following logic enough to prevent this?

static volatile int IrqCounter = 0;

void EnableIRQ()
{
    if(IrqCounter > 0)
    {
        IrqCounter--;
    }

    if(IrqCounter == 0)
    {
        __enable_irq();
    }
}

void DisableIRQ()
{
    if(IrqCounter == 0)
    {
        __disable_irq();
    }

    IrqCounter++;
}

Upvotes: 3

Views: 852

Answers (4)

Mike Stitt
Mike Stitt

Reputation: 25

static volatile int IrqCounter = 0;

void EnableIRQ(void)
{
    ASSERT(IrqCounter != 0)  //should never be 0, or we'd have an unmatched enable/disable pair

    if (IrqCounter > 0)
    {
        IrqCounter--;
    }

    if (IrqCounter == 0)
    {
        __enable_irq();
    }
}

void DisableIRQ(void)
{
    __disable_irq(); // Fix TOCTOU issues. In CMSIS there is no harm in extra disables, so always disable.
    IrqCounter++;
}

Upvotes: 0

Ross
Ross

Reputation: 1327

Assuming that you've got a system where you can't change context when interrupts are disabled, then what you've got is fine, assuming you keep careful track of when call the enable().

In the usage you're describing in the comments below, you plan on using these sections from within an interrupt service routine. Your main use is blocking higher-priority interrupts from running for a certain portion of an ISR.

Be aware that you'll have to consider the stack depth of these nested ISRs, as when you enable interrupts before your return from interrupt, you'll have interrupts enabled in the ISR.

Regarding other answers: the lack of thread-safety of the enable() (due to the if(IrqCounter > 0)) doesn't matter, because anytime you're in the enable() context switches are already disabled due to interrupts being off. (Unless for some reason you have unmatched disable/enable pairs, and in that case you've got other issues.)

The only suggestion I'd have would be to add an ASSERT to the enable instead of the run-time check, as you should never be enabling interrupts that you didn't disable.

void EnableIRQ()
{
  ASSERT(IrqCounter != 0)  //should never be 0, or we'd have an unmatched enable/disable pair

  IrqCounter--;  //doesn't matter that this isn't thread safe, as the enable is always called with interrupts disabled.

  if(IrqCounter == 0)
  {
      __enable_irq();
  }
}

I prefer the technique you've listed over the save(); disable(); restore(); technique as I don't like having to keep track of a piece of the OS' data every time I work with the interrupts. But, you do have to be aware of when you (directly or indirectly) make a call to the enable() from an ISR.

Upvotes: 2

Mats Petersson
Mats Petersson

Reputation: 129374

The way every operating system I know of does it is to save IRQ state into a local variable, and then restore that.

Clearly, your code has TOCTOU issues - if two threads run at the same time, checking the IrqCounter > 0, if IrqCounter == 1, then the first thread will see it as 1, the second thread sees it as 1, and both decrement the counter.

I would definitely try to arrange something like this:

int irq_state = irq_save();

irq_disable();

... do stuff with IRQ's turned off ... 

irq_restore(irq_state);

Now, you don't have to worry about counters that can get out of sync, etc.

Upvotes: 5

Alexey Frunze
Alexey Frunze

Reputation: 62066

That looks fine, except it's not thread-safe.

Another common option is to query the interrupt-enable/disable state and save it into a local variable, then disable interrupts, then do whatever you want to be done while interrupts are disabled, then restore the state from the local variable.

Upvotes: 0

Related Questions