Hoernchen has uploaded this change for review. ( https://gerrit.osmocom.org/c/simtrace2/+/43127?usp=email )
Change subject: firmware: protect uart_tx_queue against ISR ......................................................................
firmware: protect uart_tx_queue against ISR
cardem: - dispatch_usb_command_cardem() appends to uart_tx_queue from the main loop - tx_byte_tpdu() dequeues from the USART IRQ handler @ NVIC prio 0
card_handle_reset() has the same issue, drains queue and frees uart_tx_msg from main loop while the ISR may own them.
All of this needs protection against the irq.
Needs a fixed llist_add_tail_irqsafe(), which called __enable_irq() instead of restoring the saved PRIMASK for some unknown reason?!?!?!?
Change-Id: I7d9cdcc56263b27dfd4649dfb1da1d67761ee923 --- M firmware/libcommon/include/llist_irqsafe.h M firmware/libcommon/source/card_emu.c M firmware/libcommon/source/mode_cardemu.c 3 files changed, 21 insertions(+), 8 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/simtrace2 refs/changes/27/43127/1
diff --git a/firmware/libcommon/include/llist_irqsafe.h b/firmware/libcommon/include/llist_irqsafe.h index 8aafcfb..e6c893a 100644 --- a/firmware/libcommon/include/llist_irqsafe.h +++ b/firmware/libcommon/include/llist_irqsafe.h @@ -34,7 +34,7 @@
local_irq_save(x); llist_add_tail(_new, head); - __enable_irq(); + local_irq_restore(x); }
static inline struct llist_head *llist_head_dequeue_irqsafe(struct llist_head *head) diff --git a/firmware/libcommon/source/card_emu.c b/firmware/libcommon/source/card_emu.c index 31ba79c..98ce662 100644 --- a/firmware/libcommon/source/card_emu.c +++ b/firmware/libcommon/source/card_emu.c @@ -219,19 +219,31 @@ static void card_handle_reset(struct card_handle *ch) { struct msgb *msg; + unsigned long x;
card_emu_uart_update_wt(ch->uart_chan, 0);
- /* release any buffers we may still own */ - if (ch->uart_tx_msg) { - usb_buf_free(ch->uart_tx_msg); - ch->uart_tx_msg = NULL; - } + /* Release any buffers we may still own. + * uart_tx_msg + uart_tx_queue are shared with the UART IRQ handler, + * that preempts us here -> needs atomic detach and free */ + local_irq_save(x); + msg = ch->uart_tx_msg; + ch->uart_tx_msg = NULL; + local_irq_restore(x); + if (msg) + usb_buf_free(msg); + if (ch->uart_rx_msg) { usb_buf_free(ch->uart_rx_msg); ch->uart_rx_msg = NULL; } - while ((msg = msgb_dequeue(&ch->uart_tx_queue))) { + + while (1) { + local_irq_save(x); + msg = msgb_dequeue(&ch->uart_tx_queue); + local_irq_restore(x); + if (!msg) + break; usb_buf_free(msg); } } diff --git a/firmware/libcommon/source/mode_cardemu.c b/firmware/libcommon/source/mode_cardemu.c index 77f7454..a0014d2 100644 --- a/firmware/libcommon/source/mode_cardemu.c +++ b/firmware/libcommon/source/mode_cardemu.c @@ -737,7 +737,8 @@ switch (hdr->msg_type) { case SIMTRACE_MSGT_DT_CEMU_TX_DATA: queue = card_emu_get_uart_tx_queue(ci->ch); - llist_add_tail(&msg->list, queue); + /* drained from the USART IRQ handler at highest NVIC prio */ + llist_add_tail_irqsafe(&msg->list, queue); card_emu_have_new_uart_tx(ci->ch); break; case SIMTRACE_MSGT_DT_CEMU_SET_ATR: