[PATCH net 0/2] enic: fix V2 VF mailbox reply matching and carrier reopen

[PATCH net 0/2] enic: fix V2 VF mailbox reply matching and carrier reopen

[Satish Kharat]

Preserve ENIC V2 VF carrier state across netdev reopen and match mailbox
replies to their originating requests.

A V2 VF receives carrier state from PF MBOX notifications. enic_stop()
forces carrier off, but enic_open() does not request another notification
or restore the previous state. An ordinary netdev close/open can therefore
leave the VF without carrier until the PF sends another link-state
notification.

The first patch preserves the last PF-reported link state across an
ordinary netdev close/open. Internal reset paths invalidate the saved state
before reopening the datapath, so carrier remains off until the PF provides
a new notification.

Mailbox messages carry a message number that replies and acknowledgments
echo. ENIC currently assigns a new number to outgoing replies and accepts
VF replies by message type alone. After a request times out, a delayed
reply can therefore satisfy a later request of the same type.

The second patch makes PF replies and the VF link-state acknowledgment echo
the initiating message number. The VF accepts a reply only when both its
type and message number match the pending request. Reply handling and
timeout cleanup are protected by the same lock, and message numbering
remains monotonic across admin-channel reopen.

Validation:

- The VF module with this series applied passed multiple netdev close/open
  and ENIC module unload/reload cycles, as well as guest reboot testing,
  with carrier restored as expected after each operation.
- With this series folded into the full SR-IOV development stack, VFIO
  guests passed bidirectional cross-DUT PF-to-VF, VF-to-PF, and VF-to-VF
  traffic using standard-sized and 8972-byte jumbo ICMP packets.

Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
Satish Kharat (2):
      enic: preserve V2 VF carrier across netdev reopen
      enic: match mailbox replies to request numbers

 drivers/net/ethernet/cisco/enic/enic.h      |  26 ++-
 drivers/net/ethernet/cisco/enic/enic_main.c |  25 ++-
 drivers/net/ethernet/cisco/enic/enic_mbox.c | 309 +++++++++++++++++-----------
 drivers/net/ethernet/cisco/enic/enic_mbox.h |   2 +
 4 files changed, 233 insertions(+), 129 deletions(-)
---
base-commit: 2188569e7e1b0bc3f3b557dc97ab7a02befc11c8
change-id: 20260829-b4-enic-v2-mbox-fixes-net-f8588b813e4d

Best regards,
--  
Satish Kharat <satishkh@cisco.com>

[PATCH net 1/2] enic: preserve V2 VF carrier across netdev reopen

[Satish Kharat]

A V2 VF receives carrier state only from PF MBOX notifications.
enic_stop() forces carrier off, but enic_open() does not request a fresh
notification or restore the previous one.  An ordinary down/up cycle
therefore leaves the VF in NO-CARRIER and unable to pass traffic until the
PF repeats the link-state command, even when the physical link remained
up.

Cache each valid PF link-state notification.  Serialize updates with the V2
VF datapath running state.  Keep carrier off while the netdev is stopped.
Restore the cached state after an ordinary open.  Before either internal
reset reopens the datapath, invalidate the cache.  Carrier then remains off
until re-registration receives a fresh PF link-state notification.

Fixes: 72b65c94058e ("enic: add MBOX VF handlers for capability, register and link state")
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic.h      | 13 ++++++++++
 drivers/net/ethernet/cisco/enic/enic_main.c | 12 +++++++++-
 drivers/net/ethernet/cisco/enic/enic_mbox.c | 37 +++++++++++++++++++++++++----
 drivers/net/ethernet/cisco/enic/enic_mbox.h |  2 ++
 4 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 4a67947cfb9f..2822fbfb6474 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -137,6 +137,12 @@ struct enic_port_profile {
 	u8 mac_addr[ETH_ALEN];
 };
 
+enum enic_vf_link_state {
+	ENIC_VF_LINK_STATE_UNKNOWN,
+	ENIC_VF_LINK_STATE_DOWN,
+	ENIC_VF_LINK_STATE_UP,
+};
+
 /* enic_rfs_fltr_node - rfs filter node in hash table
  *	@@keys: IPv4 5 tuple
  *	@flow_id: flow_id of clsf filter provided by kernel
@@ -312,6 +318,13 @@ struct enic {
 	unsigned int admin_msg_count;	/* current depth of admin_msg_list */
 	void (*admin_rq_handler)(struct enic *enic, void *buf,
 				 unsigned int len);
+	/* The PF is authoritative for a V2 VF's carrier.  Keep the last
+	 * notification across an ordinary netdev close/open and serialize it
+	 * against the open/stop carrier transition.
+	 */
+	spinlock_t vf_link_state_lock;
+	enum enic_vf_link_state vf_link_state;
+	bool vf_link_running;
 
 	/* MBOX protocol state — mbox_lock serializes admin WQ sends */
 	struct mutex mbox_lock;
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 0baef7a120ec..48d16ef18c49 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -1800,6 +1800,8 @@ static int enic_open(struct net_device *netdev)
 
 	enic_notify_timer_start(enic);
 	enic_rfs_timer_start(enic);
+	if (enic_is_sriov_vf_v2(enic))
+		enic_mbox_vf_link_state_set_running(enic, true);
 
 	return 0;
 
@@ -1853,7 +1855,10 @@ static int enic_stop(struct net_device *netdev)
 	for (i = 0; i < enic->rq_count; i++)
 		napi_disable(&enic->napi[i]);
 
