Hi Felix, On Sat. 10 May 2025 at 00:07, Felix Maurer <[email protected]> wrote: > Tests for the can subsystem have been in the can-tests repository[1] so > far. Start moving the tests to kernel selftests by importing the current > tst-filter test. The test is now named test_raw_filter and is substantially > updated to be more aligned with the kernel selftests, follow the coding > style, and simplify the validation of received CAN frames. We also include > documentation of the test design. The test verifies that the single filters > on raw CAN sockets work as expected. > > We intend to import more tests from can-tests and add additional test cases > in the future. 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. > > [1]: https://github.com/linux-can/can-tests > > Signed-off-by: Felix Maurer <[email protected]>
Thanks again. I left a set of nitpicks, I expect to give my reviewed-by tag on the next version. > --- > > Notes: > I keep netdev and its reviewers and maintainers in CC because of the > changes to their paths in MAINTAINERS, even though Jakub acked them on > v1. The change should be merged through linux-can-next and subsequent > changes will not go to netdev anymore. > > 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. Ack. > Changes to v1: > - link: > https://lore.kernel.org/linux-can/[email protected]/ > - Squashed import and rewrite into a single commit > - Simplified checking of the received flags > - Pass the interface name through env (easier with the selftest > framework than adding an argument) > > I have not updated test_raw_filter.sh to work with physical CAN > interfaces so far because I don't have one to test this right now and > don't think it's a priority for selftests for now. OK but can you just: s/VCANIF/CANIF/g in the .sh and in the .c files? This way, when the test gets updated to also support the physical interfaces, the patch diff will be smaller. I have the hardware, so it is something I can easily contribute. > 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 | 395 ++++++++++++++++++ > .../selftests/net/can/test_raw_filter.sh | 37 ++ > 6 files changed, 448 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 <[email protected]> > @@ -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 <[email protected]> > 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..5b82e60a03e7 > --- /dev/null > +++ b/tools/testing/selftests/net/can/Makefile > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +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 > + > +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..3c0e43cab1e8 > --- /dev/null > +++ b/tools/testing/selftests/net/can/test_raw_filter.c > @@ -0,0 +1,395 @@ > +// 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/if.h> > + > +#include <linux/can.h> > +#include <linux/can/raw.h> > + > +#include "../../kselftest_harness.h" > + > +#define ID 0x123 > + > +char VCANIF[IFNAMSIZ]; > + > +static 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) > + goto write_err; > + > + frame.can_id = (ID | CAN_RTR_FLAG); > + if (write(sock, &frame, sizeof(frame)) < 0) > + goto write_err; > + > + frame.can_id = (ID | CAN_EFF_FLAG); > + if (write(sock, &frame, sizeof(frame)) < 0) > + goto write_err; > + > + frame.can_id = (ID | CAN_EFF_FLAG | CAN_RTR_FLAG); > + if (write(sock, &frame, sizeof(frame)) < 0) > + goto write_err; > + > + return 0; > + > +write_err: > + perror("write"); > + return 1; > + Nitpick: remove the empty line. > +} > + > +FIXTURE(can_filters) { > + int sock; > +}; > + > +FIXTURE_SETUP(can_filters) > +{ > + struct sockaddr_can addr; > + struct ifreq ifr; > + int recv_own_msgs = 1; > + int s, ret; > + > + s = socket(PF_CAN, SOCK_RAW, CAN_RAW); > + ASSERT_LT(0, s) 0 is a valid fd (OK it is used for the stout, so your code will work, but the comparison still looks unnatural). What about: ASSERT_NE(s, -1) or: ASSERT_GE(s, 0) ? (same comment for the other ASSERT_LE) > + TH_LOG("failed to create CAN_RAW socket: %d", errno); > + > + strncpy(ifr.ifr_name, VCANIF, sizeof(ifr.ifr_name)); > + 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)); > + > + ret = bind(s, (struct sockaddr *)&addr, sizeof(addr)); > + ASSERT_EQ(0, ret) > + TH_LOG("failed bind socket: %d", errno); > + > + self->sock = s; > +} > + > +FIXTURE_TEARDOWN(can_filters) > +{ > + close(self->sock); > +} > + > +FIXTURE_VARIANT(can_filters) { > + int testcase; > + canid_t id; > + canid_t mask; > + int exp_num_rx; > + canid_t exp_flags[]; > +}; > +#define T_EFF (CAN_EFF_FLAG >> 28) > +#define T_RTR (CAN_RTR_FLAG >> 28) These two macros are not needed anymore and can be removed. > +/* 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_RTR_FLAG, > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + 0, > + CAN_RTR_FLAG, > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + 0, > + CAN_RTR_FLAG, > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + 0, > + CAN_RTR_FLAG, > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > + > +/* 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_flags = { > + 0, > + CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + 0, > + CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_EFF_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > + > +/* 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_flags = { > + 0, > + CAN_EFF_FLAG, > + }, > +}; Nitpick: sometimes you have an empty line between the fixtures, sometimes you don't. Can you rationalize this and always have an empty new line between the fixtures? > +/* 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_flags = { > + 0, > + CAN_EFF_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_RTR_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_RTR_FLAG, > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > + > +/* Receive only SFF data frame when filtering for no flags */ > +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_flags = { > + 0, > + }, > +}; > +/* 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_flags = { > + CAN_EFF_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_RTR_FLAG, > + }, > +}; > +/* 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_flags = { > + CAN_EFF_FLAG | CAN_RTR_FLAG, > + }, > +}; > + > +/* 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, > + .mask = CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG, > + .exp_num_rx = 1, > + .exp_flags = { > + 0, > + }, > +}; > +/* 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_flags = { > + CAN_EFF_FLAG, > + }, > +}; > + > +/* 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) > +{ > + struct can_filter rfilter; > + int ret; > + > + rfilter.can_id = variant->id; > + rfilter.can_mask = variant->mask; > + setsockopt(self->sock, SOL_CAN_RAW, CAN_RAW_FILTER, > + &rfilter, sizeof(rfilter)); > + > + TH_LOG("filters: can_id = 0x%08X can_mask = 0x%08X", > + rfilter.can_id, rfilter.can_mask); > + > + ret = send_can_frames(self->sock, variant->testcase); > + ASSERT_EQ(0, ret) > + TH_LOG("failed to send CAN frames"); > + > + for (int i = 0; i <= variant->exp_num_rx; i++) { > + struct can_frame frame; > + struct timeval tv; Nitpick: you can directly initialize this variable: struct timeval tv = { .tv_sec = 0, .tv_usec = 50000, /* 50ms timeout */ }; > + fd_set rdfs; > + > + FD_ZERO(&rdfs); > + FD_SET(self->sock, &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, err: %d)", i, > errno); > + > + ret = FD_ISSET(self->sock, &rdfs); > + if (i == variant->exp_num_rx) { > + ASSERT_EQ(0, ret) > + TH_LOG("too many frames received"); > + } else { > + ASSERT_NE(0, ret) > + TH_LOG("too few frames received"); > + > + ret = read(self->sock, &frame, sizeof(frame)); > + ASSERT_LE(0, ret) > + TH_LOG("failed to read frame %d, err: %d", i, > errno); > + > + TH_LOG("rx: can_id = 0x%08X rx = %d", frame.can_id, > i); > + > + 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"); > + > + ASSERT_EQ(frame.can_id & ~CAN_ERR_MASK, > + variant->exp_flags[i]) > + TH_LOG("received unexpected flags"); > + Remove this empty line. > + } > + } > +} > + > +int main(int argc, char **argv) > +{ > + char *ifname = getenv("VCANIF"); > + > + if (ifname) { > + strncpy(VCANIF, ifname, sizeof(VCANIF) - 1); > + } else { > + printf("VCANIF environment variable must contain the test > interface\n"); > + return KSFT_FAIL; > + } Nitpick: test the error condition first: if (!ifname) { printf("VCANIF environment variable must contain the test interface\n"); return KSFT_FAIL; } strncpy(VCANIF, ifname, sizeof(VCANIF) - 1); > + return test_harness_run(argc, argv); > +} > 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..95f45c3c824b > --- /dev/null > +++ b/tools/testing/selftests/net/can/test_raw_filter.sh > @@ -0,0 +1,37 @@ > +#!/bin/bash > +# SPDX-License-Identifier: GPL-2.0 > + > +ALL_TESTS=" > + test_raw_filter > +" > + > +net_dir=$(dirname $0)/.. > +source $net_dir/lib.sh > + > +export VCANIF="vcan0" Could you make it so that if the environment variable is already set, you do not override it? : ${CANIF:=vcan0} export CANIF This way, users can easily test other interfaces without having to edit the script. > +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 > + check_err $? > + log_test "test_raw_filter" > +} > + > +trap cleanup EXIT > +setup > + > +tests_run > + > +exit $EXIT_STATUS > -- > 2.49.0 > >
