[PATCH 1/4] serial: fix circular rx buffer edge case

[Simon Glass]

On Thu, 3 Oct 2024 at 08:10, Rasmus Villemoes <ravi at 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 <ravi at prevas.dk>
> ---
>  drivers/serial/serial-uclass.c | 10 ++++++----
>  include/serial.h               |  4 ++--
>  2 files changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

Perhaps we should use membuff, like in other cases, since it has some tests?