-	netif_carrier_off(netdev);
+	if (enic_is_sriov_vf_v2(enic))
+		enic_mbox_vf_link_state_set_running(enic, false);
+	else
+		netif_carrier_off(netdev);
 	if (vnic_dev_get_intr_mode(enic->vdev) == VNIC_DEV_INTR_MODE_MSIX)
 		for (i = 0; i < enic->wq_count; i++)
 			napi_disable(&enic->napi[enic_cq_wq(enic, i)]);
@@ -2271,6 +2276,8 @@ static void enic_reset(struct work_struct *work)
 		enic_admin_channel_close(enic);
 
 	enic_stop(enic->netdev);
+	if (enic_is_sriov_vf_v2(enic))
+		enic_mbox_vf_link_state_reset(enic);
 
 	enic_dev_soft_reset(enic);
 	enic_reset_addr_lists(enic);
@@ -2315,6 +2322,8 @@ static void enic_tx_hang_reset(struct work_struct *work)
 
 	enic_dev_hang_notify(enic);
 	enic_stop(enic->netdev);
+	if (enic_is_sriov_vf_v2(enic))
+		enic_mbox_vf_link_state_reset(enic);
 
 	enic_dev_hang_reset(enic);
 	enic_reset_addr_lists(enic);
@@ -3015,6 +3024,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	enic = netdev_priv(netdev);
 	enic->netdev = netdev;
 	enic->pdev = pdev;
+	spin_lock_init(&enic->vf_link_state_lock);
 
 	/* Setup PCI resources
 	 */
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index 2fb0f1e2ff50..ad79d3951f3d 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -396,25 +396,32 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload)
 {
 	struct enic_mbox_pf_link_state_notif_msg *notif = payload;
 	struct enic_mbox_pf_link_state_ack_msg ack = {};
+	u32 link_state = le32_to_cpu(notif->link_state);
 	int err;
 
-	switch (le32_to_cpu(notif->link_state)) {
+	spin_lock_bh(&enic->vf_link_state_lock);
+	switch (link_state) {
 	case ENIC_MBOX_LINK_STATE_ENABLE:
-		if (!netif_carrier_ok(enic->netdev))
+		enic->vf_link_state = ENIC_VF_LINK_STATE_UP;
+		if (enic->vf_link_running &&
+		    !netif_carrier_ok(enic->netdev))
 			netif_carrier_on(enic->netdev);
 		netdev_dbg(enic->netdev, "MBOX: link state -> UP\n");
 		break;
 	case ENIC_MBOX_LINK_STATE_DISABLE:
-		if (netif_carrier_ok(enic->netdev))
+		enic->vf_link_state = ENIC_VF_LINK_STATE_DOWN;
+		if (enic->vf_link_running &&
+		    netif_carrier_ok(enic->netdev))
 			netif_carrier_off(enic->netdev);
 		netdev_dbg(enic->netdev, "MBOX: link state -> DOWN\n");
 		break;
 	default:
 		netdev_warn(enic->netdev, "MBOX: unknown link state %u\n",
-			    le32_to_cpu(notif->link_state));
+			    link_state);
 		ack.ack.ret_major = cpu_to_le16(ENIC_MBOX_ERR_GENERIC);
 		break;
 	}
+	spin_unlock_bh(&enic->vf_link_state_lock);
 
 	err = enic_mbox_send_msg(enic, ENIC_MBOX_PF_LINK_STATE_ACK,
 				 ENIC_MBOX_DST_PF, &ack, sizeof(ack));
@@ -423,6 +430,28 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload)
 			    "MBOX: failed to send link state ACK: %d\n", err);
 }
 
+void enic_mbox_vf_link_state_reset(struct enic *enic)
+{
+	spin_lock_bh(&enic->vf_link_state_lock);
+	enic->vf_link_state = ENIC_VF_LINK_STATE_UNKNOWN;
+	if (enic->vf_link_running && netif_carrier_ok(enic->netdev))
+		netif_carrier_off(enic->netdev);
+	spin_unlock_bh(&enic->vf_link_state_lock);
+}
+
+void enic_mbox_vf_link_state_set_running(struct enic *enic, bool running)
+{
+	spin_lock_bh(&enic->vf_link_state_lock);
+	enic->vf_link_running = running;
+	if (running && enic->vf_link_state == ENIC_VF_LINK_STATE_UP) {
+		if (!netif_carrier_ok(enic->netdev))
+			netif_carrier_on(enic->netdev);
+	} else if (netif_carrier_ok(enic->netdev)) {
+		netif_carrier_off(enic->netdev);
+	}
+	spin_unlock_bh(&enic->vf_link_state_lock);
+}
+
 static bool enic_mbox_vf_payload_ok(struct enic *enic, u8 msg_type,
 				    u16 payload_len, size_t min_len)
 {
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.h b/drivers/net/ethernet/cisco/enic/enic_mbox.h
index 15e30ee2b0ed..60409bad2f28 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.h
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.h
@@ -88,6 +88,8 @@ void enic_mbox_init(struct enic *enic);
 int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
 		       void *payload, u16 payload_len);
 int enic_mbox_send_link_state(struct enic *enic, u16 vf_id, u32 link_state);
+void enic_mbox_vf_link_state_reset(struct enic *enic);
+void enic_mbox_vf_link_state_set_running(struct enic *enic, bool running);
 int enic_mbox_vf_capability_check(struct enic *enic);
 int enic_mbox_vf_register(struct enic *enic);
 int enic_mbox_vf_unregister(struct enic *enic);

-- 
2.43.0

[PATCH net 2/2] enic: match mailbox replies to request numbers

