cpu/cortexm_common: irq_enable returns the current state of interrupts (not previous)

[jcarrano]

Description

I'm opening an issue and not a PR because I'm not sure that this is a bug.

See

RIOT/cpu/cortexm_common/irq_arch.c

Line 38 in b4664a5

__attribute__((used)) unsigned int irq_enable(void)

I would expect irq_enable() to return the state of the interrup mask previous to them being enabled. This is what irq_disable() does.

Does this make sense? I searched the codebase and this function is used sparingly and the result is never used, so if it is a bug it is asymptomatic now.

[miri64]

I think, just applying pure logic here, if it would be wrong we would have observed issues much sooner, since this is a very central function to most of RIOT's functionality, but I assigned @kaspar030 who has way more knowledge of the kernel, just to have a look.

[olegart]

There's nothing wrong now, but I see some reasons to change irq_enable as @jcarrano proposed

1. there may be a rare case when IRQs are disabled for some reason and we need some specific IRQ just for now - so it would be uint32_t state = irq_enable(); ... some processing and waiting for IRQ... irq_restore(state);

We never met such case in our practice, but then again, why not?

2. just to be consistent with irq_disable. Similarly-looking procedures are expected to have similar behavior.

[kaspar030]

We never met such case in our practice, but then again, why not?

Well, definitely, as is, the return value is kinda useless.

We might be able to save an instruction or two by changing the function into returning void.

Changing irq_enable() to return void would clearly break any existing code that made use of the value, so users are forced to fix code. Changing the function to return the previous state might lead to hard to find bugs for everyone not watching the changelog.

[kaspar030]

Actually, not returning the previous state is clearly not as the documentation of irq_enable() explains.

[jcarrano]

@miri64

if it would be wrong we would have observed issues much sooner,

It's more like "it is wrong I'd wish we had observed issues much sooner". Never take correctness for granted.

@kaspar030

not returning the previous state is clearly not as the documentation of irq_enable() explains.

Then it's a bug, but....

Changing irq_enable() to return void would clearly break any existing code that made use of the value

...this is one of those cases where fixing a bug breaks the API. Options:

1. Fix irq_enable(). Nothing in RIOT will break, but we don't know what will happen to random users' code.
2. Leave irq_enable() as is (and change the docs so that we don't mislead people)....
   2.1. ...and do nothing else, or
   2.2. Mark irq_enable() as deprecated and create irq_enable_whatever() that does what it should. This is the backwards compatible, non-API breaking solution, though it leaves us with an oddly shaped subroutine.

[kaspar030]

Options:

3. break the API. Add irq_getstate(). Make users upgrade.

4. (fix irq_enable()) should be a no-go, as silent breakage (semantic change) is the worst
   2.1. would mean we have to "fix" the other implementations that might depend on the other value

2.2. would be viable, though ugly.

Personally, I'd rather have the API look like we want it to and document the change. Not all API changes are equal, and this would lead to rather trivial changes.

[jcarrano]

I doubt anyone is using the return value of irq_enable(), simply because it us useless right now (returning "enabled" always).
By "fix irq_enable(), I mean "make it do what the docs say it should do".

[kaspar030]

I doubt anyone is using the return value of irq_enable(), simply because it us useless right now (returning "enabled" always).

That is why we can drop the return value.

By "fix irq_enable(), I mean "make it do what the docs say it should do".

We cannot not fix it, as it might break code that depends on the old behaviour, (however wrong it might be), without users noticing when compiling, thus leading to in-the-field bugs.

[maribu]

How about updating the API to return void?

No one is using the return value anyway:

boards/arduino-leonardo/board.c:41:    irq_enable();
boards/waspmote-pro/board.c:67:    irq_enable();
boards/common/msb-430/board_init.c:107:    irq_enable();
boards/common/atmega/board.c:47:    irq_enable();
boards/chronos/board_init.c:96:    irq_enable();
core/include/irq.h:50:unsigned irq_enable(void);
sys/arduino/serialport.cpp:160:    irq_enable();
drivers/motor_driver/motor_driver.c:176:    irq_enable();
drivers/motor_driver/motor_driver.c:240:    irq_enable();
cpu/native/native_cpu.c:227:        irq_enable();
cpu/native/startup.c:545:    irq_enable();
cpu/native/irq_cpu.c:214:        irq_enable();
cpu/atmega_common/thread_arch.c:218:    irq_enable();
cpu/cc430/cc430-adc.c:75:    irq_enable();
cpu/fe310/cpu.c:335:    irq_enable();
cpu/msp430_common/msp430-main.c:116:    irq_enable();

Unused APIs (or unused features of APIs) tend to be untested and buggy. Alternatively, I could PR a test for the correct implementation of the API.

[jcarrano]

@maribu: makes sense. That way any user will have a compilation failure if they try to use the value. That will force them to go over what they were doing (anyone using the return value is likely to have a bug).

[kaspar030]

How about updating the API to return void?

+1

[maribu]

I just checked the arches for behavior. The following do indeed return the IRQ state prior to enabling IRQs:

• Cortex-M
• RISC-V
• MSP-430
• ESP-Xtensa
• ESP-RISC-V
• AVR
• ARM7
• MIPS

So, fixed :)