[PATCH 0/4] selftest: can: Start importing selftests from can-tests

[PATCH 0/4] selftest: can: Start importing selftests from can-tests

[Felix Maurer]

This is the initial import of a CAN selftest from can-tests[1] into the
tree. For now, it is just a single test but when agreed on the
structure, we intend to import more tests from can-tests and add
additional test cases.

The goal of moving the CAN selftests into the tree is to align the tests
more closely with the kernel, improve testing of CAN in general, and to
simplify running the tests automatically in the various kernel CI
systems.

I have cc'ed netdev and its reviewers and maintainers to make sure they
are okay with the location of the tests and the changes to the paths in
MAINTAINERS. The changes should be merged through linux-can-next and
subsequent changes will not go to netdev anymore.

[1]: https://github.com/linux-can/can-tests

Felix Maurer (4):
  selftests: can: Import tst-filter from can-tests
  selftests: can: use kselftest harness in test_raw_filter
  selftests: can: Use fixtures in test_raw_filter
  selftests: can: Document test_raw_filter test cases

 MAINTAINERS                                   |   2 +
 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/net/can/.gitignore    |   2 +
 tools/testing/selftests/net/can/Makefile      |  11 +
 .../selftests/net/can/test_raw_filter.c       | 338 ++++++++++++++++++
 .../selftests/net/can/test_raw_filter.sh      |  37 ++
 6 files changed, 391 insertions(+)
 create mode 100644 tools/testing/selftests/net/can/.gitignore
 create mode 100644 tools/testing/selftests/net/can/Makefile
 create mode 100644 tools/testing/selftests/net/can/test_raw_filter.c
 create mode 100755 tools/testing/selftests/net/can/test_raw_filter.sh

-- 
2.49.0

[PATCH 1/4] selftests: can: Import tst-filter from can-tests

[Felix Maurer]

Tests for the can subsytem have been in the can-tests repository[1] so far.
Start moving the tests to kernel selftests by importing the current
tst-filter test. Subsequent patches will update the test to be more aligned
with the kernel selftests and follow the coding style.

The imported test verifies that the single filters on raw CAN sockets work
as expected.

[1]: https://github.com/linux-can/can-tests

Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---

Notes:
    I have removed the long form of the licenses in the beginning of the
    file during the import, as that is covered by the SPDX line anyways. The
    copyright is left as it was originally.

 MAINTAINERS                                   |   2 +
 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/net/can/.gitignore    |   2 +
 tools/testing/selftests/net/can/Makefile      |   9 +
 .../selftests/net/can/test_raw_filter.c       | 204 ++++++++++++++++++
 .../selftests/net/can/test_raw_filter.sh      |  37 ++++
 6 files changed, 255 insertions(+)
 create mode 100644 tools/testing/selftests/net/can/.gitignore
 create mode 100644 tools/testing/selftests/net/can/Makefile
 create mode 100644 tools/testing/selftests/net/can/test_raw_filter.c
 create mode 100755 tools/testing/selftests/net/can/test_raw_filter.sh

diff --git a/MAINTAINERS b/MAINTAINERS
index 241ca9e260a2..55749b492ebb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5155,6 +5155,7 @@ F:	include/uapi/linux/can/isotp.h
 F:	include/uapi/linux/can/raw.h
 F:	net/can/
 F:	net/sched/em_canid.c
+F:	tools/testing/selftests/net/can/
 
 CAN-J1939 NETWORK LAYER
 M:	Robin van der Gracht <robin@protonic.nl>
@@ -16577,6 +16578,7 @@ X:	net/ceph/
 X:	net/mac80211/
 X:	net/rfkill/
 X:	net/wireless/
+X:	tools/testing/selftests/net/can/
 
 NETWORKING [IPSEC]
 M:	Steffen Klassert <steffen.klassert@secunet.com>
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 8daac70c2f9d..e5c9ecd52b73 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -64,6 +64,7 @@ TARGETS += mqueue
 TARGETS += nci
 TARGETS += net
 TARGETS += net/af_unix
+TARGETS += net/can
 TARGETS += net/forwarding
 TARGETS += net/hsr
 TARGETS += net/mptcp
diff --git a/tools/testing/selftests/net/can/.gitignore b/tools/testing/selftests/net/can/.gitignore
new file mode 100644
index 000000000000..764a53fc837f
--- /dev/null
+++ b/tools/testing/selftests/net/can/.gitignore
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+test_raw_filter
diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
new file mode 100644
index 000000000000..44ef37f064ad
--- /dev/null
+++ b/tools/testing/selftests/net/can/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+
+top_srcdir = ../../../../..
+
+TEST_PROGS := test_raw_filter.sh
+
+TEST_GEN_FILES := test_raw_filter
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
new file mode 100644
index 000000000000..c84260f36565
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_raw_filter.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)
+/*
+ * Copyright (c) 2011 Volkswagen Group Electronic Research
+ * All rights reserved.
+ */
+
+#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/can.h>
+#include <linux/can/raw.h>
+
+#define ID 0x123
+#define TC 18 /* # of testcases */
+
+const int rx_res[TC] = {4, 4, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1};
+const int rxbits_res[TC] = {4369, 4369, 4369, 4369, 17, 4352, 17, 4352, 257, 257, 4112, 4112, 1, 256, 16, 4096, 1, 256};
+
+#define VCANIF "vcan0"
+
+canid_t calc_id(int testcase)
+{
+	canid_t id = ID;
+
+	if (testcase & 1)
+		id |= CAN_EFF_FLAG;
+	if (testcase & 2)
+		id |= CAN_RTR_FLAG;
+
+	return id;
+}
+
+canid_t calc_mask(int testcase)
+{
+	canid_t mask = CAN_SFF_MASK;
+
+	if (testcase > 15)
+		return (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG);
+
+	if (testcase & 4)
+		mask |= CAN_EFF_FLAG;
+	if (testcase & 8)
+		mask |= CAN_RTR_FLAG;
+
+	return mask;
+}
+
+int main(int argc, char **argv)
+{
+	fd_set rdfs;
+	struct timeval tv;
+	int s;
+	struct sockaddr_can addr;
+	struct can_filter rfilter;
+	struct can_frame frame;
+	int testcase;
+	int have_rx;
+	int rx;
+	int rxbits, rxbitval;
+	int ret;
+	int recv_own_msgs = 1;
+	int err = 0;
+	struct ifreq ifr;
+
+	if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) {
+		perror("socket");
+		err = 1;
+		goto out;
+	}
+
+	strcpy(ifr.ifr_name, VCANIF);
+	if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
+		perror("SIOCGIFINDEX");
+		err = 1;
+		goto out_socket;
+	}
+	addr.can_family = AF_CAN;
+	addr.can_ifindex = ifr.ifr_ifindex;
+
+	setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS,
+		   &recv_own_msgs, sizeof(recv_own_msgs));
+
+	if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
+		perror("bind");
+		err = 1;
+		goto out_socket;
+	}
+
+	printf("---\n");
+
+	for (testcase = 0; testcase < TC; testcase++) {
+
+		rfilter.can_id   = calc_id(testcase);
+		rfilter.can_mask = calc_mask(testcase);
+		setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER,
+			   &rfilter, sizeof(rfilter));
+
+		printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n",
+		       testcase, rfilter.can_id, rfilter.can_mask);
+
+		printf("testcase %2d sending patterns ... ", testcase);
+
+		frame.can_dlc = 1;
+		frame.data[0] = testcase;
+
+		frame.can_id = ID;
+		if (write(s, &frame, sizeof(frame)) < 0) {
+			perror("write");
+			exit(1);
+		}
+		frame.can_id = (ID | CAN_RTR_FLAG);
+		if (write(s, &frame, sizeof(frame)) < 0) {
+			perror("write");
+			exit(1);
+		}
+		frame.can_id = (ID | CAN_EFF_FLAG);
+		if (write(s, &frame, sizeof(frame)) < 0) {
+			perror("write");
+			exit(1);
+		}
+		frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
+		if (write(s, &frame, sizeof(frame)) < 0) {
+			perror("write");
+			exit(1);
+		}
+
+		printf("ok\n");
+
+		have_rx = 1;
+		rx = 0;
+		rxbits = 0;
+
+		while (have_rx) {
+
+			have_rx = 0;
+			FD_ZERO(&rdfs);
+			FD_SET(s, &rdfs);
+			tv.tv_sec = 0;
+			tv.tv_usec = 50000; /* 50ms timeout */
+
+			ret = select(s+1, &rdfs, NULL, NULL, &tv);
+			if (ret < 0) {
+				perror("select");
+				exit(1);
+			}
+
+			if (FD_ISSET(s, &rdfs)) {
+				have_rx = 1;
+				ret = read(s, &frame, sizeof(struct can_frame));
+				if (ret < 0) {
+					perror("read");
+					exit(1);
+				}
+				if ((frame.can_id & CAN_SFF_MASK) != ID) {
+					fprintf(stderr, "received wrong can_id!\n");
+					exit(1);
+				}
+				if (frame.data[0] != testcase) {
+					fprintf(stderr, "received wrong testcase!\n");
+					exit(1);
+				}
+
+				/* test & calc rxbits */
+				rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
+
+				/* only receive a rxbitval once */
+				if ((rxbits & rxbitval) == rxbitval) {
+					fprintf(stderr, "received rxbitval %d twice!\n", rxbitval);
+					exit(1);
+				}
+				rxbits |= rxbitval;
+				rx++;
+
+				printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n",
+				       testcase, frame.can_id, rx, rxbits);
+			}
+		}
+		/* rx timed out -> check the received results */
+		if (rx_res[testcase] != rx) {
+			fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n",
+				testcase, rx, rx_res[testcase]);
+			exit(1);
+		}
+		if (rxbits_res[testcase] != rxbits) {
+			fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n",
+				testcase, rxbits, rxbits_res[testcase]);
+			exit(1);
+		}
+		printf("testcase %2d ok\n---\n", testcase);
+	}
+
+out_socket:
+	close(s);
+out:
+	return err;
+}
diff --git a/tools/testing/selftests/net/can/test_raw_filter.sh b/tools/testing/selftests/net/can/test_raw_filter.sh
new file mode 100755
index 000000000000..e5f175c8b27b
--- /dev/null
+++ b/tools/testing/selftests/net/can/test_raw_filter.sh
@@ -0,0 +1,37 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+#set -x
+
+ALL_TESTS="
+	test_raw_filter
+"
+
+net_dir=$(dirname $0)/..
+source $net_dir/lib.sh
+
+VCANIF="vcan0"
+
+setup()
+{
+	ip link add name $VCANIF type vcan || exit $ksft_skip
+	ip link set dev $VCANIF up
+	pwd
+}
+
+cleanup()
+{
+	ip link delete $VCANIF
+}
+
+test_raw_filter()
+{
+	./test_raw_filter
+}
+
+trap cleanup EXIT
+setup
+
+tests_run
+
+exit $EXIT_STATUS
-- 
2.49.0

