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

[PATCH net v2 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 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     |  2 +-
 drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c  |  8 +++++---
 .../ethernet/marvell/octeontx2/nic/otx2_common.c    | 13 +++++++------
 3 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.43.0

[PATCH net v2 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>
---
 drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c  |  8 +++++---
 .../ethernet/marvell/octeontx2/nic/otx2_common.c    | 13 +++++++------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
index dbf173196608..f3903cce9bf7 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
@@ -245,10 +245,12 @@ int cn20k_register_pfvf_mbox_intr(struct otx2_nic *pf, int numvfs)
 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;
+	if (pfvf->queue_to_pfc_map)
+		return pfvf->queue_to_pfc_map[aura_id];
+	else
+		return 0;
 #endif
+	return 0;
 }
 
 static int cn20k_tc_get_entry_index(struct otx2_flow_config *flow_cfg,
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
index 3d253132a17f..b4691472d2a3 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
@@ -1132,10 +1132,10 @@ 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];
+#ifdef CONFIG_DCB
+			if (pfvf->queue_to_pfc_map)
+				aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
 #endif
 
 			/* Set backpressure level is same as cq pass level */
@@ -1433,10 +1433,11 @@ 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];
+#ifdef CONFIG_DCB
+		if (pfvf->queue_to_pfc_map)
+			aq->aura.nix0_bpid = pfvf->bpid[pfvf->queue_to_pfc_map[aura_id]];
 #endif
 
 		/* Set backpressure level for RQ's Aura */
-- 
2.43.0

[PATCH net v2 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. This patch fixes to use correct value.

Signed-off-by: Baha Mesleh <baha.mesleh@nokia.com>
Signed-off-by: Subrat Pandey <subratp@marvell.com>
---
 drivers/net/ethernet/marvell/octeontx2/af/mcs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
index a07e0b3d8d00..88658c07c391 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);
-- 
2.43.0

Re: [PATCH net v2 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 adds a NULL check on pfvf->queue_to_pfc_map before it is dereferenced
in the aura and CQ context init paths, since SDP VFs never allocate the map.
The BPID is now set to bpid[0] first and only overridden when CONFIG_DCB is
enabled and the map exists.

No Link: tags in the commit header.

> Fixes: 184fb40f731b ("octeontx2-pf: Avoid adding dcbnl_ops for LBK and SDP vf").

[Severity: Low]
This isn't a bug, but there is a trailing period after the closing
parenthesis of the Fixes tag.  The expected form is:

Fixes: 184fb40f731b ("octeontx2-pf: Avoid adding dcbnl_ops for LBK and SDP vf")

The stray '.' can confuse the Fixes tag parsers used for stable backport
selection.  Since this is a regression fix, would a [PATCH net] annotation
on the subject also be appropriate?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> index dbf1731966082..f3903cce9bf78 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn20k.c
> @@ -245,10 +245,12 @@ int cn20k_register_pfvf_mbox_intr(struct otx2_nic *pf, int numvfs)
>  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;
> +	if (pfvf->queue_to_pfc_map)
> +		return pfvf->queue_to_pfc_map[aura_id];
> +	else
> +		return 0;
>  #endif
> +	return 0;
>  }

