Re: [v9,net-next,5/8] octeontx2-af: switch: TL1 scheduling and NPC channel control

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 · Medium: 2 (1) · Low: 3

- [Critical] Missing scheduler-level validation on the new
  TXSCHQ_FREE_PAN_TL1 path in nix_txschq_free_one()…
- [High] Partial-allocation leak in rvu_mbox_handler_nix_txsch_alloc()
  (drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c):…
- [Medium] The commit message justifies the new fixed-size SQ bitmap with
  "the same span used by bitmap_weight(..., BITS_PER_LONG * 16) in…
- [Medium] PAN TL1 scheduler nodes are returned to the resource pool
  while still armed.
- [Low] Claim that rvu->rswitch.pcifunc has no writer, making
  rvu_is_switch_pcifunc() permanently false and all new PAN/set_chanmask…
- [Low] The new mailbox ABI comments in mbox.h describe the PAN scheduler
  queues as TL2 ("requests two aggregate-level TL2 queues on the PAN…
- [Low] The commit message states "without it, chan_mask was always
  forced to 0xFFF for non-AF functions" and describes set_chanmask as…

Pre-existing issues:
- [Medium] In rvu_nix_blk_aq_enq_inst() the SQ index bounds computation
  (the pfvf->sq_ctx NULL test, the sq_ctx->qsize read and the new…

--

octeontx2-af: switch: TL1 scheduling and NPC channel control

This adds a NIX_TXSCH_ALLOC_FLAG_PAN request flag so the switchdev PF can
get more than one aggregate-level scheduler queue on the PAN link, a
TXSCHQ_FREE_PAN_TL1 flag so those TL1 entries can be freed again, and a
set_chanmask field so the switchdev PF can keep its own NPC chan_mask.
It also changes the NIX LF SQ bitmap sizing and extends struct sg_list.

A few questions below.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index 8e3850f33751d..2aa1aa6599a5a 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -1162,6 +1162,13 @@ struct nix_txsch_alloc_req {
>  	/* Scheduler queue count request at each level */
>  	u16 schq_contig[NIX_TXSCH_LVL_CNT]; /* No of contiguous queues */
>  	u16 schq[NIX_TXSCH_LVL_CNT]; /* No of non-contiguous queues */
> +	/* Set only by the single switchdev PF (rvu->rswitch.pcifunc). This is
> +	 * not the eswitch representor (rvu->rep_pcifunc). That PF requests two
> +	 * aggregate-level TL2 queues on the PAN link, one for CGX and one for
> +	 * SDP steering. No other PF or VF sets this flag.
> +	 */
> +#define NIX_TXSCH_ALLOC_FLAG_PAN BIT(0)
> +	u32 flags;
>  };

[Severity: Low]
The comment says the flag requests "aggregate-level TL2 queues", but the
aggregation level is always TL1 here.

af/rvu.c:rvu_setup_hw_capabilities() has the only assignment:

	hw->cap.nix_tx_aggr_lvl = NIX_TXSCH_LVL_TL1;

and every implementation site in this patch indexes
nix_hw->txsch[NIX_TXSCH_LVL_TL1].  The companion macro is even named
TXSCHQ_FREE_PAN_TL1.

Since mbox.h is the only contract a request producer has when choosing
nix_txsch_free_req.schq_lvl, and nix_txschq_free_one() never cross-checks
schq_lvl against the flag, would a producer following the TL2 prose end up
operating on a different scheduler level than one following the TL1 macro
name?

Could the comments say TL1, and could the "PAN link" also be defined
somewhere, along with why nix_get_pan_tx_link() is cgx_links + lbk_links + 1?

[Severity: Low]
This isn't a bug, but nothing in the tree sets NIX_TXSCH_ALLOC_FLAG_PAN,
TXSCHQ_FREE_PAN_TL1 or npc_install_flow_req.set_chanmask, even at the end of
this series, so the new mailbox fields have consumers in the AF but no
in-tree emitters.  Are the producers coming in a follow-up series?

(For the record, rvu->rswitch.pcifunc itself does get a writer later in the
series in rvu_sw_l2_ctrl_work_handler(), so rvu_is_switch_pcifunc() is not
permanently false.)

> @@ -1180,6 +1187,10 @@ struct nix_txsch_alloc_rsp {
>  struct nix_txsch_free_req {
>  	struct mbox_msghdr hdr;
>  #define TXSCHQ_FREE_ALL BIT_ULL(0)
> +	/* Frees PAN TL2 queues allocated with NIX_TXSCH_ALLOC_FLAG_PAN. Used
> +	 * only by the switchdev PF (rvu->rswitch.pcifunc), not by other PFs/VFs.
> +	 */
> +#define TXSCHQ_FREE_PAN_TL1 BIT_ULL(1)
>  	u16 flags;
>  	/* Scheduler queue level to be freed */
>  	u16 schq_lvl;

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> index 1402beccf6618..e4d13adc2896c 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.c
> @@ -2006,9 +2007,18 @@ static void rvu_iface_get_qcnts(struct rvu *rvu, struct rvu_pfvf *pfvf,
>  
>  	spin_lock_irqsave(&aq->lock, flags);
>  
> -	/* Use each LF queue context size; bitmaps are sized to qsize longs. */
> -	if (pfvf->sq_ctx && pfvf->sq_bmap)
> -		info->sq_cnt = bitmap_weight(pfvf->sq_bmap, pfvf->sq_ctx->qsize);
> +	if (pfvf->sq_bmap) {
> +		/* Match switchdev sq_bmap allocation size in nix_lf_alloc(). */
> +		if (rvu_is_switch_pcifunc(rvu, pcifunc))
> +			sq_bmap_bits = NIX_SQ_BMAP_BITS;
> +		else if (pfvf->sq_ctx)
> +			sq_bmap_bits = pfvf->sq_ctx->qsize;
> +		else
> +			sq_bmap_bits = 0;
> +
> +		if (sq_bmap_bits)
> +			info->sq_cnt = bitmap_weight(pfvf->sq_bmap, sq_bmap_bits);
> +	}

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> index b8f4ad160afcf..9ee6531afbf94 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> @@ -1138,10 +1139,25 @@ static int rvu_nix_blk_aq_enq_inst(struct rvu *rvu, struct nix_hw *nix_hw,
>  		if (!pfvf->rq_ctx || req->qidx >= pfvf->rq_ctx->qsize)
>  			rc = NIX_AF_ERR_AQ_ENQUEUE;
>  		break;
> -	case NIX_AQ_CTYPE_SQ:
> -		if (!pfvf->sq_ctx || req->qidx >= pfvf->sq_ctx->qsize)
> +	case NIX_AQ_CTYPE_SQ: {
> +		if (!pfvf->sq_ctx) {
> +			rc = NIX_AF_ERR_AQ_ENQUEUE;
> +			break;
> +		}
> +
> +		/* Switchdev PF uses a fixed sq_bmap (NIX_SQ_BMAP_BITS); cap qidx
> +		 * to that span so __set_bit() cannot run past the allocation.
> +		 * nix_lf_alloc() also rejects sq_cnt above NIX_SQ_BMAP_BITS.
> +		 */
> +		sq_bmap_bits = rvu_is_switch_pcifunc(rvu, pcifunc) ?
> +			       NIX_SQ_BMAP_BITS :
> +			       (u64)pfvf->sq_ctx->qsize * BITS_PER_LONG;
> +		max_q = min_t(u64, pfvf->sq_ctx->qsize, sq_bmap_bits);
> +
> +		if ((u64)req->qidx >= max_q)
>  			rc = NIX_AF_ERR_AQ_ENQUEUE;
>  		break;
> +	}

[Severity: Medium]
This isn't a bug introduced by this patch, but the comment claims a property
the locking does not provide.

The bounds computation above (the pfvf->sq_ctx test, the qsize read and
max_q) runs with no lock held, while the mutation it is supposed to bound
runs later in the same function under aq->lock, with no re-validation and no
NULL check:

	spin_lock(&aq->lock);
	...
		if (req->ctype == NIX_AQ_CTYPE_SQ && req->sq.ena)
			__set_bit(req->qidx, pfvf->sq_bmap);

Those are exactly the fields nix_ctx_free() clears under the same lock and
frees immediately after releasing it:

	spin_lock_irqsave(&aq->lock, flags);
	...
	pfvf->sq_bmap = NULL;
	pfvf->sq_ctx = NULL;
	...
	spin_unlock_irqrestore(&aq->lock, flags);

	kfree(sq_bmap);
	...
	if (sq_ctx)
		qmem_free(rvu->dev, sq_ctx);

The unlocked-validate/locked-mutate split predates this patch (the baseline
read was "if (!pfvf->sq_ctx || req->qidx >= pfvf->sq_ctx->qsize)"), so this
is only extending it.  I could not prove that AF mailbox AQ processing for a
pcifunc can actually run concurrently with that pcifunc's teardown, so this
may be unreachable.  If it is unreachable, could the comment avoid asserting
that __set_bit() "cannot run past the allocation", since the check and the
write are not in the same critical section?

> @@ -1566,18 +1582,28 @@ int rvu_mbox_handler_nix_lf_alloc(struct rvu *rvu,
>  	if (!req->rq_cnt || !req->sq_cnt || !req->cq_cnt)
>  		return NIX_AF_ERR_PARAM;
>  
> +	/* Switchdev PF sq_bmap is fixed at NIX_SQ_BMAP_BITS; reject larger
> +	 * sq_cnt before allocating context memory or the bitmap.
> +	 */
> +	sq_bmap_bits = rvu_is_switch_pcifunc(rvu, pcifunc) ?
> +		NIX_SQ_BMAP_BITS :
> +		(u64)req->sq_cnt * BITS_PER_LONG;
> +
> +	if ((u64)req->sq_cnt > sq_bmap_bits)
> +		return NIX_AF_ERR_PARAM;
> +

[Severity: Medium]
The commit message says:

    Allocate the NIX LF SQ bitmap with the same span used by
    bitmap_weight(..., BITS_PER_LONG * 16) in rvu_get_hwinfo().

There is no function named rvu_get_hwinfo() in the driver (git grep at this
commit returns no matches), and there is no pre-existing bitmap_weight()
call with a BITS_PER_LONG * 16 span either - NIX_SQ_BMAP_BITS is added by
this patch.  The site actually changed is rvu_iface_get_qcnts(), the
IFACE_GET_INFO handler.  Could the message name the real function?

For the check above, the non-switch branch reduces to

	req->sq_cnt > (u64)req->sq_cnt * BITS_PER_LONG

which can never be true, so the "reject larger sq_cnt" validation is inert
for every non-switch caller.  Is that intentional, and could the new
NIX_AF_ERR_PARAM path and the new qidx cap in the SQ case of
rvu_nix_blk_aq_enq_inst() be mentioned in the changelog?

[ ... ]

> @@ -2209,6 +2240,25 @@ static void nix_get_txschq_range(struct rvu *rvu, u16 pcifunc,
>  	}
>  }
>  
> +static int nix_get_pan_tx_link(struct rvu *rvu)
> +{
> +	struct rvu_hwinfo *hw = rvu->hw;
> +
> +	return hw->cgx_links + hw->lbk_links + 1;
> +}
> +
> +static bool nix_txsch_is_pan_schq(struct rvu *rvu, int schq)
> +{
> +	int pan_link = nix_get_pan_tx_link(rvu);
> +
> +	return schq >= pan_link && schq <= pan_link + 1;
> +}
> +
> +static bool nix_txsch_pan_allowed(struct rvu *rvu, u16 pcifunc)
> +{
> +	return rvu_is_switch_pcifunc(rvu, pcifunc);
> +}
> +
>  static int nix_check_txschq_alloc_req(struct rvu *rvu, int lvl, u16 pcifunc,
>  				      struct nix_hw *nix_hw,
>  				      struct nix_txsch_alloc_req *req)
> @@ -2224,12 +2274,27 @@ static int nix_check_txschq_alloc_req(struct rvu *rvu, int lvl, u16 pcifunc,
>  	/* For traffic aggregating scheduler level, one queue is enough */
>  	if (lvl >= hw->cap.nix_tx_aggr_lvl) {
> -		if (req_schq != 1)
> +		if (req_schq != 1 && !(req->flags & NIX_TXSCH_ALLOC_FLAG_PAN))
>  			return NIX_AF_ERR_TLX_ALLOC_FAIL;
> +		if (req->schq[lvl] > MAX_TXSCHQ_PER_FUNC ||
> +		    req->schq_contig[lvl] > MAX_TXSCHQ_PER_FUNC)
> +			return NIX_AF_ERR_TLX_ALLOC_FAIL;
> +		if (req->flags & NIX_TXSCH_ALLOC_FLAG_PAN) {
> +			if (link >= txsch->schq.max || link + 1 >= txsch->schq.max)
> +				return NIX_AF_ERR_TLX_ALLOC_FAIL;
> +			if (req_schq > 2)
> +				return NIX_AF_ERR_TLX_ALLOC_FAIL;
> +		}
>  		return 0;
>  	}

[Severity: High]
This PAN block never test_bit()s whether the two PAN indices are actually
free, it only bounds them against txsch->schq.max and caps req_schq at 2.
So validation can pass while the two indices are already taken, and the
failure is then deferred into nix_txsch_alloc(), which is now fallible.

Can that leak the scheduler queues committed for the earlier levels?  See
the two hunks below.

> @@ -2270,6 +2335,46 @@ static void nix_txsch_alloc(struct rvu *rvu, struct nix_txsch *txsch,
>  	 * on transmit link to which PF_FUNC is mapped to.
>  	 */
>  	if (lvl >= hw->cap.nix_tx_aggr_lvl) {
> +		if (start != end) {
> +			int want_contig = rsp->schq_contig[lvl];
> +			int got_contig = 0, got = 0;
> +			int want = rsp->schq[lvl];
> +
> +			for (schq = start; schq <= end; schq++) {
> +				if (test_bit(schq, txsch->schq.bmap))
> +					continue;
> +
> +				if (got_contig < want_contig) {
> +					set_bit(schq, txsch->schq.bmap);
> +					rsp->schq_contig_list[lvl][got_contig++] = schq;
> +					continue;
> +				}
> +
> +				if (got < want) {
> +					set_bit(schq, txsch->schq.bmap);
> +					rsp->schq_list[lvl][got++] = schq;
> +				}
> +			}
> +
> +			rsp->schq_contig[lvl] = got_contig;
> +			rsp->schq[lvl] = got;
> +
> +			if (got_contig < want_contig || got < want) {
> +				for (idx = 0; idx < got_contig; idx++)
> +					clear_bit(rsp->schq_contig_list[lvl][idx],
> +						  txsch->schq.bmap);
> +				for (idx = 0; idx < got; idx++)
> +					clear_bit(rsp->schq_list[lvl][idx],
> +						  txsch->schq.bmap);
> +				rsp->schq_contig[lvl] = 0;
> +				rsp->schq[lvl] = 0;
> +				dev_err(rvu->dev,
> +					"Could not allocate schq at lvl=%u start=%u end=%u\n",
> +					lvl, start, end);
> +				return -ENOMEM;
> +			}
> +			return 0;
> +		}

[ ... ]

> @@ -2398,10 +2512,11 @@ int rvu_mbox_handler_nix_txsch_alloc(struct rvu *rvu,
>  			end = txsch->schq.max;
>  		}
>  
> -		nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end);
> +		if (nix_txsch_alloc(rvu, txsch, rsp, lvl, start, end))
> +			goto err;

[Severity: High]
Does this goto leak the scheduler queues allocated for the previous levels?

This goto is inside the per-level loop, but the error path only sets the
return code and unlocks:

	err:
		rc = NIX_AF_ERR_TLX_ALLOC_FAIL;
	exit:
		mutex_unlock(&rvu->rsrc_lock);
		return rc;

There is no clear_bit()/rvu_free_rsrc() for the bits already set in
txsch->schq.bmap, and no reset of the pfvf_map[schq] = TXSCH_MAP(pcifunc, 0)
stamps applied for SMQ/TL4/TL3/TL2 in earlier iterations.  The only failing
return is the new start != end branch at the aggregate level, which is the
last iteration, so the lower levels are already committed when it fires.

The requester gets an error and no schq lists back, so it cannot free those
indices individually - they stay used until TXSCHQ_FREE_ALL or LF teardown,
and each retry commits a fresh set.  Since the two PAN indices stay occupied
across retries, can a retry loop exhaust the shared SMQ/TL4/TL3/TL2 pool for
the other PFs/VFs on that NIX block?

Before this patch nix_txsch_alloc() was void and could not fail mid-loop, so
there was nothing to unwind.

> @@ -2679,6 +2794,20 @@ static int nix_txschq_free(struct rvu *rvu, u16 pcifunc)
>  	}
>  	nix_clear_tx_xoff(rvu, blkaddr, NIX_TXSCH_LVL_TL1,
>  			  nix_get_tx_link(rvu, pcifunc));
> +	/* TL1 is at nix_tx_aggr_lvl so the loop above skips it; also clear
> +	 * PAN TL1 XOFF on switch-owned links before flushing SMQs.
> +	 */
> +	if (nix_txsch_pan_allowed(rvu, pcifunc)) {
> +		txsch = &nix_hw->txsch[NIX_TXSCH_LVL_TL1];
> +
> +		for (schq = nix_get_pan_tx_link(rvu);
> +		     schq < txsch->schq.max &&
> +		     nix_txsch_is_pan_schq(rvu, schq); schq++) {
> +			if (TXSCH_MAP_FUNC(txsch->pfvf_map[schq]) != pcifunc)
> +				continue;
> +			nix_clear_tx_xoff(rvu, blkaddr, NIX_TXSCH_LVL_TL1, schq);
> +		}
> +	}
>  
>  	/* On PF cleanup, clear cfg done flag as
>  	 * PF would have changed default config.
> @@ -2706,11 +2835,11 @@ static int nix_txschq_free(struct rvu *rvu, u16 pcifunc)
>  		 /* TLs above aggregation level are shared across all PF
>  		  * and it's VFs, hence skip freeing them.
>  		  */
> -		if (lvl >= hw->cap.nix_tx_aggr_lvl)
> -			continue;
> -
>  		txsch = &nix_hw->txsch[lvl];
>  		for (schq = 0; schq < txsch->schq.max; schq++) {
> +			if (lvl >= hw->cap.nix_tx_aggr_lvl &&
> +			    !nix_txsch_is_pan_schq(rvu, schq))
> +				continue;
>  			if (TXSCH_MAP_FUNC(txsch->pfvf_map[schq]) != pcifunc)
>  				continue;
>  			nix_reset_tx_schedule(rvu, blkaddr, lvl, schq);
> 			rvu_free_rsrc(&txsch->schq, schq);
> 			txsch->pfvf_map[schq] = TXSCH_MAP(0, NIX_TXSCHQ_FREE);

[Severity: Medium]
Are the PAN TL1 nodes returned to the free pool while still armed?

The block above writes 0 to NIX_AF_TL1X_SW_XOFF (leaving the node
scheduling), and the loop here then calls nix_reset_tx_schedule() followed
by rvu_free_rsrc().  But nix_reset_tx_schedule() has no TL1 case:

	switch (lvl) {
	case NIX_TXSCH_LVL_TL2:
	...
	default:
		return;
	}

TL1X_CIR is not reset in this path either, because the first loop in
nix_txschq_free() still does

		if (lvl >= hw->cap.nix_tx_aggr_lvl)
			continue;

so nix_reset_tx_shaping() never runs for TL1.  The TL1 therefore goes back
to the pool enabled and still anchored through TL1X_TOPOLOGY.PRIO_ANCHOR to
TL2 children that the same loop frees and that can be handed to another PF.

The reverse direction looks asymmetric too: nothing in the allocation path
normalizes TL1X_SW_XOFF (nix_reset_tx_linkcfg() returns early for the
aggregation level, nix_reset_tx_schedule() is a no-op for TL1, and
nix_reset_tx_shaping() only touches TL1X_CIR).  If a PAN TL1 is freed while
XOFF is asserted - for instance rvu->rswitch.pcifunc has already been
cleared, so the new XOFF-clearing block is skipped while this loop still
releases the index - does the next owner get a TL1 that never transmits?

nix_txschq_free_one() on the TXSCHQ_FREE_PAN_TL1 path does call
nix_reset_tx_shaping() but likewise leaves TOPOLOGY/SCHEDULE programmed, so
the two free paths leave different hardware state behind.  Before this patch
TL1 was never freed, so leaving it armed did not matter.

> @@ -2754,7 +2883,16 @@ static int nix_txschq_free_one(struct rvu *rvu,
>  	schq = req->schq;
>  	txsch = &nix_hw->txsch[lvl];
>  
> -	if (lvl >= hw->cap.nix_tx_aggr_lvl || schq >= txsch->schq.max)
> +	if (req->flags & TXSCHQ_FREE_PAN_TL1) {
> +		if (!nix_txsch_pan_allowed(rvu, pcifunc))
> +			return NIX_AF_ERR_TLX_INVALID;
> +		if (!nix_txsch_is_pan_schq(rvu, schq))
> +			return NIX_AF_ERR_TLX_INVALID;
> +	} else if (lvl >= hw->cap.nix_tx_aggr_lvl) {
> +		return 0;
> +	}
> +
> +	if (schq >= txsch->schq.max)
>  		return 0;

[Severity: Critical]
Can this read past nix_hw->txsch[]?

lvl comes straight from the mailbox and is never bounded:

	lvl = req->schq_lvl;
	schq = req->schq;
	txsch = &nix_hw->txsch[lvl];

rvu_mbox_handler_nix_txsch_free() forwards req unvalidated:

	if (req->flags & TXSCHQ_FREE_ALL)
		return nix_txschq_free(rvu, req->hdr.pcifunc);
	else
		return nix_txschq_free_one(rvu, req);

and nix_hw->txsch[] has only NIX_TXSCH_LVL_CNT (5) entries.  Before this
patch the combined test short-circuited for every lvl >= 4 and returned 0
before txsch was dereferenced.  The new PAN branch skips that early return
and checks only the requester (nix_txsch_pan_allowed()) and the numeric
index (nix_txsch_is_pan_schq(), which is level-agnostic), so with
schq_lvl = 65535 execution reaches

	if (schq >= txsch->schq.max)

reading roughly 65535 * sizeof(struct nix_txsch) past nix_hw.  If that
garbage max happens to exceed schq, the code continues into
TXSCH_MAP_FUNC(pfvf_map[schq]) through a garbage pointer, then
rvu_free_rsrc(&txsch->schq, schq) and a pfvf_map[schq] write, all under
rvu->rsrc_lock.

Is the requester check enough to make this unreachable?  It looks like any
PF/VF can become rvu->rswitch.pcifunc, since rvu_sw_swdev2af_sender_check()
in af/switch/rvu_sw.c returns early without checking the sender:

	if (msg_type == SWDEV2AF_MSG_TYPE_FW_STATUS && req->fw_up)
		return 0;

after which rvu_sw_l2_ctrl_work_handler() stores that pcifunc into
rvu->rswitch.pcifunc.

Also, even for in-range levels there is no lvl == NIX_TXSCH_LVL_TL1 check,
so TXSCHQ_FREE_PAN_TL1 with schq_lvl = NIX_TXSCH_LVL_SMQ would tear down an
SMQ whose index happens to fall in the PAN range.  Could this path reject
lvl >= NIX_TXSCH_LVL_CNT before indexing, and require
lvl == NIX_TXSCH_LVL_TL1 when the PAN flag is set, the way
rvu_mbox_handler_nix_txschq_cfg() already validates
req->lvl >= NIX_TXSCH_LVL_CNT?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> index e36c68ee5d847..40d49a3238146 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc_fs.c
> @@ -1833,9 +1833,23 @@ int rvu_mbox_handler_npc_install_flow(struct rvu *rvu,
> -	/* ignore chan_mask in case pf func is not AF, revisit later */
> -	if (!is_pffunc_af(req->hdr.pcifunc))
> -		req->chan_mask = rvu_get_cpt_chan_mask(rvu);
> +	/* Non-AF callers get the CPT default chan_mask unless the authorized
> +	 * switchdev PF sets set_chanmask to preserve a caller-supplied mask.
> +	 * VFs and other PFs must not use set_chanmask; that would bypass
> +	 * channel isolation.
> +	 */
> +	if (!is_pffunc_af(req->hdr.pcifunc)) {
> +		if (req->set_chanmask &&
> +		    !rvu_is_switch_pcifunc(rvu, req->hdr.pcifunc)) {
> +			rvu_npc_free_entry_for_flow_install(rvu,
> +							    req->hdr.pcifunc,
> +							    allocated,
> +							    req->entry);
> +			return NPC_FLOW_VF_PERM_DENIED;
> +		}
> +		if (!req->set_chanmask)
> +			req->chan_mask = rvu_get_cpt_chan_mask(rvu);
> +	}

[Severity: Low]
The commit message says:

    For NPC install flow, add set_chanmask so callers can keep a non-default
    chan_mask when the requester is not the AF; without it, chan_mask was
    always forced to 0xFFF for non-AF functions.

Both halves look inaccurate for this code.  The replaced line called
rvu_get_cpt_chan_mask(), which is platform dependent:

	if (!is_rvu_otx2(rvu))
		return NIX_CHAN_CPT_X2P_MASK;
	else
		return 0xFFFu;

so the previous default was 0xFFF only on OTX2.  And the new behaviour is
not available to non-AF callers in general - the hunk above returns
NPC_FLOW_VF_PERM_DENIED unless the requester is exactly
rvu->rswitch.pcifunc.

Could the changelog say that set_chanmask is honored only for the switchdev
PF, and drop the claim about 0xFFF?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831131944.2649362-1-rkannoth%40marvell.com