[Satish Kharat]

The version-1 VF mailbox protocol identifies every message with a message
number, and a reply or acknowledgment echoes the number of the message it
answers.  ENIC instead generates a new number for outgoing replies and
accepts a VF reply by message type alone.

If a request times out, a delayed reply can therefore satisfy a subsequent
request of the same type and cause the VF to consume the result of the old
request.

Allow replies to reuse the initiating message number. Make the in-tree PF
handlers and the VF link-state acknowledgment echo that number. Record the
expected reply type and message number on the VF, and require both values
to match before accepting a reply.

Protect expected-reply state with a lock so reply acceptance and timeout
invalidation cannot race. Keep message numbers monotonic across an admin-
channel reopen so a delayed reply from an earlier channel generation cannot
match a new request.

Reply-number echo is part of the established version-1 protocol, so this
remains compatible with deployed V2-capable PF implementations that already
echo msg_num.

Fixes: 72b65c94058e ("enic: add MBOX VF handlers for capability, register and link state")
Signed-off-by: Satish Kharat <satishkh@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic.h      |  13 +-
 drivers/net/ethernet/cisco/enic/enic_main.c |  13 +-
 drivers/net/ethernet/cisco/enic/enic_mbox.c | 272 ++++++++++++++++------------
 3 files changed, 174 insertions(+), 124 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic.h b/drivers/net/ethernet/cisco/enic/enic.h
