serial: fix circular rx buffer edge case

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@prevas.dk>
Reviewed-by: Simon Glass <sjg@chromium.org>

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 84f02f7ac76a270c5836b3e5a0eef35a23f5702e..05fe9645bee198578181ca7cbc21b17b8537af78 100644 (file)
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -328,11 +328,12 @@ static int __serial_tstc(struct udevice *dev)
 static int _serial_tstc(struct udevice *dev)
 {
        struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
+       uint wr;
 
        /* Read all available chars into the RX buffer */
        while (__serial_tstc(dev)) {
-               upriv->buf[upriv->wr_ptr++] = __serial_getc(dev);
-               upriv->wr_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
+               wr = upriv->wr_ptr++ % CONFIG_SERIAL_RX_BUFFER_SIZE;
+               upriv->buf[wr] = __serial_getc(dev);
        }
 
        return upriv->rd_ptr != upriv->wr_ptr ? 1 : 0;
@@ -342,12 +343,13 @@ static int _serial_getc(struct udevice *dev)
 {
        struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
        char val;
+       uint rd;
 
        if (upriv->rd_ptr == upriv->wr_ptr)
                return __serial_getc(dev);
 
-       val = upriv->buf[upriv->rd_ptr++];
-       upriv->rd_ptr %= CONFIG_SERIAL_RX_BUFFER_SIZE;
+       rd = upriv->rd_ptr++ % CONFIG_SERIAL_RX_BUFFER_SIZE;
+       val = upriv->buf[rd];
 
        return val;
 }

diff --git a/include/serial.h b/include/serial.h
index d129dc3253ccb8cdf283e9a40dc723055cc44d69..14563239b7d917a9cc8bc5cfd6afb39d108f2658 100644 (file)
--- a/include/serial.h
+++ b/include/serial.h
@@ -299,8 +299,8 @@ struct serial_dev_priv {
        struct stdio_dev *sdev;
 
        char *buf;
-       int rd_ptr;
-       int wr_ptr;
+       uint rd_ptr;
+       uint wr_ptr;
 };
 
 /* Access the serial operations for a device */