drivers/periph_common/timer: protect timer_set from IRQs #13060
Conversation
MichelRottleuthner
added
Type: bug
MichelRottleuthner
requested review from
haukepetersen,
MrKevinWeiss and
fjmolinas
fjmolinas
added
CI: run tests
|
I've been doing this in ztimer for a while without problems. Note that this does not prevent underflows for very small values. E.g., samr21-xpro still underflows for values <8. |
Yeah, either within protecting it like you did in ztimer or actually inside of the irq disabled block of this function would be the only reasonable way to do such a check, to not suffer from possible de-scheduling. |
drivers/periph_common/timer.c
Outdated
|
|
||
| #ifndef PERIPH_TIMER_PROVIDES_SET | ||
| int timer_set(tim_t dev, int channel, unsigned int timeout) | ||
| { | ||
| return timer_set_absolute(dev, channel, timer_read(dev) + timeout); | ||
| unsigned int state = irq_disable(); | ||
| unsigned int base = timer_read(dev); |
There was a problem hiding this comment.
Is it necessary to use an additional local variable to read the current timer value when no further processing of the current time is taking place, such as testing for underflows?
It's just a question, the compiler will probably generate the same code and keep the current timer value in a register and not require additional stack space.
There was a problem hiding this comment.
Yeah I think the compiler will be smarter than me here. There is absolutely no reason to not make it explicitly without this variable. Updated it and squashed directly, thanks!
There was a problem hiding this comment.
Hah, that was a style nit I chose not to mention. 😉 I also like it much better now.
MichelRottleuthner
force-pushed
the
pr_periph_timer_irq_safe
branch
from
32a652e to
8e7a1ed
Compare
|
@kaspar030 I assume your ACK is still valid? Ready to merge? |
Yes! |
Contribution description
This fixes a concurrency problem in the timer_set implementation.
Somewhere between getting the current time from timer_read(), adding the timeout, and then actually writing it to the timer a thread might be de-scheduled causing the targeted time to be already passed.
Testing procedure
Reading the code + thinking ;)
Running
tests/periph_timerandtests/bench_timerbut that didn't really show errors before...Issues/PRs references