mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net v3] octeontx2-af: Fix rep link state sync and workqueue races
@ 2026-09-03  7:48 nshettyj
  2026-09-08 22:51 ` netdev-bot+sashiko
  0 siblings, 1 reply; 3+ messages in thread
From: nshettyj @ 2026-09-03  7:48 UTC (permalink / raw)
  To: netdev, linux-kernel
  Cc: Geetha sowjanya, Nitin Shetty J, Sunil Goutham, Ratheesh Kannoth,
	Subbaraya Sundeep, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Bharat Bhushan, Simon Horman,
	Harman Kalra

From: Geetha sowjanya <gakula@marvell.com>

Move rep event workqueue init to rvu_mbox_handler_get_rep_cnt(),
fix use-after-free and race conditions in rep event handling,
add bounds checking, and serialize LBK link configuration.

Fixes: b8fea84a0468 ("octeontx2-pf: Add support to sync link state between representor and VFs")
Signed-off-by: Nitin Shetty J <nshettyj@marvell.com>
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
---
changes in v3:
- Introduce RVU_MAX_REP macro for the representor map array size.
- Fix use-after-free in rvu_remove() when rep_evt_wq is destroyed
  while a mbox handler is still running.
- Fix a race in rvu_mbox_handler_rep_event_notify() where rep_evt_wq
  could be freed between the NULL check and queue_work().
- Reject REP_EVENT_NOTIFY from non-owner PFs and invalid pcifunc values.
- Fix LBK link leak when MCAM rule installation fails midway.
- Fix a race in rvu_mbox_handler_get_rep_cnt() where concurrent mbox
  handlers could both initialize rep2pfvf_map.
- Reject GET_REP_CNT from non-owner callers with -EPERM.
- Cap rep_cnt to RVU_MAX_REP to prevent out-of-bounds map writes.
- Fix inconsistent rep_cnt state when get_rep_cnt initialization fails.
- Fix a race in rvu_switch_enable_lbk_link() where nix_blkaddr could
  change mid-call, and fix NULL dereference when nix_hw is not assigned.

changes in v2:
- Reject REP event notifications before workqueue setup and
    validate PF/VF state event pcifuncs.
- Make REP map initialization atomic by using a temporary map,
    handling zero-REP cases, and rolling back on workqueue allocation
    failure.
- Use an unbound REP event workqueue.
- Serialize LBK link TL2 configuration with rsrc_lock.
---
 .../net/ethernet/marvell/octeontx2/af/mbox.h  |   4 +-
 .../net/ethernet/marvell/octeontx2/af/rvu.c   |  14 ++
 .../ethernet/marvell/octeontx2/af/rvu_rep.c   | 178 +++++++++++++-----
 .../marvell/octeontx2/af/rvu_switch.c         |  25 ++-
 .../net/ethernet/marvell/octeontx2/nic/rep.c  |  12 ++
 .../net/ethernet/marvell/octeontx2/nic/rep.h  |   1 -
 6 files changed, 180 insertions(+), 54 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
index 73f743e4a83d..f8342a71aac9 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
@@ -1775,10 +1775,12 @@ struct ptp_get_cap_rsp {
 	u64 cap;
 };
 
+#define RVU_MAX_REP	64
+
 struct get_rep_cnt_rsp {
 	struct mbox_msghdr hdr;
 	u16 rep_cnt;
-	u16 rep_pf_map[64];
+	u16 rep_pf_map[RVU_MAX_REP];
 	u64 rsvd;
 };
 
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
index ffba56ee8a60..392d02963f07 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
@@ -3709,6 +3709,20 @@ static void rvu_remove(struct pci_dev *pdev)
 	rvu_unregister_dl(rvu);
 	rvu_unregister_interrupts(rvu);
 	rvu_flr_wq_destroy(rvu);
+	if (rvu->rep_evt_wq) {
+		struct workqueue_struct *rep_wq = rvu->rep_evt_wq;
+
+		/* NULL the pointer before flushing mbox_wq.  Any mbox handler
+		 * still in flight will snapshot NULL via READ_ONCE() and return
+		 * -EINVAL without calling queue_work(), so no new items can be
+		 * added to rep_wq after flush_workqueue(mbox_wq) returns.
+		 */
+		WRITE_ONCE(rvu->rep_evt_wq, NULL);
+		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
+		destroy_workqueue(rep_wq);
+	} else {
+		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
+	}
 	rvu_cgx_exit(rvu);
 	rvu_fwdata_exit(rvu);
 	rvu_mcs_exit(rvu);
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
index a2781e0f504e..69b260fdd52d 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
@@ -44,6 +44,8 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
 	if (event->event & RVU_EVENT_MAC_ADDR_CHANGE)
 		ether_addr_copy(pfvf->mac_addr, event->evt_data.mac);
 
+	if (event->event & RVU_EVENT_PFVF_STATE)
+		pf = rvu_get_pf(rvu->pdev, event->hdr.pcifunc);
 	mutex_lock(&rvu->mbox_lock);
 	msg = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
 	if (!msg) {
@@ -53,6 +55,10 @@ static int rvu_rep_up_notify(struct rvu *rvu, struct rep_event *event)
 
 	msg->hdr.pcifunc = event->pcifunc;
 	msg->event = event->event;
+	msg->pcifunc = event->pcifunc;
+
+	if (event->event & RVU_EVENT_PFVF_STATE)
+		msg->hdr.pcifunc = event->hdr.pcifunc;
 
 	memcpy(&msg->evt_data, &event->evt_data, sizeof(struct rep_evt_data));
 
@@ -95,8 +101,17 @@ static void rvu_rep_wq_handler(struct work_struct *work)
 int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
 				      struct msg_rsp *rsp)
 {
+	struct workqueue_struct *wq;
 	struct rep_evtq_ent *qentry;
 
+	wq = READ_ONCE(rvu->rep_evt_wq);
+	if (!wq)
+		return -EINVAL;
+
+	/* Only the registered representor PF may send REP_EVENT_NOTIFY. */
+	if (req->hdr.pcifunc != rvu->rep_pcifunc)
+		return -EPERM;
+
 	/* The mailbox dispatcher normalises only the header pcifunc; the
 	 * nested struct rep_event::pcifunc body field is sender-controlled
 	 * and is later used by rvu_rep_up_notify() to index rvu->pf[] /
@@ -105,6 +120,14 @@ int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
 	if (!is_pf_func_valid(rvu, req->pcifunc))
 		return -EINVAL;
 
+	/* Only CGX-mapped PFs are present in the representor map. */
+	if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, req->pcifunc)))
+		return -EINVAL;
+
+	if ((req->event & RVU_EVENT_PFVF_STATE) &&
+	    rvu_get_pf(rvu->pdev, req->hdr.pcifunc) >= rvu->hw->total_pfs)
+		return -EINVAL;
+
 	qentry = kmalloc_obj(*qentry, GFP_ATOMIC);
 	if (!qentry)
 		return -ENOMEM;
@@ -113,37 +136,23 @@ int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
 	spin_lock(&rvu->rep_evtq_lock);
 	list_add_tail(&qentry->node, &rvu->rep_evtq_head);
 	spin_unlock(&rvu->rep_evtq_lock);
-	queue_work(rvu->rep_evt_wq, &rvu->rep_evt_work);
+	queue_work(wq, &rvu->rep_evt_work);
 	return 0;
 }
 
 int rvu_rep_notify_pfvf_state(struct rvu *rvu, u16 pcifunc, bool enable)
 {
-	struct rep_event *req;
-	int pf;
+	struct rep_event req = { 0 };
+	struct msg_rsp rsp;
 
 	if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc)))
 		return 0;
 
-	pf = rvu_get_pf(rvu->pdev, rvu->rep_pcifunc);
-
-	mutex_lock(&rvu->mbox_lock);
-	req = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
-	if (!req) {
-		mutex_unlock(&rvu->mbox_lock);
-		return -ENOMEM;
-	}
-
-	req->hdr.pcifunc = rvu->rep_pcifunc;
-	req->event |= RVU_EVENT_PFVF_STATE;
-	req->pcifunc = pcifunc;
-	req->evt_data.vf_state = enable;
-
-	otx2_mbox_wait_for_zero(&rvu->afpf_wq_info.mbox_up, pf);
-	otx2_mbox_msg_send_up(&rvu->afpf_wq_info.mbox_up, pf);
-
-	mutex_unlock(&rvu->mbox_lock);
-	return 0;
+	req.hdr.pcifunc = rvu->rep_pcifunc;
+	req.event = RVU_EVENT_PFVF_STATE;
+	req.pcifunc = pcifunc;
+	req.evt_data.vf_state = enable;
+	return rvu_mbox_handler_rep_event_notify(rvu, &req, &rsp);
 }
 
 #define RVU_LF_RX_STATS(reg) \
@@ -325,6 +334,7 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
 	u16 start = rswitch->start_entry;
 	struct rvu_hwinfo *hw = rvu->hw;
 	u16 pcifunc, entry = 0;
+	struct rvu_pfvf *pfvf;
 	int pf, vf, numvfs;
 	int err, nixlf, i;
 	u8 rep;
@@ -334,19 +344,22 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
 			continue;
 
 		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+		pfvf = rvu_get_pfvf(rvu, pcifunc);
 		rvu_get_nix_blkaddr(rvu, pcifunc);
+		if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
+			rvu_switch_enable_lbk_link(rvu, pcifunc, true);
 		rep = true;
 		for (i = 0; i < 2; i++) {
 			err = rvu_rep_install_rx_rule(rvu, pcifunc,
 						      start + entry, rep);
 			if (err)
-				return err;
+				goto err_disable_lbk;
 			rswitch->entry2pcifunc[entry++] = pcifunc;
 
 			err = rvu_rep_install_tx_rule(rvu, pcifunc,
 						      start + entry, rep);
 			if (err)
-				return err;
+				goto err_disable_lbk;
 			rswitch->entry2pcifunc[entry++] = pcifunc;
 			rep = false;
 		}
@@ -354,9 +367,12 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
 		rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
 		for (vf = 0; vf < numvfs; vf++) {
 			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+			pfvf = rvu_get_pfvf(rvu, pcifunc);
+			if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
+				rvu_switch_enable_lbk_link(rvu, pcifunc, true);
 			rvu_get_nix_blkaddr(rvu, pcifunc);
 
-			/* Skip installimg rules if nixlf is not attached */
+			/* Skip installing rules if nixlf is not attached */
 			err = nix_get_nixlf(rvu, pcifunc, &nixlf, NULL);
 			if (err)
 				continue;
@@ -366,30 +382,37 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
 							      start + entry,
 							      rep);
 				if (err)
-					return err;
+					goto err_disable_lbk;
 				rswitch->entry2pcifunc[entry++] = pcifunc;
 
 				err = rvu_rep_install_tx_rule(rvu, pcifunc,
 							      start + entry,
 							      rep);
 				if (err)
-					return err;
+					goto err_disable_lbk;
 				rswitch->entry2pcifunc[entry++] = pcifunc;
 				rep = false;
 			}
 		}
 	}
+	return 0;
 
-	/* Initialize the wq for handling REP events */
-	spin_lock_init(&rvu->rep_evtq_lock);
-	INIT_LIST_HEAD(&rvu->rep_evtq_head);
-	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
-	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_PERCPU, 0);
-	if (!rvu->rep_evt_wq) {
-		dev_err(rvu->dev, "REP workqueue allocation failed\n");
-		return -ENOMEM;
+err_disable_lbk:
+	/* Undo any LBK links enabled above before the MCAM rule failure.
+	 * Disabling a link that was never enabled is a safe no-op.
+	 */
+	for (pf = 1; pf < hw->total_pfs; pf++) {
+		if (!is_pf_cgxmapped(rvu, pf))
+			continue;
+		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+		rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+		rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
+		for (vf = 0; vf < numvfs; vf++) {
+			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+		}
 	}
-	return 0;
+	return err;
 }
 
 void rvu_rep_update_rules(struct rvu *rvu, u16 pcifunc, bool ena)
@@ -443,35 +466,92 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
 	return 0;
 }
 
+static int rvu_rep_get_rep_map(struct rvu *rvu, struct msg_req *req,
+			       struct get_rep_cnt_rsp *rsp)
+{
+	int rep;
+
+	if (req->hdr.pcifunc != rvu->rep_pcifunc)
+		return -EPERM;
+
+	rsp->rep_cnt = rvu->rep_cnt;
+	for (rep = 0; rep < rvu->rep_cnt; rep++)
+		rsp->rep_pf_map[rep] = rvu->rep2pfvf_map[rep];
+
+	return 0;
+}
+
 int rvu_mbox_handler_get_rep_cnt(struct rvu *rvu, struct msg_req *req,
 				 struct get_rep_cnt_rsp *rsp)
 {
-	int pf, vf, numvfs, hwvf, rep = 0;
+	int pf, vf, numvfs, hwvf, rep = 0, cnt;
+	int ret = 0;
 	u16 pcifunc;
+	u16 *map;
+
+	/* Serialize first-time initialization. mbox_wq is WQ_PERCPU so
+	 * handlers for different PFs can run concurrently; without this
+	 * lock two callers could both observe rep2pfvf_map == NULL and
+	 * double-allocate the workqueue, leaking one permanently.
+	 */
+	mutex_lock(&rvu->rsrc_lock);
+
+	if (rvu->rep2pfvf_map) {
+		ret = rvu_rep_get_rep_map(rvu, req, rsp);
+		goto unlock;
+	}
 
 	rvu->rep_pcifunc = req->hdr.pcifunc;
-	rsp->rep_cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
-	rvu->rep_cnt = rsp->rep_cnt;
+	cnt = min_t(int, rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs,
+		    RVU_MAX_REP);
 
-	rvu->rep2pfvf_map = devm_kzalloc(rvu->dev, rvu->rep_cnt *
-					 sizeof(u16), GFP_KERNEL);
-	if (!rvu->rep2pfvf_map)
-		return -ENOMEM;
+	/* Allocate at least one element so the pointer is always non-NULL
+	 * once published, keeping the fast-path check above reliable.
+	 */
+	map = devm_kzalloc(rvu->dev, (cnt ?: 1) * sizeof(u16), GFP_KERNEL);
+	if (!map) {
+		ret = -ENOMEM;
+		goto unlock;
+	}
 
 	for (pf = 0; pf < rvu->hw->total_pfs; pf++) {
 		if (!is_pf_cgxmapped(rvu, pf))
 			continue;
+		if (rep >= cnt)
+			break;
 		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
-		rvu->rep2pfvf_map[rep] = pcifunc;
+		map[rep] = pcifunc;
 		rsp->rep_pf_map[rep] = pcifunc;
 		rep++;
 		rvu_get_pf_numvfs(rvu, pf, &numvfs, &hwvf);
-		for (vf = 0; vf < numvfs; vf++) {
-			rvu->rep2pfvf_map[rep] = pcifunc |
-				((vf + 1) & RVU_PFVF_FUNC_MASK);
-			rsp->rep_pf_map[rep] = rvu->rep2pfvf_map[rep];
+		for (vf = 0; vf < numvfs && rep < cnt; vf++) {
+			map[rep] = pcifunc | ((vf + 1) & RVU_PFVF_FUNC_MASK);
+			rsp->rep_pf_map[rep] = map[rep];
 			rep++;
 		}
 	}
-	return 0;
+
+	if (!cnt) {
+		rvu->rep2pfvf_map = map;
+		goto unlock;
+	}
+
+	/* Initialize the wq for handling REP events */
+	spin_lock_init(&rvu->rep_evtq_lock);
+	INIT_LIST_HEAD(&rvu->rep_evtq_head);
+	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
+	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_UNBOUND, 0);
+	if (!rvu->rep_evt_wq) {
+		dev_err(rvu->dev, "REP workqueue allocation failed\n");
+		devm_kfree(rvu->dev, map);
+		ret = -ENOMEM;
+		goto unlock;
+	}
+
+	rvu->rep_cnt = cnt;
+	rsp->rep_cnt = cnt;
+	rvu->rep2pfvf_map = map;
+unlock:
+	mutex_unlock(&rvu->rsrc_lock);
+	return ret;
 }
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
index 49ce38685a7e..8c2c98403507 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
@@ -12,11 +12,18 @@ void rvu_switch_enable_lbk_link(struct rvu *rvu, u16 pcifunc, bool enable)
 {
 	struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, pcifunc);
 	struct nix_hw *nix_hw;
+	int blkaddr;
 
-	nix_hw = get_nix_hw(rvu->hw, pfvf->nix_blkaddr);
+	mutex_lock(&rvu->rsrc_lock);
+	blkaddr = pfvf->nix_blkaddr;
+	nix_hw = get_nix_hw(rvu->hw, blkaddr);
 	/* Enable LBK links with channel 63 for TX MCAM rule */
-	rvu_nix_tx_tl2_cfg(rvu, pfvf->nix_blkaddr, pcifunc,
+	if (!nix_hw)
+		goto unlock;
+	rvu_nix_tx_tl2_cfg(rvu, blkaddr, pcifunc,
 			   &nix_hw->txsch[NIX_TXSCH_LVL_TL2], enable);
+unlock:
+	mutex_unlock(&rvu->rsrc_lock);
 }
 
 static int rvu_switch_install_rx_rule(struct rvu *rvu, u16 pcifunc,
@@ -229,8 +236,20 @@ void rvu_switch_disable(struct rvu *rvu)
 	if (!rswitch->used_entries)
 		return;
 
-	if (rvu->rep_mode)
+	if (rvu->rep_mode) {
+		for (pf = 1; pf < hw->total_pfs; pf++) {
+			if (!is_pf_cgxmapped(rvu, pf))
+				continue;
+			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
+			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+			rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
+			for (vf = 0; vf < numvfs; vf++) {
+				pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
+				rvu_switch_enable_lbk_link(rvu, pcifunc, false);
+			}
+		}
 		goto free_ents;
+	}
 
 	for (pf = 1; pf < hw->total_pfs; pf++) {
 		if (!is_pf_cgxmapped(rvu, pf))
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
index 0f5d5642d3f7..ef47e7e21901 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
@@ -301,6 +301,12 @@ static void rvu_rep_state_evt_handler(struct otx2_nic *priv,
 	int rep_id;
 
 	rep_id = rvu_rep_get_repid(priv, info->pcifunc);
+	if (rep_id < 0) {
+		dev_warn_ratelimited(priv->dev,
+				     "REP state event for unknown pcifunc 0x%x (err %d)\n",
+				     info->pcifunc, rep_id);
+		return;
+	}
 	rep = priv->reps[rep_id];
 	if (info->evt_data.vf_state)
 		rep->flags |= RVU_REP_VF_INITIALIZED;
@@ -459,6 +465,9 @@ static int rvu_rep_open(struct net_device *dev)
 	netif_carrier_on(dev);
 	netif_tx_start_all_queues(dev);
 
+	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
+		return 0;
+
 	evt.event = RVU_EVENT_PORT_STATE;
 	evt.evt_data.port_state = 1;
 	evt.pcifunc = rep->pcifunc;
@@ -478,6 +487,9 @@ static int rvu_rep_stop(struct net_device *dev)
 	netif_carrier_off(dev);
 	netif_tx_disable(dev);
 
+	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
+		return 0;
+
 	evt.event = RVU_EVENT_PORT_STATE;
 	evt.pcifunc = rep->pcifunc;
 	rvu_rep_notify_pfvf(priv, RVU_EVENT_PORT_STATE, &evt);
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
index 5bc9e2c7d800..b98fe191e83a 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.h
@@ -16,7 +16,6 @@
 
 #define PCI_DEVID_RVU_REP	0xA0E0
 
-#define RVU_MAX_REP	OTX2_MAX_CQ_CNT
 
 struct rep_stats {
 	u64 rx_bytes;
-- 
2.48.1


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net v3] octeontx2-af: Fix rep link state sync and workqueue races
@ 2026-09-16 16:49 Nitin Shetty J
  0 siblings, 0 replies; 3+ messages in thread
From: Nitin Shetty J @ 2026-09-16 16:49 UTC (permalink / raw)
  To: netdev-bot+sashiko
  Cc: netdev, linux-kernel, gakula, sgoutham, rkannoth, sbhatta,
	andrew+netdev, davem, edumazet, kuba, pabeni, bbhushan2, horms,
	hkalra

On 2026-09-09 at 04:21:15, netdev-bot+sashiko@kernel.org (netdev-bot+sashiko@kernel.org) wrote:
> Thank you for your contribution! Sashiko AI review found 10 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 2 (1) · Medium: 7 (1) · Low: 1
> 
> - [High] The new authorization gates added by this patch
>   (`req->hdr.pcifunc != rvu->rep_pcifunc -> -EPERM` in…
> - [High] rvu_mbox_handler_get_rep_cnt() now truncates rvu->rep_cnt /
>   rep2pfvf_map to RVU_MAX_REP (64) entries, but every consumer in the AF…
> - [Medium] rvu_rep_open() and rvu_rep_stop() in nic/rep.c now return
>   early for every VF representor (`if (rep->pcifunc &…
> - [Medium] The new teardown barrier in rvu_remove() only flushes
>   rvu->afpf_wq_info.mbox_wq before destroy_workqueue(rep_wq), and its…
> - [Medium] The representor PF's own TL2->LBK link enable performed by
>   rvu_rep_pf_init() (rvu_rep.c:445-451, rvu_switch_enable_lbk_link(rvu,…
> - [Medium] rvu_remove() decides whether to destroy rvu->rep_evt_wq, and
>   clears the pointer, without holding rvu->rsrc_lock, whereas…
> - [Medium] The patch introduces a lockless publish/consume protocol for
>   rvu->rep_evt_wq (rvu_remove() uses WRITE_ONCE(),…
> - [Medium] rvu_rep_install_mcam_rules() tests test_bit(NIXLF_INITIALIZED,
>   &pfvf->flags) outside any lock and then calls…
> - [Medium] Draining the representor event workqueue after unregistering
>   interrupts causes mailbox communication timeouts.
> - [Low] In the first-time GET_REP_CNT path, `rvu->rep_pcifunc =
>   req->hdr.pcifunc;` (rvu_rep.c:504) executes before both -ENOMEM exits…
> 
> Pre-existing issues:
> - [High] When rvu_rep_install_mcam_rules() fails (the new err_disable_lbk
>   path still returns err), rvu_switch_enable()'s uninstall_rules label…
> - [Medium] rvu_rep_install_tx_rule() first calls rvu_rep_tx_vlan_cfg(),
>   which allocates a NIX TX VTAG definition entry and programs…
> 
> --
> 
> octeontx2-af: Fix rep link state sync and workqueue races
> 
> This moves the representor event workqueue creation from
> rvu_rep_install_mcam_rules() into rvu_mbox_handler_get_rep_cnt(), adds
> sender and bounds validation to the REP mailbox handlers, and makes LBK
> link configuration take rsrc_lock.  A few questions below about the new
> validation and teardown paths.
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > index 937b085582b5..dc4e1fa900b7 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> > @@ -3721,6 +3721,20 @@ static void rvu_remove(struct pci_dev *pdev)
> >  	rvu_unregister_dl(rvu);
> >  	rvu_unregister_interrupts(rvu);
> >  	rvu_flr_wq_destroy(rvu);
> > +	if (rvu->rep_evt_wq) {
> > +		struct workqueue_struct *rep_wq = rvu->rep_evt_wq;
> > +
> > +		/* NULL the pointer before flushing mbox_wq.  Any mbox handler
> > +		 * still in flight will snapshot NULL via READ_ONCE() and return
> > +		 * -EINVAL without calling queue_work(), so no new items can be
> > +		 * added to rep_wq after flush_workqueue(mbox_wq) returns.
> > +		 */
> > +		WRITE_ONCE(rvu->rep_evt_wq, NULL);
> > +		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
> > +		destroy_workqueue(rep_wq);
> > +	} else {
> > +		flush_workqueue(rvu->afpf_wq_info.mbox_wq);
> > +	}
> 
> [Severity: Medium]
> This teardown sits after rvu_unregister_interrupts(), which has
> already disabled the AF-PF mailbox interrupt (RVU_AF_PFAF_MBOX_INT_ENA_W1C,
> or cn20k_rvu_unregister_interrupts()).  destroy_workqueue() calls
> drain_workqueue(), so any rep_evt_work still queued now runs to
> completion here, and rvu_rep_wq_handler() -> rvu_rep_up_notify() does
> otx2_mbox_msg_send_up() followed by otx2_mbox_wait_for_zero() and
> otx2_mbox_wait_for_rsp() on rvu->afpf_wq_info.mbox_up.
> 
> With the mailbox interrupt already gone, the PF's reply is never
> processed by the AF, so each of those waits has to time out rather
> than complete.  With a non-empty rep_evtq_head that is paid per
> queued event, all while holding rvu->mbox_lock, which stretches
> rvu_remove() out by seconds at a time on a path userspace is
> blocked on.
> 
> Would it be better to move this block ahead of
> rvu_unregister_interrupts() so the pending up-notifies can still be
> acknowledged, or alternatively to stop rvu_rep_wq_handler() from
> sending anything once removal has started (a teardown flag checked
> before otx2_mbox_msg_send_up(), or draining rep_evtq_head and
> freeing the qentries without notifying)?  Note the same question
> applies to the qentry list itself: nothing appears to free the
> entries that are still on rvu->rep_evtq_head if the handler is not
> allowed to consume them.
> 
wiil fix it in next patch version.

> [Severity: Medium]
> Is flushing only rvu->afpf_wq_info.mbox_wq enough to close the
> queue_work() window the comment describes?
> 
> The same rvu_mbox_handler_* dispatch also runs on
> rvu->afvf_wq_info.mbox_wq via rvu_afvf_mbox_handler(), and that
> workqueue is destroyed much later, inside rvu_disable_sriov() ->
> rvu_mbox_destroy().  For AF-VF messages the dispatcher clears only the
> func bits and keeps the sender-supplied PF field, so
> rvu_mbox_handler_nix_lf_start_rx()/nix_lf_stop_rx()/rvu_nix_lf_teardown()
> can reach rvu_rep_notify_pfvf_state(), which sets
> req.hdr.pcifunc = rvu->rep_pcifunc and therefore passes the new -EPERM
> gate:
> 
> CPU0 (afvf mbox work)               CPU1 (rvu_remove)
>   wq = READ_ONCE(rvu->rep_evt_wq);  /* non-NULL */
>                                     WRITE_ONCE(rvu->rep_evt_wq, NULL);
>                                     flush_workqueue(afpf mbox_wq);
>                                     destroy_workqueue(rep_wq);
>   queue_work(wq, &rvu->rep_evt_work);  /* freed wq */
> 
> Should the afvf mailbox workqueue be flushed here too (or the VF mailbox
> torn down before this point)?  The qentry that handler already kmalloc'd
> and put on rep_evtq_head is leaked in that case as well.
> 
wiil fix it in next patch version.

> [Severity: Medium]
> Can rvu->rep_evt_wq become non-NULL after this if () test?  The
> producer, rvu_mbox_handler_get_rep_cnt(), now publishes the pointer
> under rvu->rsrc_lock, while this teardown reads and clears it with no
> lock and never re-checks:
> 
> CPU0 (get_rep_cnt work on mbox_wq)   CPU1 (rvu_remove)
>                                      if (rvu->rep_evt_wq)  /* NULL */
>                                      else
>                                        flush_workqueue(mbox_wq);
>   mutex_lock(&rvu->rsrc_lock);
>   rvu->rep_evt_wq = alloc_workqueue(...);
> 
> The freshly created workqueue and its worker pool are then never
> destroyed, and a REP_EVENT_NOTIFY message processed later in the same
> drained batch can queue_work(&rvu->rep_evt_work) on a workqueue nothing
> waits for, so rvu_rep_wq_handler() may touch rvu after
> devm_kfree(&pdev->dev, rvu).  Would taking rsrc_lock here (or a
> "removing" flag checked by the handler) close that?
>
wiil fix it in next patch version.
 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> > index a2781e0f504e..69b260fdd52d 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_rep.c
> 
> [ ... ]
> 
> > @@ -95,8 +101,17 @@ static void rvu_rep_wq_handler(struct work_struct *work)
> >  int rvu_mbox_handler_rep_event_notify(struct rvu *rvu, struct rep_event *req,
> >  				      struct msg_rsp *rsp)
> >  {
> > +	struct workqueue_struct *wq;
> >  	struct rep_evtq_ent *qentry;
> >  
> > +	wq = READ_ONCE(rvu->rep_evt_wq);
> > +	if (!wq)
> > +		return -EINVAL;
> > +
> > +	/* Only the registered representor PF may send REP_EVENT_NOTIFY. */
> > +	if (req->hdr.pcifunc != rvu->rep_pcifunc)
> > +		return -EPERM;
> 
> [Severity: High]
> How much does this gate actually restrict, given that rvu->rep_pcifunc
> is whatever function sent MBOX_MSG_GET_REP_CNT first?
> 
> rvu_mbox_handler_get_rep_cnt() still does an unconditional
> rvu->rep_pcifunc = req->hdr.pcifunc with no check that the sender is the
> representor function (PCI_DEVID_RVU_REP), no capability check and no
> rep_mode gating.  The PF driver forwards VF mailbox messages to the AF
> verbatim - otx2_pfvf_mbox_handler() only rewrites the func bits and does
> no message-ID filtering:
> 
> 	msg->pcifunc &= ~RVU_PFVF_FUNC_MASK;
> 	msg->pcifunc |= (vf_idx + 1) & RVU_PFVF_FUNC_MASK;
> 
> So can a guest VF send GET_REP_CNT before the representor driver probes,
> become rvu->rep_pcifunc, and then satisfy both new -EPERM checks?  From
> there REP_EVENT_NOTIFY with req->pcifunc pointing at any other
> CGX-mapped function passes is_pf_func_valid()/is_pf_cgxmapped() (those
> only confirm the victim exists), and rvu_rep_up_notify() then does:
> 
> 	if (event->event & RVU_EVENT_MAC_ADDR_CHANGE)
> 		ether_addr_copy(pfvf->mac_addr, event->evt_data.mac);
> 
> overwriting the victim's AF-side MAC and sending a forged up-notify to
> the victim's PF.  is_rep_dev() is just a pcifunc comparison against
> rvu->rep_pcifunc, so the same caller also gets representor treatment in
> rvu_nix.c / rvu_npc_fs.c, and the real representor driver is locked out
> afterwards because its GET_REP_CNT now returns -EPERM.  Should the
> registration itself authenticate the representor rather than trusting
> the first mailbox caller?
> 
For the guest VF path, we can check req->hdr.pcifunc & RVU_PFVF_FUNC_MASK
in rvu_mbox_handler_get_rep_cnt() and reject with -EPERM if set,
so a VF can not register as rep_pcifunc.
Full PF authentication is a larger change, we'll address that in a
separate patch set.

> [ ... ]
> 
> >  int rvu_rep_notify_pfvf_state(struct rvu *rvu, u16 pcifunc, bool enable)
> >  {
> > -	struct rep_event *req;
> > -	int pf;
> > +	struct rep_event req = { 0 };
> > +	struct msg_rsp rsp;
> >  
> >  	if (!is_pf_cgxmapped(rvu, rvu_get_pf(rvu->pdev, pcifunc)))
> >  		return 0;
> >  
> > -	pf = rvu_get_pf(rvu->pdev, rvu->rep_pcifunc);
> > -
> > -	mutex_lock(&rvu->mbox_lock);
> > -	req = otx2_mbox_alloc_msg_rep_event_up_notify(rvu, pf);
> 
> [ ... ]
> 
> > +	req.hdr.pcifunc = rvu->rep_pcifunc;
> > +	req.event = RVU_EVENT_PFVF_STATE;
> > +	req.pcifunc = pcifunc;
> > +	req.evt_data.vf_state = enable;
> > +	return rvu_mbox_handler_rep_event_notify(rvu, &req, &rsp);
> >  }
> 
> [ ... ]
> 
> > @@ -334,19 +344,22 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
> >  			continue;
> >  
> >  		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> > +		pfvf = rvu_get_pfvf(rvu, pcifunc);
> >  		rvu_get_nix_blkaddr(rvu, pcifunc);
> > +		if (test_bit(NIXLF_INITIALIZED, &pfvf->flags))
> > +			rvu_switch_enable_lbk_link(rvu, pcifunc, true);
> 
> [Severity: Medium]
> Does moving rsrc_lock inside rvu_switch_enable_lbk_link() serialize
> anything useful here?  The NIXLF_INITIALIZED test is outside the lock,
> and the flag is set in rvu_mbox_handler_nix_lf_start_rx() and cleared in
> rvu_mbox_handler_nix_lf_stop_rx()/rvu_nix_lf_teardown() with no lock at
> all.  rvu_rep_install_mcam_rules() runs from
> rvu_devlink_eswitch_mode_set() -> rvu_switch_enable(), which holds no
> RVU lock:
> 
> CPU0 (devlink switchdev)              CPU1 (mbox NIX_LF_STOP_RX)
>   test_bit(NIXLF_INITIALIZED) /* 1 */
>                                       clear_bit(NIXLF_INITIALIZED);
>                                       rvu_switch_update_rules(false)
>                                         rvu_rep_update_rules()
>                                           lbk_link(..., false); /* lock */
>   rvu_switch_enable_lbk_link(true);
>     /* lock, writes enable */
> 
> Doesn't that leave the TL2 LBK link on channel 63 enabled for a function
> whose NIXLF was stopped or freed?  The new err_disable_lbk rollback and
> the new rep_mode loop in rvu_switch_disable() can lose the same race
> against a concurrent LF start.
> 
Yes. this race is still open since rsrc_lock isn't held across the
test_bit(NIXLF_INITIALIZED, ...) check and the subsequent LBK link
enable/disable. Fixing it properly needs broader lock coverage across
that check-then-act sequence, so we'd like to take it up in a
follow-up patch.

> [ ... ]
> 
> > @@ -366,30 +382,37 @@ int rvu_rep_install_mcam_rules(struct rvu *rvu)
> >  		}
> >  	}
> > +	return 0;
> >  
> > -	/* Initialize the wq for handling REP events */
> > -	spin_lock_init(&rvu->rep_evtq_lock);
> > -	INIT_LIST_HEAD(&rvu->rep_evtq_head);
> > -	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
> > -	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_PERCPU, 0);
> > -	if (!rvu->rep_evt_wq) {
> > -		dev_err(rvu->dev, "REP workqueue allocation failed\n");
> > -		return -ENOMEM;
> > +err_disable_lbk:
> > +	/* Undo any LBK links enabled above before the MCAM rule failure.
> > +	 * Disabling a link that was never enabled is a safe no-op.
> > +	 */
> > +	for (pf = 1; pf < hw->total_pfs; pf++) {
> > +		if (!is_pf_cgxmapped(rvu, pf))
> > +			continue;
> > +		pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> > +		rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> > +		rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
> > +		for (vf = 0; vf < numvfs; vf++) {
> > +			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
> > +			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> > +		}
> >  	}
> > -	return 0;
> > +	return err;
> >  }
> 
> [Severity: Medium]
> Does this rollback miss the representor function itself?
> rvu_rep_pf_init() enables the LBK link for rvu->rep_pcifunc:
> 
> 	set_bit(NIXLF_INITIALIZED, &pfvf->flags);
> 	rvu_switch_enable_lbk_link(rvu, pcifunc, true);
> 
> but this loop, and the new rep_mode loop in rvu_switch_disable(), both
> skip anything that is not CGX-mapped, and the representor is a separate
> non-CGX-mapped function (PCI_DEVID_RVU_REP).  rvu_nix_tx_tl2_cfg()
> explicitly allows the write for it:
> 
> 	if (!is_pf_cgxmapped(rvu, pf) && !is_rep_dev(rvu, pcifunc))
> 		return;
> 
> So after a failed install, or after "devlink dev eswitch set mode
> legacy", does the representor PF keep its TL2 nodes pointed at LBK
> channel 63 with all matching NPC entries deleted?  Nothing re-evaluates
> it either, since rvu_rep_pf_init() leaves NIXLF_INITIALIZED set on the
> rep pfvf.
> 
will fix it in next patch version.

> [Severity: High]
> This isn't a bug introduced by this patch, but every rule-installation
> failure now routes through this return err, so it may be worth fixing
> alongside.  rvu_switch_enable()'s uninstall_rules label does
> kfree(rswitch->entry2pcifunc) without clearing the pointer or resetting
> rswitch->used_entries, and rvu_devlink_eswitch_mode_set() has already
> committed rswitch->mode.  A following "devlink dev eswitch set mode
> legacy" reaches rvu_switch_disable(), whose only guard is:
> 
> 	if (!rswitch->used_entries)
> 		return;
> 	...
> free_ents:
> 	...
> 	kfree(rswitch->entry2pcifunc);
> 
> which frees the same pointer a second time.  rvu_rep_update_rules() and
> rvu_switch_update_rules(), reachable from NIX_LF_START_RX/STOP_RX
> mailbox messages, are gated only on used_entries as well and then read
> rswitch->entry2pcifunc[entry].  Should the failure path also clear
> rswitch->entry2pcifunc and rswitch->used_entries?
> 
will fix it in next patch version.

> [Severity: Medium]
> This is a pre-existing issue, but does the new cleanup also need to
> release TX VTAG definitions?  rvu_rep_install_tx_rule() calls
> rvu_rep_tx_vlan_cfg() first, which allocates a NIX TX VTAG definition
> entry and programs NIX_AF_TX_VTAG_DEFX_DATA/CTL, and only then calls
> rvu_mbox_handler_npc_install_flow().  When the MCAM install fails the
> returned vtag0_idx is dropped: err_disable_lbk only touches LBK links,
> and rvu_switch_enable()'s uninstall_rules/free_entries labels only
> delete NPC flows and free MCAM entries, while VTAG entries live in
> nix_hw->txvlan and need nix_tx_vtag_free().  Do repeated failed
> switchdev enables then consume the hardware VTAG definition table?
> 
Yes. since it is exisitng issue will fix it in separate patch.

> [ ... ]
> 
> > @@ -443,35 +466,92 @@ int rvu_mbox_handler_esw_cfg(struct rvu *rvu, struct esw_cfg_req *req,
> 
> [ ... ]
> 
> >  int rvu_mbox_handler_get_rep_cnt(struct rvu *rvu, struct msg_req *req,
> >  				 struct get_rep_cnt_rsp *rsp)
> >  {
> > -	int pf, vf, numvfs, hwvf, rep = 0;
> > +	int pf, vf, numvfs, hwvf, rep = 0, cnt;
> > +	int ret = 0;
> >  	u16 pcifunc;
> > +	u16 *map;
> > +
> > +	/* Serialize first-time initialization. mbox_wq is WQ_PERCPU so
> > +	 * handlers for different PFs can run concurrently; without this
> > +	 * lock two callers could both observe rep2pfvf_map == NULL and
> > +	 * double-allocate the workqueue, leaking one permanently.
> > +	 */
> > +	mutex_lock(&rvu->rsrc_lock);
> > +
> > +	if (rvu->rep2pfvf_map) {
> > +		ret = rvu_rep_get_rep_map(rvu, req, rsp);
> > +		goto unlock;
> > +	}
> >  
> >  	rvu->rep_pcifunc = req->hdr.pcifunc;
> 
> [Severity: Low]
> Is it intentional that rvu->rep_pcifunc is published before both -ENOMEM
> exits below?  On either failure rvu->rep2pfvf_map stays NULL, so the
> fast-path guard above does not fire for the next caller, which re-enters
> this branch and overwrites rvu->rep_pcifunc.  Meanwhile the failed
> caller already has is_rep_dev() true, and its pcifunc is consumed by
> rvu_rep_install_rx_rule()/rvu_rep_install_tx_rule() (req.vf =
> rvu->rep_pcifunc) and by rvu_rep_pf_init(), which sets
> NIXLF_INITIALIZED and enables the LBK link for it.  Would assigning
> rep_pcifunc only on the success path avoid that stale state?
> 
> > -	rsp->rep_cnt = rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs;
> > -	rvu->rep_cnt = rsp->rep_cnt;
> > +	cnt = min_t(int, rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs,
> > +		    RVU_MAX_REP);
> 
will fix it in next patch version.

> [Severity: High]
> What happens on a part where rvu->cgx_mapped_pfs + rvu->cgx_mapped_vfs
> exceeds RVU_MAX_REP?  The map is capped here, but the consumers are not:
> rvu_switch_enable() still sizes the MCAM allocation from the uncapped
> sum (times 4 in rep mode), and rvu_rep_install_mcam_rules() still loops
> pf = 1..hw->total_pfs and vf = 0..numvfs with no bound tied to
> rvu->rep_cnt.
> 
> Both rule installers take their VLAN/index from rvu_rep_get_vlan_id(),
> which falls off its loop and returns the identifier of representor 0
> when the pcifunc is not in the map:
> 
> 	for (id = 0; id < rvu->rep_cnt; id++)
> 		if (rvu->rep2pfvf_map[id] == pcifunc)
> 			return id;
> 	return 0;
> 
> So for every function past the cap, does rvu_rep_install_rx_rule()
> program a rule identical to representor 0's -
> 
> 	vlan_tci = rep_id | BIT_ULL(8);
> 	req.vf = rvu->rep_pcifunc;
> 	req.index = rep_id;
> 
> - and steer that traffic to the wrong representor?  Their state events
> are dropped on the peer side as well, since rvu_rep_get_repid() returns
> -EINVAL and only the new warning fires.  Since numvfs is an 8-bit
> per-PF field summed over all CGX PFs, 8 PFs with 8 VFs each already
> passes 64.  Would rejecting such a configuration with a diagnostic here,
> or applying the same cap in the rule installer and event producers, be
> preferable to silent truncation?
> 
the cgx_mapped_pfs  and cgx_mapped_vfs are programmed from firmware.
max cgx_mapped_pfs is 8 and max cgx_mapped_vfs per PF is 3. this is
well within the limit 64. will fix it in next patch version.

> [ ... ]
> 
> > +	/* Initialize the wq for handling REP events */
> > +	spin_lock_init(&rvu->rep_evtq_lock);
> > +	INIT_LIST_HEAD(&rvu->rep_evtq_head);
> > +	INIT_WORK(&rvu->rep_evt_work, rvu_rep_wq_handler);
> > +	rvu->rep_evt_wq = alloc_workqueue("rep_evt_wq", WQ_UNBOUND, 0);
> 
> [Severity: Medium]
> The consumer side uses READ_ONCE(rvu->rep_evt_wq) as the gate for
> touching rep_evtq_lock, rep_evtq_head and rep_evt_work, but the
> publication here is a plain store with no smp_store_release() and the
> reader has no acquire.  On a weakly ordered machine, can a consumer that
> is not serialized with this function by rvu->mbox_lock - for example
> rvu_mbox_handler_rep_event_notify() reached from an AF-VF mailbox work
> item, since rvu_afvf_mbox_handler() does not take mbox_lock - see
> rep_evt_wq non-NULL before the spin_lock_init()/INIT_LIST_HEAD()/
> INIT_WORK() stores are visible, and then run spin_lock() and
> list_add_tail() on uninitialized state?  The same ordering question
> applies to rvu->rep_pcifunc, which now gates the -EPERM check.
> 
will fix it in next patch version.

> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> > index 49ce38685a7e..8c2c98403507 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_switch.c
> > @@ -12,11 +12,18 @@ void rvu_switch_enable_lbk_link(struct rvu *rvu, u16 pcifunc, bool enable)
> >  {
> >  	struct rvu_pfvf *pfvf = rvu_get_pfvf(rvu, pcifunc);
> >  	struct nix_hw *nix_hw;
> > +	int blkaddr;
> >  
> > -	nix_hw = get_nix_hw(rvu->hw, pfvf->nix_blkaddr);
> > +	mutex_lock(&rvu->rsrc_lock);
> > +	blkaddr = pfvf->nix_blkaddr;
> > +	nix_hw = get_nix_hw(rvu->hw, blkaddr);
> >  	/* Enable LBK links with channel 63 for TX MCAM rule */
> > -	rvu_nix_tx_tl2_cfg(rvu, pfvf->nix_blkaddr, pcifunc,
> > +	if (!nix_hw)
> > +		goto unlock;
> > +	rvu_nix_tx_tl2_cfg(rvu, blkaddr, pcifunc,
> >  			   &nix_hw->txsch[NIX_TXSCH_LVL_TL2], enable);
> > +unlock:
> > +	mutex_unlock(&rvu->rsrc_lock);
> >  }
> 
> [ ... ]
> 
> > @@ -229,8 +236,20 @@ void rvu_switch_disable(struct rvu *rvu)
> >  	if (!rswitch->used_entries)
> >  		return;
> >  
> > -	if (rvu->rep_mode)
> > +	if (rvu->rep_mode) {
> > +		for (pf = 1; pf < hw->total_pfs; pf++) {
> > +			if (!is_pf_cgxmapped(rvu, pf))
> > +				continue;
> > +			pcifunc = rvu_make_pcifunc(rvu->pdev, pf, 0);
> > +			rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> > +			rvu_get_pf_numvfs(rvu, pf, &numvfs, NULL);
> > +			for (vf = 0; vf < numvfs; vf++) {
> > +				pcifunc = rvu_make_pcifunc(rvu->pdev, pf, vf + 1);
> > +				rvu_switch_enable_lbk_link(rvu, pcifunc, false);
> > +			}
> > +		}
> >  		goto free_ents;
> > +	}
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > index 0f5d5642d3f7..ef47e7e21901 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> 
> [ ... ]
> 
> > @@ -459,6 +465,9 @@ static int rvu_rep_open(struct net_device *dev)
> >  	netif_carrier_on(dev);
> >  	netif_tx_start_all_queues(dev);
> >  
> > +	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
> > +		return 0;
> > +
> >  	evt.event = RVU_EVENT_PORT_STATE;
> >  	evt.evt_data.port_state = 1;
> >  	evt.pcifunc = rep->pcifunc;
> 
> [Severity: Medium]
> These two early returns in rvu_rep_open() and rvu_rep_stop() skip the
> RVU_EVENT_PORT_STATE notification for every VF representor, which is the
> representor-to-representee link state propagation added by the commit in
> the Fixes: tag (b8fea84a0468).  After this, does "ip link set <vf-rep>
> up/down" still affect the representee?
> 
> The reason the notification does not work today looks like the missing
> handler on the VF side - otx2vf_process_mbox_msg_up() only handles
> MBOX_MSG_CGX_LINK_EVENT and answers everything else with:
> 
> 	default:
> 		otx2_reply_invalid_msg(&vf->mbox.mbox_up, 0, 0, req->id);
> 		return -ENODEV;
> 
> while the PF side does implement
> otx2_mbox_up_handler_rep_event_up_notify().  Would adding
> MBOX_MSG_REP_EVENT_UP_NOTIFY to otx2vf_process_mbox_msg_up() be the fix
> instead?  Neither the commit message nor the changelog mentions these
> two hunks.
> 
I agree this should be properly handled instead of simply early return.
will fix this issue in separate patch.

> > @@ -478,6 +487,9 @@ static int rvu_rep_stop(struct net_device *dev)
> >  	netif_carrier_off(dev);
> >  	netif_tx_disable(dev);
> >  
> > +	if (rep->pcifunc & RVU_PFVF_FUNC_MASK)
> > +		return 0;
> > +
> >  	evt.event = RVU_EVENT_PORT_STATE;
> >  	evt.pcifunc = rep->pcifunc;
> >  	rvu_rep_notify_pfvf(priv, RVU_EVENT_PORT_STATE, &evt);
> 
> Thanks for looking at these.
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903074840.2373506-1-nshettyj%40marvell.com
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-16 16:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-03  7:48 [PATCH net v3] octeontx2-af: Fix rep link state sync and workqueue races nshettyj
2026-09-08 22:51 ` netdev-bot+sashiko
2026-09-16 16:49 Nitin Shetty J

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®