[PATCH 2/4] selftests: can: use kselftest harness in test_raw_filter

[Felix Maurer]

Update test_raw_filter to use the kselftest harness to make the test output
conform with TAP. Use the logging and assertations from the harness.

Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 tools/testing/selftests/net/can/Makefile      |   2 +
 .../selftests/net/can/test_raw_filter.c       | 152 ++++++++----------
 2 files changed, 73 insertions(+), 81 deletions(-)

diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
index 44ef37f064ad..5b82e60a03e7 100644
--- a/tools/testing/selftests/net/can/Makefile
+++ b/tools/testing/selftests/net/can/Makefile
@@ -2,6 +2,8 @@
 
 top_srcdir = ../../../../..
 
+CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
+
 TEST_PROGS := test_raw_filter.sh
 
 TEST_GEN_FILES := test_raw_filter
diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
index c84260f36565..7414b9aef823 100644
--- a/tools/testing/selftests/net/can/test_raw_filter.c
+++ b/tools/testing/selftests/net/can/test_raw_filter.c
@@ -18,6 +18,8 @@
 #include <linux/can.h>
 #include <linux/can/raw.h>
 
+#include "../../kselftest_harness.h"
+
 #define ID 0x123
 #define TC 18 /* # of testcases */
 
@@ -53,7 +55,38 @@ canid_t calc_mask(int testcase)
 	return mask;
 }
 
-int main(int argc, char **argv)
+int send_can_frames(int sock, int testcase)
+{
+	struct can_frame frame;
+
+	frame.can_dlc = 1;
+	frame.data[0] = testcase;
+
+	frame.can_id = ID;
+	if (write(sock, &frame, sizeof(frame)) < 0) {
+		perror("write");
+		return 1;
+	}
+	frame.can_id = (ID | CAN_RTR_FLAG);
+	if (write(sock, &frame, sizeof(frame)) < 0) {
+		perror("write");
+		return 1;
+	}
+	frame.can_id = (ID | CAN_EFF_FLAG);
+	if (write(sock, &frame, sizeof(frame)) < 0) {
+		perror("write");
+		return 1;
+	}
+	frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
+	if (write(sock, &frame, sizeof(frame)) < 0) {
+		perror("write");
+		return 1;
+	}
+
+	return 0;
+}
+
+TEST(can_filter)
 {
 	fd_set rdfs;
 	struct timeval tv;
@@ -67,34 +100,26 @@ int main(int argc, char **argv)
 	int rxbits, rxbitval;
 	int ret;
 	int recv_own_msgs = 1;
-	int err = 0;
 	struct ifreq ifr;
 
-	if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) {
-		perror("socket");
-		err = 1;
-		goto out;
-	}
+	s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
+	ASSERT_LT(0, s)
+		TH_LOG("failed to create CAN_RAW socket (%d)", errno);
 
 	strcpy(ifr.ifr_name, VCANIF);
-	if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
-		perror("SIOCGIFINDEX");
-		err = 1;
-		goto out_socket;
-	}
+	ret = ioctl(s, SIOCGIFINDEX, &ifr);
+	ASSERT_LE(0, ret)
+		TH_LOG("failed SIOCGIFINDEX (%d)", errno);
+
 	addr.can_family = AF_CAN;
 	addr.can_ifindex = ifr.ifr_ifindex;
 
 	setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS,
 		   &recv_own_msgs, sizeof(recv_own_msgs));
 
-	if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
-		perror("bind");
-		err = 1;
-		goto out_socket;
-	}
-
-	printf("---\n");
+	ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
+	ASSERT_EQ(0, ret)
+		TH_LOG("failed bind socket (%d)", errno);
 
 	for (testcase = 0; testcase < TC; testcase++) {
 
@@ -103,36 +128,14 @@ int main(int argc, char **argv)
 		setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER,
 			   &rfilter, sizeof(rfilter));
 
-		printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n",
+		TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X",
 		       testcase, rfilter.can_id, rfilter.can_mask);
 
-		printf("testcase %2d sending patterns ... ", testcase);
-
-		frame.can_dlc = 1;
-		frame.data[0] = testcase;
-
-		frame.can_id = ID;
-		if (write(s, &frame, sizeof(frame)) < 0) {
-			perror("write");
-			exit(1);
-		}
-		frame.can_id = (ID | CAN_RTR_FLAG);
-		if (write(s, &frame, sizeof(frame)) < 0) {
-			perror("write");
-			exit(1);
-		}
-		frame.can_id = (ID | CAN_EFF_FLAG);
-		if (write(s, &frame, sizeof(frame)) < 0) {
-			perror("write");
-			exit(1);
-		}
-		frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
-		if (write(s, &frame, sizeof(frame)) < 0) {
-			perror("write");
-			exit(1);
-		}
+		TH_LOG("testcase %2d sending patterns...", testcase);
 
-		printf("ok\n");
+		ret = send_can_frames(s, testcase);
+		ASSERT_EQ(0, ret)
+			TH_LOG("failed to send CAN frames");
 
 		have_rx = 1;
 		rx = 0;
@@ -147,58 +150,45 @@ int main(int argc, char **argv)
 			tv.tv_usec = 50000; /* 50ms timeout */
 
 			ret = select(s+1, &rdfs, NULL, NULL, &tv);
-			if (ret < 0) {
-				perror("select");
-				exit(1);
-			}
+			ASSERT_LE(0, ret)
+				TH_LOG("failed select for frame %d (%d)", rx, errno);
 
 			if (FD_ISSET(s, &rdfs)) {
 				have_rx = 1;
 				ret = read(s, &frame, sizeof(struct can_frame));
-				if (ret < 0) {
-					perror("read");
-					exit(1);
-				}
-				if ((frame.can_id & CAN_SFF_MASK) != ID) {
-					fprintf(stderr, "received wrong can_id!\n");
-					exit(1);
-				}
-				if (frame.data[0] != testcase) {
-					fprintf(stderr, "received wrong testcase!\n");
-					exit(1);
-				}
+				ASSERT_LE(0, ret)
+					TH_LOG("failed to read frame %d (%d)", rx, errno);
+
+				ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
+					TH_LOG("received wrong can_id");
+				ASSERT_EQ(testcase, frame.data[0])
+					TH_LOG("received wrong test case");
 
 				/* test & calc rxbits */
 				rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
 
 				/* only receive a rxbitval once */
-				if ((rxbits & rxbitval) == rxbitval) {
-					fprintf(stderr, "received rxbitval %d twice!\n", rxbitval);
-					exit(1);
-				}
+				ASSERT_NE(rxbitval, rxbits & rxbitval)
+					TH_LOG("received rxbitval %d twice", rxbitval);
 				rxbits |= rxbitval;
 				rx++;
 
-				printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n",
+				TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d",
 				       testcase, frame.can_id, rx, rxbits);
 			}
 		}
 		/* rx timed out -> check the received results */
-		if (rx_res[testcase] != rx) {
-			fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n",
-				testcase, rx, rx_res[testcase]);
-			exit(1);
-		}
-		if (rxbits_res[testcase] != rxbits) {
-			fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n",
-				testcase, rxbits, rxbits_res[testcase]);
-			exit(1);
-		}
-		printf("testcase %2d ok\n---\n", testcase);
+		ASSERT_EQ(rx_res[testcase], rx)
+			TH_LOG("wrong number of received frames %d", testcase);
+		ASSERT_EQ(rxbits_res[testcase], rxbits)
+			TH_LOG("wrong rxbits value in testcase %d", testcase);
+
+		TH_LOG("testcase %2d ok", testcase);
+		TH_LOG("---");
 	}
 
-out_socket:
 	close(s);
-out:
-	return err;
+	return;
 }
+
+TEST_HARNESS_MAIN
-- 
2.49.0

[PATCH 3/4] selftests: can: Use fixtures in test_raw_filter

[Felix Maurer]

