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

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

[kmahammadraf]

From: Satha Rao <skoteshwar@marvell.com>

In CN20K, the maximum number of TX schedulers per layer increased from
1K to 2K, so the scheduler queue index field is now 11 bits wide.

Update the TX scheduler masks used for mailbox/config register access:

Increase TXSCHQ_IDX_MASK from 10-bit to 11-bit
Expand NIX_TX_SCHQ_MASK to include bit 26

Signed-off-by: kmahammadraf <kmahammadraf@marvell.com>
Signed-off-by: Satha Rao <skoteshwar@marvell.com>
---
---
 drivers/net/ethernet/marvell/octeontx2/af/mbox.h    | 2 +-
 drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

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)
 	u8 num_regs;
 #define MAX_REGS_PER_MBOX_MSG	20
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)
 
 static int nix_txschq_cfg_read(struct rvu *rvu, struct nix_hw *nix_hw,
 			       int blkaddr, struct nix_txschq_config *req,
-- 
2.25.1

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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…

--

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@marvell.com>
> Signed-off-by: Satha Rao <skoteshwar@marvell.com>

[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_valid() {
    ...
	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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022042653.2640537-1-kmahammadraf%40marvell.com