Re: [PATCH can-next v2 6/9] selftests: can: add J1939 wire-format tests against CAN_RAW

[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] The comments on two helpers in
  tools/testing/selftests/net/can/test_j1939_wire.c say more than the…

--

selftests: can: add J1939 wire-format tests against CAN_RAW

This adds a selftest that checks J1939 single frames against a CAN_RAW
socket in both directions, for PDU1 (with the DP bit set) and PDU2. On TX
it checks the CAN id and payload of a J1939 send. On RX it checks the
payload, source address and PGN that the J1939 socket gets for an
injected frame.

> diff --git a/tools/testing/selftests/net/can/test_j1939_wire.c b/tools/testing/selftests/net/can/test_j1939_wire.c
> new file mode 100644
> index 0000000000000..ad542ed241c0b
> --- /dev/null
> +++ b/tools/testing/selftests/net/can/test_j1939_wire.c

[ ... ]

> +/*
> + * Inject one CAN frame from the raw peer and check that the J1939 socket
> + * receives the same CAN_MAX_DLEN-byte payload, from SA_PEER and with PGN @pgn.
> + * Opens the J1939 RX socket into self->j1939 (the fixture teardown closes it).
> + * @bcast selects a broadcast (PDU2) socket and frame; @pgn and @da address the
> + * injected frame.
> + */
> +static bool rx_single_frame_ok(FIXTURE_DATA(j1939_wire) *self, bool bcast,
> +			       pgn_t pgn, uint8_t da,
> +			       const uint8_t payload[CAN_MAX_DLEN])
> +{
> +	struct sockaddr_can src;
> +
> +	if (bcast)
> +		self->j1939 = j1939_open_flags(self->ifindex, J1939_NO_NAME,
> +					       SA_LOCAL, pgn, J1939_OPEN_BCAST);
> +	else
> +		self->j1939 = j1939_open(self->ifindex, SA_LOCAL, pgn);
> +	if (self->j1939 < 0)
> +		return false;
> +
> +	if (!send_raw8_ok(self->raw, PRIO, pgn, da, SA_PEER, payload))
> +		return false;

[Severity: Low]
This isn't a bug, but is the "@bcast selects a broadcast (PDU2) socket
and frame" wording accurate?

In this function, bcast only decides between
j1939_open_flags(..., J1939_OPEN_BCAST), which sets SO_BROADCAST, and a
plain j1939_open().

The injected frame never sees bcast. send_raw8_ok() builds the CAN id
with j1939_canid(), and j1939_canid() picks the PDU class from the PGN
alone:

tools/testing/selftests/net/can/j1939_tests.h:j1939_canid() {
    ...
	/* the destination goes into the PS byte of the PGN field */
	if (J1939_IS_PDU1(pgn))
		id |= da << J1939_ID_PGN_SHIFT;
	return id;
}

The comment above tx_to_raw_send_recv_ok() has the same problem:

 * (the fixture teardown closes it) and connects it to (@da, @pgn); @bcast
 * opens a broadcast (PDU2) socket.

A socket has no PDU class there either. bcast only sets SO_BROADCAST, and
the frame format comes from the connected (@da, @pgn).

All four current callers pair bcast=true with PGN_PDU2 and bcast=false
with PGN_PDU1, so the tests work today. A later caller that goes by these
comments and passes bcast=true with a PDU1 PGN would get a frame with a
destination address, not a broadcast frame.

Both comments are still unchanged at the end of the series, in
"selftests: can: check SO_J1939_PROMISC receive effect".

Could both comments say that @bcast only sets SO_BROADCAST and that @pgn
selects the PDU class?

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