Use fixtures in test_raw_filter instead of generating the test inputs
during execution. This should make tests easier to follow and extend.

Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 .../selftests/net/can/test_raw_filter.c       | 311 ++++++++++++------
 1 file changed, 211 insertions(+), 100 deletions(-)

diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
index 7414b9aef823..7fe11e020a1c 100644
--- a/tools/testing/selftests/net/can/test_raw_filter.c
+++ b/tools/testing/selftests/net/can/test_raw_filter.c
@@ -18,43 +18,13 @@
 #include <linux/can.h>
 #include <linux/can/raw.h>
 
+#define TH_LOG_ENABLED 0
 #include "../../kselftest_harness.h"
 
 #define ID 0x123
-#define TC 18 /* # of testcases */
-
-const int rx_res[TC] = {4, 4, 4, 4, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 1};
-const int rxbits_res[TC] = {4369, 4369, 4369, 4369, 17, 4352, 17, 4352, 257, 257, 4112, 4112, 1, 256, 16, 4096, 1, 256};
 
 #define VCANIF "vcan0"
 
-canid_t calc_id(int testcase)
-{
-	canid_t id = ID;
-
-	if (testcase & 1)
-		id |= CAN_EFF_FLAG;
-	if (testcase & 2)
-		id |= CAN_RTR_FLAG;
-
-	return id;
-}
-
-canid_t calc_mask(int testcase)
-{
-	canid_t mask = CAN_SFF_MASK;
-
-	if (testcase > 15)
-		return (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG);
-
-	if (testcase & 4)
-		mask |= CAN_EFF_FLAG;
-	if (testcase & 8)
-		mask |= CAN_RTR_FLAG;
-
-	return mask;
-}
-
 int send_can_frames(int sock, int testcase)
 {
 	struct can_frame frame;
@@ -86,21 +56,16 @@ int send_can_frames(int sock, int testcase)
 	return 0;
 }
 
-TEST(can_filter)
+FIXTURE(can_filters) {
+	int sock;
+};
+
+FIXTURE_SETUP(can_filters)
 {
-	fd_set rdfs;
-	struct timeval tv;
-	int s;
 	struct sockaddr_can addr;
-	struct can_filter rfilter;
-	struct can_frame frame;
-	int testcase;
-	int have_rx;
-	int rx;
-	int rxbits, rxbitval;
-	int ret;
-	int recv_own_msgs = 1;
 	struct ifreq ifr;
+	int recv_own_msgs = 1;
+	int s, ret;
 
 	s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
 	ASSERT_LT(0, s)
@@ -121,74 +86,220 @@ TEST(can_filter)
 	ASSERT_EQ(0, ret)
 		TH_LOG("failed bind socket (%d)", errno);
 
-	for (testcase = 0; testcase < TC; testcase++) {
+	self->sock = s;
+}
+
+FIXTURE_TEARDOWN(can_filters)
+{
+	close(self->sock);
+}
 
-		rfilter.can_id   = calc_id(testcase);
-		rfilter.can_mask = calc_mask(testcase);
-		setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER,
-			   &rfilter, sizeof(rfilter));
+FIXTURE_VARIANT(can_filters) {
+	int testcase;
+	canid_t id;
+	canid_t mask;
+	int exp_num_rx;
+	int exp_rxbits;
+};
+
+FIXTURE_VARIANT_ADD(can_filters, base) {
+	.testcase = 1,
+	.id = ID,
+	.mask = CAN_SFF_MASK,
+	.exp_num_rx = 4,
+	.exp_rxbits = 4369,
+};
+FIXTURE_VARIANT_ADD(can_filters, base_eff) {
+	.testcase = 2,
+	.id = ID | CAN_EFF_FLAG,
+	.mask = CAN_SFF_MASK,
+	.exp_num_rx = 4,
+	.exp_rxbits = 4369,
+};
+FIXTURE_VARIANT_ADD(can_filters, base_rtr) {
+	.testcase = 3,
+	.id = ID | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK,
+	.exp_num_rx = 4,
+	.exp_rxbits = 4369,
+};
+FIXTURE_VARIANT_ADD(can_filters, base_effrtr) {
+	.testcase = 4,
+	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK,
+	.exp_num_rx = 4,
+	.exp_rxbits = 4369,
+};
+
+FIXTURE_VARIANT_ADD(can_filters, filter_eff) {
+	.testcase = 5,
+	.id = ID,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 17,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_eff_eff) {
+	.testcase = 6,
+	.id = ID | CAN_EFF_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 4352,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_eff_rtr) {
+	.testcase = 7,
+	.id = ID | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 17,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_eff_effrtr) {
+	.testcase = 8,
+	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 4352,
+};
+
+FIXTURE_VARIANT_ADD(can_filters, filter_rtr) {
+	.testcase = 9,
+	.id = ID,
+	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 257,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_rtr_eff) {
+	.testcase = 10,
+	.id = ID | CAN_EFF_FLAG,
+	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 257,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_rtr_rtr) {
+	.testcase = 11,
+	.id = ID | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 4112,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_rtr_effrtr) {
+	.testcase = 12,
+	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
+	.exp_num_rx = 2,
+	.exp_rxbits = 4112,
+};
+
+FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) {
+	.testcase = 13,
+	.id = ID,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 1,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_eff) {
+	.testcase = 14,
+	.id = ID | CAN_EFF_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 256,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_rtr) {
+	.testcase = 15,
+	.id = ID | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 16,
+};
+FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_effrtr) {
+	.testcase = 16,
+	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 4096,
+};
+
+FIXTURE_VARIANT_ADD(can_filters, eff) {
+	.testcase = 17,
+	.id = ID,
+	.mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 1,
+};
+FIXTURE_VARIANT_ADD(can_filters, eff_eff) {
+	.testcase = 18,
+	.id = ID | CAN_EFF_FLAG,
+	.mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
+	.exp_num_rx = 1,
+	.exp_rxbits = 256,
+};
+
+TEST_F(can_filters, test_filter)
+{
+	fd_set rdfs;
+	struct timeval tv;
+	struct can_filter rfilter;
+	struct can_frame frame;
+	int have_rx;
+	int rx;
+	int rxbits, rxbitval;
+	int ret;
 
-		TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X",
-		       testcase, rfilter.can_id, rfilter.can_mask);
+	rfilter.can_id = variant->id;
+	rfilter.can_mask = variant->mask;
+	setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER,
+		   &rfilter, sizeof(rfilter));
 
-		TH_LOG("testcase %2d sending patterns...", testcase);
+	TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X",
+		rfilter.can_id, rfilter.can_mask);
 
-		ret = send_can_frames(s, testcase);
-		ASSERT_EQ(0, ret)
-			TH_LOG("failed to send CAN frames");
+	ret = send_can_frames(self->sock, variant->testcase);
+	ASSERT_EQ(0, ret)
+		TH_LOG("failed to send CAN frames");
 
-		have_rx = 1;
-		rx = 0;
-		rxbits = 0;
+	rx = 0;
+	rxbits = 0;
 
-		while (have_rx) {
+	do {
+		have_rx = 0;
+		FD_ZERO(&rdfs);
+		FD_SET(self->sock, &rdfs);
+		tv.tv_sec = 0;
+		tv.tv_usec = 50000; /* 50ms timeout */
 
-			have_rx = 0;
-			FD_ZERO(&rdfs);
-			FD_SET(s, &rdfs);
-			tv.tv_sec = 0;
-			tv.tv_usec = 50000; /* 50ms timeout */
+		ret = select(self->sock + 1, &rdfs, NULL, NULL, &tv);
+		ASSERT_LE(0, ret)
+			TH_LOG("failed select for frame %d (%d)", rx, errno);
 
-			ret = select(s+1, &rdfs, NULL, NULL, &tv);
+		if (FD_ISSET(self->sock, &rdfs)) {
+			have_rx = 1;
+			ret = read(self->sock, &frame, sizeof(struct can_frame));
 			ASSERT_LE(0, ret)
-				TH_LOG("failed select for frame %d (%d)", rx, errno);
-
-			if (FD_ISSET(s, &rdfs)) {
-				have_rx = 1;
-				ret = read(s, &frame, sizeof(struct can_frame));
-				ASSERT_LE(0, ret)
-					TH_LOG("failed to read frame %d (%d)", rx, errno);
-
-				ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
-					TH_LOG("received wrong can_id");
-				ASSERT_EQ(testcase, frame.data[0])
-					TH_LOG("received wrong test case");
-
-				/* test & calc rxbits */
-				rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
-
-				/* only receive a rxbitval once */
-				ASSERT_NE(rxbitval, rxbits & rxbitval)
-					TH_LOG("received rxbitval %d twice", rxbitval);
-				rxbits |= rxbitval;
-				rx++;
-
-				TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d",
-				       testcase, frame.can_id, rx, rxbits);
-			}
+				TH_LOG("failed to read frame %d (%d)", rx, errno);
+
+			ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
+				TH_LOG("received wrong can_id");
+			ASSERT_EQ(variant->testcase, frame.data[0])
+				TH_LOG("received wrong test case");
+
+			/* test & calc rxbits */
+			rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
+
+			/* only receive a rxbitval once */
+			ASSERT_NE(rxbitval, rxbits & rxbitval)
+				TH_LOG("received rxbitval %d twice", rxbitval);
+			rxbits |= rxbitval;
+			rx++;
+
+			TH_LOG("rx: can_id = 0x%08X rx = %d rxbits = %d",
+			       frame.can_id, rx, rxbits);
 		}
-		/* rx timed out -> check the received results */
-		ASSERT_EQ(rx_res[testcase], rx)
-			TH_LOG("wrong number of received frames %d", testcase);
-		ASSERT_EQ(rxbits_res[testcase], rxbits)
-			TH_LOG("wrong rxbits value in testcase %d", testcase);
-
-		TH_LOG("testcase %2d ok", testcase);
-		TH_LOG("---");
-	}
+	} while (have_rx);
 
