Re: [PATCH net-next v2 4/6] enic: validate V2 VF configuration replies

[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] At this commit the message says the patch adds VF-side
  administrative-MAC notification operations, but…
- [Low] enic_mbox_vf_add_del_macs(), enic_mbox_vf_add_del_mac() and
  enic_mbox_vf_set_pkt_filter() have no callers at this commit, and…

--

enic: validate V2 VF configuration replies

This commit adds VF-side V2 mailbox message types for MAC filter add/delete
and packet-filter settings, and validates their replies (framing, echoed
entries, result counts and applied flags). A reply that is malformed or
contradictory, or that reports registration loss, forces the VF to
re-register. The PF side stays dormant.

> Add the established V2 mailbox operations for MAC filters,
> administrative-MAC notifications, and packet-filter settings. Keep request
> buffers alive until the VF request completes so detailed replies can be
> checked entry by entry.

[ ... ]

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index 30a5f6fda676d..265b58cd39be3 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c

[ ... ]

> @@ -680,6 +905,34 @@ static bool enic_mbox_vf_payload_ok(struct enic *enic, u8 msg_type,
>  	return true;
>  }
>  
> +static void enic_mbox_vf_malformed_msg(struct enic *enic, u8 msg_type,
> +				       u64 msg_num)
> +{
> +	u8 expected;
> +
> +	switch (msg_type) {
> +	case ENIC_MBOX_PF_LINK_STATE_NOTIF:
> +	case ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF:
> +		enic_mbox_vf_require_reconnect(enic);
> +		return;

[ ... ]

> @@ -718,11 +977,21 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
>  		size_t exp = sizeof(struct enic_mbox_pf_link_state_notif_msg);
>  
>  		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
> -					     payload_len, exp))
> +					     payload_len, exp)) {
> +			enic_mbox_vf_malformed_msg(enic, hdr->msg_type, msg_num);
>  			return;
> +		}
>  		enic_mbox_vf_handle_link_state(enic, payload, msg_num);
>  		break;
>  	}
> +	case ENIC_MBOX_VF_ADD_DEL_MAC_REPLY:
> +		enic_mbox_vf_handle_add_del_mac_reply(enic, payload,
> +						      payload_len, msg_num);
> +		break;
> +	case ENIC_MBOX_VF_SET_PKT_FILTER_REPLY:
> +		enic_mbox_vf_handle_set_pkt_filter_reply(enic, payload,
> +							 payload_len, msg_num);
> +		break;
>  	default:
>  		netdev_dbg(enic->netdev,
>  			   "MBOX: VF unhandled msg type %u\n",

[Severity: Low]

Is the administrative-MAC part of the commit message accurate for this
commit? The commit message says:

  Add the established V2 mailbox operations for MAC filters,
  administrative-MAC notifications, and packet-filter settings.

This patch adds cases to enic_mbox_vf_process_msg() for
ENIC_MBOX_VF_ADD_DEL_MAC_REPLY and ENIC_MBOX_VF_SET_PKT_FILTER_REPLY. There
is no case for ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF, so:

- A well-formed notification falls through to the default netdev_dbg()
  branch. It never gets an ENIC_MBOX_PF_SET_ADMIN_MAC_ACK.
- A malformed notification goes through enic_mbox_vf_malformed_msg(),
  which forces a reconnect.

The later patch in this series, "enic: manage V2 VF station and
administrative MAC", appears to add the ENIC_MBOX_PF_SET_ADMIN_MAC_NOTIF
case and enic_mbox_vf_handle_admin_mac(). Also, no in-tree PF sends this
notification yet, because the V2 PF path is dormant.

Could the commit message say that this patch only adds the admin-MAC
message definitions, and that the later patch adds the handling?

[ ... ]

> @@ -880,14 +1166,182 @@ int enic_mbox_vf_unregister(struct enic *enic)
>  	if (registered)
>  		return -EACCES;
>  	return 0;
>  }
>  
> +int enic_mbox_vf_add_del_macs(struct enic *enic,
> +			      struct enic_mac_addr *macs, u16 num_macs)
> +{

[ ... ]

> +int enic_mbox_vf_add_del_mac(struct enic *enic, const u8 *addr, bool add,
> +			     bool station)
> +{

[ ... ]

> +int enic_mbox_vf_set_pkt_filter(struct enic *enic, int directed,
> +				int multicast, int broadcast,
> +				int promisc, int allmulti, u16 *applied_flags)
> +{

[Severity: Low]

This isn't a bug, but at this point in the series nothing calls
enic_mbox_vf_add_del_macs(), enic_mbox_vf_add_del_mac() or
enic_mbox_vf_set_pkt_filter(). ENIC_MAC_ADDR_FLAG_PERMANENT_MASK in
enic_mbox.h is also unused.

Two later patches wire them up: "enic: manage V2 VF station and
administrative MAC" and "enic: configure V2 VF receive mode over mailbox".
The callers are enic_vf_station_addr_del(), enic_vf_station_addr_replace(),
enic_vf_sync_mac_filters() and enic_vf_set_pkt_filter().

Would it make sense to move these helpers into the patches that add their
callers?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-b4-enic-sriov-v2-vf-receive-control-v2-0-5962c10061b2%40cisco.com