On Thu, 3 Oct 2024 at 08:10, Rasmus Villemoes <r...@prevas.dk> wrote: > > The current implementation of the circular rx buffer falls into a > common trap with circular buffers: It keeps the head/tail indices > reduced modulo the buffer size. The problem with that is that it makes > it impossible to distinguish "buffer full" from "buffer empty", > because in both situations one has head==tail. > > This can easily be demonstrated: Build sandbox with RX_BUFFER enabled, > set the RX_BUFFER_SIZE to 32, and try pasting the string > > 01234567890123456789012345678901 > > Nothing seems to happen, but in reality, all characters have been read > and put into the buffer, but then tstc ends up believing nothing is in > the buffer anyway because upriv->rd_ptr == upriv->wr_ptr. > > A better approach is to let the indices be free-running, and only > reduce them modulo the buffer size when accessing the array. Then > "empty" is head-tail==0 and "full" is head-tail==size. This does rely > on the buffer size being a power-of-two and the free-running > indices simply wrapping around to 0 when incremented beyond the > maximal positive value. > > Incidentally, that change from signed to unsigned int also improves > code generation quite a bit: In C, (signed int)%(signed int) is > defined to have the sign of the dividend (so (-35) % 32 is -3, not > 29), and hence despite the modulus being a power-of-two, x % 32 does > not actually compile to the same as a simple x & 31 - on x86 with -Os, > it seems that gcc ends up emitting an idiv instruction, which is quite > expensive. > > Signed-off-by: Rasmus Villemoes <r...@prevas.dk> > --- > drivers/serial/serial-uclass.c | 10 ++++++---- > include/serial.h | 4 ++-- > 2 files changed, 8 insertions(+), 6 deletions(-)
Reviewed-by: Simon Glass <s...@chromium.org> Perhaps we should use membuff, like in other cases, since it has some tests?