-	close(s);
-	return;
+	/* rx timed out -> check the received results */
+	ASSERT_EQ(variant->exp_num_rx, rx)
+		TH_LOG("wrong number of received frames");
+	ASSERT_EQ(variant->exp_rxbits, rxbits)
+		TH_LOG("wrong rxbits value");
 }
 
 TEST_HARNESS_MAIN
-- 
2.49.0

[PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Felix Maurer]

The expected results did not explain very well what was really tested. Make
the expectations more clear by writing out the flags that should be set in
the received frames and add a short explanation for each test case. Also,
document the overall test design.

Signed-off-by: Felix Maurer <fmaurer@redhat.com>
---
 .../selftests/net/can/test_raw_filter.c       | 65 ++++++++++++++-----
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
index 7fe11e020a1c..8d43053824d2 100644
--- a/tools/testing/selftests/net/can/test_raw_filter.c
+++ b/tools/testing/selftests/net/can/test_raw_filter.c
@@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
 	int exp_num_rx;
 	int exp_rxbits;
 };
+#define T_EFF (CAN_EFF_FLAG >> 28)
+#define T_RTR (CAN_RTR_FLAG >> 28)
 
+/* Receive all frames when filtering for the ID in standard frame format */
 FIXTURE_VARIANT_ADD(can_filters, base) {
 	.testcase = 1,
 	.id = ID,
 	.mask = CAN_SFF_MASK,
 	.exp_num_rx = 4,
-	.exp_rxbits = 4369,
+	.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
+/* Ignore EFF flag in filter ID if not covered by filter mask */
 FIXTURE_VARIANT_ADD(can_filters, base_eff) {
 	.testcase = 2,
 	.id = ID | CAN_EFF_FLAG,
 	.mask = CAN_SFF_MASK,
 	.exp_num_rx = 4,
-	.exp_rxbits = 4369,
+	.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
+/* Ignore RTR flag in filter ID if not covered by filter mask */
 FIXTURE_VARIANT_ADD(can_filters, base_rtr) {
 	.testcase = 3,
 	.id = ID | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK,
 	.exp_num_rx = 4,
-	.exp_rxbits = 4369,
+	.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
+/* Ignore EFF and RTR flags in filter ID if not covered by filter mask */
 FIXTURE_VARIANT_ADD(can_filters, base_effrtr) {
 	.testcase = 4,
 	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK,
 	.exp_num_rx = 4,
-	.exp_rxbits = 4369,
+	.exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
 
+/* Receive only SFF frames when expecting no EFF flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_eff) {
 	.testcase = 5,
 	.id = ID,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 17,
+	.exp_rxbits = (1 | 1 << (T_RTR)),
 };
+/* Receive only EFF frames when filter id and filter mask include EFF flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_eff_eff) {
 	.testcase = 6,
 	.id = ID | CAN_EFF_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 4352,
+	.exp_rxbits = (1 << (T_EFF) | 1 << (T_EFF | T_RTR)),
 };
+/* Receive only SFF frames when expecting no EFF flag, ignoring RTR flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_eff_rtr) {
 	.testcase = 7,
 	.id = ID | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 17,
+	.exp_rxbits = (1 | 1 << (T_RTR)),
 };
+/* Receive only EFF frames when filter id and filter mask include EFF flag,
+ * ignoring RTR flag
+ */
 FIXTURE_VARIANT_ADD(can_filters, filter_eff_effrtr) {
 	.testcase = 8,
 	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 4352,
+	.exp_rxbits = (1 << (T_EFF) | 1 << (T_EFF | T_RTR)),
 };
 
+/* Receive no remote frames when filtering for no RTR flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_rtr) {
 	.testcase = 9,
 	.id = ID,
 	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 257,
+	.exp_rxbits = (1 | 1 << (T_EFF)),
 };
+/* Receive no remote frames when filtering for no RTR flag, ignoring EFF flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_rtr_eff) {
 	.testcase = 10,
 	.id = ID | CAN_EFF_FLAG,
 	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 257,
+	.exp_rxbits = (1 | 1 << (T_EFF)),
 };
+/* Receive only remote frames when filter includes RTR flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_rtr_rtr) {
 	.testcase = 11,
 	.id = ID | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 4112,
+	.exp_rxbits = (1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
+/* Receive only remote frames when filter includes RTR flag, ignoring EFF
+ * flag
+ */
 FIXTURE_VARIANT_ADD(can_filters, filter_rtr_effrtr) {
 	.testcase = 12,
 	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_RTR_FLAG,
 	.exp_num_rx = 2,
-	.exp_rxbits = 4112,
+	.exp_rxbits = (1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
 };
 
+/* Receive only SFF data frame when filtering for no flags */
 FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) {
 	.testcase = 13,
 	.id = ID,
@@ -196,28 +215,34 @@ FIXTURE_VARIANT_ADD(can_filters, filter_effrtr) {
 	.exp_num_rx = 1,
 	.exp_rxbits = 1,
 };
+/* Receive only EFF data frame when filtering for EFF but no RTR flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_eff) {
 	.testcase = 14,
 	.id = ID | CAN_EFF_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.exp_num_rx = 1,
-	.exp_rxbits = 256,
+	.exp_rxbits = (1 << (T_EFF)),
 };
+/* Receive only SFF remote frame when filtering for RTR but no EFF flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_rtr) {
 	.testcase = 15,
 	.id = ID | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.exp_num_rx = 1,
-	.exp_rxbits = 16,
+	.exp_rxbits = (1 << (T_RTR)),
 };
+/* Receive only EFF remote frame when filtering for EFF and RTR flag */
 FIXTURE_VARIANT_ADD(can_filters, filter_effrtr_effrtr) {
 	.testcase = 16,
 	.id = ID | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.mask = CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.exp_num_rx = 1,
-	.exp_rxbits = 4096,
+	.exp_rxbits = (1 << (T_EFF | T_RTR)),
 };
 
+/* Receive only SFF data frame when filtering for no EFF flag and no RTR flag
+ * but based on EFF mask
+ */
 FIXTURE_VARIANT_ADD(can_filters, eff) {
 	.testcase = 17,
 	.id = ID,
@@ -225,14 +250,22 @@ FIXTURE_VARIANT_ADD(can_filters, eff) {
 	.exp_num_rx = 1,
 	.exp_rxbits = 1,
 };
+/* Receive only EFF data frame when filtering for EFF flag and no RTR flag but
+ * based on EFF mask
+ */
 FIXTURE_VARIANT_ADD(can_filters, eff_eff) {
 	.testcase = 18,
 	.id = ID | CAN_EFF_FLAG,
 	.mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG,
 	.exp_num_rx = 1,
-	.exp_rxbits = 256,
+	.exp_rxbits = (1 << (T_EFF)),
 };
 
+/* This test verifies that the raw CAN filters work, by checking if only frames
+ * with the expected set of flags are received. For each test case, the given
+ * filter (id and mask) is added and four CAN frames are sent with every
+ * combination of set/unset EFF/RTR flags.
+ */
 TEST_F(can_filters, test_filter)
 {
 	fd_set rdfs;
-- 
2.49.0

Re: [PATCH 1/4] selftests: can: Import tst-filter from can-tests

[Vincent Mailhol]

On Tue. 22 Apr. 2025 at 21:08, Felix Maurer <fmaurer@redhat.com> wrote:
> diff --git a/tools/testing/selftests/net/can/test_raw_filter.sh b/tools/testing/selftests/net/can/test_raw_filter.sh
> new file mode 100755
> index 000000000000..e5f175c8b27b
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_raw_filter.sh
> @@ -0,0 +1,37 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +#set -x

Leftover from the debug?
Can you remove this line?

> +ALL_TESTS="
> +       test_raw_filter
> +"
> +
> +net_dir=$(dirname $0)/..
> +source $net_dir/lib.sh
> +
> +VCANIF="vcan0"

Here, you are making the VCANIF variable configuration, but then, in
your test_raw_filter.c I see:

  #define VCANIF "vcan0"

This means that in order to modify the interface, one would have to
both modify the .sh script and the .c source. Wouldn't it be possible
to centralize this? For example by reading the environment variable in
the C file?

Or maybe there is a smarter way to pass values in the kernel selftests
framework which I am not aware of?

> +setup()
> +{
> +       ip link add name $VCANIF type vcan || exit $ksft_skip
> +       ip link set dev $VCANIF up
> +       pwd
> +}
> +
> +cleanup()
> +{
> +       ip link delete $VCANIF
> +}

I guess that this setup() and this cleanup() is something that you
will also need in the other can tests. Would it make sense to declare
these in a common.sh file and just do a

  source common.sh

here?

> +test_raw_filter()
> +{
> +       ./test_raw_filter
> +}
> +
> +trap cleanup EXIT
> +setup
> +
> +tests_run
> +
> +exit $EXIT_STATUS

Yours sincerely,
Vincent Mailhol

Re: [PATCH 2/4] selftests: can: use kselftest harness in test_raw_filter

[Vincent Mailhol]

On Tue. 22 Apr. 2025 at 21:04, Felix Maurer <fmaurer@redhat.com> wrote:
> Update test_raw_filter to use the kselftest harness to make the test output
> conform with TAP. Use the logging and assertations from the harness.
                                        ^^^^^^^^^^^^
assertions?

>
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> ---
>  tools/testing/selftests/net/can/Makefile      |   2 +
>  .../selftests/net/can/test_raw_filter.c       | 152 ++++++++----------
>  2 files changed, 73 insertions(+), 81 deletions(-)
>
> diff --git a/tools/testing/selftests/net/can/Makefile b/tools/testing/selftests/net/can/Makefile
> index 44ef37f064ad..5b82e60a03e7 100644
> --- a/tools/testing/selftests/net/can/Makefile
> +++ b/tools/testing/selftests/net/can/Makefile
> @@ -2,6 +2,8 @@
>
>  top_srcdir = ../../../../..
>
> +CFLAGS += -Wall -Wl,--no-as-needed -O2 -g -I$(top_srcdir)/usr/include $(KHDR_INCLUDES)
> +
>  TEST_PROGS := test_raw_filter.sh
>
>  TEST_GEN_FILES := test_raw_filter
> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
> index c84260f36565..7414b9aef823 100644
> --- a/tools/testing/selftests/net/can/test_raw_filter.c
> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
> @@ -18,6 +18,8 @@
>  #include <linux/can.h>
>  #include <linux/can/raw.h>
>
> +#include "../../kselftest_harness.h"
> +
>  #define ID 0x123
>  #define TC 18 /* # of testcases */
>
> @@ -53,7 +55,38 @@ canid_t calc_mask(int testcase)
>         return mask;
>  }
>
> -int main(int argc, char **argv)
> +int send_can_frames(int sock, int testcase)
> +{
> +       struct can_frame frame;
> +
> +       frame.can_dlc = 1;
> +       frame.data[0] = testcase;
> +
> +       frame.can_id = ID;
> +       if (write(sock, &frame, sizeof(frame)) < 0) {
> +               perror("write");
> +               return 1;
> +       }
> +       frame.can_id = (ID | CAN_RTR_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0) {
> +               perror("write");
> +               return 1;
> +       }
> +       frame.can_id = (ID | CAN_EFF_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0) {
> +               perror("write");
> +               return 1;
> +       }
> +       frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
> +       if (write(sock, &frame, sizeof(frame)) < 0) {
> +               perror("write");
> +               return 1;
> +       }
> +
> +       return 0;

Nitpick: can you centralize the error handling under a goto label?

  write_err:
          perror("write");
          return 1;

> +}
> +
> +TEST(can_filter)
>  {
>         fd_set rdfs;
>         struct timeval tv;
> @@ -67,34 +100,26 @@ int main(int argc, char **argv)
>         int rxbits, rxbitval;
>         int ret;
>         int recv_own_msgs = 1;
> -       int err = 0;
>         struct ifreq ifr;
>
> -       if ((s = socket(PF_CAN, SOCK_RAW, CAN_RAW)) < 0) {
> -               perror("socket");
> -               err = 1;
> -               goto out;
> -       }
> +       s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
> +       ASSERT_LT(0, s)
> +               TH_LOG("failed to create CAN_RAW socket (%d)", errno);
>
>         strcpy(ifr.ifr_name, VCANIF);
> -       if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
> -               perror("SIOCGIFINDEX");
> -               err = 1;
> -               goto out_socket;
> -       }
> +       ret = ioctl(s, SIOCGIFINDEX, &ifr);
> +       ASSERT_LE(0, ret)
> +               TH_LOG("failed SIOCGIFINDEX (%d)", errno);
> +
>         addr.can_family = AF_CAN;
>         addr.can_ifindex = ifr.ifr_ifindex;
>
>         setsockopt(s, SOL_CAN_RAW, CAN_RAW_RECV_OWN_MSGS,
>                    &recv_own_msgs, sizeof(recv_own_msgs));
>
> -       if (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) {
> -               perror("bind");
> -               err = 1;
> -               goto out_socket;
> -       }
> -
> -       printf("---\n");
> +       ret = bind(s, (struct sockaddr *)&addr, sizeof(addr));
> +       ASSERT_EQ(0, ret)
> +               TH_LOG("failed bind socket (%d)", errno);
>
>         for (testcase = 0; testcase < TC; testcase++) {
>
> @@ -103,36 +128,14 @@ int main(int argc, char **argv)
>                 setsockopt(s, SOL_CAN_RAW, CAN_RAW_FILTER,
>                            &rfilter, sizeof(rfilter));
>
> -               printf("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X\n",
> +               TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X",
>                        testcase, rfilter.can_id, rfilter.can_mask);
>
> -               printf("testcase %2d sending patterns ... ", testcase);
> -
> -               frame.can_dlc = 1;
> -               frame.data[0] = testcase;
> -
> -               frame.can_id = ID;
> -               if (write(s, &frame, sizeof(frame)) < 0) {
> -                       perror("write");
> -                       exit(1);
> -               }
> -               frame.can_id = (ID | CAN_RTR_FLAG);
> -               if (write(s, &frame, sizeof(frame)) < 0) {
> -                       perror("write");
> -                       exit(1);
> -               }
> -               frame.can_id = (ID | CAN_EFF_FLAG);
> -               if (write(s, &frame, sizeof(frame)) < 0) {
> -                       perror("write");
> -                       exit(1);
> -               }
> -               frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG);
> -               if (write(s, &frame, sizeof(frame)) < 0) {
> -                       perror("write");
> -                       exit(1);
> -               }
> +               TH_LOG("testcase %2d sending patterns...", testcase);
>
> -               printf("ok\n");
> +               ret = send_can_frames(s, testcase);
> +               ASSERT_EQ(0, ret)
> +                       TH_LOG("failed to send CAN frames");
>
>                 have_rx = 1;
>                 rx = 0;
> @@ -147,58 +150,45 @@ int main(int argc, char **argv)
>                         tv.tv_usec = 50000; /* 50ms timeout */
>
>                         ret = select(s+1, &rdfs, NULL, NULL, &tv);
> -                       if (ret < 0) {
> -                               perror("select");
> -                               exit(1);
> -                       }
> +                       ASSERT_LE(0, ret)
> +                               TH_LOG("failed select for frame %d (%d)", rx, errno);
                                                                     ^^^^
Nitpick: the Linux coding style suggests not printing numbers in
parentheses (%d).

https://www.kernel.org/doc/html/latest/process/coding-style.html#printing-kernel-messages

Suggestion:

  TH_LOG("failed select for frame %d, err: %d", rx, errno);


>                         if (FD_ISSET(s, &rdfs)) {
>                                 have_rx = 1;
>                                 ret = read(s, &frame, sizeof(struct can_frame));
> -                               if (ret < 0) {
> -                                       perror("read");
> -                                       exit(1);
> -                               }
> -                               if ((frame.can_id & CAN_SFF_MASK) != ID) {
> -                                       fprintf(stderr, "received wrong can_id!\n");
> -                                       exit(1);
> -                               }
> -                               if (frame.data[0] != testcase) {
> -                                       fprintf(stderr, "received wrong testcase!\n");
> -                                       exit(1);
> -                               }
> +                               ASSERT_LE(0, ret)
> +                                       TH_LOG("failed to read frame %d (%d)", rx, errno);
> +
> +                               ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
> +                                       TH_LOG("received wrong can_id");
> +                               ASSERT_EQ(testcase, frame.data[0])
> +                                       TH_LOG("received wrong test case");
>
>                                 /* test & calc rxbits */
>                                 rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
>
>                                 /* only receive a rxbitval once */
> -                               if ((rxbits & rxbitval) == rxbitval) {
> -                                       fprintf(stderr, "received rxbitval %d twice!\n", rxbitval);
> -                                       exit(1);
> -                               }
> +                               ASSERT_NE(rxbitval, rxbits & rxbitval)
> +                                       TH_LOG("received rxbitval %d twice", rxbitval);
>                                 rxbits |= rxbitval;
>                                 rx++;
>
> -                               printf("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d\n",
> +                               TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d",
>                                        testcase, frame.can_id, rx, rxbits);
>                         }
>                 }
>                 /* rx timed out -> check the received results */
> -               if (rx_res[testcase] != rx) {
> -                       fprintf(stderr, "wrong rx value in testcase %d : %d (expected %d)\n",
> -                               testcase, rx, rx_res[testcase]);
> -                       exit(1);
> -               }
> -               if (rxbits_res[testcase] != rxbits) {
> -                       fprintf(stderr, "wrong rxbits value in testcase %d : %d (expected %d)\n",
> -                               testcase, rxbits, rxbits_res[testcase]);
> -                       exit(1);
> -               }
> -               printf("testcase %2d ok\n---\n", testcase);
> +               ASSERT_EQ(rx_res[testcase], rx)
> +                       TH_LOG("wrong number of received frames %d", testcase);
> +               ASSERT_EQ(rxbits_res[testcase], rxbits)
> +                       TH_LOG("wrong rxbits value in testcase %d", testcase);
> +
> +               TH_LOG("testcase %2d ok", testcase);
> +               TH_LOG("---");
>         }
>
> -out_socket:
>         close(s);
> -out:
> -       return err;
> +       return;
>  }
> +
> +TEST_HARNESS_MAIN

Yours sincerely,
Vincent Mailhol

Re: [PATCH 3/4] selftests: can: Use fixtures in test_raw_filter

[Vincent Mailhol]

On Tue. 22 Apr. 2025 at 21:04, Felix Maurer <fmaurer@redhat.com> wrote:
> Use fixtures in test_raw_filter instead of generating the test inputs
> during execution. This should make tests easier to follow and extend.
>
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> ---
>  .../selftests/net/can/test_raw_filter.c       | 311 ++++++++++++------
>  1 file changed, 211 insertions(+), 100 deletions(-)
>
> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
> index 7414b9aef823..7fe11e020a1c 100644
> --- a/tools/testing/selftests/net/can/test_raw_filter.c
> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
> @@ -18,43 +18,13 @@

(...)

> +TEST_F(can_filters, test_filter)
> +{
> +       fd_set rdfs;
> +       struct timeval tv;
> +       struct can_filter rfilter;
> +       struct can_frame frame;
> +       int have_rx;
> +       int rx;
> +       int rxbits, rxbitval;
> +       int ret;

Can you reduce the scope of the variables? More precisely, can you
move those declarations

  fd_set rdfs;
  struct timeval tv;

to the do {} while(); loop and move those declarations

  struct can_frame frame;
  int rxbitval;

to the if (FD_ISSET(self->sock, &rdfs)) block?

> -               TH_LOG("testcase %2d filters : can_id = 0x%08X can_mask = 0x%08X",
> -                      testcase, rfilter.can_id, rfilter.can_mask);
> +       rfilter.can_id = variant->id;
> +       rfilter.can_mask = variant->mask;
> +       setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER,
> +                  &rfilter, sizeof(rfilter));
>
> -               TH_LOG("testcase %2d sending patterns...", testcase);
> +       TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X",
> +               rfilter.can_id, rfilter.can_mask);
>
> -               ret = send_can_frames(s, testcase);
> -               ASSERT_EQ(0, ret)
> -                       TH_LOG("failed to send CAN frames");
> +       ret = send_can_frames(self->sock, variant->testcase);
> +       ASSERT_EQ(0, ret)
> +               TH_LOG("failed to send CAN frames");
>
> -               have_rx = 1;
> -               rx = 0;
> -               rxbits = 0;
> +       rx = 0;
> +       rxbits = 0;
>
> -               while (have_rx) {
> +       do {
> +               have_rx = 0;
> +               FD_ZERO(&rdfs);
> +               FD_SET(self->sock, &rdfs);
> +               tv.tv_sec = 0;
> +               tv.tv_usec = 50000; /* 50ms timeout */
>
> -                       have_rx = 0;
> -                       FD_ZERO(&rdfs);
> -                       FD_SET(s, &rdfs);
> -                       tv.tv_sec = 0;
> -                       tv.tv_usec = 50000; /* 50ms timeout */
> +               ret = select(self->sock + 1, &rdfs, NULL, NULL, &tv);
> +               ASSERT_LE(0, ret)
> +                       TH_LOG("failed select for frame %d (%d)", rx, errno);
>
> -                       ret = select(s+1, &rdfs, NULL, NULL, &tv);
> +               if (FD_ISSET(self->sock, &rdfs)) {
> +                       have_rx = 1;
> +                       ret = read(self->sock, &frame, sizeof(struct can_frame));
>                         ASSERT_LE(0, ret)
> -                               TH_LOG("failed select for frame %d (%d)", rx, errno);
> -
> -                       if (FD_ISSET(s, &rdfs)) {
> -                               have_rx = 1;
> -                               ret = read(s, &frame, sizeof(struct can_frame));
                                                                ^^^^^^^^^^^^^^^^
sizeof(frame)

> -                               ASSERT_LE(0, ret)
> -                                       TH_LOG("failed to read frame %d (%d)", rx, errno);
> -
> -                               ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
> -                                       TH_LOG("received wrong can_id");
> -                               ASSERT_EQ(testcase, frame.data[0])
> -                                       TH_LOG("received wrong test case");
> -
> -                               /* test & calc rxbits */
> -                               rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
> -
> -                               /* only receive a rxbitval once */
> -                               ASSERT_NE(rxbitval, rxbits & rxbitval)
> -                                       TH_LOG("received rxbitval %d twice", rxbitval);
> -                               rxbits |= rxbitval;
> -                               rx++;
> -
> -                               TH_LOG("testcase %2d rx : can_id = 0x%08X rx = %d rxbits = %d",
> -                                      testcase, frame.can_id, rx, rxbits);
> -                       }
> +                               TH_LOG("failed to read frame %d (%d)", rx, errno);
> +
> +                       ASSERT_EQ(ID, frame.can_id & CAN_SFF_MASK)
> +                               TH_LOG("received wrong can_id");
> +                       ASSERT_EQ(variant->testcase, frame.data[0])
> +                               TH_LOG("received wrong test case");
> +
> +                       /* test & calc rxbits */
> +                       rxbitval = 1 << ((frame.can_id & (CAN_EFF_FLAG|CAN_RTR_FLAG|CAN_ERR_FLAG)) >> 28);
> +
> +                       /* only receive a rxbitval once */
> +                       ASSERT_NE(rxbitval, rxbits & rxbitval)
> +                               TH_LOG("received rxbitval %d twice", rxbitval);
> +                       rxbits |= rxbitval;
> +                       rx++;
> +
> +                       TH_LOG("rx: can_id = 0x%08X rx = %d rxbits = %d",
> +                              frame.can_id, rx, rxbits);
>                 }
> -               /* rx timed out -> check the received results */
> -               ASSERT_EQ(rx_res[testcase], rx)
> -                       TH_LOG("wrong number of received frames %d", testcase);
> -               ASSERT_EQ(rxbits_res[testcase], rxbits)
> -                       TH_LOG("wrong rxbits value in testcase %d", testcase);
> -
> -               TH_LOG("testcase %2d ok", testcase);
> -               TH_LOG("---");
> -       }
> +       } while (have_rx);
>
> -       close(s);
> -       return;
> +       /* rx timed out -> check the received results */
> +       ASSERT_EQ(variant->exp_num_rx, rx)
> +               TH_LOG("wrong number of received frames");
> +       ASSERT_EQ(variant->exp_rxbits, rxbits)
> +               TH_LOG("wrong rxbits value");
>  }

Yours sincerely,
Vincent Mailhol

Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Vincent Mailhol]

On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@redhat.com> wrote:
> The expected results did not explain very well what was really tested. Make
> the expectations more clear by writing out the flags that should be set in
> the received frames and add a short explanation for each test case. Also,
> document the overall test design.
>
> Signed-off-by: Felix Maurer <fmaurer@redhat.com>
> ---
>  .../selftests/net/can/test_raw_filter.c       | 65 ++++++++++++++-----
>  1 file changed, 49 insertions(+), 16 deletions(-)
>
> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
> index 7fe11e020a1c..8d43053824d2 100644
> --- a/tools/testing/selftests/net/can/test_raw_filter.c
> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>         int exp_num_rx;
>         int exp_rxbits;
>  };
> +#define T_EFF (CAN_EFF_FLAG >> 28)
> +#define T_RTR (CAN_RTR_FLAG >> 28)

I do not like this

  >> 28

shift. I understand that it is part of the original design, but for
me, this is just obfuscation.

Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
expected values? What benefit does this shift add?

> +/* Receive all frames when filtering for the ID in standard frame format */
>  FIXTURE_VARIANT_ADD(can_filters, base) {
>         .testcase = 1,
>         .id = ID,
>         .mask = CAN_SFF_MASK,
>         .exp_num_rx = 4,
> -       .exp_rxbits = 4369,
> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
                        ^                                                      ^
Nitpick: those outermost parentheses are not needed.

This took me time to process. Isn't your expression redundant? What about

  .exp_rxbits = 1 | 1 << (T_EFF | T_RTR),

?

This gives me the same result:

  https://godbolt.org/z/cr3q5vjMr

>  };
> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>  FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>         .testcase = 2,
>         .id = ID | CAN_EFF_FLAG,
>         .mask = CAN_SFF_MASK,
>         .exp_num_rx = 4,
> -       .exp_rxbits = 4369,
> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
                         ^
