Hoernchen has uploaded this change for review.
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();
}
}
To view, visit change 42319. To unsubscribe, or for help writing mail filters, visit settings.