laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35326?usp=email )
Change subject: soft_uart: demonstrate a problem with manual flush()ing
......................................................................
soft_uart: demonstrate a problem with manual flush()ing
This problem can only happen if the user is flush()ing the Rx buffer
manually by calling osmo_soft_uart_flush_rx(). Let's demonstrate it
in the unit test, so that we don't forget about it (add FIXME).
Change-Id: Iad932a505d6fd98360f90510651501f8708ff5d2
---
M tests/soft_uart/soft_uart_test.c
M tests/soft_uart/soft_uart_test.ok
2 files changed, 37 insertions(+), 0 deletions(-)
Approvals:
laforge: Looks good to me, approved
Jenkins Builder: Verified
pespin: Looks good to me, but someone else must approve
diff --git a/tests/soft_uart/soft_uart_test.c b/tests/soft_uart/soft_uart_test.c
index e9adb29..0ea7fa8 100644
--- a/tests/soft_uart/soft_uart_test.c
+++ b/tests/soft_uart/soft_uart_test.c
@@ -226,6 +226,7 @@
static void test_rx_flush(void)
{
+ struct osmo_soft_uart_cfg cfg;
struct osmo_soft_uart *suart;
SUART_TEST_BEGIN;
@@ -242,6 +243,25 @@
printf("calling osmo_soft_uart_flush_rx() while Rx enabled, but no data\n");
osmo_soft_uart_flush_rx(suart);
+ /* FIXME: this scenario demonstrates a problem that may occur when the user
+ * flushes the Rx buffer manually while the soft-UART state reflects flags
+ * of an incomplete symbol, for which we're waiting the stop bit. */
+ printf("testing corner case: manual flushing during a parity error (8-E-1)\n");
+ cfg = suart_test_default_cfg;
+ cfg.parity_mode = OSMO_SUART_PARITY_EVEN;
+ osmo_soft_uart_configure(suart, &cfg);
+ test_rx_exec(suart, "1111111" /* no data */
+ "0 01010101 0 1" /* even parity, correct */
+ "0 10101010 0 1" /* even parity, correct */
+ "0 11111111 1" /* odd parity, incorrect, but stop bit is pending */
+ "F" /* manual flush happens before receiving the stop bit */
+ "1" /* finally, the stop bit is received */
+ );
+ /* test_rx_exec() @ 47: flush the Rx buffer
+ * suart_rx_cb(flags=02): aa 55 <--- this is wrong, should be flags=00
+ * suart_rx_cb(flags=02): ff <--- this is expected due to odd parity */
+
+
osmo_soft_uart_free(suart);
}
diff --git a/tests/soft_uart/soft_uart_test.ok b/tests/soft_uart/soft_uart_test.ok
index c42d0f5..baf3483 100644
--- a/tests/soft_uart/soft_uart_test.ok
+++ b/tests/soft_uart/soft_uart_test.ok
@@ -71,6 +71,10 @@
calling osmo_soft_uart_flush_rx() while Rx disabled
enabling the receiver
calling osmo_soft_uart_flush_rx() while Rx enabled, but no data
+testing corner case: manual flushing during a parity error (8-E-1)
+test_rx_exec() @ 47: flush the Rx buffer
+suart_rx_cb(flags=02): aa 55
+suart_rx_cb(flags=02): ff
Executing test_tx_rx
======== testing 8-N-1
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35326?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Iad932a505d6fd98360f90510651501f8708ff5d2
Gerrit-Change-Number: 35326
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email )
Change subject: soft_uart: fix the Rx flushing logic, add a unit test
......................................................................
soft_uart: fix the Rx flushing logic, add a unit test
Coverity tells us that with the current logic it's possible (in theory)
that we may dereference NULL pointer in osmo_soft_uart_flush_rx(). This
is highly unlikely, because the Rx buffer gets allocated once when the
Rx is enabled and remains even after the Rx gets disabled. The Rx flags
cannot be anything than 0x00 before the Rx gets enabled.
Even though this NULL pointer dereference is unlikely, the Rx flushing
logic is still not entirely correct. As can be seen from the unit test
output, the Rx callback of the application may be called with an empty
msgb if the following conditions are both met:
a) the osmo_soft_uart_flush_rx() is invoked manually, and
b) a parity and/or a framing error has occurred previously.
We should not be checking suart->rx.flags in osmo_soft_uart_flush_rx(),
since this is already done in suart_rx_ch(), which is calling it.
Removing this check also eliminates a theoretical possibility of the
NULL pointer dereference, so we're killing two birds with one stone.
- Do not check suart->rx.flags in osmo_soft_uart_flush_rx().
- Add a unit test for various flush()ing scenarios.
Change-Id: I5179f5fd2361e4e96ac9bf48e80b99e53a7e4712
Fixes: CID#336545
---
M src/core/soft_uart.c
M tests/soft_uart/soft_uart_test.c
M tests/soft_uart/soft_uart_test.ok
3 files changed, 60 insertions(+), 3 deletions(-)
Approvals:
pespin: Looks good to me, but someone else must approve
Jenkins Builder: Verified
laforge: Looks good to me, approved
diff --git a/src/core/soft_uart.c b/src/core/soft_uart.c
index c6a6dbd..f969ab7 100644
--- a/src/core/soft_uart.c
+++ b/src/core/soft_uart.c
@@ -79,7 +79,7 @@
* \param[in] suart soft-UART instance holding the receive buffer. */
void osmo_soft_uart_flush_rx(struct osmo_soft_uart *suart)
{
- if ((suart->rx.msg && msgb_length(suart->rx.msg)) || suart->rx.flags) {
+ if (suart->rx.msg && msgb_length(suart->rx.msg)) {
osmo_timer_del(&suart->rx.timer);
if (suart->cfg.rx_cb) {
suart->cfg.rx_cb(suart->cfg.priv, suart->rx.msg, suart->rx.flags);
diff --git a/tests/soft_uart/soft_uart_test.c b/tests/soft_uart/soft_uart_test.c
index 8c12dcd..e9adb29 100644
--- a/tests/soft_uart/soft_uart_test.c
+++ b/tests/soft_uart/soft_uart_test.c
@@ -224,6 +224,27 @@
osmo_soft_uart_free(suart);
}
+static void test_rx_flush(void)
+{
+ struct osmo_soft_uart *suart;
+
+ SUART_TEST_BEGIN;
+
+ suart = osmo_soft_uart_alloc(NULL, __func__, &suart_test_default_cfg);
+ OSMO_ASSERT(suart != NULL);
+
+ printf("calling osmo_soft_uart_flush_rx() while Rx disabled\n");
+ osmo_soft_uart_flush_rx(suart);
+
+ printf("enabling the receiver\n");
+ osmo_soft_uart_set_rx(suart, true);
+
+ printf("calling osmo_soft_uart_flush_rx() while Rx enabled, but no data\n");
+ osmo_soft_uart_flush_rx(suart);
+
+ osmo_soft_uart_free(suart);
+}
+
static void test_tx_rx_exec_one(struct osmo_soft_uart *suart,
size_t n_bits_total, size_t n_bits_frame)
{
@@ -566,6 +587,7 @@
int main(int argc, char **argv)
{
test_rx();
+ test_rx_flush();
test_tx_rx();
/* test pulling small number of bits at a time */
diff --git a/tests/soft_uart/soft_uart_test.ok b/tests/soft_uart/soft_uart_test.ok
index 6edac16..c42d0f5 100644
--- a/tests/soft_uart/soft_uart_test.ok
+++ b/tests/soft_uart/soft_uart_test.ok
@@ -51,7 +51,6 @@
suart_rx_cb(flags=02): 01
suart_rx_cb(flags=02): ff
test_rx_exec() @ 49: flush the Rx buffer
-suart_rx_cb(flags=02):
======== testing 8-E-1 (valid parity)
test_rx_exec() @ 63: flush the Rx buffer
suart_rx_cb(flags=00): 00 ff aa 55
@@ -62,13 +61,17 @@
suart_rx_cb(flags=02): 01
suart_rx_cb(flags=02): ff
test_rx_exec() @ 42: flush the Rx buffer
-suart_rx_cb(flags=02):
======== testing 8-O-1 (valid parity)
test_rx_exec() @ 63: flush the Rx buffer
suart_rx_cb(flags=00): 00 ff aa 55
test_rx_exec() @ 120: flush the Rx buffer
suart_rx_cb(flags=00): 80 e0 f8 fe
+Executing test_rx_flush
+calling osmo_soft_uart_flush_rx() while Rx disabled
+enabling the receiver
+calling osmo_soft_uart_flush_rx() while Rx enabled, but no data
+
Executing test_tx_rx
======== testing 8-N-1
suart_tx_cb(len=4/4): de ad be ef
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5179f5fd2361e4e96ac9bf48e80b99e53a7e4712
Gerrit-Change-Number: 35297
Gerrit-PatchSet: 6
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35327?usp=email )
Change subject: soft_uart: demonstrate a problem with inefficient polling
......................................................................
soft_uart: demonstrate a problem with inefficient polling
As outlined in the test case, we pull a total of 50 bits from the
transmitter in two rounds, pulling 25 bits at a time. In the default
8-N-1 configuration, 50 bits should ideally comprise 5 characters.
However, as observed, only a total of 4 characters are retrieved
from the application, leaving the remaining 10 bits (5 + 5) unused.
Change-Id: Ic2539681a4adf6c1822e0bc256e4c829813d0e21
---
M tests/soft_uart/soft_uart_test.c
M tests/soft_uart/soft_uart_test.ok
2 files changed, 54 insertions(+), 0 deletions(-)
Approvals:
Jenkins Builder: Verified
laforge: Looks good to me, approved
pespin: Looks good to me, but someone else must approve
diff --git a/tests/soft_uart/soft_uart_test.c b/tests/soft_uart/soft_uart_test.c
index 0ea7fa8..7280bdc 100644
--- a/tests/soft_uart/soft_uart_test.c
+++ b/tests/soft_uart/soft_uart_test.c
@@ -604,6 +604,37 @@
osmo_soft_uart_free(suart);
}
+static void test_tx_pull(void)
+{
+ struct osmo_soft_uart *suart;
+ ubit_t tx_buf[25 * 2];
+ int rc;
+
+ SUART_TEST_BEGIN;
+
+ g_tx_cb_cfg.data = (void *)"\x42\x42\x42\x42\x42";
+ g_tx_cb_cfg.data_len = 5;
+
+ suart = osmo_soft_uart_alloc(NULL, __func__, &suart_test_default_cfg);
+ OSMO_ASSERT(suart != NULL);
+
+ osmo_soft_uart_set_tx(suart, true);
+
+ printf("pulling 25 bits (first time) out of the transmitter\n");
+ rc = osmo_soft_uart_tx_ubits(suart, &tx_buf[0], sizeof(tx_buf) / 2);
+ OSMO_ASSERT(rc == 25);
+
+ printf("pulling 25 bits (second time) out of the transmitter\n");
+ rc = osmo_soft_uart_tx_ubits(suart, &tx_buf[25], sizeof(tx_buf) / 2);
+ OSMO_ASSERT(rc == 25);
+
+ /* FIXME: we pull total 25 + 25 == 50 bits out of the transmitter, which is enough
+ * to fit 5 characters (assuming 8-N-1). However, the current impelementation would
+ * pull only 2 + 2 == characters total, wasting 5 + 5 == 10 bits for padding. */
+
+ osmo_soft_uart_free(suart);
+}
+
int main(int argc, char **argv)
{
test_rx();
@@ -616,6 +647,8 @@
test_tx_rx_pull_n(4);
test_tx_rx_pull_n(8);
+ test_tx_pull();
+
/* test flow control */
test_modem_status();
test_flow_control_dtr_dsr();
diff --git a/tests/soft_uart/soft_uart_test.ok b/tests/soft_uart/soft_uart_test.ok
index baf3483..dcd7ceb 100644
--- a/tests/soft_uart/soft_uart_test.ok
+++ b/tests/soft_uart/soft_uart_test.ok
@@ -220,6 +220,12 @@
======== feeding 32 bits into the receiver
suart_rx_cb(flags=00): 55 55
+Executing test_tx_pull
+pulling 25 bits (first time) out of the transmitter
+suart_tx_cb(len=2/2): 42 42
+pulling 25 bits (second time) out of the transmitter
+suart_tx_cb(len=2/2): 42 42
+
Executing test_modem_status
initial status=0x00000000
de-asserting DCD, which was not asserted
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35327?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ic2539681a4adf6c1822e0bc256e4c829813d0e21
Gerrit-Change-Number: 35327
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35327?usp=email )
Change subject: soft_uart: demonstrate a problem with inefficient polling
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35327?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ic2539681a4adf6c1822e0bc256e4c829813d0e21
Gerrit-Change-Number: 35327
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 11 Dec 2023 20:49:18 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35326?usp=email )
Change subject: soft_uart: demonstrate a problem with manual flush()ing
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35326?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Iad932a505d6fd98360f90510651501f8708ff5d2
Gerrit-Change-Number: 35326
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 11 Dec 2023 20:48:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email )
Change subject: soft_uart: fix the Rx flushing logic, add a unit test
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35297?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I5179f5fd2361e4e96ac9bf48e80b99e53a7e4712
Gerrit-Change-Number: 35297
Gerrit-PatchSet: 5
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 11 Dec 2023 20:48:39 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment