Hoernchen has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-ccid-firmware/+/42319?usp=email )

Change subject: firmware: fix msgb/list mixed irq access handling ......................................................................

firmware: fix msgb/list mixed irq access handling

This fixes the mishandling of in_progress and the queues in the submit functions that are in part irq driven, too, and ensures nothing breaks by preventing concurrent access from irqs to the queues. Applying the same pattern to all functions/cases is fine either way.

On top of all of tha the out ep was irq driven, so if submit_next_out fails it was stuck, now fixed by attempted submit from main loop, and of course the queue manipulation should be irq safe here as well.

Change-Id: I9212bfa7688cb4d3161ba963b854225744128632 --- M sysmoOCTSIM/main.c 1 file changed, 43 insertions(+), 30 deletions(-)

git pull ssh://gerrit.osmocom.org:29418/osmo-ccid-firmware refs/changes/19/42319/1

diff --git a/sysmoOCTSIM/main.c b/sysmoOCTSIM/main.c index 92c5bbd..e1e83c7 100644 --- a/sysmoOCTSIM/main.c +++ b/sysmoOCTSIM/main.c @@ -159,19 +159,26 @@ static int submit_next_in(void) { struct usb_ep_q *ep_q = &g_ccid_s.in_ep; - struct msgb *msg; + struct msgb *msg = NULL; int rc;

- if (ep_q->in_progress) - return 0; + CRITICAL_SECTION_ENTER() + if (!ep_q->in_progress) { + msg = msgb_dequeue(&ep_q->list); + if (msg) + ep_q->in_progress = msg; + } + CRITICAL_SECTION_LEAVE()

- msg = msgb_dequeue_irqsafe(&ep_q->list); if (!msg) return 0;

- ep_q->in_progress = msg; rc = ccid_df_write_in(msgb_data(msg), msgb_length(msg)); if (rc != ERR_NONE) { + CRITICAL_SECTION_ENTER() + ep_q->in_progress = NULL; + msgb_enqueue(&g_ccid_s.free_q, msg); + CRITICAL_SECTION_LEAVE() printf("EP %s failed: %d\r\n", ep_q->name, rc); return -1; } @@ -183,22 +190,27 @@ static int submit_next_irq(void) { struct usb_ep_q *ep_q = &g_ccid_s.irq_ep; - struct msgb *msg; + struct msgb *msg = NULL; int rc;

- if (ep_q->in_progress) - return 0; + CRITICAL_SECTION_ENTER() + if (!ep_q->in_progress) { + msg = msgb_dequeue(&ep_q->list); + if (msg) + ep_q->in_progress = msg; + } + CRITICAL_SECTION_LEAVE()

- msg = msgb_dequeue_irqsafe(&ep_q->list); if (!msg) return 0;

- ep_q->in_progress = msg; rc = ccid_df_write_irq(msgb_data(msg), msgb_length(msg)); /* may return HALTED/ERROR/DISABLED/BUSY/ERR_PARAM/ERR_FUNC/ERR_DENIED */ if (rc != ERR_NONE) { - // ep_q->in_progress = msg; - // msgb_enqueue_irqsafe(&ep_q->list, msg); + CRITICAL_SECTION_ENTER() + ep_q->in_progress = NULL; + msgb_enqueue(&g_ccid_s.free_q, msg); + CRITICAL_SECTION_LEAVE() printf("EP %s failed: %d\r\n", ep_q->name, rc); return -1; } @@ -208,22 +220,29 @@ static int submit_next_out(void) { struct usb_ep_q *ep_q = &g_ccid_s.out_ep; - struct msgb *msg; + struct msgb *msg = NULL; int rc;

- if (ep_q->in_progress) - return 0; + CRITICAL_SECTION_ENTER() + if (!ep_q->in_progress) { + msg = msgb_dequeue(&g_ccid_s.free_q); + if (msg) { + msgb_reset(msg); + ep_q->in_progress = msg; + } + } + CRITICAL_SECTION_LEAVE()

- msg = msgb_dequeue_irqsafe(&g_ccid_s.free_q); if (!msg) - return -1; - msgb_reset(msg); - ep_q->in_progress = msg; + return 0;

rc = ccid_df_read_out(msgb_data(msg), msgb_tailroom(msg)); if (rc != ERR_NONE) { + CRITICAL_SECTION_ENTER() + ep_q->in_progress = NULL; /* re-add to the list of free msgb's */ - llist_add_tail_at(&msg->list, &g_ccid_s.free_q); + msgb_enqueue(&g_ccid_s.free_q, msg); + CRITICAL_SECTION_LEAVE() return 0; } return 1; @@ -494,7 +513,7 @@ OSMO_ASSERT(msg);

/* append to list of pending-to-be-handed messages */ - llist_add_tail_at(&msg->list, &g_ccid_s.in_ep.list); + msgb_enqueue_irqsafe(&g_ccid_s.in_ep.list, msg); submit_next_in(); return 0; } @@ -557,19 +576,12 @@ msgb_reset(msg); llist_add_tail_at(&msg->list, &g_ccid_s.free_q); } -#if 0 struct msgb *cur_msg = cur_epq->in_progress; if (cur_msg) { - /* - if (i == 2 && usb_d_ep_get_status(/*_ccid_df_funcd.func_ep_out*/ 2, 0) == USB_BUSY) - continue; - */ - msgb_reset(cur_msg); - llist_add_tail_at(&cur_msg->list, &g_ccid_s.free_q); + msgb_enqueue(&g_ccid_s.free_q, cur_msg); cur_epq->in_progress = NULL; } -#endif }

SysTick->CTRL |= SysTick_CTRL_ENABLE_Msk; @@ -715,7 +727,8 @@ struct msgb *msg = msgb_alloc(300, "ccid"); OSMO_ASSERT(msg); /* return the message back to the queue of free message buffers */ - llist_add_tail_at(&msg->list, &g_ccid_s.free_q); + msgb_enqueue_irqsafe(&g_ccid_s.free_q, msg); } + submit_next_out(); } }