[Severity: Low]
Is the else branch here needed?  With CONFIG_DCB=y both branches return, so
the trailing return 0 after #endif is unreachable, and cn20k_aura_bpid_idx()
does not follow the "default first, then conditionally override" shape the
commit message describes for the other two hunks.  Would this be equivalent
and match the rest of the patch?

	static u8 cn20k_aura_bpid_idx(struct otx2_nic *pfvf, int aura_id)
	{
	#ifdef CONFIG_DCB
		if (pfvf->queue_to_pfc_map)
			return pfvf->queue_to_pfc_map[aura_id];
	#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 3d253132a17f0..b4691472d2a3e 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> @@ -1132,10 +1132,10 @@ 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];
> +#ifdef CONFIG_DCB
> +			if (pfvf->queue_to_pfc_map)
> +				aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
>  #endif

[Severity: Low]
This isn't a bug, but the same #ifdef CONFIG_DCB plus NULL test idiom now
appears in three places: cn20k_aura_bpid_idx(), otx2_cq_init() and
otx2_aura_aq_init().  cn20k_aura_bpid_idx() already encapsulates exactly this
logic.  Would it be cleaner to generalize it into a single accessor, say
otx2_get_bpid_idx(pfvf, qidx), and use it from all three sites?

Alternatively, could queue_to_pfc_map simply be allocated unconditionally
(it is only pfvf->hw.rx_queues bytes) instead of being tied to dcbnl_ops
registration in otx2_dcbnl_set_ops()?  That coupling is what broke in
184fb40f731b.

Note that otx2_update_bpid_in_rqctx() in otx2_dcbnl.c still dereferences the
map with no NULL check:

	if (pfvf->queue_to_pfc_map[qidx] && pfc_enable) {
	...
	pfvf->queue_to_pfc_map[qidx] = vlan_prio;

That stays safe only because SDP/LBK VFs never register dcbnl_ops, so
pfvf->pfc_en remains 0 and the PFC ntuple path is never taken for them.  Is
that invariant worth documenting, or would the shared accessor make it moot?

> @@ -1433,10 +1433,11 @@ 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];
> +#ifdef CONFIG_DCB
> +		if (pfvf->queue_to_pfc_map)
> +			aq->aura.nix0_bpid = pfvf->bpid[pfvf->queue_to_pfc_map[aura_id]];
>  #endif

[Severity: High]
This isn't a bug introduced by this patch, but since the patch is hardening
accesses to queue_to_pfc_map, is it worth fixing the bounds problem on the
same array at the same time?

The map is allocated with only pfvf->hw.rx_queues bytes in
otx2_dcbnl_set_ops():

	pfvf->queue_to_pfc_map = devm_kzalloc(pfvf->dev, pfvf->hw.rx_queues,
					      GFP_KERNEL);

and otx2_update_bpid_in_rqctx() in otx2_dcbnl.c indexes it with a queue
index that comes from the ethtool ntuple ring_cookie:

	if (pfvf->queue_to_pfc_map[qidx] && pfc_enable) {
	...
	pfvf->queue_to_pfc_map[qidx] = vlan_prio;

Can qidx exceed hw.rx_queues here?  The range check in otx2_add_flow() is
gated on the interface not being a VF:

	if (!is_otx2_vf(pfvf->pcifunc) && !vf_num &&
	    ring >= pfvf->hw.rx_queues && fsp->ring_cookie != RX_CLS_FLOW_DISC)
		return -EINVAL;

so for a VF netdev the ring index is not validated, and otx2_add_flow_msg()
propagates it:

	req->index = ethtool_get_flow_spec_ring(ring_cookie);
	...
			if (pfvf->pfc_en & BIT(vlan_prio)) {
				pfc_rule = true;
				qidx = req->index;
			}
	...
	if (!err && pfc_rule)
		otx2_update_bpid_in_rqctx(pfvf, vlan_prio, qidx, true);

The AF handler in rvu_npc_fs.c assigns action.index = req->index without an
RQ count check, so the mbox call succeeds and the helper runs.  That looks
like an out-of-bounds read at a caller-chosen offset plus a one byte
out-of-bounds write of the VLAN priority.  The delete path in
otx2_remove_flow() passes the raw 64-bit ring_cookie into the same int qidx:

	otx2_update_bpid_in_rqctx(pfvf, 0,
				  flow->flow_spec.ring_cookie,
				  false);

Would adding the ring >= pfvf->hw.rx_queues check for VF netdevs, or bounds
checking qidx inside otx2_update_bpid_in_rqctx(), be the right fix?  This
requires CAP_NET_ADMIN on the VF netdev, which matters when the VF is handed
to a container or guest owner.
-- 
pw-bot: cr

Re: [PATCH net v2 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

The SC cleanup loop in mcs_free_all_rsrc() was bounded by map->secy.max
while indexing the SC arrays, and this changes the bound to map->sc.max.
Since mcs_alloc_rsrc_bmap() sets secy.max = secy_entries - MCS_RSRC_RSVD_CNT
and sc.max = sc_entries, the two bounds differ by one.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> index a07e0b3d8d000..88658c07c391f 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++) {

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but the same
wrong bound still exists in mcs_clear_all_stats() in the same file, where the
"Clear SC stats" loop indexes map->sc2pf_map[] and clears MCS_SC_STATS:

	/* Clear SC stats */
	for (id = 0; id < map->secy.max; id++) {
		if (map->sc2pf_map[id] != pcifunc)
			continue;
		mcs_clear_stats(mcs, MCS_SC_STATS, id, dir);
	}

With mcs_alloc_rsrc_bmap() setting:

	res->secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT;
	...
	res->sc.max = hw->sc_entries;

and MCS_RSRC_RSVD_CNT == 1 with secy_entries == sc_entries (128/128 on
CN10K-B, 64/64 on CNF10K-B), secy.max == sc.max - 1, so SC id
sc_entries-1 is never visited by the stats loop.

Should this hunk also change the bound in mcs_clear_all_stats() to
map->sc.max?

Before this patch, SC id sc_entries-1 was never freed by
mcs_free_all_rsrc(), so no other function could take it over. Now that it
is freed and re-allocatable, can the next owner of that id read the
previous owner's pkt/octet counters?

The teardown path in cn10k_mcs_free_rsrc() always sends the clear-stats
request with the same "all" flag just before the free-resources request:

	clear_req = otx2_mbox_alloc_msg_mcs_clear_stats(mbox);
	...
	clear_req->all = all;
	...
	if (all)
		req->all = 1;

so with the stats loop still bounded by secy.max, MCS_GET_SC_STATS on the
re-allocated id would return the departing owner's values.

[Severity: Low]
Would a Fixes: tag be appropriate here? git blame on the pre-patch line
shows the secy.max bound came from:

	Fixes: cfc14181d497 ("octeontx2-af: cn10k: mcs: Manage MCS block hardware resources")

which is in released kernels since v6.1, so stable tooling can pick it up.

Could the changelog also state the user-visible impact? With the old bound,
the sc bitmap bit and map->sc2pf_map[] ownership for id sc_entries-1 leak
permanently (rvu_mcs_flr_handler() is the only other teardown path and
calls nothing else), making that SC id unallocatable, and on RX the SC CAM
entry stays enabled because mcs_ena_dis_sc_cam_entry(mcs, id, false) is
skipped.

One wording note on the changelog:

> The SC resource cleanup loop was incorrectly iterating over secy.max
> instead of sc.max. This patch fixes to use correct value.

Documentation/process/submitting-patches.rst asks for imperative mood and
discourages "This patch"; something like "Use sc.max as the loop bound"
would match.

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

[Jakub Kicinski]

On Thu,  6 Aug 2026 08:26:50 -0700 Jakub Kicinski wrote:
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > index 3d253132a17f0..b4691472d2a3e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > @@ -1132,10 +1132,10 @@ 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];
> > +#ifdef CONFIG_DCB
> > +			if (pfvf->queue_to_pfc_map)
> > +				aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
> >  #endif  
> 
> [Severity: Low]
> This isn't a bug, but the same #ifdef CONFIG_DCB plus NULL test idiom now
> appears in three places: cn20k_aura_bpid_idx(), otx2_cq_init() and
> otx2_aura_aq_init().  cn20k_aura_bpid_idx() already encapsulates exactly this
> logic.  Would it be cleaner to generalize it into a single accessor, say
> otx2_get_bpid_idx(pfvf, qidx), and use it from all three sites?
> 
> Alternatively, could queue_to_pfc_map simply be allocated unconditionally
> (it is only pfvf->hw.rx_queues bytes) instead of being tied to dcbnl_ops

AI is getting better I guess. I was going to suggest this as well.
Please avoid hiding struct members under ifdefs unless there's a major
size win on the structures. If you care about runtime cost you can use
IS_ENABLED() inside the if () condition and compiler will eliminate it.
No ifdefs needed at that point.

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

[Subrat Pandey]

________________________________________
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, August 6, 2026 9:00 PM
To: Subrat Pandey
Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org; pabeni@redhat.com; edumazet@google.com; davem@davemloft.net; andrew+netdev@lunn.ch; Subbaraya Sundeep Bhatta; Ratheesh Kannoth; Geethasowjanya Akula; Sunil Kovvuri Goutham
Subject: [EXTERNAL] Re: [PATCH net v2 1/2] octeontx2-pf: Fix aura BPID assignment when CONFIG_DCB is enabled

On Thu, 6 Aug 2026 08: 26: 50 -0700 Jakub Kicinski wrote: > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common. c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common. c > > index 3d253132a17f0. . b4691472d2a3e 100644

> On Thu,  6 Aug 2026 08:26:50 -0700 Jakub Kicinski wrote:
> > > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > > index 3d253132a17f0..b4691472d2a3e 100644
> > > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_common.c
> > > @@ -1132,10 +1132,10 @@ 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];
> > > +#ifdef CONFIG_DCB
> > > +                   if (pfvf->queue_to_pfc_map)
> > > +                           aq->cq.bpid = pfvf->bpid[pfvf->queue_to_pfc_map[qidx]];
> > >  #endif
> >
> > [Severity: Low]
> > This isn't a bug, but the same #ifdef CONFIG_DCB plus NULL test idiom now
> > appears in three places: cn20k_aura_bpid_idx(), otx2_cq_init() and
> > otx2_aura_aq_init().  cn20k_aura_bpid_idx() already encapsulates exactly this
> > logic.  Would it be cleaner to generalize it into a single accessor, say
> > otx2_get_bpid_idx(pfvf, qidx), and use it from all three sites?
> >
> > Alternatively, could queue_to_pfc_map simply be allocated unconditionally
> > (it is only pfvf->hw.rx_queues bytes) instead of being tied to dcbnl_ops
>
> AI is getting better I guess. I was going to suggest this as well.
> Please avoid hiding struct members under ifdefs unless there's a major
> size win on the structures. If you care about runtime cost you can use
> IS_ENABLED() inside the if () condition and compiler will eliminate it.
> No ifdefs needed at that point.
>

Thanks for the feedback.
We will fix it and submit next version.

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

[Subrat Pandey]

________________________________________
From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, August 6, 2026 8:57 PM
To: Subrat Pandey
Cc: Jakub Kicinski; linux-kernel@vger.kernel.org; netdev@vger.kernel.org; pabeni@redhat.com; edumazet@google.com; davem@davemloft.net; andrew+netdev@lunn.ch; Subbaraya Sundeep Bhatta; Ratheesh Kannoth; Geethasowjanya Akula; Sunil Kovvuri Goutham
Subject: [EXTERNAL] Re: [PATCH net v2 2/2] octeontx2-af: mcs: Fix SC resource cleanup loop

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 The SC cleanup loop in mcs_free_all_rsrc() was bounded

> 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
>
> The SC cleanup loop in mcs_free_all_rsrc() was bounded by map->secy.max
> while indexing the SC arrays, and this changes the bound to map->sc.max.
> Since mcs_alloc_rsrc_bmap() sets secy.max = secy_entries - MCS_RSRC_RSVD_CNT
> and sc.max = sc_entries, the two bounds differ by one.
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mcs.c b/drivers/net/ethernet/marvell/octeontx2/af/mcs.c
> > index a07e0b3d8d000..88658c07c391f 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++) {
>
> [Severity: Medium]
> This is a pre-existing issue and not introduced by this patch, but the same
> wrong bound still exists in mcs_clear_all_stats() in the same file, where the
> "Clear SC stats" loop indexes map->sc2pf_map[] and clears MCS_SC_STATS:
>
>        /* Clear SC stats */
>        for (id = 0; id < map->secy.max; id++) {
>                if (map->sc2pf_map[id] != pcifunc)
>                        continue;
>                mcs_clear_stats(mcs, MCS_SC_STATS, id, dir);
>        }
>
> With mcs_alloc_rsrc_bmap() setting:
>
>        res->secy.max = hw->secy_entries - MCS_RSRC_RSVD_CNT;
>        ...
>        res->sc.max = hw->sc_entries;
>
> and MCS_RSRC_RSVD_CNT == 1 with secy_entries == sc_entries (128/128 on
> CN10K-B, 64/64 on CNF10K-B), secy.max == sc.max - 1, so SC id
> sc_entries-1 is never visited by the stats loop.
>
> Should this hunk also change the bound in mcs_clear_all_stats() to
> map->sc.max?
>
> Before this patch, SC id sc_entries-1 was never freed by
> mcs_free_all_rsrc(), so no other function could take it over. Now that it
> is freed and re-allocatable, can the next owner of that id read the
> previous owner's pkt/octet counters?
>
> The teardown path in cn10k_mcs_free_rsrc() always sends the clear-stats
> request with the same "all" flag just before the free-resources request:
>
>         clear_req = otx2_mbox_alloc_msg_mcs_clear_stats(mbox);
>         ...
>         clear_req->all = all;
>         ...
>         if (all)
>                 req->all = 1;
>
> so with the stats loop still bounded by secy.max, MCS_GET_SC_STATS on the
> re-allocated id would return the departing owner's values.
> 
> [Severity: Low]
> Would a Fixes: tag be appropriate here? git blame on the pre-patch line
> shows the secy.max bound came from:
> 
>         Fixes: cfc14181d497 ("octeontx2-af: cn10k: mcs: Manage MCS block hardware resources")
>
> which is in released kernels since v6.1, so stable tooling can pick it up.
>
> Could the changelog also state the user-visible impact? With the old bound,
> the sc bitmap bit and map->sc2pf_map[] ownership for id sc_entries-1 leak
> permanently (rvu_mcs_flr_handler() is the only other teardown path and
> calls nothing else), making that SC id unallocatable, and on RX the SC CAM
> entry stays enabled because mcs_ena_dis_sc_cam_entry(mcs, id, false) is
> skipped.
>
> One wording note on the changelog:
>
> > The SC resource cleanup loop was incorrectly iterating over secy.max
> > instead of sc.max. This patch fixes to use correct value.
>
> Documentation/process/submitting-patches.rst asks for imperative mood and
> discourages "This patch"; something like "Use sc.max as the loop bound"
> would match.
>

Thanks for the feedback.
We will fix it and submit next version.