RE: [EXTERNAL] Re: [PATCH net] Subject: [PATCH] octeontx2-af: fix TX scheduler queue index mask for 2K schedulers

[Kalluru Mahammad Rafi [C]]

> -----Original Message-----
> From: netdev-bot+sashiko@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx>
> Sent: Monday, October 26, 2026 10:13 AM
> To: Kalluru Mahammad Rafi [C] <kmahammadraf@xxxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Sunil Kovvuri
> Goutham <sgoutham@xxxxxxxxxxx>; Ratheesh Kannoth
> <rkannoth@xxxxxxxxxxx>; Geethasowjanya Akula <gakula@xxxxxxxxxxx>;
> Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxxx>;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Satha Koteswara Rao Kottidi
> <skoteshwar@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [PATCH net] Subject: [PATCH] octeontx2-af: fix TX
> scheduler queue index mask for 2K schedulers
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider. Critical: 0 · High: 1 (1) · Medium: 0 · Low: 2 If you believe none of
> the issues are legitimate, please reply to each issue of Medium severity or
> higher ZjQcmQRYFpfptBannerStart Prioritize security for external emails:
> Confirm sender and content safety before clicking links or opening
> attachments <https://us-phishalarm-
> ewt.proofpoint.com/EWT/v1/CRVmXkqW!tg3ZtR5041Tb1O6c-
> R0ZrnbrsGx07DSifl2qkGG8hIv05yiyJJ1R8wXjFw0zLdZvNLolaZ2EpsPY08eT4rpN
> GI_zKLMxZBDtrdlTGQ$>
> Report Suspicious
> 
> ZjQcmQRYFpfptBannerEnd
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 2
> 
> If you believe none of the issues are legitimate, please reply to each issue of
> Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] Incomplete fix: the patch widens only the child scheduler index,
>   which comes from the register address (TXSCHQ_IDX_MASK becomes 11
> bits…
> - [Low] The patch is a 'fix' for a user-visible bug on hardware the tree
>   already supports, but it has no Fixes: tag.
> - [Low] Submission/tag problems in the commit message: (1) The subject
>   literally reads 'Subject: [PATCH] octeontx2-af: fix TX scheduler queue…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but
>   rvu_mbox_handler_nix_txschq_cfg() validates the hierarchy on the raw…
> 
ACK all issues
pw-bot: cr
> --
> 
> Subject: [PATCH] octeontx2-af: fix TX scheduler queue index mask for 2K
> schedulers
> 
> CN20K raised the number of TX schedulers per layer from 1K to 2K. To match,
> this patch widens TXSCHQ_IDX_MASK from 10 to 11 bits and extends
> NIX_TX_SCHQ_MASK to cover bit 26.
> 
> > Subject: [PATCH] octeontx2-af: fix TX scheduler queue index mask for
> > 2K schedulers
> 
> [ ... ]
> 
> > Signed-off-by: kmahammadraf <kmahammadraf@xxxxxxxxxxx>
> > Signed-off-by: Satha Rao <skoteshwar@xxxxxxxxxxx>
> 
> [Severity: Low]
> This isn't a bug, but the mail header ended up in the commit subject:
> 
>   Subject: [PATCH] octeontx2-af: fix TX scheduler queue index mask ...
> 
> Since this is a fix, should it also be sent as [PATCH net]?
> 
> The first Signed-off-by uses kmahammadraf, which isn't a real name as
> submitting-patches.rst asks for. It also doesn't match the author.
> 
> If kmahammadraf wrote the patch, should the From: say so? If both people
> contributed, is a Co-developed-by: tag missing?
> 
> [Severity: Low]
> Should this have a Fixes: tag?
> 
> CN20K NIX support is already in the tree. nix_setup_txschq() sizes schq.max
> from the NIX_AF_*_CONST registers, so CN20K can already hand out
> scheduler queues >= 1024. With the old 10-bit TXSCHQ_IDX_MASK and a
> NIX_TX_SCHQ_MASK that leaves out bit 26, the AF would either reject a
> queue the function really owns or program schq - 1024.
> 
> Could this get a Fixes: tag pointing at the commit that enabled CN20K TX
> scheduler handling, and target the net tree, so the stable tooling picks it up?
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > index cece197d1074..1b62f9d7f00a 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> > @@ -1164,7 +1164,7 @@ struct nix_txschq_config {
> >  	u8 lvl;	/* SMQ/MDQ/TL4/TL3/TL2/TL1 */
> >  	u8 read;
> >  #define TXSCHQ_IDX_SHIFT	16
> > -#define TXSCHQ_IDX_MASK		(BIT_ULL(10) - 1)
> > +#define TXSCHQ_IDX_MASK		(BIT_ULL(11) - 1)
> >  #define TXSCHQ_IDX(reg, shift)	(((reg) >> (shift)) &
> TXSCHQ_IDX_MASK)
> 
> [Severity: High]
> The child index is now 11 bits, but does the parent index need the same
> change? is_txschq_hierarchy_valid() still cuts the parent read from the
> PARENT register value down to 9 bits:
> 
> drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:is_txschq_hierarchy_vali
> d() {
>     ...
> 	schq = TXSCHQ_IDX(reg, TXSCHQ_IDX_SHIFT);
>     ...
> 	parent = (regval >> 16) & 0x1FF;
> 	/* Validate MDQ's TL4 parent */
> 	if (regbase == NIX_AF_MDQX_PARENT(0) &&
> 	    !is_valid_txschq(rvu, blkaddr, NIX_TXSCH_LVL_TL4, pcifunc, parent))
> 		return false;
>     ...
> }
> 
> After that, rvu_mbox_handler_nix_txschq_cfg() writes the full, unmasked
> regval with rvu_write64().
> 
> nix_setup_txschq() takes schq.max from the NIX_AF_*_CONST registers, so on
> CN20K, TL4/TL3/TL2 indices of 512 and above can be allocated. On the PF/VF
> side, otx2_txschq_config() encodes the full parent as parent << 16.
> 
> Say a PF or VF uses TL4 600 as an MDQ parent. The AF checks who owns TL4
> 88 instead. Would that reject a valid config with NIX_AF_INVAL_TXSCHQ_CFG?
> Or would it pass only because TL4 88 happens to belong to the same
> function?
> 
> The reverse case also looks possible. A VF that owns parent X < 512 could
> send X + 512 * k. The AF checks X, but hardware gets a parent that may belong
> to another function. Does that break the cross-function isolation that
> is_valid_txschq() is meant to enforce?
> 
> The SMQ flush walk uses the same 9-bit decode:
> 
> drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:nix_smq_flush_fill_ctx()
> {
>     ...
> 		if (parent_off) {
> 			regval = rvu_read64(rvu, blkaddr, parent_off);
> 			schq = (regval >> 16) & 0x1FF;
> 		}
>     ...
> }
> 
> With parents >= 512, the saved and restored CIR/PIR offsets would point at
> other schedulers. nix_smq_flush_enadis_xoff() would also skip the wrong
> TL2, so the real TL2 of the SMQ being flushed could get XOFF'd. Could the
> flush time out, and could other functions' rate limits be zeroed and then
> restored?
> 
> The NIX_AF_MDQ/TL4/TL3/TL2_PARENT_MASK definitions in rvu_reg.h (9, 8,
> 8 and 5 bits wide) look too narrow for 2K schedulers per layer as well.
> 
> The tree doesn't document the CN20K PARENT field width. Still, with 2K TL4s
> per layer, the MDQ PARENT field has to hold TL4 indices up to 2047, so a 9-bit
> decode doesn't fit the premise of this patch.
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > index 95f782d47927..a1d68afd7c9e 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
> > @@ -2850,9 +2850,9 @@ static void nix_tl1_default_cfg(struct rvu *rvu,
> > struct nix_hw *nix_hw,  }
> >
> >  /* Register offset - [15:0]
> > - * Scheduler Queue number - [25:16]
> > + * Scheduler Queue number - [26:16]
> >   */
> > -#define NIX_TX_SCHQ_MASK	GENMASK_ULL(25, 0)
> > +#define NIX_TX_SCHQ_MASK	GENMASK_ULL(26, 0)
> 
> [Severity: High]
> This is a pre-existing issue, not one this patch introduced.
> rvu_mbox_handler_nix_txschq_cfg() runs the hierarchy check on the raw
> request regval, and only then merges in the caller-controlled
> regval_mask:
> 
> 		if (!is_txschq_hierarchy_valid(rvu, pcifunc, blkaddr,
> 					       txsch->lvl, reg, regval))
> 			return NIX_AF_INVAL_TXSCHQ_CFG;
> 		...
> 		val = rvu_read64(rvu, blkaddr, reg);
> 		regval = (val & val_mask) | (regval & ~val_mask);
> 		...
> 		rvu_write64(rvu, blkaddr, reg, regval);
> 
> For a PARENT register, can the parent that gets written differ from the one that
> was checked?
> 
> For example, say the child's current parent is 1 (bit 16 set) and the caller owns
> parents 1 and 2. The caller sends regval = 2 << 16 with regval_mask = 1 << 16.
> is_txschq_hierarchy_valid() approves parent 2.
> The merge then keeps bit 16 from hardware and bit 17 from the request, so
> parent 3 is written and its owner is never checked.
> 
> MDQ, TL4 and TL3 sit below nix_tx_aggr_lvl, so a VF can reach this path.
> Could a VF use it to attach its scheduler under another PF's or VF's scheduler?
> 
> --
> Sashiko AI review · https://urldefense.com/v3/__https://netdev-
> ai.bots.linux.dev/sashiko/*/patchset/20261022042653.2640537-1-
> kmahammadraf*40marvell.com__;IyU!!CRVmXkqW!SGzjnQ6pXZ5t5F7EW3j9l
> udBLULroSq7hGDKG4_8OraqjtNLE6C6FLzwGrvzDeIwm3oOUplZ7W6dl9UuqM
> wUdL7K7HIcUsM$