index 2822fbfb6474..7a509a056990 100644
--- a/drivers/net/ethernet/cisco/enic/enic.h
+++ b/drivers/net/ethernet/cisco/enic/enic.h
@@ -329,15 +329,14 @@ struct enic {
 	/* MBOX protocol state — mbox_lock serializes admin WQ sends */
 	struct mutex mbox_lock;
 	u64 mbox_msg_num;
-	/* MBOX request-reply state.  mbox_expected_reply is written and
-	 * cleared by the process-context request helpers (capability/register/
-	 * unregister) and only read by the admin_msg_work receive handlers, so
-	 * it is annotated with READ_ONCE()/WRITE_ONCE() rather than locked:
-	 * only one request is in flight at a time (requesters run under RTNL or
-	 * single-threaded probe/remove), so each request is serialized and its
-	 * reply completes mbox_comp before the next request is issued.
+	/* MBOX request-reply state.  Existing request callers allow only one
+	 * request in flight.  The state lock arbitrates reply acceptance against
+	 * timeout invalidation, while mbox_comp publishes the accepted result to
+	 * the requester.
 	 */
 	struct completion mbox_comp;
+	spinlock_t mbox_state_lock;	/* protects expected reply state */
+	u64 mbox_expected_msg_num;
 	u8 mbox_expected_reply;
 	bool mbox_initialized;
 
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 48d16ef18c49..14d1637a4342 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -2206,9 +2206,10 @@ static void enic_admin_chan_reopen(struct enic *enic)
 {
 	int err;
 
-	/* Install the MBOX receive handler and reset the sequence number
-	 * before opening the channel, so the handler is in place before the
-	 * admin interrupt is unmasked and no early completion is dropped.
+	/* Install the MBOX receive handler and clear pending reply state before
+	 * opening the channel, so the handler is in place before the admin
+	 * interrupt is unmasked and no early completion is dropped.  Keep the
+	 * sequence number monotonic across channel generations.
 	 */
 	enic_mbox_init(enic);
 
@@ -2220,7 +2221,7 @@ static void enic_admin_chan_reopen(struct enic *enic)
 	 * registration over a dead channel.
 	 */
 	if (enic_is_sriov_vf_v2(enic))
-		enic->vf_registered = false;
+		WRITE_ONCE(enic->vf_registered, false);
 
 	err = enic_admin_channel_open(enic);
 	if (err) {
@@ -3349,7 +3350,7 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 err_out_admin_close:
 	if (enic_is_sriov_vf_v2(enic)) {
-		if (enic->vf_registered) {
+		if (READ_ONCE(enic->vf_registered)) {
 			int unreg_err = enic_mbox_vf_unregister(enic);
 
 			if (unreg_err)
@@ -3402,7 +3403,7 @@ static void enic_remove(struct pci_dev *pdev)
 		 * touching a netdev that is being torn down.
 		 */
 		if (enic_is_sriov_vf_v2(enic)) {
-			if (enic->vf_registered) {
+			if (READ_ONCE(enic->vf_registered)) {
 				int unreg_err = enic_mbox_vf_unregister(enic);
 
 				if (unreg_err)
diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
index ad79d3951f3d..5c93ca49552a 100644
--- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
+++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c
@@ -18,22 +18,25 @@
 #define ENIC_MBOX_POLL_TIMEOUT_US	5000000
 #define ENIC_MBOX_POLL_INTERVAL_US	100
 
-static void enic_mbox_fill_hdr(struct enic *enic, struct enic_mbox_hdr *hdr,
-			       u8 msg_type, u16 dst_vnic_id, u16 msg_len)
+static void enic_mbox_fill_hdr(struct enic_mbox_hdr *hdr, u8 msg_type,
+			       u16 dst_vnic_id, u16 msg_len, u64 msg_num)
 {
 	memset(hdr, 0, sizeof(*hdr));
 	hdr->dst_vnic_id = cpu_to_le16(dst_vnic_id);
 	hdr->msg_type = msg_type;
 	hdr->msg_len = cpu_to_le16(msg_len);
-	hdr->msg_num = cpu_to_le64(++enic->mbox_msg_num);
+	hdr->msg_num = cpu_to_le64(msg_num);
 }
 
-int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
-		       void *payload, u16 payload_len)
+static int enic_mbox_send_msg_id(struct enic *enic, u8 msg_type,
+				 u16 dst_vnic_id, void *payload,
+				 u16 payload_len, u64 msg_num, bool reuse_msg_num,
+				 u8 expected_reply)
 {
 	size_t total_len = sizeof(struct enic_mbox_hdr) + payload_len;
 	struct vnic_wq *wq = &enic->admin_wq;
 	struct wq_enet_desc *desc;
+	bool reply_expected = false;
 	unsigned long timeout;
 	dma_addr_t dma_addr;
 	u16 vlan_tag;
@@ -68,7 +71,21 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
 		goto unlock;
 	}
 
-	enic_mbox_fill_hdr(enic, buf, msg_type, dst_vnic_id, total_len);
+	/* Replies reuse the initiating message number.  Requests and
+	 * notifications allocate a new one.
+	 */
+	if (!reuse_msg_num)
+		msg_num = ++enic->mbox_msg_num;
+	if (expected_reply) {
+		reinit_completion(&enic->mbox_comp);
+		spin_lock_bh(&enic->mbox_state_lock);
+		enic->mbox_expected_reply = expected_reply;
+		enic->mbox_expected_msg_num = msg_num;
+		spin_unlock_bh(&enic->mbox_state_lock);
+		reply_expected = true;
+	}
+
+	enic_mbox_fill_hdr(buf, msg_type, dst_vnic_id, total_len, msg_num);
 	if (payload_len) {
 		void *dst = buf + sizeof(struct enic_mbox_hdr);
 
@@ -139,18 +156,66 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
 		   "MBOX send msg_type %u dst %u vlan %u err %d\n",
 		   msg_type, dst_vnic_id, vlan_tag, err);
 unlock:
+	if (err && reply_expected) {
+		spin_lock_bh(&enic->mbox_state_lock);
+		if (enic->mbox_expected_reply == expected_reply &&
+		    enic->mbox_expected_msg_num == msg_num) {
+			enic->mbox_expected_reply = 0;
+			enic->mbox_expected_msg_num = 0;
+		}
+		spin_unlock_bh(&enic->mbox_state_lock);
+	}
 	mutex_unlock(&enic->mbox_lock);
 	return err;
 }
 
+int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
+		       void *payload, u16 payload_len)
+{
+	return enic_mbox_send_msg_id(enic, msg_type, dst_vnic_id, payload,
+				     payload_len, 0, false, 0);
+}
+
+static int enic_mbox_send_reply(struct enic *enic, u8 msg_type,
+				u16 dst_vnic_id, void *payload, u16 payload_len,
+				u64 msg_num)
+{
+	return enic_mbox_send_msg_id(enic, msg_type, dst_vnic_id, payload,
+				     payload_len, msg_num, true, 0);
+}
+
+static int enic_mbox_vf_send_request(struct enic *enic, u8 request_type,
+				     u8 expected_reply, void *payload,
+				     u16 payload_len)
+{
+	return enic_mbox_send_msg_id(enic, request_type, ENIC_MBOX_DST_PF,
+				     payload, payload_len, 0, false,
+				     expected_reply);
+}
+
 static int enic_mbox_wait_reply(struct enic *enic, unsigned long timeout_ms)
 {
 	unsigned long left;
+	int err = 0;
 
 	left = wait_for_completion_timeout(&enic->mbox_comp,
 					   msecs_to_jiffies(timeout_ms));
+	if (left)
+		return 0;
+
+	/* Invalidate a request that the handler has not already accepted.  A
+	 * delayed reply cannot match a later request because message numbers are
+	 * monotonic across channel reopen.
+	 */
+	spin_lock_bh(&enic->mbox_state_lock);
+	if (enic->mbox_expected_reply) {
+		enic->mbox_expected_reply = 0;
+		enic->mbox_expected_msg_num = 0;
+		err = -ETIMEDOUT;
+	}
+	spin_unlock_bh(&enic->mbox_state_lock);
 
-	return left ? 0 : -ETIMEDOUT;
+	return err;
 }
 
 int enic_mbox_send_link_state(struct enic *enic, u16 vf_id, u32 link_state)
@@ -178,8 +243,8 @@ static int enic_mbox_pf_handle_capability(struct enic *enic, void *msg,
 	reply.reply.ret_major = cpu_to_le16(0);
 	reply.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
 
-	return enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
-				  &reply, sizeof(reply));
+	return enic_mbox_send_reply(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
+				    &reply, sizeof(reply), msg_num);
 }
 
 static int enic_mbox_pf_handle_register(struct enic *enic, void *msg,
@@ -208,8 +273,8 @@ static int enic_mbox_pf_handle_register(struct enic *enic, void *msg,
 	}
 
 	reply.reply.ret_major = cpu_to_le16(0);
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_REGISTER_REPLY, vf_id,
-				 &reply, sizeof(reply));
+	err = enic_mbox_send_reply(enic, ENIC_MBOX_VF_REGISTER_REPLY, vf_id,
+				   &reply, sizeof(reply), msg_num);
 	if (err)
 		return err;
 
@@ -253,8 +318,8 @@ static int enic_mbox_pf_handle_unregister(struct enic *enic, void *msg,
 	enic->vf_state[vf_id].registered = false;
 
 	reply.reply.ret_major = cpu_to_le16(0);
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_UNREGISTER_REPLY, vf_id,
-				 &reply, sizeof(reply));
+	err = enic_mbox_send_reply(enic, ENIC_MBOX_VF_UNREGISTER_REPLY, vf_id,
+				   &reply, sizeof(reply), msg_num);
 
 	if (net_ratelimit())
 		netdev_info(enic->netdev,
@@ -324,75 +389,57 @@ static void enic_mbox_pf_process_msg(struct enic *enic,
 			    hdr->msg_type, vf_id, err);
 }
 
-static void enic_mbox_vf_handle_capability_reply(struct enic *enic,
-						 void *payload)
+static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
+				      void *payload, u64 msg_num)
 {
-	struct enic_mbox_vf_capability_reply_msg *reply = payload;
-
-	if (READ_ONCE(enic->mbox_expected_reply) != ENIC_MBOX_VF_CAPABILITY_REPLY) {
+	struct enic_mbox_generic_reply *reply = payload;
+	u16 ret_major = le16_to_cpu(reply->ret_major);
+	u64 expected_msg_num;
+	u8 expected_type;
+
+	spin_lock_bh(&enic->mbox_state_lock);
+	expected_type = enic->mbox_expected_reply;
+	expected_msg_num = enic->mbox_expected_msg_num;
+	if (expected_type != reply_type || expected_msg_num != msg_num) {
+		spin_unlock_bh(&enic->mbox_state_lock);
 		netdev_warn(enic->netdev,
-			    "MBOX: stale capability reply (expected %u), drop\n",
-			    READ_ONCE(enic->mbox_expected_reply));
+			    "MBOX: stale reply %u/%llu (expected %u/%llu), drop\n",
+			    reply_type, (unsigned long long)msg_num,
+			    expected_type, (unsigned long long)expected_msg_num);
 		return;
 	}
 
-	if (le16_to_cpu(reply->reply.ret_major) == 0)
-		enic->pf_cap_version = le32_to_cpu(reply->version);
-	else
-		netdev_warn(enic->netdev,
-			    "MBOX: PF rejected capability request: %u/%u\n",
-			    le16_to_cpu(reply->reply.ret_major),
-			    le16_to_cpu(reply->reply.ret_minor));
-	complete(&enic->mbox_comp);
-}
-
-static void enic_mbox_vf_handle_register_reply(struct enic *enic,
-					       void *payload)
-{
-	struct enic_mbox_vf_register_reply_msg *reply = payload;
-
-	if (READ_ONCE(enic->mbox_expected_reply) != ENIC_MBOX_VF_REGISTER_REPLY) {
-		netdev_warn(enic->netdev,
-			    "MBOX: stale register reply (expected %u), drop\n",
-			    READ_ONCE(enic->mbox_expected_reply));
-		return;
-	}
+	if (!ret_major) {
+		switch (reply_type) {
+		case ENIC_MBOX_VF_CAPABILITY_REPLY: {
+			struct enic_mbox_vf_capability_reply_msg *cap = payload;
 
-	if (le16_to_cpu(reply->reply.ret_major)) {
-		netdev_warn(enic->netdev,
-			    "MBOX: VF register rejected by PF: %u/%u\n",
-			    le16_to_cpu(reply->reply.ret_major),
-			    le16_to_cpu(reply->reply.ret_minor));
-	} else {
-		enic->vf_registered = true;
+			WRITE_ONCE(enic->pf_cap_version,
+				   le32_to_cpu(cap->version));
+			break;
+		}
+		case ENIC_MBOX_VF_REGISTER_REPLY:
+			WRITE_ONCE(enic->vf_registered, true);
+			break;
+		case ENIC_MBOX_VF_UNREGISTER_REPLY:
+			WRITE_ONCE(enic->vf_registered, false);
+			break;
+		}
 	}
+	enic->mbox_expected_reply = 0;
+	enic->mbox_expected_msg_num = 0;
 	complete(&enic->mbox_comp);
-}
-
-static void enic_mbox_vf_handle_unregister_reply(struct enic *enic,
-						 void *payload)
-{
-	struct enic_mbox_vf_register_reply_msg *reply = payload;
+	spin_unlock_bh(&enic->mbox_state_lock);
 
-	if (READ_ONCE(enic->mbox_expected_reply) != ENIC_MBOX_VF_UNREGISTER_REPLY) {
+	if (ret_major)
 		netdev_warn(enic->netdev,
-			    "MBOX: stale unregister reply (expected %u), drop\n",
-			    READ_ONCE(enic->mbox_expected_reply));
-		return;
-	}
-
-	if (le16_to_cpu(reply->reply.ret_major)) {
-		netdev_warn(enic->netdev,
-			    "MBOX: VF unregister rejected by PF: %u/%u\n",
-			    le16_to_cpu(reply->reply.ret_major),
-			    le16_to_cpu(reply->reply.ret_minor));
-	} else {
-		enic->vf_registered = false;
-	}
-	complete(&enic->mbox_comp);
+			    "MBOX: PF rejected reply type %u: %u/%u\n",
+			    reply_type, ret_major,
+			    le16_to_cpu(reply->ret_minor));
 }
 
-static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload)
+static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload,
+					   u64 msg_num)
 {
 	struct enic_mbox_pf_link_state_notif_msg *notif = payload;
 	struct enic_mbox_pf_link_state_ack_msg ack = {};
@@ -423,8 +470,8 @@ static void enic_mbox_vf_handle_link_state(struct enic *enic, void *payload)
 	}
 	spin_unlock_bh(&enic->vf_link_state_lock);
 
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_PF_LINK_STATE_ACK,
-				 ENIC_MBOX_DST_PF, &ack, sizeof(ack));
+	err = enic_mbox_send_reply(enic, ENIC_MBOX_PF_LINK_STATE_ACK,
+				   ENIC_MBOX_DST_PF, &ack, sizeof(ack), msg_num);
 	if (err && net_ratelimit())
 		netdev_warn(enic->netdev,
 			    "MBOX: failed to send link state ACK: %d\n", err);
@@ -468,6 +515,8 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
 				     struct enic_mbox_hdr *hdr, void *payload,
 				     u16 payload_len)
 {
+	u64 msg_num = le64_to_cpu(hdr->msg_num);
+
 	switch (hdr->msg_type) {
 	case ENIC_MBOX_VF_CAPABILITY_REPLY: {
 		size_t exp = sizeof(struct enic_mbox_vf_capability_reply_msg);
@@ -475,7 +524,7 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
 		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
 					     payload_len, exp))
 			return;
-		enic_mbox_vf_handle_capability_reply(enic, payload);
+		enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
 		break;
 	}
 	case ENIC_MBOX_VF_REGISTER_REPLY: {
@@ -484,7 +533,7 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
 		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
 					     payload_len, exp))
 			return;
-		enic_mbox_vf_handle_register_reply(enic, payload);
+		enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
 		break;
 	}
 	case ENIC_MBOX_VF_UNREGISTER_REPLY: {
@@ -493,7 +542,7 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
 		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
 					     payload_len, exp))
 			return;
-		enic_mbox_vf_handle_unregister_reply(enic, payload);
+		enic_mbox_vf_handle_reply(enic, hdr->msg_type, payload, msg_num);
 		break;
 	}
 	case ENIC_MBOX_PF_LINK_STATE_NOTIF: {
@@ -502,7 +551,7 @@ static void enic_mbox_vf_process_msg(struct enic *enic,
 		if (!enic_mbox_vf_payload_ok(enic, hdr->msg_type,
 					     payload_len, exp))
 			return;
-		enic_mbox_vf_handle_link_state(enic, payload);
+		enic_mbox_vf_handle_link_state(enic, payload, msg_num);
 		break;
 	}
 	default:
@@ -571,32 +620,31 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
 int enic_mbox_vf_capability_check(struct enic *enic)
 {
 	struct enic_mbox_vf_capability_msg req = {};
+	u32 version;
 	int err;
 
-	enic->pf_cap_version = 0;
-	reinit_completion(&enic->mbox_comp);
-	WRITE_ONCE(enic->mbox_expected_reply, ENIC_MBOX_VF_CAPABILITY_REPLY);
+	WRITE_ONCE(enic->pf_cap_version, 0);
 	req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
 
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REQUEST,
-				 ENIC_MBOX_DST_PF, &req, sizeof(req));
-	if (err) {
-		WRITE_ONCE(enic->mbox_expected_reply, 0);
+	err = enic_mbox_vf_send_request(enic,
+					ENIC_MBOX_VF_CAPABILITY_REQUEST,
+					ENIC_MBOX_VF_CAPABILITY_REPLY,
+					&req, sizeof(req));
+	if (err)
 		return err;
-	}
 
 	err = enic_mbox_wait_reply(enic, 3000);
-	WRITE_ONCE(enic->mbox_expected_reply, 0);
+	version = READ_ONCE(enic->pf_cap_version);
 	if (err) {
 		netdev_warn(enic->netdev,
 			    "MBOX: no capability reply from PF\n");
 		return err;
 	}
 
-	if (enic->pf_cap_version < ENIC_MBOX_CAP_VERSION_1) {
+	if (version < ENIC_MBOX_CAP_VERSION_1) {
 		netdev_warn(enic->netdev,
 			    "MBOX: PF rejected capability request or reported unsupported version %u\n",
-			    enic->pf_cap_version);
+			    version);
 		return -EOPNOTSUPP;
 	}
 
@@ -605,28 +653,25 @@ int enic_mbox_vf_capability_check(struct enic *enic)
 
 int enic_mbox_vf_register(struct enic *enic)
 {
+	bool registered;
 	int err;
 
-	enic->vf_registered = false;
-	reinit_completion(&enic->mbox_comp);
-	WRITE_ONCE(enic->mbox_expected_reply, ENIC_MBOX_VF_REGISTER_REPLY);
+	WRITE_ONCE(enic->vf_registered, false);
 
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_REGISTER_REQUEST,
-				 ENIC_MBOX_DST_PF, NULL, 0);
-	if (err) {
-		WRITE_ONCE(enic->mbox_expected_reply, 0);
+	err = enic_mbox_vf_send_request(enic, ENIC_MBOX_VF_REGISTER_REQUEST,
+					ENIC_MBOX_VF_REGISTER_REPLY, NULL, 0);
+	if (err)
 		return err;
-	}
 
 	err = enic_mbox_wait_reply(enic, 3000);
-	WRITE_ONCE(enic->mbox_expected_reply, 0);
+	registered = READ_ONCE(enic->vf_registered);
 	if (err) {
 		netdev_warn(enic->netdev,
 			    "MBOX: VF registration with PF timed out\n");
 		return err;
 	}
 
-	if (!enic->vf_registered)
+	if (!registered)
 		return -ENODEV;
 
 	return 0;
@@ -634,43 +679,48 @@ int enic_mbox_vf_register(struct enic *enic)
 
 int enic_mbox_vf_unregister(struct enic *enic)
 {
+	bool registered;
 	int err;
 
-	if (!enic->vf_registered)
+	if (!READ_ONCE(enic->vf_registered))
 		return 0;
 
-	reinit_completion(&enic->mbox_comp);
-	WRITE_ONCE(enic->mbox_expected_reply, ENIC_MBOX_VF_UNREGISTER_REPLY);
-
-	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_UNREGISTER_REQUEST,
-				 ENIC_MBOX_DST_PF, NULL, 0);
-	if (err) {
-		WRITE_ONCE(enic->mbox_expected_reply, 0);
+	err = enic_mbox_vf_send_request(enic,
+					ENIC_MBOX_VF_UNREGISTER_REQUEST,
+					ENIC_MBOX_VF_UNREGISTER_REPLY,
+					NULL, 0);
+	if (err)
 		return err;
-	}
 
 	err = enic_mbox_wait_reply(enic, 3000);
-	WRITE_ONCE(enic->mbox_expected_reply, 0);
+	registered = READ_ONCE(enic->vf_registered);
 	if (err)
 		return err;
-	if (enic->vf_registered)
+	if (registered)
 		return -EACCES;
 	return 0;
 }
 
 void enic_mbox_init(struct enic *enic)
 {
-	/* mbox_lock and mbox_comp must be initialized exactly once per
+	bool reinit = enic->mbox_initialized;
+
+	/* MBOX locks and mbox_comp must be initialized exactly once per
 	 * device lifetime; the PF sriov_configure path can re-enter this
 	 * on each enable cycle where these primitives are already set up.
 	 */
-	if (!enic->mbox_initialized) {
+	if (!reinit) {
 		mutex_init(&enic->mbox_lock);
 		init_completion(&enic->mbox_comp);
+		spin_lock_init(&enic->mbox_state_lock);
+		enic->mbox_msg_num = 0;
 		enic->mbox_initialized = true;
 	} else {
 		reinit_completion(&enic->mbox_comp);
 	}
-	enic->mbox_msg_num = 0;
+	spin_lock_bh(&enic->mbox_state_lock);
+	enic->mbox_expected_reply = 0;
+	enic->mbox_expected_msg_num = 0;
+	spin_unlock_bh(&enic->mbox_state_lock);
 	enic->admin_rq_handler = enic_mbox_recv_handler;
 }

-- 
2.43.0

Re: [net,2/2] enic: match mailbox replies to request numbers

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] enic_mbox_vf_handle_reply()
  (drivers/net/ethernet/cisco/enic/enic_mbox.c:403) now requires the…

