Re: [PATCH can-next v1 5/8] selftests: can: add J1939 wire-format tests against CAN_RAW

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/can: test_j1939_wire missing assertion for CAN_EFF_FLAG on transmitted J1939 frames
--

commit 19b14aab4922e084b42afa782b6d7a0743ee25d8
Author: Oleksij Rempel <o.rempel@pengutronix.de>

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

Add a test that pairs a J1939 socket with a CAN_RAW socket so the actual
CAN frames on the wire are checked in both directions. Both PDU1 (destination
specific) and PDU2 (broadcast) single frames are covered.

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

[ ... ]

> +/* J1939 TX -> CAN_RAW: destination-specific (PDU1) single frame. */
> +TEST_F(j1939_wire, tx_pdu1_to_raw)
> +{
> +	uint8_t payload[8] = { 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88 };
> +	struct can_frame frame;
> +	canid_t id;
> +
> +	ASSERT_EQ(tx_to_raw_send_recv(self, false, PGN_PDU1, SA_PEER, payload,
> +				      &frame), 0);
> +	id = frame.can_id & CAN_EFF_MASK;

[Severity: Medium]
Does this code need to assert that CAN_EFF_FLAG is set before masking it out?

By applying CAN_EFF_MASK, the extended frame flag is dropped. Should there be
a check to verify that the transmitted J1939 frames are actually extended
frames, rather than standard frames that happen to match the lower bits?

> +
> +	EXPECT_EQ(J1939_ID_PRIO(id), PRIO);
> +	EXPECT_EQ(J1939_ID_SA(id), SA_LOCAL);			/* source address */
> +	EXPECT_EQ(J1939_ID_PS(id), SA_PEER);		/* destination (PS) */
> +	EXPECT_EQ(J1939_ID_PF(id), J1939_PF(PGN_PDU1));
> +	ASSERT_EQ(frame.can_dlc, sizeof(payload));
> +	EXPECT_EQ(memcmp(frame.data, payload, sizeof(payload)), 0);
> +}

[ ... ]

> +/* J1939 TX -> CAN_RAW: broadcast (PDU2) single frame. */
> +TEST_F(j1939_wire, tx_pdu2_to_raw)
> +{
> +	uint8_t payload[8] = { 1, 2, 3, 4, 5, 6, 7, 8 };
> +	struct can_frame frame;
> +	canid_t id;
> +
> +	ASSERT_EQ(tx_to_raw_send_recv(self, true, PGN_PDU2, J1939_NO_ADDR,
> +				      payload, &frame), 0);
> +	id = frame.can_id & CAN_EFF_MASK;

[Severity: Medium]
Similarly, should this test in tx_pdu2_to_raw() also assert that the
CAN_EFF_FLAG is set on the received frame.can_id?

> +
> +	EXPECT_EQ(J1939_ID_PRIO(id), PRIO);
> +	EXPECT_EQ(J1939_ID_SA(id), SA_LOCAL);			/* source address */
> +	EXPECT_EQ((id >> 8) & J1939_PGN_MAX, PGN_PDU2);	/* full broadcast PGN */
> +	ASSERT_EQ(frame.can_dlc, sizeof(payload));
> +	EXPECT_EQ(memcmp(frame.data, payload, sizeof(payload)), 0);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014154435.3577983-1-o.rempel@pengutronix.de?part=5