From e698608680d1250ea50213551ce7c2b296529930 Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Thu, 3 Oct 2024 16:10:26 +0200 Subject: 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 Reviewed-by: Simon Glass --- include/serial.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/serial.h b/include/serial.h index d129dc3253c..14563239b7d 100644 --- 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 */ -- cgit v1.2.3 From 6cc6a2f6992ebe0c087a0da29d1ded3f8799d6ca Mon Sep 17 00:00:00 2001 From: Rasmus Villemoes Date: Thu, 3 Oct 2024 16:10:29 +0200 Subject: serial: embed the rx buffer in struct serial_dev_priv The initialization of upriv->buf doesn't check for a NULL return. But there's actually no point in doing a separate, unconditional malloc() in post_probe; we can just make serial_dev_priv contain the rx buffer itself, and let the (larger) allocation be handled by the driver core when it allocates the ->per_device_auto. The total run-time memory used is mostly the same, we reduce the code size a little, and as a bonus, struct serial_dev_priv does not contain the unused members when !SERIAL_RX_BUFFER. Signed-off-by: Rasmus Villemoes Reviewed-by: Simon Glass --- include/serial.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/serial.h b/include/serial.h index 14563239b7d..eabc49f820f 100644 --- a/include/serial.h +++ b/include/serial.h @@ -298,9 +298,11 @@ struct dm_serial_ops { struct serial_dev_priv { struct stdio_dev *sdev; - char *buf; +#if CONFIG_IS_ENABLED(SERIAL_RX_BUFFER) + char buf[CONFIG_SERIAL_RX_BUFFER_SIZE]; uint rd_ptr; uint wr_ptr; +#endif }; /* Access the serial operations for a device */ -- cgit v1.2.3