--

enic: match mailbox replies to request numbers

This adds a message-number match to the VF mailbox reply path: outgoing
replies now reuse the initiating message number, the VF records both the
expected reply type and the expected message number, and a reply is only
accepted when both match.  The expected-reply state is moved under a new
spinlock and the message counter is kept monotonic across an admin channel
reopen.

> diff --git a/drivers/net/ethernet/cisco/enic/enic_mbox.c b/drivers/net/ethernet/cisco/enic/enic_mbox.c
> index ad79d3951f3d..5c93ca49552a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_mbox.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_mbox.c

[ ... ]

> @@ -68,7 +71,21 @@ int enic_mbox_send_msg(struct enic *enic, u8 msg_type, u16 dst_vnic_id,
>  		goto unlock;
>  	}
>  
> -	enic_mbox_fill_hdr(enic, buf, msg_type, dst_vnic_id, total_len);
> +	/* Replies reuse the initiating message number.  Requests and
> +	 * notifications allocate a new one.
> +	 */
> +	if (!reuse_msg_num)
> +		msg_num = ++enic->mbox_msg_num;
> +	if (expected_reply) {
> +		reinit_completion(&enic->mbox_comp);
> +		spin_lock_bh(&enic->mbox_state_lock);
> +		enic->mbox_expected_reply = expected_reply;
> +		enic->mbox_expected_msg_num = msg_num;
> +		spin_unlock_bh(&enic->mbox_state_lock);
> +		reply_expected = true;
> +	}

