core/msg: use disable/restoreIRQ #1887
Conversation
LudwigKnuepfer
added
Area: core
|
@kaspar030 Do you think it's worthwhile to fix the old module now? I assume it might take a while before #1890 gets merged. |
|
@kaspar030 ping |
|
This patch is needed in the meantime! I just stumbled upon the fact that msg_send() activates interrupts without me asking it to do so. |
|
Ok, let's get this in. +1 for the concept, looks OK to me, have no time to test at the moment. |
LudwigKnuepfer
force-pushed
the
msg_new_irq_api
branch
from
cc84edf
to
116592f
Compare
|
rebased, comment addressed |
|
(Now it might be confused with |
int msg_send(msg_t *m, kernel_pid_t target_pid)
{
return _msg_send(m, target_pid, true, disableIRQ());
}
int msg_try_send(msg_t *m, kernel_pid_t target_pid)
{
return _msg_send(m, target_pid, false, disableIRQ());
}
static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block, unsigned state)
{
....
} |
|
Yes, that would have been the right thing to do from the start =) |
LudwigKnuepfer
added
the
CI: needs squashing
| tcb_t *me = (tcb_t*) sched_threads[sched_active_pid]; | ||
| sched_set_status(me, STATUS_REPLY_BLOCKED); | ||
| me->wait_data = (void*) reply; | ||
|
|
||
| /* msg_send blocks until reply received */ | ||
|
|
||
| return msg_send(m, target_pid); | ||
| return _msg_send(m, target_pid, true, state); |
There was a problem hiding this comment.
After more thinking, I'm not sure this works as expected.
This will restore IRQ state on reply reception to the state before sending the request.
This will disable IRQ, then switch to other threads. Whatever they do, IRQs are disabled. Imagine a hwtimer_sleep() in the thread receiving the msg_send_received message, that would never return.
I think we have to just protect this function with a pair of disable/restore and then call the old msg_send.
Don't know if it can cause problems if active_thread is REPLY_BLOCKED, though...
There was a problem hiding this comment.
Is that something we want to fix in a separate PR? At least this PR fixes things as it is (and this particular issue is not introduced by it).
There was a problem hiding this comment.
I don't understand the problem, @kaspar030, can you try a different wording, plz?
There was a problem hiding this comment.
@kaspar030 reading _msg_send() you'll notice that every exit/control hand off is prefixed with a restoreIRQ. Did you overlook this?
There was a problem hiding this comment.
@LudwigOrtmann You are right. Sorry. ACK!
LudwigKnuepfer
force-pushed
the
msg_new_irq_api
branch
from
8884e1c
to
c21fdc8
Compare
|
Squashed. |
LudwigKnuepfer
removed
the
CI: needs squashing
| } | ||
|
|
||
| static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block) | ||
| static int _msg_send(msg_t *m, kernel_pid_t target_pid, bool block, unsigned state) | ||
| { | ||
| if (inISR()) { | ||
| return msg_send_int(m, target_pid); |
There was a problem hiding this comment.
what about this return? By calling _msg_send interrupts are disabled (even if we are inside an ISR). But returning here just leaves them disabled...
There was a problem hiding this comment.
Yep, this error was introduced with the application of #1887 (comment) ... addressed now.
LudwigKnuepfer
added
the
CI: needs squashing
|
|
||
| int msg_try_send(msg_t *m, kernel_pid_t target_pid) | ||
| { | ||
| return _msg_send(m, target_pid, false, disableIRQ()); |
There was a problem hiding this comment.
I think msg_try_send() still needs the inISR() test.
There was a problem hiding this comment.
Yep, I just noticed myself.
|
@Kijewski @haukepetersen @kaspar030 shall I squash? ACK? |
|
ACK after squashing. |
* needed to change internal `msg_send` to allow external disabling of interrupts
LudwigKnuepfer
force-pushed
the
msg_new_irq_api
branch
from
d677543
to
b4d9c9e
Compare
LudwigKnuepfer
removed
the
CI: needs squashing
|
ACK |
core/msg: use disable/restoreIRQ
msg_sendto allow external disabling of interruptsDisclaimer:This is not beautiful, but I thought it's better than the status quo and might serve as a starting point for discussion.