[PATCH net v6 0/2] octeontx2: Misc fixes for RVU drivers

[PATCH net v6 0/2] octeontx2: Misc fixes for RVU drivers

[Subrat Pandey]

A few independent fixes for the Marvell OcteonTX2 (RVU AF/PF) drivers:

 - Fix aura/CQ BPID assignment under CONFIG_DCB, which assumed
   queue_to_pfc_map was always allocated (not true for SDP VFs).
 - Fix the SC resource cleanup loop that iterated over secy.max
   instead of sc.max.

Changes in v6:
 - None. Rebased onto latest net tree.

Changes in v5:
 - patch 1: Fixing the compilation issue when CONFIG_DCB is
   disabled.

Changes in v4:
 - patch 1: Add 'inline' qualifier to otx2_get_bpid_idx() helper
   defined in the header.
 - patch 2: Correct commit subject in Fixes: tag to match the actual
   upstream commit title.

Changes in v3:
 - patch 1: Unify the BPID index logic into a shared otx2_get_bpid_idx()
   helper, dropping the per-file #ifdef CONFIG_DCB.
 - patch 2: Extend the sc.max fix to the SC stats loop in
   mcs_clear_all_stats(); add Fixes: tag.

Changes in v2:
 - Dropped "octeontx2-af: Fix mcs string buffer size", will
   re-target it to net-next.

Baha Mesleh (1):
  octeontx2-af: mcs: Fix SC resource cleanup loop

Geetha sowjanya (1):
  octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

 drivers/net/ethernet/marvell/octeontx2/af/mcs.c  |  4 ++--
 .../net/ethernet/marvell/octeontx2/nic/cn20k.c   | 11 +----------
 .../ethernet/marvell/octeontx2/nic/otx2_common.c | 16 ++++++----------
 .../ethernet/marvell/octeontx2/nic/otx2_common.h |  8 ++++++++
 4 files changed, 17 insertions(+), 22 deletions(-)

-- 
2.43.0

[PATCH net v6 1/2] octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

[Subrat Pandey]

From: Geetha sowjanya <gakula@marvell.com>

Previously, BPID assignment under CONFIG_DCB assumed
`queue_to_pfc_map` was always initialized. For SDP VFs this leads to
invalid memory access as it was not initialized.

This patch adds a NULL check for `queue_to_pfc_map` before
dereferencing it. Also, simplifies the logic by always assigning a
default BPID first, then conditionally overriding it if CONFIG_DCB is
enabled and the map exists.

Fixes: 184fb40f731b ("octeontx2-pf: Avoid adding dcbnl_ops for LBK and SDP vf")
Signed-off-by: Geetha sowjanya <gakula@marvell.com>
Signed-off-by: Subrat Pandey <subratp@marvell.com>
---
v5:
 - Fixing the compilation issue when CONFIG_DCB is
   disabled.
v4:
 - Fix otx2_get_bpid_idx() to be static inline.
v3:
 - Unify BPID index logic into a shared otx2_get_bpid_idx() helper,
   dropping the per-file #ifdef CONFIG_DCB and cn20k_aura_bpid_idx().
 - Drop trailing period from the Fixes: tag.

 .../net/ethernet/marvell/octeontx2/nic/cn20k.c   | 11 +----------
 .../ethernet/marvell/octeontx2/nic/otx2_common.c | 16 ++++++----------
 .../ethernet/marvell/octeontx2/nic/otx2_common.h |  8 ++++++++
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
index 8e41431c7f9c..e92b77789f08 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
@@ -242,15 +242,6 @@ int cn20k_register_pfvf_mbox_intr(struct otx2_nic *pf, int numvfs)
 
 #define RQ_BP_LVL_AURA   (255 - ((85 * 256) / 100)) /* BP when 85% is full */
 
