Re: [PATCH v4 net-next 09/15] net: enetc: add MAC address filtering support for VFs of ENETC v4

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] DISCARDED (resolved later in the same series).
- [Low] struct enetc_msg_mac_hash_filter
  (drivers/net/ethernet/freescale/enetc/enetc_mailbox.h:226-232)…

--

net: enetc: add MAC address filtering support for VFs of ENETC v4

This adds two VSI-to-PSI mailbox commands under the MAC filter class so an
ENETC v4 VF can ask the PF to program its unicast/multicast MAC hash filter
tables and to toggle promiscuous mode, with trusted-VF checks and a limit of
8 multicast hash buckets for untrusted VFs. It also takes si->gen_lock around
the read-modify-write of the shared PSIPMMR register, and clears a VF's
unicast hash filter and promiscuous mode when it loses trusted status.

A couple of questions below.

[ ... ]

[Severity: Low]

Should hash_tbl[] carry an explicit endianness annotation?

This struct describes a wire format: the message lives in a DMA buffer that
the mailbox hardware copies from the VSI buffer into the PSI buffer, and the
header in this same file already declares the byte order of its multi-byte
field:

	struct enetc_msg_header {
		__be16 crc16;
		...
	};

hash_tbl[] is the only other multi-byte payload field and it is plain u32,
so both endpoints are implicitly required to have the same byte order. The
producer fills it in host order and enetc_msg_set_vf_mac_hash_filter()
recombines it in host order:

		uc_hash = (u64)msg->hash_tbl[1] << 32 | msg->hash_tbl[0];

If a big-endian VSI kernel ever talks to a little-endian PSI (or a
non-Linux PSI implementing the documented protocol), would the hash buckets
end up byte-swapped, so the SI filters on the wrong buckets while both
sides report success? The CRC16 is computed byte-wise over the buffer, so
it would not catch that.

Would declaring the array as __le32 and converting with cpu_to_le32() /
le32_to_cpu() on both ends be preferable now, while the format is being
introduced?

+
+/* message format of class_id 0x20 for MAC promiscuous mode.
+ * cmd_id 0x5: set MAC promiscuous mode
+ */
+struct enetc_msg_mac_promisc_mode {
+	struct enetc_msg_header hdr;
+	/* bit 0: ENETC_MSG_MAC_FLUSH_MACS
+	 * bit 1: ENETC_MSG_MAC_PROMISC_MODE
+	 * bit 6~7: ENETC_MSG_MAC_TYPE
+	 */
+	u8 config;
+	u8 resv[15];
+};
+
 /* The generic message format applies to the following messages:
  * Get IP revision message, class_id 0xf0.
  * cmd_id 1: get IP minor revision

[ ... ]

[Severity: Low]

At this commit, is there anything in the tree that sends
ENETC_MSG_SET_MAC_HASH_TABLE or ENETC_MSG_SET_MAC_PROMISC_MODE?

Both new handlers here, and the UCF_DENY mapping added to
enetc_msg_vsi_send() below, only become reachable with the later patch in
the series ("net: enetc: implement ndo_set_rx_mode_async for ENETC v4 VF"),
which adds enetc_vf_set_mac_hash_filter() and enetc_vf_set_mac_promisc()
and wires .ndo_set_rx_mode_async = enetc_vf_set_rx_mode into
enetc4_ndev_ops. So this is only a patch-ordering observation, the receiver
landing one patch before the sender, not a defect at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909100733.1139689-1-wei.fang%40oss.nxp.com