What is the meaning of this 1?

>  };

(...)

Yours sincerely,
Vincent Mailhol

Re: [PATCH 0/4] selftest: can: Start importing selftests from can-tests

[Vincent Mailhol]

Hi Felix,

On Tue. 22 Apr. 2025 at 21:08, Felix Maurer <fmaurer@redhat.com> wrote:
> This is the initial import of a CAN selftest from can-tests[1] into the
> tree. For now, it is just a single test but when agreed on the
> structure, we intend to import more tests from can-tests and add
> additional test cases.

Excellent initiative!

> The goal of moving the CAN selftests into the tree is to align the tests
> more closely with the kernel, improve testing of CAN in general, and to
> simplify running the tests automatically in the various kernel CI
> systems.
>
> I have cc'ed netdev and its reviewers and maintainers to make sure they
> are okay with the location of the tests and the changes to the paths in
> MAINTAINERS. The changes should be merged through linux-can-next and
> subsequent changes will not go to netdev anymore.

I am not a netdev maintainer, just a /drivers/net/can maintainer, but
you have my blessing on this. As far the location goes, your proposal
makes perfect sense to me. Actually, I can not think of any other
places than

  tools/testing/selftests/net/can

for this kind of thing.

> [1]: https://github.com/linux-can/can-tests
>
> Felix Maurer (4):
>   selftests: can: Import tst-filter from can-tests
>   selftests: can: use kselftest harness in test_raw_filter
>   selftests: can: Use fixtures in test_raw_filter
>   selftests: can: Document test_raw_filter test cases

