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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_mailbox.h b/drivers/net/ethernet/freescale/enetc/enetc_mailbox.h
> index 832d2f6ddcd8..6fa66c863748 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_mailbox.h
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_mailbox.h

[ ... ]

> @@ -204,6 +220,32 @@ struct enetc_msg_mac_exact_filter {
>  	struct enetc_mac_addr mac[];
>  };
>  
> +/* message format of class_id 0x20 for hash MAC filter.
> + * cmd_id 0x3: set MAC hash table
> + */
> +struct enetc_msg_mac_hash_filter {
> +	struct enetc_msg_header hdr;
> +	/* bit 0 ~ 5: ENETC_MSG_MAC_HASH_SIZE
> +	 * bit 6~7: ENETC_MSG_MAC_TYPE
> +	 */
> +	u8 sz_type;
> +	u8 resv[3];
> +	u32 hash_tbl[];
> +};

[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

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_msg.c b/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> index 08a9ffdd3eb0..22f5485e968a 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_msg.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_msg.c

[ ... ]

> @@ -116,6 +242,10 @@ static u16 enetc_msg_handle_mac_filter(struct enetc_pf *pf, int vf_id,
>  	switch (msg_hdr->cmd_id) {
>  	case ENETC_MSG_SET_PRIMARY_MAC:
>  		return enetc_msg_set_vf_primary_mac_addr(pf, vf_id, vf_msg);
> +	case ENETC_MSG_SET_MAC_HASH_TABLE:
> +		return enetc_msg_set_vf_mac_hash_filter(pf, vf_id, vf_msg);
> +	case ENETC_MSG_SET_MAC_PROMISC_MODE:
> +		return enetc_msg_set_vf_mac_promisc_mode(pf, vf_id, vf_msg);
>  	default:
>  		return ENETC_PF_MSG_NOTSUPP;
>  	}

[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.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc_vf.c b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> index 7dcb4a0246f5..a60af40d8546 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc_vf.c
> @@ -107,8 +107,12 @@ static int enetc_msg_vsi_send(struct enetc_si *si, struct enetc_msg_swbd *msg)
>  		case ENETC_MSG_CLASS_ID_CMD_TIMEOUT:
>  			err = -ETIME;
>  			break;
> -		case ENETC_MSG_CLASS_ID_INVALID_MSG_LEN:
>  		case ENETC_MSG_CLASS_ID_MAC_FILTER:
> +			if (FIELD_GET(ENETC_PF_MSG_CLASS_CODE, pf_msg) ==
> +			    ENETC_MF_CLASS_CODE_UCF_DENY)
> +				return -EACCES;
> +			fallthrough;
> +		case ENETC_MSG_CLASS_ID_INVALID_MSG_LEN:
>  			err = -EINVAL;
>  			break;
>  		case ENETC_MSG_CLASS_ID_CMD_NOT_PERMITTED:

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