lynxis lazus has submitted this change. ( https://gerrit.osmocom.org/c/simtrace2/+/43127?usp=email ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )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(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve lynxis lazus: Looks good to me, approved 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 9c90850..a1d4483 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 3a0808d..6c1ac8d 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: -- To view, visit https://gerrit.osmocom.org/c/simtrace2/+/43127?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: simtrace2 Gerrit-Branch: master Gerrit-Change-Id: I7d9cdcc56263b27dfd4649dfb1da1d67761ee923 Gerrit-Change-Number: 43127 Gerrit-PatchSet: 3 Gerrit-Owner: Hoernchen <ewild@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-Reviewer: lynxis lazus <lynxis@fe80.eu>