net/can/j1939/transport.c | 48 ++- tools/testing/selftests/net/can/.gitignore | 1 + tools/testing/selftests/net/can/Makefile | 8 +- tools/testing/selftests/net/can/config | 1 + .../testing/selftests/net/can/test_cts_hold.c | 322 ++++++++++++++++++ .../selftests/net/can/test_cts_hold.sh | 45 +++ 6 files changed, 406 insertions(+), 19 deletions(-) create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
The J1939 protocol allows the receiver of directed segemented messages
to hold the data transfer. The kernel implementation did not handle hold
messages correctly was not able to resume from a hold.
Fix J1939 RTS/CTS session not being able to resume from hold.
Replace hardcoded timeout with define.
Add CTS hold behavior tests.
Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
---
Compared to the last patch I removed all of the todo comments I had
still had in the code. I will implement hold notification in the error
queue in different patch.
I replaced the hardcoded hold timeout with a define. I can replace the
other hardcoded timeouts in a different patch if you want me to, but
for now I only touched code related to the holds.
I also added a helper function to check if a CTS is a hold.
I also added a baseline test case as you wanted. Altough in the future
it should probably moved to another file which specfically tests normal
RTS/CTS behavior.
Also addressed all the comments Sashiko had on the test file and I'm
also now sending an EOMA in the test as without it between every test
there was a 1250ms wait until the session timed out...
net/can/j1939/transport.c | 48 ++-
tools/testing/selftests/net/can/.gitignore | 1 +
tools/testing/selftests/net/can/Makefile | 8 +-
tools/testing/selftests/net/can/config | 1 +
.../testing/selftests/net/can/test_cts_hold.c | 322 ++++++++++++++++++
.../selftests/net/can/test_cts_hold.sh | 45 +++
6 files changed, 406 insertions(+), 19 deletions(-)
create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c
create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index df93d57907da..a959272f32b2 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -32,6 +32,11 @@
#define J1939_ETP_CMD_EOMA 0x17
#define J1939_ETP_CMD_ABORT 0xff
+/* Time until session invalidation upon reception of a hold message.
+ * Corresponds to T4 in the specification.
+ */
+#define J1939_CTS_HOLD_TIMEOUT_MS 1050
+
enum j1939_xtp_abort {
J1939_XTP_NO_ABORT = 0,
J1939_XTP_ABORT_BUSY = 1,
@@ -1428,6 +1433,11 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
j1939_session_put(session);
}
+static inline bool j1939_cts_is_hold(const struct sk_buff *skb)
+{
+ return (!skb->data[1]);
+}
+
static void
j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
{
@@ -1442,9 +1452,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
- if (session->last_cmd == dat[0]) {
- err = J1939_XTP_ABORT_DUP_SEQ;
- goto out_session_cancel;
+ session->last_cmd = dat[0];
+
+ if (j1939_cts_is_hold(skb)) {
+ if (session->transmission)
+ j1939_session_txtimer_cancel(session);
+
+ j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
+ netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
+ return;
}
if (session->skcb.addr.type == J1939_ETP)
@@ -1457,7 +1473,11 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
else if (dat[1] > session->pkt.block /* 0xff for etp */)
goto out_session_cancel;
- /* set packet counters only when not CTS(0) */
+ if (session->pkt.tx_acked >= pkt) {
+ err = J1939_XTP_ABORT_DUP_SEQ;
+ goto out_session_cancel;
+ }
+
session->pkt.tx_acked = pkt - 1;
j1939_session_skb_drop_old(session);
session->pkt.last = session->pkt.tx_acked + dat[1];
@@ -1467,19 +1487,13 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
/* TODO: do not set tx here, do it in txtimer */
session->pkt.tx = session->pkt.tx_acked;
- session->last_cmd = dat[0];
- if (dat[1]) {
- j1939_tp_set_rxtimeout(session, 1250);
- if (session->transmission) {
- if (session->pkt.tx_acked)
- j1939_sk_errqueue(session,
- J1939_ERRQUEUE_TX_SCHED);
- j1939_session_txtimer_cancel(session);
- j1939_tp_schedule_txtimer(session, 0);
- }
- } else {
- /* CTS(0) */
- j1939_tp_set_rxtimeout(session, 550);
+ j1939_tp_set_rxtimeout(session, 1250);
+ if (session->transmission) {
+ if (session->pkt.tx_acked)
+ j1939_sk_errqueue(session,
+ J1939_ERRQUEUE_TX_SCHED);
+ j1939_session_txtimer_cancel(session);
+ j1939_tp_schedule_txtimer(session, 0);
}
return;
diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
index 764a53fc837f..96ef18ae986d 100644
--- a/tools/testing/selftests/net/can/.gitignore
+++ b/tools/testing/selftests/net/can/.gitignore
@@ -1,2 +1,3 @@
# SPDX-License-Identifier: GPL-2.0-only
test_raw_filter
+test_cts_hold
\ No newline at end of file
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
index 5b82e60a03e7..182346682bce 100644
--- a/tools/testing/selftests/net/can/Makefile
+++ b/tools/testing/selftests/net/can/Makefile
@@ -4,8 +4,12 @@ top_srcdir = ../../../../..
CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
-TEST_PROGS := test_raw_filter.sh
+TEST_PROGS := \
+ test_raw_filter.sh \
+ test_cts_hold.sh \
-TEST_GEN_FILES := test_raw_filter
+TEST_GEN_FILES := \
+ test_raw_filter \
+ test_cts_hold \
include ../../lib.mk
diff --git a/tools/testing/selftests/net/can/config b/tools/testing/selftests/net/can/config
index 188f79796670..cb538ed93ae4 100644
--- a/tools/testing/selftests/net/can/config
+++ b/tools/testing/selftests/net/can/config
@@ -1,3 +1,4 @@
CONFIG_CAN=m
CONFIG_CAN_DEV=m
CONFIG_CAN_VCAN=m
+CONFIG_CAN_J1939=m
\ No newline at end of file
diff --git a/tools/testing/selftests/net/can/test_cts_hold.c b/tools/testing/selftests/net/can/test_cts_hold.c
new file mode 100644
index 000000000000..2b2dffd5d5ef
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_cts_hold.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <net/if.h>
+#include <linux/if.h>
+
+#include <linux/can.h>
+#include <linux/can/raw.h>
+#include <linux/can/j1939.h>
+
+#include "kselftest_harness.h"
+
+
+#define SENDER_ADDR 0x10
+#define RECEIVER_ADDR 0x20
+
+#define TEST_PGN 0xAB00
+#define SENDER_TP_CM_ID (0x18EC2010 | CAN_EFF_FLAG)
+#define RECEIVER_TP_CM_ID (0x18EC1020 | CAN_EFF_FLAG)
+
+//10 millisecond timeout for raw socket
+#define RAW_RCVTIMEOUT_US 10000
+
+/* Segemented payload sent by the J1939 socket*/
+const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
+
+/* Expected RTS payload */
+const uint8_t RTS_PAYLOAD[] = {0x10, 0x0A, 0x00, 0x02, 0x02, 0x00, 0xAB, 0x00};
+/* Hold payload to be sent by raw socket */
+const uint8_t HOLD_PAYLOAD[] = {0x11, 0x00, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* CTS to send to only allow for the transmission of one data frame */
+const uint8_t CTS_1_FRAME_PAYLOAD[] = {0x11, 0x01, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* Resume payload to resume from connection which has been held directly after RTS*/
+const uint8_t RESUME_IMMEDIATE_PAYLOAD[] = {0x11, 0x02, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* Resume payload to resume session which has been held after first data frame */
+const uint8_t RESUME_PAYLOAD[] = {0x11, 0x01, 0x02, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+/* Data payloads */
+const uint8_t DATA_1_PAYLOAD[] = {0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06};
+const uint8_t DATA_2_PAYLOAD[] = {0x02, 0x07, 0x08, 0x09, 0xFF, 0xFF, 0xFF, 0xFF};
+
+/* EOMA payload to cleanup session */
+const uint8_t EOMA_PAYLOAD[] = {0x13, 0x0A, 0x00, 0x02, 0xFF, 0x00, 0xAB, 0x00};
+
+/* Timeout payload sent on connection timeout */
+const uint8_t ABORT_TIMEOUT_PAYLOAD[] = {0xFF, 0x03, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
+char CANIF[IFNAMSIZ];
+
+static int recv_payload(int sock, const uint8_t *payload, size_t len)
+{
+ struct can_frame rx_frame = {};
+
+ if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
+ perror("failed to recv can raw frame");
+ return 1;
+ }
+
+ if (rx_frame.len != len) {
+ printf("received data length does not match expected value\n");
+ return 1;
+ }
+
+ if (memcmp(rx_frame.data, payload, len)) {
+ printf("received data does not match expected value\n");
+ return 1;
+ }
+
+ return 0;
+}
+
+
+FIXTURE(can_env)
+{
+ int j1939_sock;
+ int raw_sock;
+};
+
+FIXTURE_SETUP(can_env)
+{
+ struct sockaddr_can addr = {};
+ struct ifreq ifr = {};
+ struct timeval raw_rcvtimeout = {.tv_sec = 0, .tv_usec = RAW_RCVTIMEOUT_US};
+ int ret;
+
+ self->raw_sock = -1;
+ self->j1939_sock = -1;
+
+ self->raw_sock = socket(PF_CAN, SOCK_RAW, CAN_RAW);
+ ASSERT_GE(self->raw_sock, 0)
+ TH_LOG("failed to create CAN_RAW socket: %d", errno);
+
+ strncpy(ifr.ifr_name, CANIF, sizeof(ifr.ifr_name));
+ ret = ioctl(self->raw_sock, SIOCGIFINDEX, &ifr);
+ ASSERT_GE(ret, 0)
+ TH_LOG("failed SIOCGIFINDEX: %d", errno);
+
+
+ addr.can_family = AF_CAN;
+ addr.can_ifindex = ifr.ifr_ifindex;
+
+ ret = bind(self->raw_sock, (struct sockaddr *)&addr, sizeof(addr));
+ ASSERT_EQ(ret, 0)
+ TH_LOG("failed bind CAN_RAW socket: %d", errno);
+
+ ret = setsockopt(self->raw_sock, SOL_SOCKET, SO_RCVTIMEO, &raw_rcvtimeout, sizeof(raw_rcvtimeout));
+ ASSERT_EQ(ret, 0)
+ TH_LOG("failed setting SO_RCVTIMEO on CAN_RAW socket: %d", errno);
+
+ self->j1939_sock = socket(PF_CAN, SOCK_DGRAM, CAN_J1939);
+ ASSERT_GE(self->j1939_sock, 0)
+ TH_LOG("failed to create CAN_J1939 socket: %d", errno);
+
+ addr.can_addr.j1939.addr = SENDER_ADDR;
+ addr.can_addr.j1939.name = J1939_NO_NAME;
+ addr.can_addr.j1939.pgn = J1939_NO_PGN;
+
+ ret = bind(self->j1939_sock, (struct sockaddr *)&addr, sizeof(addr));
+ ASSERT_EQ(ret, 0)
+ TH_LOG("failed bind CAN_J1939 socket: %d", errno);
+
+ addr.can_addr.j1939.addr = RECEIVER_ADDR;
+ addr.can_addr.j1939.pgn = TEST_PGN;
+ ret = connect(self->j1939_sock, (struct sockaddr *)&addr, sizeof(addr));
+ ASSERT_EQ(ret, 0)
+ TH_LOG("failed connect CAN_J1939 socket: %d", errno);
+}
+
+FIXTURE_TEARDOWN(can_env)
+{
+ if (self->j1939_sock != -1)
+ close(self->j1939_sock);
+
+ if (self->raw_sock != -1)
+ close(self->raw_sock);
+}
+
+/* Test RTS/CTS transport without hold as baseline */
+TEST_F(can_env, test_no_hold)
+{
+ struct can_frame tx_frame = {
+ .can_id = RECEIVER_TP_CM_ID,
+ .len = 8,
+ };
+
+ memcpy(tx_frame.data, RESUME_IMMEDIATE_PAYLOAD, sizeof(RESUME_IMMEDIATE_PAYLOAD));
+
+ int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
+
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send j1939 payload: %d", errno);
+
+
+ res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive RTS as expeceted");
+
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send hold with raw sock: %d", errno);
+
+ res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive DATA 1 as expeceted");
+
+ res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive DATA 2 as expeceted");
+
+ memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* Test holding RTS/CTS transport on first frame and resuming immediatley */
+TEST_F(can_env, test_hold_resume_immediate)
+{
+ struct can_frame tx_frame = {
+ .can_id = RECEIVER_TP_CM_ID,
+ .len = 8,
+ };
+
+ memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
+
+ int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
+
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send j1939 payload: %d", errno);
+
+
+ res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive RTS as expeceted");
+
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send hold with raw sock: %d", errno);
+
+ /* Wait for 300ms before sending the resume */
+ usleep(300000);
+
+ memcpy(tx_frame.data, RESUME_IMMEDIATE_PAYLOAD, sizeof(RESUME_IMMEDIATE_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send resume with raw sock: %d", errno);
+
+ res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive DATA 1 as expeceted");
+
+ res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive DATA 2 as expeceted");
+
+ memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* Test send hold in transport session and resuming */
+TEST_F(can_env, test_hold_resume)
+{
+ struct can_frame tx_frame = {
+ .can_id = RECEIVER_TP_CM_ID,
+ .len = 8,
+ };
+
+ memcpy(tx_frame.data, CTS_1_FRAME_PAYLOAD, sizeof(CTS_1_FRAME_PAYLOAD));
+
+ int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
+
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send j1939 payload: %d", errno);
+
+ res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive RTS as expeceted");
+
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send cts(1) with raw sock: %d", errno);
+
+ res = recv_payload(self->raw_sock, DATA_1_PAYLOAD, sizeof(DATA_1_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive RTS as expeceted");
+
+ memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send hold with raw sock: %d", errno);
+
+ /* Wait for 300ms before sending the resume */
+ usleep(300000);
+
+ memcpy(tx_frame.data, RESUME_PAYLOAD, sizeof(RESUME_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send resume with raw sock: %d", errno);
+
+ res = recv_payload(self->raw_sock, DATA_2_PAYLOAD, sizeof(DATA_2_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive DATA 2 as expeceted");
+
+ memcpy(tx_frame.data, EOMA_PAYLOAD, sizeof(EOMA_PAYLOAD));
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send EOMA with raw sock: %d", errno);
+}
+
+/* Test timeout after not resuming hold */
+TEST_F(can_env, test_hold_timeout)
+{
+ struct can_frame tx_frame = {
+ .can_id = RECEIVER_TP_CM_ID,
+ .len = 8,
+ };
+
+ memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
+ int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
+
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send j1939 payload: %d", errno);
+
+ res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive RTS as expeceted");
+
+ res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
+ ASSERT_GT(res, 0)
+ TH_LOG("failed to send hold with raw sock: %d", errno);
+
+ /* Wait for 1100 ms and receive the abort due to CTS hold timeout.
+ * The actual timeout is 1050ms but with this test setup there is no point
+ * in trying to be this exact.
+ */
+ usleep(1100 * 1000);
+
+ res = recv_payload(self->raw_sock, ABORT_TIMEOUT_PAYLOAD, sizeof(ABORT_TIMEOUT_PAYLOAD));
+ ASSERT_EQ(res, 0)
+ TH_LOG("Failed to receive abort as expeceted");
+}
+
+int main(int argc, char **argv)
+{
+ char *ifname = getenv("CANIF");
+
+ if (!ifname) {
+ printf("CANIF environment variable must contain the test interface\n");
+ return KSFT_FAIL;
+ }
+
+ strncpy(CANIF, ifname, sizeof(CANIF) - 1);
+
+ return test_harness_run(argc, argv);
+}
diff --git a/tools/testing/selftests/net/can/test_cts_hold.sh b/tools/testing/selftests/net/can/test_cts_hold.sh
new file mode 100755
index 000000000000..e69e9109245c
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_cts_hold.sh
@@ -0,0 +1,45 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+ALL_TESTS="
+ test_cts_hold
+"
+
+net_dir=$(dirname $0)/..
+source $net_dir/lib.sh
+
+export CANIF=${CANIF:-"vcan0"}
+BITRATE=${BITRATE:-500000}
+
+setup()
+{
+ if [[ $CANIF == vcan* ]]; then
+ ip link add name $CANIF type vcan || exit $ksft_skip
+ else
+ ip link set dev $CANIF type can bitrate $BITRATE || exit $ksft_skip
+ fi
+ ip link set dev $CANIF up
+ pwd
+}
+
+cleanup()
+{
+ ip link set dev $CANIF down
+ if [[ $CANIF == vcan* ]]; then
+ ip link delete $CANIF
+ fi
+}
+
+test_cts_hold()
+{
+ ./test_cts_hold
+ check_err $?
+ log_test "test_cts_hold"
+}
+
+trap cleanup EXIT
+setup
+
+tests_run
+
+exit $EXIT_STATUS
--
2.54.0Hi Alexander,
On Sat, May 16, 2026 at 04:35:25PM +0200, Alexander Hölzl wrote:
> The J1939 protocol allows the receiver of directed segemented messages
> to hold the data transfer. The kernel implementation did not handle hold
> messages correctly was not able to resume from a hold.
>
> Fix J1939 RTS/CTS session not being able to resume from hold.
> Replace hardcoded timeout with define.
> Add CTS hold behavior tests.
>
> Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
> ---
> Compared to the last patch I removed all of the todo comments I had
> still had in the code. I will implement hold notification in the error
> queue in different patch.
>
> I replaced the hardcoded hold timeout with a define. I can replace the
> other hardcoded timeouts in a different patch if you want me to, but
> for now I only touched code related to the holds.
> I also added a helper function to check if a CTS is a hold.
>
> I also added a baseline test case as you wanted. Altough in the future
> it should probably moved to another file which specfically tests normal
> RTS/CTS behavior.
>
> Also addressed all the comments Sashiko had on the test file and I'm
> also now sending an EOMA in the test as without it between every test
> there was a 1250ms wait until the session timed out...
Nice!
> net/can/j1939/transport.c | 48 ++-
Please split fix and testing patches. Otherwise reverting it for testing
will break testing (at least it will make things a bit more
complicated).
> tools/testing/selftests/net/can/.gitignore | 1 +
> tools/testing/selftests/net/can/Makefile | 8 +-
> tools/testing/selftests/net/can/config | 1 +
> .../testing/selftests/net/can/test_cts_hold.c | 322 ++++++++++++++++++
> .../selftests/net/can/test_cts_hold.sh | 45 +++
> 6 files changed, 406 insertions(+), 19 deletions(-)
> create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c
> create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
> +/* Time until session invalidation upon reception of a hold message.
> + * Corresponds to T4 in the specification.
I would add here documentation references, it would help another me in
two yars to understand things :)
ISO 11783-3 2018 - 5.10.3.5 Connection closure
SAE J1939-21 2001 - 5.10.2.4 Connection Closure
You can update version if you have more recent variants.
> + */
> +#define J1939_CTS_HOLD_TIMEOUT_MS 1050
> +
> enum j1939_xtp_abort {
> J1939_XTP_NO_ABORT = 0,
> J1939_XTP_ABORT_BUSY = 1,
> @@ -1428,6 +1433,11 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
> j1939_session_put(session);
> }
>
Here comment:
SAE J1939-21 2001 - 5.10.2.4 Connection Closure
ISO 11783-3 2018 - 5.11.5.4 Extended Connection Mode Clear To Send (ETP.CM_CTS)
The number of packets to send can be set to 0 to hold the connection
> +static inline bool j1939_cts_is_hold(const struct sk_buff *skb)
> +{
> + return (!skb->data[1]);
> +}
> +
> static void
> j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
> {
> @@ -1442,9 +1452,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>
> netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>
> - if (session->last_cmd == dat[0]) {
> - err = J1939_XTP_ABORT_DUP_SEQ;
> - goto out_session_cancel;
We need to document old and new sanity check behavior in the commit
message.
Before this change it protects only against a flood of CTS where the
linux stack didn't managed to start sending the data - less probable
scenario. And it prevents valid CTS hold support - most probable
scenario.
After the patch it allows to sende multiple CTS including CTS(0), but
prevents requesting the already transferred and acked packets. So, the
kernel will abort immediately instead of going in to timeout.
> + session->last_cmd = dat[0];
> +
> + if (j1939_cts_is_hold(skb)) {
> + if (session->transmission)
> + j1939_session_txtimer_cancel(session);
> +
Here we need a comment:
The originator should abort the session after T4 (=< 1050ms):
SAE J1939-21 2001 - 5.10.2.4 Connection Closure
a lack of a CTS for more than (T4) seconds after a CTS (0) message to
"hold the connection open" will all cause a connection closure to occur.
The receiver should send followup CTS not later then Th (=< 500ms):
SAE J1939-21 2001 - C.1 Connection Mode Data Transfer
The responder station then issues a TP.CM_CTS indicating that it wants
to hold the connection open but cannot receive any packets right now. A
maximum of 500 ms later it must send another TP.CM_CTS message to hold
the connection.
But we care only about T4 value on our side.
> + j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
> + netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
> + return;
> }
>
> if (session->skcb.addr.type == J1939_ETP)
> @@ -1457,7 +1473,11 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
> else if (dat[1] > session->pkt.block /* 0xff for etp */)
> goto out_session_cancel;
>
> - /* set packet counters only when not CTS(0) */
> + if (session->pkt.tx_acked >= pkt) {
> + err = J1939_XTP_ABORT_DUP_SEQ;
> + goto out_session_cancel;
> + }
> +
> session->pkt.tx_acked = pkt - 1;
> j1939_session_skb_drop_old(session);
> session->pkt.last = session->pkt.tx_acked + dat[1];
> @@ -1467,19 +1487,13 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
> /* TODO: do not set tx here, do it in txtimer */
> session->pkt.tx = session->pkt.tx_acked;
>
> - session->last_cmd = dat[0];
> - if (dat[1]) {
> - j1939_tp_set_rxtimeout(session, 1250);
> - if (session->transmission) {
> - if (session->pkt.tx_acked)
> - j1939_sk_errqueue(session,
> - J1939_ERRQUEUE_TX_SCHED);
> - j1939_session_txtimer_cancel(session);
> - j1939_tp_schedule_txtimer(session, 0);
> - }
> - } else {
> - /* CTS(0) */
> - j1939_tp_set_rxtimeout(session, 550);
Now I understand where 550 comes from. It is Th + 50. But you are
correct, we need to use T4 here.
> + j1939_tp_set_rxtimeout(session, 1250);
> + if (session->transmission) {
> + if (session->pkt.tx_acked)
> + j1939_sk_errqueue(session,
> + J1939_ERRQUEUE_TX_SCHED);
> + j1939_session_txtimer_cancel(session);
> + j1939_tp_schedule_txtimer(session, 0);
> }
> return;
>
> diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
> +
> +/* Segemented payload sent by the J1939 socket*/
> +const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
can you please remove tabs or extra spaces between [] and =
const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
> +
> +/* Expected RTS payload */
> +const uint8_t RTS_PAYLOAD[] = {0x10, 0x0A, 0x00, 0x02, 0x02, 0x00, 0xAB, 0x00};
> +/* Hold payload to be sent by raw socket */
> +const uint8_t HOLD_PAYLOAD[] = {0x11, 0x00, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
> +/* CTS to send to only allow for the transmission of one data frame */
> +const uint8_t CTS_1_FRAME_PAYLOAD[] = {0x11, 0x01, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
> +/* Resume payload to resume from connection which has been held directly after RTS*/
> +const uint8_t RESUME_IMMEDIATE_PAYLOAD[] = {0x11, 0x02, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
> +/* Resume payload to resume session which has been held after first data frame */
> +const uint8_t RESUME_PAYLOAD[] = {0x11, 0x01, 0x02, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
> +/* Data payloads */
> +const uint8_t DATA_1_PAYLOAD[] = {0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06};
> +const uint8_t DATA_2_PAYLOAD[] = {0x02, 0x07, 0x08, 0x09, 0xFF, 0xFF, 0xFF, 0xFF};
> +
> +/* EOMA payload to cleanup session */
> +const uint8_t EOMA_PAYLOAD[] = {0x13, 0x0A, 0x00, 0x02, 0xFF, 0x00, 0xAB, 0x00};
> +
> +/* Timeout payload sent on connection timeout */
> +const uint8_t ABORT_TIMEOUT_PAYLOAD[] = {0xFF, 0x03, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
> +char CANIF[IFNAMSIZ];
> +
> +static int recv_payload(int sock, const uint8_t *payload, size_t len)
> +{
> + struct can_frame rx_frame = {};
> +
> + if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
recv will block until we receive something, it will be not good if test fails
and nothing is send. Probably something like this:
static int recv_payload_timeout(int sock, const uint8_t *payload, size_t len, int timeout_ms)
{
struct can_frame rx_frame = {};
struct pollfd pfd = {
.fd = sock,
.events = POLLIN,
};
int ret;
/* Wait for data to be ready to read, up to timeout_ms */
ret = poll(&pfd, 1, timeout_ms);
if (ret < 0) {
perror("poll failed");
return 1;
}
if (ret == 0) {
fprintf(stderr, "timeout waiting for can raw frame\n");
return 1;
}
/* Socket is readable, recv will not block */
if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
perror("failed to recv can raw frame");
return 1;
}
if (rx_frame.len != len) {
fprintf(stderr, "received data length does not match expected value\n");
return 1;
}
if (memcmp(rx_frame.data, payload, len)) {
fprintf(stderr, "received data does not match expected value\n");
return 1;
}
return 0;
}
....
> +/* Test timeout after not resuming hold */
> +TEST_F(can_env, test_hold_timeout)
> +{
> + struct can_frame tx_frame = {
> + .can_id = RECEIVER_TP_CM_ID,
> + .len = 8,
> + };
> +
> + memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
> + int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
> +
> + ASSERT_GT(res, 0)
> + TH_LOG("failed to send j1939 payload: %d", errno);
> +
> + res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
> + ASSERT_EQ(res, 0)
> + TH_LOG("Failed to receive RTS as expeceted");
> +
> + res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
> + ASSERT_GT(res, 0)
> + TH_LOG("failed to send hold with raw sock: %d", errno);
> +
> + /* Wait for 1100 ms and receive the abort due to CTS hold timeout.
> + * The actual timeout is 1050ms but with this test setup there is no point
> + * in trying to be this exact.
> + */
> + usleep(1100 * 1000);
> +
> + res = recv_payload(self->raw_sock, ABORT_TIMEOUT_PAYLOAD, sizeof(ABORT_TIMEOUT_PAYLOAD));
> + ASSERT_EQ(res, 0)
> + TH_LOG("Failed to receive abort as expeceted");
> +}
This test may potentially have random fails due to CI system load.
May be:
#define DEFAULT_RECV_TIMEOUT_MS 2000
static int recv_payload(int sock, const uint8_t *payload, size_t len)
{
return recv_payload_timeout(sock, payload, len, DEFAULT_RECV_TIMEOUT_MS);
}
/* Test timeout after not resuming hold */
TEST_F(can_env, test_hold_timeout)
{
struct can_frame tx_frame = {
.can_id = RECEIVER_TP_CM_ID,
.len = 8,
};
struct timespec start, end;
long elapsed_ms;
int res;
memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
ASSERT_GT(res, 0)
TH_LOG("failed to send j1939 payload: %d", errno);
res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
ASSERT_EQ(res, 0)
TH_LOG("Failed to receive RTS as expected");
res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
ASSERT_GT(res, 0)
TH_LOG("failed to send hold with raw sock: %d", errno);
/* Record start time */
clock_gettime(CLOCK_MONOTONIC, &start);
/*
* Receive with a timeout larger than the expected 1050ms J1939 timeout.
* 2000ms provides plenty of headroom for CI without hanging indefinitely.
*/
res = recv_payload_timeout(self->raw_sock, ABORT_TIMEOUT_PAYLOAD,
sizeof(ABORT_TIMEOUT_PAYLOAD), 2000);
ASSERT_EQ(res, 0)
TH_LOG("Failed to receive abort as expected");
/* Record end time and calculate elapsed milliseconds */
clock_gettime(CLOCK_MONOTONIC, &end);
elapsed_ms = (end.tv_sec - start.tv_sec) * 1000 +
(end.tv_nsec - start.tv_nsec) / 1000000;
/*
* The actual timeout is 1050ms. We define an acceptable window
* to account for CI scheduling variations.
*/
ASSERT_GE(elapsed_ms, 1000)
TH_LOG("Abort received too early: %ld ms", elapsed_ms);
ASSERT_LE(elapsed_ms, 1500)
TH_LOG("Abort received too late: %ld ms", elapsed_ms);
}
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hello,
once again thank you for your thorough review :)
I'll try to send the next patch by tomorrow evening.
On 5/17/26 09:46, Oleksij Rempel wrote:
> Hi Alexander,
>
> On Sat, May 16, 2026 at 04:35:25PM +0200, Alexander Hölzl wrote:
>> The J1939 protocol allows the receiver of directed segemented messages
>> to hold the data transfer. The kernel implementation did not handle hold
>> messages correctly was not able to resume from a hold.
>>
>> Fix J1939 RTS/CTS session not being able to resume from hold.
>> Replace hardcoded timeout with define.
>> Add CTS hold behavior tests.
>>
>> Signed-off-by: Alexander Hölzl <alexander.hoelzl@gmx.net>
>> ---
>> Compared to the last patch I removed all of the todo comments I had
>> still had in the code. I will implement hold notification in the error
>> queue in different patch.
>>
>> I replaced the hardcoded hold timeout with a define. I can replace the
>> other hardcoded timeouts in a different patch if you want me to, but
>> for now I only touched code related to the holds.
>> I also added a helper function to check if a CTS is a hold.
>>
>> I also added a baseline test case as you wanted. Altough in the future
>> it should probably moved to another file which specfically tests normal
>> RTS/CTS behavior.
>>
>> Also addressed all the comments Sashiko had on the test file and I'm
>> also now sending an EOMA in the test as without it between every test
>> there was a 1250ms wait until the session timed out...
>
> Nice!
>
>> net/can/j1939/transport.c | 48 ++-
>
> Please split fix and testing patches. Otherwise reverting it for testing
> will break testing (at least it will make things a bit more
> complicated).
Sure that's a good point! Also the test script test_cts_hold.sh is very
much based on the test_raw_filter.sh script. Now that I'm thinking
about it, it would be probably cleaner to move out the common parts into
a helper file?>
>> tools/testing/selftests/net/can/.gitignore | 1 +
>> tools/testing/selftests/net/can/Makefile | 8 +-
>> tools/testing/selftests/net/can/config | 1 +
>> .../testing/selftests/net/can/test_cts_hold.c | 322 ++++++++++++++++++
>> .../selftests/net/can/test_cts_hold.sh | 45 +++
>> 6 files changed, 406 insertions(+), 19 deletions(-)
>> create mode 100644 tools/testing/selftests/net/can/test_cts_hold.c
>> create mode 100755 tools/testing/selftests/net/can/test_cts_hold.sh
>
>
>> +/* Time until session invalidation upon reception of a hold message.
>> + * Corresponds to T4 in the specification.
>
> I would add here documentation references, it would help another me in
> two yars to understand things :)
> ISO 11783-3 2018 - 5.10.3.5 Connection closure
> SAE J1939-21 2001 - 5.10.2.4 Connection Closure
>
> You can update version if you have more recent variants.
Oh yeah, the 2001 version is quite old, I'm using the 2022 revision.
Although I don't think there are any changes in the spec which would be
relevant for this commit in there.
Will add the comments.>
>> + */
>> +#define J1939_CTS_HOLD_TIMEOUT_MS 1050
>> +
>> enum j1939_xtp_abort {
>> J1939_XTP_NO_ABORT = 0,
>> J1939_XTP_ABORT_BUSY = 1,
>> @@ -1428,6 +1433,11 @@ j1939_xtp_rx_eoma(struct j1939_priv *priv, struct sk_buff *skb,
>> j1939_session_put(session);
>> }
>>
>
> Here comment:
> SAE J1939-21 2001 - 5.10.2.4 Connection Closure
> ISO 11783-3 2018 - 5.11.5.4 Extended Connection Mode Clear To Send (ETP.CM_CTS)
>
> The number of packets to send can be set to 0 to hold the connection
>
>> +static inline bool j1939_cts_is_hold(const struct sk_buff *skb)
>> +{
>> + return (!skb->data[1]);
>> +}
>> +
>> static void
>> j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>> {
>> @@ -1442,9 +1452,15 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>>
>> netdev_dbg(session->priv->ndev, "%s: 0x%p\n", __func__, session);
>>
>> - if (session->last_cmd == dat[0]) {
>> - err = J1939_XTP_ABORT_DUP_SEQ;
>> - goto out_session_cancel;
>
> We need to document old and new sanity check behavior in the commit
> message.
>
> Before this change it protects only against a flood of CTS where the
> linux stack didn't managed to start sending the data - less probable
> scenario. And it prevents valid CTS hold support - most probable
> scenario.
>
> After the patch it allows to sende multiple CTS including CTS(0), but
> prevents requesting the already transferred and acked packets. So, the
> kernel will abort immediately instead of going in to timeout.
Correct, and just to say it again sending a CTS while the transmission
is ongoing is also not correct according to the standard. So a strict
implementation would abort the session on reception of a CTS while
transmitting. But that's a problem for a future patch.
>> + session->last_cmd = dat[0];
>> +
>> + if (j1939_cts_is_hold(skb)) {
>> + if (session->transmission)
>> + j1939_session_txtimer_cancel(session);
>> +
>
> Here we need a comment:
> The originator should abort the session after T4 (=< 1050ms):
> SAE J1939-21 2001 - 5.10.2.4 Connection Closure
> a lack of a CTS for more than (T4) seconds after a CTS (0) message to
> "hold the connection open" will all cause a connection closure to occur.
>
> The receiver should send followup CTS not later then Th (=< 500ms):
> SAE J1939-21 2001 - C.1 Connection Mode Data Transfer
> The responder station then issues a TP.CM_CTS indicating that it wants
> to hold the connection open but cannot receive any packets right now. A
> maximum of 500 ms later it must send another TP.CM_CTS message to hold
> the connection.
>
> But we care only about T4 value on our side.
Will add!>
>> + j1939_tp_set_rxtimeout(session, J1939_CTS_HOLD_TIMEOUT_MS);
>
>
>> + netdev_dbg(session->priv->ndev, "%s: 0x%p received CTS hold\n", __func__, session);
>> + return;
>> }
>>
>> if (session->skcb.addr.type == J1939_ETP)
>> @@ -1457,7 +1473,11 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>> else if (dat[1] > session->pkt.block /* 0xff for etp */)
>> goto out_session_cancel;
>>
>> - /* set packet counters only when not CTS(0) */
>> + if (session->pkt.tx_acked >= pkt) {
>> + err = J1939_XTP_ABORT_DUP_SEQ;
>> + goto out_session_cancel;
>> + }
>> +
>> session->pkt.tx_acked = pkt - 1;
>> j1939_session_skb_drop_old(session);
>> session->pkt.last = session->pkt.tx_acked + dat[1];
>> @@ -1467,19 +1487,13 @@ j1939_xtp_rx_cts_one(struct j1939_session *session, struct sk_buff *skb)
>> /* TODO: do not set tx here, do it in txtimer */
>> session->pkt.tx = session->pkt.tx_acked;
>>
>> - session->last_cmd = dat[0];
>> - if (dat[1]) {
>> - j1939_tp_set_rxtimeout(session, 1250);
>> - if (session->transmission) {
>> - if (session->pkt.tx_acked)
>> - j1939_sk_errqueue(session,
>> - J1939_ERRQUEUE_TX_SCHED);
>> - j1939_session_txtimer_cancel(session);
>> - j1939_tp_schedule_txtimer(session, 0);
>> - }
>> - } else {
>> - /* CTS(0) */
>> - j1939_tp_set_rxtimeout(session, 550);
>
> Now I understand where 550 comes from. It is Th + 50. But you are
> correct, we need to use T4 here.
Exactly, the Th is a receiver side performance requirement. But it is a
quite confusing standard. It's very easy to get confused.>
>> + j1939_tp_set_rxtimeout(session, 1250);
>> + if (session->transmission) {
>> + if (session->pkt.tx_acked)
>> + j1939_sk_errqueue(session,
>> + J1939_ERRQUEUE_TX_SCHED);
>> + j1939_session_txtimer_cancel(session);
>> + j1939_tp_schedule_txtimer(session, 0);
>> }
>> return;
>>
>> diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
>> +
>> +/* Segemented payload sent by the J1939 socket*/
>> +const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
>
> can you please remove tabs or extra spaces between [] and =
> const uint8_t J1939_PAYLOAD[] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09};
>
Sure!
>> +
>> +/* Expected RTS payload */
>> +const uint8_t RTS_PAYLOAD[] = {0x10, 0x0A, 0x00, 0x02, 0x02, 0x00, 0xAB, 0x00};
>> +/* Hold payload to be sent by raw socket */
>> +const uint8_t HOLD_PAYLOAD[] = {0x11, 0x00, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* CTS to send to only allow for the transmission of one data frame */
>> +const uint8_t CTS_1_FRAME_PAYLOAD[] = {0x11, 0x01, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Resume payload to resume from connection which has been held directly after RTS*/
>> +const uint8_t RESUME_IMMEDIATE_PAYLOAD[] = {0x11, 0x02, 0x01, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Resume payload to resume session which has been held after first data frame */
>> +const uint8_t RESUME_PAYLOAD[] = {0x11, 0x01, 0x02, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +/* Data payloads */
>> +const uint8_t DATA_1_PAYLOAD[] = {0x01, 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06};
>> +const uint8_t DATA_2_PAYLOAD[] = {0x02, 0x07, 0x08, 0x09, 0xFF, 0xFF, 0xFF, 0xFF};
>> +
>> +/* EOMA payload to cleanup session */
>> +const uint8_t EOMA_PAYLOAD[] = {0x13, 0x0A, 0x00, 0x02, 0xFF, 0x00, 0xAB, 0x00};
>> +
>> +/* Timeout payload sent on connection timeout */
>> +const uint8_t ABORT_TIMEOUT_PAYLOAD[] = {0xFF, 0x03, 0xFF, 0xFF, 0xFF, 0x00, 0xAB, 0x00};
>> +char CANIF[IFNAMSIZ];
>> +
>> +static int recv_payload(int sock, const uint8_t *payload, size_t len)
>> +{
>> + struct can_frame rx_frame = {};
>> +
>> + if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
>
> recv will block until we receive something, it will be not good if test fails
> and nothing is send. Probably something like this:
>
> static int recv_payload_timeout(int sock, const uint8_t *payload, size_t len, int timeout_ms)
> {
> struct can_frame rx_frame = {};
> struct pollfd pfd = {
> .fd = sock,
> .events = POLLIN,
> };
> int ret;
>
> /* Wait for data to be ready to read, up to timeout_ms */
> ret = poll(&pfd, 1, timeout_ms);
> if (ret < 0) {
> perror("poll failed");
> return 1;
> }
>
> if (ret == 0) {
> fprintf(stderr, "timeout waiting for can raw frame\n");
> return 1;
> }
>
> /* Socket is readable, recv will not block */
> if (recv(sock, &rx_frame, sizeof(rx_frame), 0) < 0) {
> perror("failed to recv can raw frame");
> return 1;
> }
>
> if (rx_frame.len != len) {
> fprintf(stderr, "received data length does not match expected value\n");
> return 1;
> }
>
> if (memcmp(rx_frame.data, payload, len)) {
> fprintf(stderr, "received data does not match expected value\n");
> return 1;
> }
>
> return 0;
> }
>
My original way to avoid this problem was setting SO_RCVTIMEO on the raw
socket but I can use poll instead. That would also have the advantage
that you could change the timeout more easily. Will do!
> ....
>
>> +/* Test timeout after not resuming hold */
>> +TEST_F(can_env, test_hold_timeout)
>> +{
>> + struct can_frame tx_frame = {
>> + .can_id = RECEIVER_TP_CM_ID,
>> + .len = 8,
>> + };
>> +
>> + memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
>> + int res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
>> +
>> + ASSERT_GT(res, 0)
>> + TH_LOG("failed to send j1939 payload: %d", errno);
>> +
>> + res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
>> + ASSERT_EQ(res, 0)
>> + TH_LOG("Failed to receive RTS as expeceted");
>> +
>> + res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
>> + ASSERT_GT(res, 0)
>> + TH_LOG("failed to send hold with raw sock: %d", errno);
>> +
>> + /* Wait for 1100 ms and receive the abort due to CTS hold timeout.
>> + * The actual timeout is 1050ms but with this test setup there is no point
>> + * in trying to be this exact.
>> + */
>> + usleep(1100 * 1000);
>> +
>> + res = recv_payload(self->raw_sock, ABORT_TIMEOUT_PAYLOAD, sizeof(ABORT_TIMEOUT_PAYLOAD));
>> + ASSERT_EQ(res, 0)
>> + TH_LOG("Failed to receive abort as expeceted");
>> +}
>
> This test may potentially have random fails due to CI system load.
> May be:
>
> #define DEFAULT_RECV_TIMEOUT_MS 2000
>
> static int recv_payload(int sock, const uint8_t *payload, size_t len)
> {
> return recv_payload_timeout(sock, payload, len, DEFAULT_RECV_TIMEOUT_MS);
> }
>
> /* Test timeout after not resuming hold */
> TEST_F(can_env, test_hold_timeout)
> {
> struct can_frame tx_frame = {
> .can_id = RECEIVER_TP_CM_ID,
> .len = 8,
> };
> struct timespec start, end;
> long elapsed_ms;
> int res;
>
> memcpy(tx_frame.data, HOLD_PAYLOAD, sizeof(HOLD_PAYLOAD));
> res = send(self->j1939_sock, J1939_PAYLOAD, sizeof(J1939_PAYLOAD), 0);
> ASSERT_GT(res, 0)
> TH_LOG("failed to send j1939 payload: %d", errno);
>
> res = recv_payload(self->raw_sock, RTS_PAYLOAD, sizeof(RTS_PAYLOAD));
> ASSERT_EQ(res, 0)
> TH_LOG("Failed to receive RTS as expected");
>
> res = send(self->raw_sock, &tx_frame, sizeof(tx_frame), 0);
> ASSERT_GT(res, 0)
> TH_LOG("failed to send hold with raw sock: %d", errno);
>
> /* Record start time */
> clock_gettime(CLOCK_MONOTONIC, &start);
>
> /*
> * Receive with a timeout larger than the expected 1050ms J1939 timeout.
> * 2000ms provides plenty of headroom for CI without hanging indefinitely.
> */
> res = recv_payload_timeout(self->raw_sock, ABORT_TIMEOUT_PAYLOAD,
> sizeof(ABORT_TIMEOUT_PAYLOAD), 2000);
>
> ASSERT_EQ(res, 0)
> TH_LOG("Failed to receive abort as expected");
>
> /* Record end time and calculate elapsed milliseconds */
> clock_gettime(CLOCK_MONOTONIC, &end);
> elapsed_ms = (end.tv_sec - start.tv_sec) * 1000 +
> (end.tv_nsec - start.tv_nsec) / 1000000;
>
> /*
> * The actual timeout is 1050ms. We define an acceptable window
> * to account for CI scheduling variations.
> */
> ASSERT_GE(elapsed_ms, 1000)
> TH_LOG("Abort received too early: %ld ms", elapsed_ms);
> ASSERT_LE(elapsed_ms, 1500)
> TH_LOG("Abort received too late: %ld ms", elapsed_ms);
> }
>
>
Perfect, thank you!
© 2016 - 2026 Red Hat, Inc.