Re: [PATCH can-next v3 3/9] selftests: can: add J1939 base functional tests

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] test_j1939_base never opens a CAN_RAW socket, yet it skips the
  whole suite when CAN_RAW is missing.

--

selftests: can: add J1939 base functional tests

This adds a J1939 functional selftest. It sends between CAN_J1939 sockets
over vcan and checks payload integrity for single frame, TP and ETP sizes,
the boundaries between them, and single-frame and BAM broadcast. It also
enables CONFIG_CAN_J1939 and CONFIG_CAN_RAW in the selftest config.

> diff --git a/tools/testing/selftests/net/can/test_j1939_base.c b/tools/testing/selftests/net/can/test_j1939_base.c
> new file mode 100644
> index 0000000000000..7bf492c767bf6
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_base.c
> @@ -0,0 +1,236 @@

[ ... ]

> +int main(int argc, char **argv)
> +{
> +	return j1939_test_main(argc, argv);
> +}

[Severity: Low]
Is this binary skipped on a kernel that has CONFIG_CAN_J1939 but not
CONFIG_CAN_RAW?

In j1939_tests.h, j1939_test_main() makes every J1939 binary depend on
CAN_RAW:

	if (can_proto_missing(SOCK_RAW, CAN_RAW)) {
		fprintf(stderr, "# kernel lacks CAN_RAW\n");
		return KSFT_SKIP;
	}

test_j1939_base only uses SOCK_DGRAM/CAN_J1939 sockets:

- pair_up_ok() calls j1939_open_flags() and j1939_connect_ok().
- bcast_needs_so_broadcast calls j1939_open().
- The data path is send_recv_verify_ok() and recv_timeout() on those
  sockets.

Nothing in this file calls open_raw(), open_raw_rcvbuf() or
J1939_SETUP_RAW.

On such a kernel, the single frame, TP, ETP, size boundary, broadcast, BAM
and SO_BROADCAST tests would all report SKIP, even though they could run.

The v2 changelog says CONFIG_CAN_RAW is added here because "every test
needs it". That seems true only because of this check, not because of
anything the base test does.

Could the CAN_RAW check move into the binaries that actually open raw
sockets, or become a per-binary or per-test requirement?

The check itself came from the earlier patch in this series ("selftests:
can: add J1939 test helper header and library"). This is the first binary
that gets skipped by it for no reason.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-j1939-selftests-v3-0-5a80277ab363%40pengutronix.de