You are doing a lot of change to the original to the point that this
is more a full rewrite. I have no intent of reviewing the first patch
which is just the copy paste from the original. If no one else has a
strong opinion on this, I would rather prefer if you just squash
everything and send a single patch with the final result. This will
also save you some effort when migrating the other tests.

I have a few comments on the individual patches, but overall very
good. Thanks a lot!


Yours sincerely,
Vincent Mailhol

Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Vincent Mailhol]

On Thu. 24 Apr. 2025 at 16:44, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@redhat.com> wrote:

(...)

> > +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
>                         ^                                                      ^
> Nitpick: those outermost parentheses are not needed.
>
> This took me time to process. Isn't your expression redundant? What about
>
>   .exp_rxbits = 1 | 1 << (T_EFF | T_RTR),
>
> ?
>
> This gives me the same result:
>
>   https://godbolt.org/z/cr3q5vjMr

Never mind. This was a silly comment. I messed up the operator
precedence in the above example, these are obviously different.

Please disregard my comment and sorry for the noise.

Yours sincerely,
Vincent Mailhol

Re: [PATCH 0/4] selftest: can: Start importing selftests from can-tests

[Felix Maurer]

On 24.04.25 09:45, Vincent Mailhol wrote:
[...]
>> Felix Maurer (4):
>>   selftests: can: Import tst-filter from can-tests
>>   selftests: can: use kselftest harness in test_raw_filter
>>   selftests: can: Use fixtures in test_raw_filter
>>   selftests: can: Document test_raw_filter test cases
> 
> You are doing a lot of change to the original to the point that this
> is more a full rewrite. I have no intent of reviewing the first patch
> which is just the copy paste from the original. If no one else has a
> strong opinion on this, I would rather prefer if you just squash
> everything and send a single patch with the final result. This will
> also save you some effort when migrating the other tests.
> 
> I have a few comments on the individual patches, but overall very
> good. Thanks a lot!

Thank you very much for your feedback! I'll silently include most of it
and will only reply where I think discussions are necessary.

For squashing / keeping this as individual patches: I usually like to
have some kind of history available, but here it might not provide a lot
of value. I would be fine with squashing as well. If there are any
stronger opinions on this, keep them coming.

Thanks,
   Felix

Re: [PATCH 1/4] selftests: can: Import tst-filter from can-tests

[Felix Maurer]

On 24.04.25 09:42, Vincent Mailhol wrote:
> On Tue. 22 Apr. 2025 at 21:08, Felix Maurer <fmaurer@redhat.com> wrote:
[...]
>> +ALL_TESTS="
>> +       test_raw_filter
>> +"
>> +
>> +net_dir=$(dirname $0)/..
>> +source $net_dir/lib.sh
>> +
>> +VCANIF="vcan0"
> 
> Here, you are making the VCANIF variable configuration, but then, in
> your test_raw_filter.c I see:
> 
>   #define VCANIF "vcan0"
> 
> This means that in order to modify the interface, one would have to
> both modify the .sh script and the .c source. Wouldn't it be possible
> to centralize this? For example by reading the environment variable in
> the C file?
> 
> Or maybe there is a smarter way to pass values in the kernel selftests
> framework which I am not aware of?

Good point, I'll try to come up with something to avoid the duplication
(either from the selftest framework or just for the CAN tests). I'd
prefer an argument to the program though, as I find this the more usual
way to pass info if one ever wants to run the test directly.

>> +setup()
>> +{
>> +       ip link add name $VCANIF type vcan || exit $ksft_skip
>> +       ip link set dev $VCANIF up
>> +       pwd
>> +}
>> +
>> +cleanup()
>> +{
>> +       ip link delete $VCANIF
>> +}
> 
> I guess that this setup() and this cleanup() is something that you
> will also need in the other can tests. Would it make sense to declare
> these in a common.sh file and just do a
> 
>   source common.sh
> 
> here?

I usually try to avoid making changes in anticipation of the future. I'm
not sure if all the tests need a similar environment and would prefer to
split this when we encounter that they do. Are you okay with that?

Thanks,
   Felix

Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Felix Maurer]

On 24.04.25 09:44, Vincent Mailhol wrote:
> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@redhat.com> wrote:
[...]
>> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
>> index 7fe11e020a1c..8d43053824d2 100644
>> --- a/tools/testing/selftests/net/can/test_raw_filter.c
>> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
>> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>>         int exp_num_rx;
>>         int exp_rxbits;
>>  };
>> +#define T_EFF (CAN_EFF_FLAG >> 28)
>> +#define T_RTR (CAN_RTR_FLAG >> 28)
> 
> I do not like this
> 
>   >> 28
> 
> shift. I understand that it is part of the original design, but for
> me, this is just obfuscation.
> 
> Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
> expected values? What benefit does this shift add?

I agree, that looks like magic numbers and the original design is not
very nice here. The main reason for the >>28 is that later on values are
shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
>>28, the shift value later is in the range 0-14). See below for a
slightly different idea.

>> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>>  FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>>         .testcase = 2,
>>         .id = ID | CAN_EFF_FLAG,
>>         .mask = CAN_SFF_MASK,
>>         .exp_num_rx = 4,
>> -       .exp_rxbits = 4369,
>> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
>                          ^
> What is the meaning of this 1?

The 1 means that a packet will be received with no flags set.

The whole rxbit thing took me a while to understand and the result now
is not straightforward either. Let's see if we can come up with
something better.

The exp_rxbits is basically a bitfield that describes which flags should
be set on the received frames. Maybe this could be made more explicit
with something like this:

.exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,

And in the receive loop something like this:

rxbits |= FRAME_RCVD(frame.can_id);

Of course, the definitions of these macros would still have the >>28,
but at a central point, with better explanation. Do you think that's
more understandable? Or do you have a different idea?

Thanks,
   Felix

Re: [PATCH 1/4] selftests: can: Import tst-filter from can-tests

[Vincent Mailhol]

On 24/04/2025 at 23:02, Felix Maurer wrote:
> On 24.04.25 09:42, Vincent Mailhol wrote:
>> On Tue. 22 Apr. 2025 at 21:08, Felix Maurer <fmaurer@redhat.com> wrote:
> [...]
>>> +ALL_TESTS="
>>> +       test_raw_filter
>>> +"
>>> +
>>> +net_dir=$(dirname $0)/..
>>> +source $net_dir/lib.sh
>>> +
>>> +VCANIF="vcan0"
>>
>> Here, you are making the VCANIF variable configuration, but then, in
>> your test_raw_filter.c I see:
>>
>>   #define VCANIF "vcan0"
>>
>> This means that in order to modify the interface, one would have to
>> both modify the .sh script and the .c source. Wouldn't it be possible
>> to centralize this? For example by reading the environment variable in
>> the C file?
>>
>> Or maybe there is a smarter way to pass values in the kernel selftests
>> framework which I am not aware of?
> 
> Good point, I'll try to come up with something to avoid the duplication
> (either from the selftest framework or just for the CAN tests). I'd
> prefer an argument to the program though, as I find this the more usual
> way to pass info if one ever wants to run the test directly.

Passing an argument would be the best. I am not sure how to do this with the
selftests (but I did not investigate either).

>>> +setup()
>>> +{
>>> +       ip link add name $VCANIF type vcan || exit $ksft_skip
>>> +       ip link set dev $VCANIF up
>>> +       pwd
>>> +}

Speaking of which, if you allow the user to modify the interface, then you will
one additional check here to see whether it is a virtual can interface or not
(the ip link commands are not the same for the vcan and the physical can).

Something like:

  CANIF="${CANIF:-vcan}"
  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 500000
  	fi
  	ip link set dev $VCANIF up
  	pwd
  }

>>> +cleanup()
>>> +{
>>> +       ip link delete $VCANIF
>>> +}
>>
>> I guess that this setup() and this cleanup() is something that you
>> will also need in the other can tests. Would it make sense to declare
>> these in a common.sh file and just do a
>>
>>   source common.sh
>>
>> here?
> 
> I usually try to avoid making changes in anticipation of the future. I'm
> not sure if all the tests need a similar environment and would prefer to
> split this when we encounter that they do. Are you okay with that?

Yes, this works. Keep this idea in back of your mind and if there is a need to
reuse those in the future, then it will be a good timing to do the factorize the
code.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Vincent Mailhol]

On 24/04/2025 at 23:02, Felix Maurer wrote:
> On 24.04.25 09:44, Vincent Mailhol wrote:
>> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@redhat.com> wrote:
> [...]
>>> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
>>> index 7fe11e020a1c..8d43053824d2 100644
>>> --- a/tools/testing/selftests/net/can/test_raw_filter.c
>>> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
>>> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>>>         int exp_num_rx;
>>>         int exp_rxbits;
>>>  };
>>> +#define T_EFF (CAN_EFF_FLAG >> 28)
>>> +#define T_RTR (CAN_RTR_FLAG >> 28)
>>
>> I do not like this
>>
>>   >> 28
>>
>> shift. I understand that it is part of the original design, but for
>> me, this is just obfuscation.
>>
>> Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
>> expected values? What benefit does this shift add?
> 
> I agree, that looks like magic numbers and the original design is not
> very nice here. The main reason for the >>28 is that later on values are
> shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
>>> 28, the shift value later is in the range 0-14). See below for a
> slightly different idea.
> 
>>> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>>>  FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>>>         .testcase = 2,
>>>         .id = ID | CAN_EFF_FLAG,
>>>         .mask = CAN_SFF_MASK,
>>>         .exp_num_rx = 4,
>>> -       .exp_rxbits = 4369,
>>> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
>>                          ^
>> What is the meaning of this 1?
> 
> The 1 means that a packet will be received with no flags set.