-static u8 cn20k_aura_bpid_idx(struct otx2_nic *pfvf, int aura_id)
-{
-#ifdef CONFIG_DCB
-	return pfvf->queue_to_pfc_map[aura_id];
-#else
-	return 0;
-#endif
-}
-
 static int cn20k_tc_get_entry_index(struct otx2_flow_config *flow_cfg,
 				    struct otx2_tc_flow *node)
 {
@@ -580,7 +571,7 @@ static int cn20k_aura_aq_init(struct otx2_nic *pfvf, int aura_id,
 		if (pfvf->nix_blkaddr == BLKADDR_NIX1)
 			aq->aura.bp_ena = 1;
 
-		bpid_idx = cn20k_aura_bpid_idx(pfvf, aura_id);
+		bpid_idx = otx2_get_bpid_idx(pfvf, aura_id);
 		aq->aura.bpid = pfvf->bpid[bpid_idx];
 
 		/* Set backpressure level for RQ's Aura */
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 175992188c18..0f3f405d9f1b 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -1055,6 +1055,7 @@ int otx2_cq_init(struct otx2_nic *pfvf, u16 qidx)
 	struct nix_aq_enq_req *aq;
 	struct otx2_cq_queue *cq;
 	struct otx2_pool *pool;
+	u8 bpid_idx;
 
 	cq = &qset->cq[qidx];
 	cq->cq_idx = qidx;
@@ -1132,11 +1133,8 @@ int otx2_cq_init(struct otx2_nic *pfvf, u16 qidx)
 		if (!is_otx2_lbkvf(pfvf->pdev)) {
 			/* Enable receive CQ backpressure */
 			aq->cq.bp_ena = 1;
-#ifdef CONFIG_DCB
-			aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
-#else
-			aq->cq.bpid = pfvf->bpid[0];
-#endif
+			bpid_idx = otx2_get_bpid_idx(pfvf, qidx);
+			aq->cq.bpid = pfvf->bpid[bpid_idx];
 
 			/* Set backpressure level is same as cq pass level */
 			aq->cq.bp = RQ_PASS_LVL_CQ(pfvf->hw.rq_skid, qset->rqe_cnt);
@@ -1378,6 +1376,7 @@ int otx2_aura_aq_init(struct otx2_nic *pfvf, int aura_id,
 {
 	struct npa_aq_enq_req *aq;
 	struct otx2_pool *pool;
+	u8 bpid_idx;
 	int err;
 
 	pool = &pfvf->qset.pool[pool_id];
@@ -1433,11 +1432,8 @@ int otx2_aura_aq_init(struct otx2_nic *pfvf, int aura_id,
 		 */
 		if (pfvf->nix_blkaddr == BLKADDR_NIX1)
 			aq->aura.bp_ena = 1;
-#ifdef CONFIG_DCB
-		aq->aura.nix0_bpid = pfvf->bpid[pfvf->queue_to_pfc_map[aura_id]];
-#else
-		aq->aura.nix0_bpid = pfvf->bpid[0];
-#endif
+		bpid_idx = otx2_get_bpid_idx(pfvf, aura_id);
+		aq->aura.nix0_bpid = pfvf->bpid[bpid_idx];
 
 		/* Set backpressure level for RQ's Aura */
 		aq->aura.bp = RQ_BP_LVL_AURA;
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
index eecee612b7b2..2a63d22cbd8d 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
@@ -1029,6 +1029,14 @@ static inline int otx2_tc_flower_rule_cnt(struct otx2_nic *pfvf)
 	return pfvf->flow_cfg->nr_flows;
 }
 
+static inline u8 otx2_get_bpid_idx(struct otx2_nic *pfvf, int qidx)
+{
+#ifdef CONFIG_DCB
+	return pfvf->queue_to_pfc_map[qidx];
+#endif
+	return 0;
+}
+
 /* MSI-X APIs */
 void otx2_free_cints(struct otx2_nic *pfvf, int n);
 void otx2_set_cints_affinity(struct otx2_nic *pfvf);
-- 
2.43.0

Re: [PATCH net v6 1/2] octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

This consolidates the per-file CONFIG_DCB ifdef blocks that pick the BPID
index for CQ and aura contexts into a new otx2_get_bpid_idx() helper in
otx2_common.h, and drops the equivalent cn20k_aura_bpid_idx().

>     Previously, BPID assignment under CONFIG_DCB assumed
>     `queue_to_pfc_map` was always initialized. For SDP VFs this leads to
>     invalid memory access as it was not initialized.
>
>     This patch adds a NULL check for `queue_to_pfc_map` before
>     dereferencing it. Also, simplifies the logic by always assigning a
>     default BPID first, then conditionally overriding it if CONFIG_DCB is
>     enabled and the map exists.

[Severity: High]
Does the patch match this description?  I do not see a NULL check for
queue_to_pfc_map anywhere in the diff, and no place where a default BPID is
assigned first and then conditionally overridden.  The new helper appears to
dereference the map unconditionally when CONFIG_DCB=y, which looks
byte-for-byte equivalent to the ifdef blocks it replaces.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> index eecee612b7b2c..2a63d22cbd8d0 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h
> @@ -1029,6 +1029,14 @@ static inline int otx2_tc_flower_rule_cnt(struct otx2_nic *pfvf)
>  	return pfvf->flow_cfg->nr_flows;
>  }
>  
> +static inline u8 otx2_get_bpid_idx(struct otx2_nic *pfvf, int qidx)
> +{
> +#ifdef CONFIG_DCB
> +	return pfvf->queue_to_pfc_map[qidx];
> +#endif
> +	return 0;
> +}
> +

[Severity: High]
With CONFIG_DCB=y, isn't the trailing return 0 dead code here, leaving
pfvf->queue_to_pfc_map[qidx] as an unguarded dereference?

queue_to_pfc_map is allocated only in otx2_dcbnl_set_ops():

drivers/net/ethernet/marvell/octeontx2/nic/otx2_dcbnl.c:otx2_dcbnl_set_ops() {
	pfvf->queue_to_pfc_map = devm_kzalloc(pfvf->dev, pfvf->hw.rx_queues,
					      GFP_KERNEL);
}

and since the Fixes: commit, otx2vf_probe() skips that call for SDP and LBK
VFs:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_vf.c:otx2vf_probe() {
#ifdef CONFIG_DCB
	/* Priority flow control is not supported for LBK and SDP vf(s) */
	if (!(is_otx2_lbkvf(vf->pdev) || is_otx2_sdp_rep(vf->pdev))) {
		err = otx2_dcbnl_set_ops(netdev);
#endif
}

so for an SDP VF the pointer stays NULL for the lifetime of the device.

Would something like this be what was intended?

	static inline u8 otx2_get_bpid_idx(struct otx2_nic *pfvf, int qidx)
	{
	#ifdef CONFIG_DCB
		if (pfvf->queue_to_pfc_map)
			return pfvf->queue_to_pfc_map[qidx];
	#endif
		return 0;
	}

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> index 175992188c18a..0f3f405d9f1b7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c

[ ... ]

> @@ -1132,11 +1133,8 @@ int otx2_cq_init(struct otx2_nic *pfvf, u16 qidx)
>  		if (!is_otx2_lbkvf(pfvf->pdev)) {
>  			/* Enable receive CQ backpressure */
>  			aq->cq.bp_ena = 1;
> -#ifdef CONFIG_DCB
> -			aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
> -#else
> -			aq->cq.bpid = pfvf->bpid[0];
> -#endif
> +			bpid_idx = otx2_get_bpid_idx(pfvf, qidx);
> +			aq->cq.bpid = pfvf->bpid[bpid_idx];

[Severity: High]
Does this call site still oops on an SDP VF with CONFIG_DCB=y?  The only
guard here is !is_otx2_lbkvf(), which matches PCI_DEVID_OCTEONTX2_RVU_AFVF
and PCI_DEVID_RVU_REP but not PCI_DEVID_OCTEONTX2_SDP_REP:

drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.h {
	static inline bool is_otx2_lbkvf(struct pci_dev *pdev)
	{
		return (pdev->device == PCI_DEVID_OCTEONTX2_RVU_AFVF) ||
			(pdev->device == PCI_DEVID_RVU_REP);
	}
}

so interface bring up on an SDP VF reaches otx2_get_bpid_idx() with a NULL
map:

ndo_open -> otx2_init_hw_resources() -> otx2_cq_init() ->
otx2_get_bpid_idx()

The same holds for the other two converted sites, which carry identical
guards:

otx2_init_hw_resources() -> otx2_rq_aura_pool_init() ->
otx2_aura_aq_init() -> otx2_get_bpid_idx()

and cn20k_aura_aq_init() -> otx2_get_bpid_idx(), where the removed
cn20k_aura_bpid_idx() had the same shape.

Given the Fixes: tag will pull this into stable, could the helper be
respun with the guard the changelog describes, so that SDP VFs fall back to
pfvf->bpid[0]?

[ ... ]
-- 
pw-bot: cr

[PATCH net v6 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

[Subrat Pandey]

From: Baha Mesleh <baha.mesleh@nokia.com>

The SC resource cleanup loop was incorrectly iterating over secy.max
instead of sc.max. Use sc.max as the loop bound.

Fixes: cfc14181d497 ("octeontx2-af: cn10k: mcs: Manage the MCS block hardware resources")
Signed-off-by: Baha Mesleh <baha.mesleh@nokia.com>
Signed-off-by: Subrat Pandey <subratp@marvell.com>
---
v4:
 - Correct the Fixes: tag commit subject to match the actual upstream
   commit title.
v3:
 - Extend the sc.max fix to the SC stats loop in mcs_clear_all_stats()
   (v2 fixed only mcs_free_all_rsrc()).
 - Add Fixes: tag and reword commit message.

 drivers/net/ethernet/marvell/octeontx2/af/mcs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
index 211c10aa5880..30c672d526a0 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
@@ -314,7 +314,7 @@ int mcs_clear_all_stats(struct mcs *mcs, u16 pcifunc, int dir)
 	}
 
 	/* Clear SC stats */
-	for (id = 0; id < map->secy.max; id++) {
+	for (id = 0; id < map->sc.max; id++) {
 		if (map->sc2pf_map[id] != pcifunc)
 			continue;
 		mcs_clear_stats(mcs, MCS_SC_STATS, id, dir);
@@ -738,7 +738,7 @@ int mcs_free_all_rsrc(struct mcs *mcs, int dir, u16 pcifunc)
 	}
 
 	/* free sc entries */
-	for (id = 0; id < map->secy.max; id++) {
+	for (id = 0; id < map->sc.max; id++) {
 		if (map->sc2pf_map[id] != pcifunc)
 			continue;
 		mcs_free_rsrc(&map->sc, map->sc2pf_map, id, pcifunc);
-- 
2.43.0

Re: [PATCH net v6 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
octeontx2-af: mcs: Fix SC resource cleanup loop

This changes the SC loops in mcs_clear_all_stats() and mcs_free_all_rsrc()
to use map->sc.max instead of map->secy.max as the loop bound.  Since
mcs_alloc_struct_mem() sets secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT
while sc.max = hw->sc_entries, the old bound was one short.

> octeontx2-af: mcs: Fix SC resource cleanup loop
>
>     The SC resource cleanup loop was incorrectly iterating over secy.max
>     instead of sc.max. Use sc.max as the loop bound.

[Severity: Medium]
The message describes a single "SC resource cleanup loop", but the diff
changes two loops in two different functions: the SC statistics loop in
mcs_clear_all_stats() and the SC free loop in mcs_free_all_rsrc().  Could
the message mention both call sites, since one of them is statistics
clearing rather than resource cleanup?

Could the message also state the observable consequence?  The numbers make
this look like an off-by-one that skipped exactly the highest SC id:

mcs_alloc_struct_mem():
	res->secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT;
	...
	res->sc.max = hw->sc_entries;

with MCS_RSRC_RSVD_CNT == 1, and both HW variants setting
secy_entries == sc_entries (128 for cn10kb, 64 for cnf10kb).  So SC id 127
(63 on cnf10kb) could be handed out by rvu_alloc_rsrc() but was never
freed on teardown or FLR, its sc2pf_map[] entry stayed owned by a dead
pcifunc, its RX SC CAM entry stayed enabled because
mcs_ena_dis_sc_cam_entry() was skipped, and its stats were never cleared
for the next owner.

That information seems useful for anyone deciding whether to backport this.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> index 211c10aa5880f..30c672d526a0c 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c

[ ... ]

> @@ -738,7 +738,7 @@ int mcs_free_all_rsrc(struct mcs *mcs, int dir, u16 pcifunc)
>  	}
>  
>  	/* free sc entries */
> -	for (id = 0; id < map->secy.max; id++) {
> +	for (id = 0; id < map->sc.max; id++) {
>  		if (map->sc2pf_map[id] != pcifunc)
>  			continue;
>  		mcs_free_rsrc(&map->sc, map->sc2pf_map, id, pcifunc);

[Severity: Low]
This isn't a bug introduced by this patch, but the new bound now passes
sc_id == 127 into mcs_ena_dis_sc_cam_entry() a few lines below, in the
dir == MCS_RX case.  That helper picks the second register but shifts by
the unmasked index:

mcs_ena_dis_sc_cam_entry() {
	reg = MCSX_CPM_RX_SLAVE_SC_CAM_ENA(0);
	if (sc_id > 63)
		reg = MCSX_CPM_RX_SLAVE_SC_CAM_ENA(1);

	if (ena)
		val = mcs_reg_read(mcs, reg) | BIT_ULL(sc_id);
	else
		val = mcs_reg_read(mcs, reg) & ~BIT_ULL(sc_id);
}

For sc_id in 64..127 (cn10kb sets hw->sc_entries = 128) the shift count is
at least 64, which is undefined in C, so this can produce a UBSAN "shift
exponent too large" report.  On arm64 the variable shift is masked to six
bits and 127 % 64 == 63, so the bit that gets touched happens to be the
intended one, and the enable side via mcs_rx_sc_cam_write() uses the same
idiom, so the two stay consistent.  Would BIT_ULL(sc_id % 64) be a
worthwhile follow-up cleanup here and in mcs_ena_dis_flowid_entry()?

[Severity: High]
This is a pre-existing issue, but this patch widens the region it applies
to from map->secy.max to map->sc.max entries, so it may be worth a look.
Is mcs_free_all_rsrc() safe to call without rvu->rsrc_lock?

The mailbox paths serialize the same bitmaps and ownership arrays:

rvu_mbox_handler_mcs_alloc_resources()
   mutex_lock(&rvu->rsrc_lock);
   ... mcs_alloc_rsrc() -> rvu_alloc_rsrc() -> __set_bit()

rvu_mbox_handler_mcs_free_resources()
   mutex_lock(&rvu->rsrc_lock);
   ... mcs_free_rsrc() / mcs_free_all_rsrc()

The FLR path does not:

__rvu_flr_handler()
   mutex_lock(&rvu->flr_lock);
   ...
   rvu_mcs_flr_handler()
      mcs_free_all_rsrc(mcs, MCS_RX, pcifunc);
      mcs_free_all_rsrc(mcs, MCS_TX, pcifunc);

rvu_alloc_rsrc() and rvu_free_rsrc() use the non-atomic __set_bit() /
__clear_bit(), so the read-modify-write of a bitmap word in the FLR work
item can race with another function's mailbox thread, and both sides also
write the same pf_map[] array.

Can a lost update here hand the same SC/SA/SECY/flowid id to two different
pcifuncs, or leak an id permanently?

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but
mcs_free_rsrc() is the callee whose use this loop widens, so mentioning it
here.  Is req->rsrc_id range checked anywhere before it reaches this
function?

rvu_mbox_handler_mcs_free_resources() validates only req->mcs_id and then
passes the caller-supplied u8 rsrc_id (0-255, struct mcs_free_rsrc_req)
straight through:

	switch (req->rsrc_type) {
	case MCS_RSRC_TYPE_FLOWID:
		rc = mcs_free_rsrc(&map->flow_ids, map->flowid2pf_map, req->rsrc_id, pcifunc);
		mcs_ena_dis_flowid_entry(mcs, req->rsrc_id, req->dir, false);

mcs_free_rsrc() then dereferences it with no bound against rsrc->max:

	/* Check if the rsrc_id is mapped to PF/VF */
	if (pf_map[rsrc_id] != pcifunc)
		return -EINVAL;

	rvu_free_rsrc(rsrc, rsrc_id);
	pf_map[rsrc_id] = 0;

The pf_map arrays are sized from hardware capability in
mcs_alloc_struct_mem(): flowid2pf_map, secy2pf_map and sc2pf_map are 128
u16 entries on cn10kb (64 on cnf10kb), sa2pf_map is 256 (128 on cnf10kb).
So a rsrc_id above the corresponding max reads up to 254 bytes past the
devm_kcalloc() allocation.

If that out-of-bounds slot happens to contain the caller's pcifunc, does
this then also write out of bounds?  rvu_free_rsrc() does
__clear_bit(rsrc_id, rsrc->bmap) on a bitmap allocated as
BITS_TO_LONGS(max) longs by rvu_alloc_bitmap(), which is 16 bytes for
max 127/128, and pf_map[rsrc_id] = 0 stores past the array.  Adjacent slab
allocations make this plausible: flowid2pf_map and secy2pf_map are both
256 bytes and allocated back-to-back, so flowid2pf_map[128 + i] can read
secy2pf_map[i], which legitimately holds the caller's pcifunc after an
"alloc all" request.

Separately, mcs_ena_dis_flowid_entry(), mcs_clear_secy_plcy() and
mcs_ena_dis_sc_cam_entry() are called with the same unvalidated index
regardless of the mcs_free_rsrc() return code, so the index also reaches
the MMIO offset arithmetic.  Should the handler reject rsrc_id >= max for
the requested type before the switch?

[Severity: Medium]
This is a pre-existing gap rather than something this patch introduces,
but since mcs_free_all_rsrc() is the "free everything owned by pcifunc"
helper being touched: should it also release ctrlpktrule resources?

It frees flow_ids, secy, sc and sa, but there is no map->ctrlpktrule /
ctrlpktrule2pf_map loop, and rvu_mcs_flr_handler() calls only
mcs_free_all_rsrc() for RX and TX.  The only release path is the req->all
branch of mcs_free_ctrlpktrule(), which needs an explicit mailbox request
from the function that just went through FLR.

So a rule allocated via rvu_mbox_handler_mcs_alloc_ctrl_pkt_rule() stays
marked allocated in map->ctrlpktrule and stays enabled in hardware, since
MCSX_PEX_{RX,TX}_SLAVE_RULE_ENABLE is only cleared by
mcs_free_ctrlpktrule().  Recovery appears to require a driver reload.

No in-tree consumer allocates these rules today (nothing under
octeontx2/nic/ references ctrl_pkt_rule, and cn10k_macsec.c only uses
FLOWID/SECY/SC/SA), so this needs an out-of-tree or other user of the AF
mailbox to trigger.

Re: [PATCH net v6 0/2] octeontx2: Misc fixes for RVU drivers

[Jakub Kicinski]

On Thu, 10 Sep 2026 11:56:55 +0530 Subrat Pandey wrote:
> Changes in v6:
>  - None. Rebased onto latest net tree.

Could you please add the links to the previous posting in the lore
archive going forward? Eg
https://lore.kernel.org/all/20260910062657.3712256-1-subratp@marvell.com/
for this posting? (just starting from this posting, you don't have to
dig out the full history)

It's useful for checking if AI comments have already been replied to,
if not the review itself.