From 47d3ffd1e1f73b95bee3dc4fb1281f60c184c245 Mon Sep 17 00:00:00 2001 From: Anirudh Sahu Date: Tue, 10 Nov 2026 20:28:44 +0530 Subject: [PATCH 1/4] fifo: keep overwritable fifo full and ordered on double overflow A single-byte tu_fifo_write() on a full overwritable fifo had no double-overflow guard, so 2*depth writes without a read made the write index alias the read index and the fifo report empty. The bulk path avoided the alias by repositioning the write index before the copy, which left stale bytes at the head of the buffer. Write at the write index as usual and drop one depth from the new index afterwards when the overflow count reaches 2*depth. Both paths share advance_write_index(); the write pointer is unchanged and the fifo holds the last depth items in order. --- src/common/tusb_fifo.c | 35 +++++++++------------ test/unit-test/test/test_fifo.c | 56 +++++++++++++++++++++++++++++++-- 2 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/common/tusb_fifo.c b/src/common/tusb_fifo.c index e5855fcbc0..7f100d97b3 100644 --- a/src/common/tusb_fifo.c +++ b/src/common/tusb_fifo.c @@ -411,6 +411,17 @@ static uint16_t advance_index(uint16_t depth, uint16_t idx, uint16_t offset) { return new_idx; } +// Advance write index by n after data is written. In overwritable mode, if the write would make the overflow count +// reach 2*depth (double overflow), the index would alias to an empty fifo. Drop one depth from it instead: the +// write pointer is unchanged and the fifo is still full, holding the last depth items in order. +static uint16_t advance_write_index(uint16_t depth, uint16_t wr_idx, uint16_t rd_idx, uint16_t n) { + const uint16_t new_idx = advance_index(depth, wr_idx, n); + if (tu_ff_overflow_count(depth, wr_idx, rd_idx) + n >= 2 * depth) { + return advance_index(depth, new_idx, depth); + } + return new_idx; +} + // index to pointer (0..depth-1), simply a modulo with minus. TU_ATTR_ALWAYS_INLINE static inline uint16_t idx2ptr(uint16_t depth, uint16_t idx) { // Only run at most 3 times since index is limit in the range of [0..2*depth) @@ -534,25 +545,9 @@ uint16_t tu_fifo_write_n_access_mode(tu_fifo_t *f, const void *data, uint16_t n, // We start writing at the read pointer's position since we fill the whole buffer wr_idx = rd_idx; - } else { - const uint16_t overflowable_count = tu_ff_overflow_count(f->depth, wr_idx, rd_idx); - if (overflowable_count + n >= 2 * f->depth) { - // Double overflowed - // Index is bigger than the allowed range [0,2*depth) - // re-position write index to have a full fifo after pushed - wr_idx = advance_index(f->depth, rd_idx, f->depth - n); - - // TODO we should also shift out n bytes from read index since we avoid changing rd index !! - // However memmove() is expensive due to actual copying + wrapping consideration. - // Also race condition could happen anyway if read() is invoke while moving result in corrupted memory - // currently deliberately not implemented --> result in incorrect data read back - } else { - // normal + single overflowed: - // Index is in the range of [0,2*depth) and thus detect and recoverable. Recovering is handled in read() - // Therefore we just increase write index - // we will correct (re-position) read index later on in fifo_read() function - } } + // Otherwise (n < depth) the data is written at the write index. Single overflow is recoverable (handled in + // read(), which re-positions the read index), double overflow is handled by advance_write_index() } if (n) { @@ -567,7 +562,7 @@ uint16_t tu_fifo_write_n_access_mode(tu_fifo_t *f, const void *data, uint16_t n, { ff_push_n(f, buf8, n, wr_ptr); } - f->wr_idx = advance_index(f->depth, wr_idx, n); + f->wr_idx = advance_write_index(f->depth, wr_idx, rd_idx, n); TU_LOG(TU_FIFO_DBG, "\tnew_wr = %u\r\n", f->wr_idx); } @@ -645,7 +640,7 @@ bool tu_fifo_write(tu_fifo_t *f, const void *data) { } else { const uint16_t wr_ptr = idx2ptr(f->depth, wr_idx); memcpy(f->buffer + wr_ptr, data, 1); - f->wr_idx = advance_index(f->depth, wr_idx, 1); + f->wr_idx = advance_write_index(f->depth, wr_idx, f->rd_idx, 1); ret = true; } diff --git a/test/unit-test/test/test_fifo.c b/test/unit-test/test/test_fifo.c index 30420a5279..967daca665 100644 --- a/test/unit-test/test/test_fifo.c +++ b/test/unit-test/test/test_fifo.c @@ -150,8 +150,60 @@ void test_write_double_overflowed(void) { TEST_ASSERT_EQUAL_MEMORY(buf - 16, rd_buf + FIFO_SIZE - 16, 16); - // TODO whole buffer should match, but we deliberately not implement it - // TEST_ASSERT_EQUAL_MEMORY(buf-FIFO_SIZE, rd_buf, FIFO_SIZE); + // whole buffer holds the last FIFO_SIZE bytes written, in order + TEST_ASSERT_EQUAL_MEMORY(buf - FIFO_SIZE, rd_buf, FIFO_SIZE); +} + +void test_write_overwritable_single_byte(void) { + tu_fifo_set_overwritable(ff, true); + + // keep writing one byte at a time, never reading: the fifo must stay full across several index wraps + for (uint16_t i = 0; i < 3 * FIFO_SIZE + 5; i++) { + TEST_ASSERT_TRUE(tu_fifo_write(ff, &test_data[i])); + TEST_ASSERT_EQUAL(tu_min16(i + 1, FIFO_SIZE), tu_fifo_count(ff)); + } + + // oldest bytes are gone, the last FIFO_SIZE bytes are read back in order + uint8_t rd[FIFO_SIZE] = {0}; + TEST_ASSERT_EQUAL(FIFO_SIZE, tu_fifo_read_n(ff, rd, FIFO_SIZE)); + TEST_ASSERT_EQUAL_MEMORY(test_data + 2 * FIFO_SIZE + 5, rd, FIFO_SIZE); + TEST_ASSERT_TRUE(tu_fifo_empty(ff)); +} + +void test_write_overwritable_mixed(void) { + tu_fifo_set_overwritable(ff, true); + + // deterministic mix of single and bulk writes (each below depth, so double overflows are not masked by whole-buffer rewrites) checked against the last FIFO_SIZE bytes + uint8_t stream[4 * FIFO_SIZE * 64]; + uint32_t total = 0; + uint32_t seed = 12345; + + for (int iter = 0; iter < 2000; iter++) { + seed = seed * 1103515245u + 12345u; + uint16_t n = (seed >> 16) % FIFO_SIZE; + for (uint16_t i = 0; i < n; i++) { + stream[total + i] = (uint8_t)(seed >> (i % 16)) ^ (uint8_t)i; + } + if (n == 1) { + tu_fifo_write(ff, &stream[total]); + } else { + tu_fifo_write_n(ff, &stream[total], n); + } + total += n; + + const uint16_t expected = tu_min16(total, FIFO_SIZE); + TEST_ASSERT_EQUAL(expected, tu_fifo_count(ff)); + + if (iter % 5 == 0) { + uint8_t rd[FIFO_SIZE] = {0}; + TEST_ASSERT_EQUAL(expected, tu_fifo_peek_n(ff, rd, FIFO_SIZE)); + TEST_ASSERT_EQUAL_MEMORY(stream + total - expected, rd, expected); + } + + if (total + FIFO_SIZE > sizeof(stream)) { + break; + } + } } static uint16_t help_write(uint16_t total, uint16_t n) { From 221e786a8be933b0891fcc4db9e67bb64be8ce09 Mon Sep 17 00:00:00 2001 From: Anirudh Sahu Date: Tue, 10 Nov 2026 20:28:44 +0530 Subject: [PATCH 2/4] docs: document data tearing of overwritable fifo An overwriting write only holds the write mutex and can overwrite bytes a reader is copying out. Note this in tusb_fifo.h and in the CDC and audio reference. --- docs/reference/class/audio.rst | 3 ++- docs/reference/class/cdc.rst | 5 ++++- src/common/tusb_fifo.h | 23 ++++++++--------------- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/docs/reference/class/audio.rst b/docs/reference/class/audio.rst index f2ca5178b2..f551256a04 100644 --- a/docs/reference/class/audio.rst +++ b/docs/reference/class/audio.rst @@ -93,7 +93,8 @@ Data path * - ``tud_audio_write()`` - Copies microphone bytes into the IN software FIFO and returns the number written. When the FIFO is full the oldest unsent bytes are overwritten, - so pace writes to the stream. + so pace writes to the stream. An overwrite can tear bytes the endpoint + is copying out at that moment (partly old, partly new data). * - ``tud_audio_clear_ep_*_ff()`` - Discards queued samples in the selected endpoint FIFO, useful when a streaming alternate setting closes. diff --git a/docs/reference/class/cdc.rst b/docs/reference/class/cdc.rst index 138ebdf3f5..1f1224b449 100644 --- a/docs/reference/class/cdc.rst +++ b/docs/reference/class/cdc.rst @@ -45,7 +45,10 @@ Enable ``CFG_TUD_CDC`` with the required port count and add one * - ``CFG_TUD_CDC_TX_OVERWRITABLE_IF_NOT_CONNECTED`` - ``1`` - Allows writes made before DTR connection to replace old queued data - rather than permanently filling the FIFO. + rather than permanently filling the FIFO. When the FIFO overflows the + oldest bytes are dropped, and bytes the endpoint is copying out at that + moment can be torn (partly old, partly new data); this affects only + data written while DTR is clear. Use speed-dependent values from an example when the device can enumerate at high speed. diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index c5a0aaaef8..15bdd35fee 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -80,22 +80,15 @@ extern "C" { * This occurs when we continue to write after 1st overflowed to 2nd overflowed. e.g: * write(3), write(1), write(2) * This must be prevented since it will cause unrecoverable state, in above example - * if not handled the fifo will be empty instead of continue-to-be full. Since we must not modify - * read index in write() function, which cause race condition. We will re-position write index so that - * after data is written it is a full fifo i.e W = depth - R + * if not handled the write index catches up with the read index and the fifo reads as empty instead of + * continue-to-be full. Since we must not modify read index in write() function, which cause race condition, + * the data is written at the write index as usual, then the write index is moved back by one depth i.e it + * still points to the same location. The fifo is full and holds the last depth items in order. * - * re-position W = 1 before write(2) - * Note: we should also move data from mem[3] to read index as well, but deliberately skipped here - * since it is an expensive operation !!! - * | - * ------------------------- - * | R | W | 2 | 3 | 4 | 5 | - * - * perform write(2), result is still a full fifo. - * - * | - * ------------------------- - * | R | 1 | 2 | W | 4 | 5 | + * Overwritable mode and concurrency: an overwriting write() only holds the write mutex, and it may overwrite items + * that a concurrent read()/peek() (holding the read mutex or running in another context e.g. ISR) is copying out. + * The indices stay consistent but the data read in that case can be torn i.e a mix of old and new items. + * Use overwritable mode only when losing/tearing the oldest data on overflow is acceptable. */ typedef struct { uint8_t *buffer; // buffer pointer From 669403f681b0e0699a555d0dc2a0f790aa1daf76 Mon Sep 17 00:00:00 2001 From: Anirudh Sahu Date: Tue, 10 Nov 2026 20:35:33 +0530 Subject: [PATCH 3/4] fifo: wrap long comment in test_fifo --- test/unit-test/test/test_fifo.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit-test/test/test_fifo.c b/test/unit-test/test/test_fifo.c index 967daca665..e279b62ab6 100644 --- a/test/unit-test/test/test_fifo.c +++ b/test/unit-test/test/test_fifo.c @@ -173,7 +173,8 @@ void test_write_overwritable_single_byte(void) { void test_write_overwritable_mixed(void) { tu_fifo_set_overwritable(ff, true); - // deterministic mix of single and bulk writes (each below depth, so double overflows are not masked by whole-buffer rewrites) checked against the last FIFO_SIZE bytes + // deterministic mix of single and bulk writes, checked against the last FIFO_SIZE bytes. + // Each write is below depth so double overflows are not masked by whole-buffer rewrites. uint8_t stream[4 * FIFO_SIZE * 64]; uint32_t total = 0; uint32_t seed = 12345; From f62a491e9913c4190d1a29c9aa836c8ce4c67081 Mon Sep 17 00:00:00 2001 From: Anirudh Sahu Date: Thu, 12 Nov 2026 10:05:31 +0530 Subject: [PATCH 4/4] fifo: document overwrite/read index race, check write results in test --- src/common/tusb_fifo.h | 4 +++- test/unit-test/test/test_fifo.c | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/common/tusb_fifo.h b/src/common/tusb_fifo.h index 15bdd35fee..f56fb53f99 100644 --- a/src/common/tusb_fifo.h +++ b/src/common/tusb_fifo.h @@ -87,7 +87,9 @@ extern "C" { * * Overwritable mode and concurrency: an overwriting write() only holds the write mutex, and it may overwrite items * that a concurrent read()/peek() (holding the read mutex or running in another context e.g. ISR) is copying out. - * The indices stay consistent but the data read in that case can be torn i.e a mix of old and new items. + * The data read in that case can be torn i.e a mix of old and new items. The write index is also derived from a + * snapshot of the read index, so if read()/peek() re-positions the read index meanwhile, the item count can end up + * wrong and some items may be skipped or replayed. A write that does not overflow is not affected. * Use overwritable mode only when losing/tearing the oldest data on overflow is acceptable. */ typedef struct { diff --git a/test/unit-test/test/test_fifo.c b/test/unit-test/test/test_fifo.c index e279b62ab6..c0f82bdf86 100644 --- a/test/unit-test/test/test_fifo.c +++ b/test/unit-test/test/test_fifo.c @@ -186,9 +186,9 @@ void test_write_overwritable_mixed(void) { stream[total + i] = (uint8_t)(seed >> (i % 16)) ^ (uint8_t)i; } if (n == 1) { - tu_fifo_write(ff, &stream[total]); + TEST_ASSERT_TRUE(tu_fifo_write(ff, &stream[total])); } else { - tu_fifo_write_n(ff, &stream[total], n); + TEST_ASSERT_EQUAL(n, tu_fifo_write_n(ff, &stream[total], n)); } total += n;