OK. Now I understand.

> The whole rxbit thing took me a while to understand and the result now
> is not straightforward either. Let's see if we can come up with
> something better.
> 
> The exp_rxbits is basically a bitfield that describes which flags should
> be set on the received frames. Maybe this could be made more explicit
> with something like this:
> 
> .exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,

This is better. But yet, how would this scale in the future if we introduce the
CAN FD? For n flags, you have n combinations.

> And in the receive loop something like this:
> 
> rxbits |= FRAME_RCVD(frame.can_id);
> 
> Of course, the definitions of these macros would still have the >>28,
> but at a central point, with better explanation. Do you think that's
> more understandable? Or do you have a different idea?

The

  >> 28

trick just allows to save a couple line but by doing so, adds a ton of
complexity. What is wrong in writing this:


  FIXTURE_VARIANT(can_filters) {
  	int testcase;
  	canid_t id;
  	canid_t mask;
  	int exp_num_rx;
  	canid_t exp_flags[];
  };

  /* Receive all frames when filtering for the ID in standard frame format */
  FIXTURE_VARIANT_ADD(can_filters, base) {
  	.testcase = 1,
  	.id = ID,
  	.mask = CAN_SFF_MASK,
  	.exp_num_rx = 4,
  	.exp_flags = {
  		0,
  		CAN_EFF_FLAG,
  		CAN_RTR_FLAG,
  		CAN_EFF_FLAG | CAN_RTR_FLAG,
  	},
  };

And then, in your TEST_F(), the do {} while loops becomes a:

  for (int i = 0; i <= variant->exp_num_rx; i++) {
  	/* FD logic here */
  	ret = FD_ISSET(self->sock, &rdfs);
	if (i == variant->exp_num_rx) {
  		ASSERT_EQ(ret == 0);
  	} else (i < variant->exp_num_rx)
  		/* other relevant checks */
  		ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK ==
  		          variant->exp_flags[i]);
  	}
  }

Here, you even check that the frames are received in order.

OK, the bitmap saved some memory, but here, we are speaking of selftests. The
priority is readability. I will happily get rid of the bitmap and just simplify
the logic.



Yours sincerely,
Vincent Mailhol

Re: [PATCH 0/4] selftest: can: Start importing selftests from can-tests

[Vincent Mailhol]

On 24/04/2025 at 23:01, Felix Maurer wrote:
> On 24.04.25 09:45, Vincent Mailhol wrote:
> [...]
>>> Felix Maurer (4):
>>>   selftests: can: Import tst-filter from can-tests
>>>   selftests: can: use kselftest harness in test_raw_filter
>>>   selftests: can: Use fixtures in test_raw_filter
>>>   selftests: can: Document test_raw_filter test cases
>>
>> You are doing a lot of change to the original to the point that this
>> is more a full rewrite. I have no intent of reviewing the first patch
>> which is just the copy paste from the original. If no one else has a
>> strong opinion on this, I would rather prefer if you just squash
>> everything and send a single patch with the final result. This will
>> also save you some effort when migrating the other tests.
>>
>> I have a few comments on the individual patches, but overall very
>> good. Thanks a lot!
> 
> Thank you very much for your feedback! I'll silently include most of it
> and will only reply where I think discussions are necessary.
> 
> For squashing / keeping this as individual patches: I usually like to
> have some kind of history available, but here it might not provide a lot
> of value. I would be fine with squashing as well. If there are any
> stronger opinions on this, keep them coming.

I would normally agree, but here, I did a git diff between the first patch and
the final result, and there is just fives lines of code which you did not touch.

If I want to review each patch (which I normally always do), this becomes double
work. Splitting a series in small patches should simplify the review process,
unfortunately, here, it had the opposite effect for me.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 0/4] selftest: can: Start importing selftests from can-tests

[Jakub Kicinski]

On Tue, 22 Apr 2025 14:02:33 +0200 Felix Maurer wrote:
> I have cc'ed netdev and its reviewers and maintainers to make sure they
> are okay with the location of the tests and the changes to the paths in
> MAINTAINERS. The changes should be merged through linux-can-next and
> subsequent changes will not go to netdev anymore.

Makes perfect sense to me as well, FWIW.

Re: [PATCH 4/4] selftests: can: Document test_raw_filter test cases

[Felix Maurer]

On 24.04.25 17:08, Vincent Mailhol wrote:
> On 24/04/2025 at 23:02, Felix Maurer wrote:
>> On 24.04.25 09:44, Vincent Mailhol wrote:
>>> On Tue. 22 Apr. 2025 at 21:03, Felix Maurer <fmaurer@redhat.com> wrote:
>> [...]
>>>> diff --git a/tools/testing/selftests/net/can/test_raw_filter.c b/tools/testing/selftests/net/can/test_raw_filter.c
>>>> index 7fe11e020a1c..8d43053824d2 100644
>>>> --- a/tools/testing/selftests/net/can/test_raw_filter.c
>>>> +++ b/tools/testing/selftests/net/can/test_raw_filter.c
>>>> @@ -101,94 +101,113 @@ FIXTURE_VARIANT(can_filters) {
>>>>         int exp_num_rx;
>>>>         int exp_rxbits;
>>>>  };
>>>> +#define T_EFF (CAN_EFF_FLAG >> 28)
>>>> +#define T_RTR (CAN_RTR_FLAG >> 28)
>>>
>>> I do not like this
>>>
>>>   >> 28
>>>
>>> shift. I understand that it is part of the original design, but for
>>> me, this is just obfuscation.
>>>
>>> Why just not using CAN_EFF_FLAG and CAN_RTR_FLAG as-is for the
>>> expected values? What benefit does this shift add?
>>
>> I agree, that looks like magic numbers and the original design is not
>> very nice here. The main reason for the >>28 is that later on values are
>> shifted by T_EFF and/or T_RTR, so they shouldn't be too large (with the
>>>> 28, the shift value later is in the range 0-14). See below for a
>> slightly different idea.
>>
>>>> +/* Ignore EFF flag in filter ID if not covered by filter mask */
>>>>  FIXTURE_VARIANT_ADD(can_filters, base_eff) {
>>>>         .testcase = 2,
>>>>         .id = ID | CAN_EFF_FLAG,
>>>>         .mask = CAN_SFF_MASK,
>>>>         .exp_num_rx = 4,
>>>> -       .exp_rxbits = 4369,
>>>> +       .exp_rxbits = (1 | 1 << (T_EFF) | 1 << (T_RTR) | 1 << (T_EFF | T_RTR)),
>>>                          ^
>>> What is the meaning of this 1?
>>
>> The 1 means that a packet will be received with no flags set.
> 
> OK. Now I understand.
> 
>> The whole rxbit thing took me a while to understand and the result now
>> is not straightforward either. Let's see if we can come up with
>> something better.
>>
>> The exp_rxbits is basically a bitfield that describes which flags should
>> be set on the received frames. Maybe this could be made more explicit
>> with something like this:
>>
>> .exp_rxbits = FRAME_NOFLAGS | FRAME_EFF | FRAME_RTR | FRAME_EFFRTR,
> 
> This is better. But yet, how would this scale in the future if we introduce the
> CAN FD? For n flags, you have n combinations.
> 
>> And in the receive loop something like this:
>>
>> rxbits |= FRAME_RCVD(frame.can_id);
>>
>> Of course, the definitions of these macros would still have the >>28,
>> but at a central point, with better explanation. Do you think that's
>> more understandable? Or do you have a different idea?
> 
> The
> 
>   >> 28
> 
> trick just allows to save a couple line but by doing so, adds a ton of
> complexity. What is wrong in writing this:

I don't see anything wrong with it, I like it :) I'll send an updated
version of the patches soon (probably squashed as well).

>   FIXTURE_VARIANT(can_filters) {
>   	int testcase;
>   	canid_t id;
>   	canid_t mask;
>   	int exp_num_rx;
>   	canid_t exp_flags[];
>   };
> 
>   /* Receive all frames when filtering for the ID in standard frame format */
>   FIXTURE_VARIANT_ADD(can_filters, base) {
>   	.testcase = 1,
>   	.id = ID,
>   	.mask = CAN_SFF_MASK,
>   	.exp_num_rx = 4,
>   	.exp_flags = {
>   		0,
>   		CAN_EFF_FLAG,
>   		CAN_RTR_FLAG,
>   		CAN_EFF_FLAG | CAN_RTR_FLAG,
>   	},
>   };
> 
> And then, in your TEST_F(), the do {} while loops becomes a:
> 
>   for (int i = 0; i <= variant->exp_num_rx; i++) {
>   	/* FD logic here */
>   	ret = FD_ISSET(self->sock, &rdfs);
> 	if (i == variant->exp_num_rx) {
>   		ASSERT_EQ(ret == 0);
>   	} else (i < variant->exp_num_rx)
>   		/* other relevant checks */
>   		ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK ==
>   		          variant->exp_flags[i]);
>   	}
>   }
> 
> Here, you even check that the frames are received in order.
> 
> OK, the bitmap saved some memory, but here, we are speaking of selftests. The
> priority is readability. I will happily get rid of the bitmap and just simplify
> the logic.

I fully agree, thank you!
   Felix