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

[netdev-bot+sashiko]

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] The `bam_broadcast` test in
  tools/testing/selftests/net/can/test_j1939_base.c sends a TP_SIZE…

--

selftests: can: add J1939 base functional tests

This adds a selftest for the SAE J1939 socket interface. It opens a CAN_J1939
sender socket and a receiver socket on vcan. It checks payload integrity for
single frame, TP and ETP sizes, the boundaries between them, and single-frame
and BAM broadcast.

> 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..d01f4c2c186cb
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_base.c

[ ... ]

> +static bool roundtrip_ok(FIXTURE_DATA(j1939_base) *self, size_t size)
> +{

[ ... ]

> +	errno = 0;
> +	ok = send_recv_verify_ok(self->tx, self->rx, tx_buf, size,
> +				 J1939_RECV_TIMEOUT_MS);

[ ... ]

> +/* BAM (broadcast transport). */
> +TEST_F(j1939_base, bam_broadcast)
> +{
> +	ASSERT_TRUE(pair_up_ok(self, SA_RX, J1939_NO_ADDR, PGN_PDU2, true));
> +	EXPECT_TRUE(roundtrip_ok(self, TP_SIZE));

[Severity: Low]
Is J1939_RECV_TIMEOUT_MS long enough for the whole BAM transfer in
bam_broadcast?

roundtrip_ok()->send_recv_verify_ok()->recvfrom_verify_ok() waits with a
single poll() for the fully reassembled message:

tools/testing/selftests/net/can/j1939_tests.h:recvfrom_verify_ok() {
    ...
	if (poll(&pfd, 1, timeout_ms) > 0)
		n = recvfrom(s, rx_buf, len + 1, 0, (struct sockaddr *)src,
			     &src_len);
    ...
}

The kernel paces broadcast transport at a fixed 50 ms. The first delay comes
after the BAM CM frame:

net/can/j1939/transport.c:j1939_session_tx_rts() {
    ...
	if (dat[0] == J1939_TP_CMD_BAM) {
		j1939_tp_schedule_txtimer(session, 50);
    ...
}

Every broadcast DT frame except the last one adds another 50 ms:

net/can/j1939/transport.c:j1939_session_tx_dat() {
    ...
		pdelay = j1939_cb_is_broadcast(&session->skcb) ? 50 :
			j1939_tp_packet_delay;
    ...
}

TP_SIZE is 128 bytes, which is 19 DT packets. So delivery can't finish in
less than about 50 + 18 * 50 = 950 ms. send() returns once the session is
queued, so the poll() starts at about the same time as the session. That
leaves about 1050 ms of the 2000 ms as headroom for a slow VM.

The comment in j1939_tests.h describes the timeout as a wait for one frame:

/*
 * How long to wait for an expected frame. Not a protocol timer: a guess with
 * room for a slow VM.
 */
#define J1939_RECV_TIMEOUT_MS	2000

For BAM, though, it limits the whole message, and about half of it goes to
the protocol's own pacing.

Could this fail spuriously on a loaded KASAN or lockdep QEMU-TCG runner, where
the soft hrtimers that drive the 19 packets are deferred to ksoftirqd?

The timeout also doesn't grow with the message size. If TP_SIZE were raised
above about 280 bytes (more than 40 BAM packets), wouldn't bam_broadcast fail
every time?

The unicast TP and ETP tests aren't affected, since j1939_tp_packet_delay
defaults to 0. J1939_RECV_TIMEOUT_MS, its comment, and the bam_broadcast size
are unchanged at the end of the series.

Would it make sense to scale the timeout with the number of BAM packets, or
to use a separate, longer timeout for broadcast transport?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-j1939-selftests-v2-0-cf797c83317b%40pengutronix.de