[ ... ]

> @@ -178,8 +243,8 @@ static int enic_mbox_pf_handle_capability(struct enic *enic, void *msg,
>  	reply.reply.ret_major = cpu_to_le16(0);
>  	reply.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
>  
> -	return enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
> -				  &reply, sizeof(reply));
> +	return enic_mbox_send_reply(enic, ENIC_MBOX_VF_CAPABILITY_REPLY, vf_id,
> +				    &reply, sizeof(reply), msg_num);
>  }

[ ... ]

> @@ -324,75 +389,57 @@ static void enic_mbox_pf_process_msg(struct enic *enic,
> +static void enic_mbox_vf_handle_reply(struct enic *enic, u8 reply_type,
> +				      void *payload, u64 msg_num)
>  {
> +	struct enic_mbox_generic_reply *reply = payload;
> +	u16 ret_major = le16_to_cpu(reply->ret_major);
> +	u64 expected_msg_num;
> +	u8 expected_type;
> +
> +	spin_lock_bh(&enic->mbox_state_lock);
> +	expected_type = enic->mbox_expected_reply;
> +	expected_msg_num = enic->mbox_expected_msg_num;
> +	if (expected_type != reply_type || expected_msg_num != msg_num) {
> +		spin_unlock_bh(&enic->mbox_state_lock);
>  		netdev_warn(enic->netdev,
> -			    "MBOX: stale capability reply (expected %u), drop\n",
> -			    READ_ONCE(enic->mbox_expected_reply));
> +			    "MBOX: stale reply %u/%llu (expected %u/%llu), drop\n",
> +			    reply_type, (unsigned long long)msg_num,
> +			    expected_type, (unsigned long long)expected_msg_num);
>  		return;
>  	}

[Severity: High]
What happens when the VF runs this code against a PF that does not echo
msg_num?

The expected number armed in enic_mbox_send_msg_id() is the VF's own
private counter value:

	if (!reuse_msg_num)
		msg_num = ++enic->mbox_msg_num;
	if (expected_reply) {
		...
		enic->mbox_expected_msg_num = msg_num;

so acceptance now depends on the peer returning that exact value.  The
mismatch branch in enic_mbox_vf_handle_reply() returns without calling
complete(&enic->mbox_comp), so enic_mbox_wait_reply() runs out its 3000 ms
and returns -ETIMEDOUT for every reply that carries a different number.

The only PF implementation that can be inspected is the in-tree one added
by the commit named in the Fixes: tag, and before this patch it stamped a
fresh number on every outgoing message, replies included:

drivers/net/ethernet/cisco/enic/enic_mbox.c:enic_mbox_fill_hdr() {
	...
	hdr->msg_num = cpu_to_le64(++enic->mbox_msg_num);
}

That is also why this patch has to convert
enic_mbox_pf_handle_capability(), enic_mbox_pf_handle_register() and
enic_mbox_pf_handle_unregister() over to enic_mbox_send_reply(..., msg_num).

Given that, is this statement in the commit message accurate?

    Reply-number echo is part of the established version-1 protocol, so this
    remains compatible with deployed V2-capable PF implementations that
    already echo msg_num.

For a VF with this patch talking to a PF running the pre-patch in-tree
driver (older host kernel than guest, or a partial stable backport that
picks up only the VF-side hunks), does the capability handshake fail
outright?

enic_mbox_vf_capability_check() -> enic_mbox_wait_reply() returns
-ETIMEDOUT, and in enic_probe() that error is fatal:

		err = enic_mbox_vf_capability_check(enic);
		if (err) {
			dev_err(dev, "MBOX capability check failed: %d\n", err);
			goto err_out_admin_close;
		}

so the SR-IOV V2 VF netdev is never registered.  On the reset path,
enic_admin_chan_reopen() calls enic_admin_channel_close() on the same
error, leaving the VF without PF-driven link state or registration.

Would it make sense to keep a fallback here, for example accepting a reply
whose msg_num is 0 (or otherwise not echoed), or gating the strict match on
the negotiated pf_cap_version, so that the VF does not depend on peer
behaviour that this same patch introduces?

> -	if (le16_to_cpu(reply->reply.ret_major) == 0)
> -		enic->pf_cap_version = le32_to_cpu(reply->version);
> -	else
> -		netdev_warn(enic->netdev,
> -			    "MBOX: PF rejected capability request: %u/%u\n",
> -			    le16_to_cpu(reply->reply.ret_major),
> -			    le16_to_cpu(reply->reply.ret_minor));
> -	complete(&enic->mbox_comp);
> -}

[ ... ]

> @@ -571,32 +620,31 @@ static void enic_mbox_recv_handler(struct enic *enic, void *buf,
>  int enic_mbox_vf_capability_check(struct enic *enic)
>  {
>  	struct enic_mbox_vf_capability_msg req = {};
> +	u32 version;
>  	int err;
>  
> -	enic->pf_cap_version = 0;
> -	reinit_completion(&enic->mbox_comp);
> -	WRITE_ONCE(enic->mbox_expected_reply, ENIC_MBOX_VF_CAPABILITY_REPLY);
> +	WRITE_ONCE(enic->pf_cap_version, 0);
>  	req.version = cpu_to_le32(ENIC_MBOX_CAP_VERSION_1);
>  
> -	err = enic_mbox_send_msg(enic, ENIC_MBOX_VF_CAPABILITY_REQUEST,
> -				 ENIC_MBOX_DST_PF, &req, sizeof(req));
> -	if (err) {
> -		WRITE_ONCE(enic->mbox_expected_reply, 0);
> +	err = enic_mbox_vf_send_request(enic,
> +					ENIC_MBOX_VF_CAPABILITY_REQUEST,
> +					ENIC_MBOX_VF_CAPABILITY_REPLY,
> +					&req, sizeof(req));
> +	if (err)
>  		return err;
> -	}
>  
>  	err = enic_mbox_wait_reply(enic, 3000);
> -	WRITE_ONCE(enic->mbox_expected_reply, 0);
> +	version = READ_ONCE(enic->pf_cap_version);
>  	if (err) {
>  		netdev_warn(enic->netdev,
>  			    "MBOX: no capability reply from PF\n");
>  		return err;
>  	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830-b4-enic-v2-mbox-fixes-net-v1-0-23adf9bfd426%40cisco.com

Re: [PATCH net 0/2] enic: fix V2 VF mailbox reply matching and carrier reopen

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 30 Aug 2026 15:22:51 -0700 you wrote:
> Preserve ENIC V2 VF carrier state across netdev reopen and match mailbox
> replies to their originating requests.
> 
> A V2 VF receives carrier state from PF MBOX notifications. enic_stop()
> forces carrier off, but enic_open() does not request another notification
> or restore the previous state. An ordinary netdev close/open can therefore
> leave the VF without carrier until the PF sends another link-state
> notification.
> 
> [...]

Here is the summary with links:
  - [net,1/2] enic: preserve V2 VF carrier across netdev reopen
    https://git.kernel.org/netdev/net/c/b752e041d584
  - [net,2/2] enic: match mailbox replies to request numbers
    https://git.kernel.org/netdev/net/c/8972d252f495

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html