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

[Wei Fang]

> > +/* 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?

This combination does not exist in practice for ENETC. Both PF(host) and VF(VM)
use the same endianness, so no need an explicit endianness annotation.

> 
> > +
> > +/* 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.

This is not an issue, no need fix.