Re: [PATCH v4 net] octeontx2-pf: reset HTB scheduler topology before freeing queues

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] Incomplete fix: the stale-TOPOLOGY bug class the commit claims
  to close stays reachable through the AF's bulk release path.
- [Medium] Set/reset asymmetry at the aggregation level: HTB offload
  programs NIX_AF_TL1X_TOPOLOGY (PRIO_ANCHOR/RR_PRIO) on the shared per-…

--

octeontx2-pf: reset HTB scheduler topology before freeing queues

This adds otx2_qos_reset_schq_topology() and otx2_qos_free_hw_schq() so
that NIX_AF_TL{4,3,2}X_TOPOLOGY is zeroed before each QoS scheduler queue
is handed back to the AF during HTB hierarchy teardown and cfg rollback.
The aggregation level (TL1) is intentionally skipped.

[Severity: Medium]
Should the TL1 case be handled here too, or at least commented in the
code?  otx2_qos_txschq_set_parent_topology() does program TL1:

	else if (parent->level == NIX_TXSCH_LVL_TL1)
		cfg->reg[0] = NIX_AF_TL1X_TOPOLOGY(parent->schq);

	cfg->regval[0] = (u64)parent->prio_anchor << 32;
	cfg->regval[0] |= ((parent->child_dwrr_prio != OTX2_QOS_DEFAULT_PRIO) ?
			    parent->child_dwrr_prio : 0)  << 1;

and for a PF the HTB root sits at that level, in otx2_qos_alloc_root():

	if (!is_otx2_vf(pfvf->pcifunc)) {
		node->level = NIX_TXSCH_LVL_TL1;

so otx2_qos_txschq_push_cfg_tl() -> otx2_qos_txschq_set_parent_topology()
writes NIX_AF_TL1X_TOPOLOGY with the HTB prio_anchor, which is the base
index of the contiguous TL2 child block.

On tc qdisc del, otx2_qos_root_destroy() -> otx2_qos_destroy_node() ->
otx2_qos_free_hw_cfg() calls otx2_qos_free_hw_schq(pfvf, node->level,
node->schq) with node->level == NIX_TXSCH_LVL_TL1, which takes the early
return above.

Does anything else restore the default TL1 topology while the interface
stays up?  nix_txschq_free_one() early-returns for lvl >=
hw->cap.nix_tx_aggr_lvl, nix_reset_tx_schedule() has no TL1 case, and
nix_tl1_default_cfg() declines to reprogram once the PF has configured
TL1:

	/* Skip if PF has already done the config */
	if (TXSCH_MAP_FLAGS(pfvf_map[schq]) & NIX_TXSCHQ_CFG_DONE)
		return;
	rvu_write64(rvu, blkaddr, NIX_AF_TL1X_TOPOLOGY(schq),
		    (TXSCH_TL1_DFLT_RR_PRIO << 1));

The only writer of the default TL1 value appears to be
otx2_txschq_config():

	req->reg[1] = NIX_AF_TL1X_TOPOLOGY(schq);
	req->regval[1] = hw->txschq_aggr_lvl_rr_prio << 1;

which runs at interface open rather than on TC_HTB_DESTROY.  So after HTB
teardown with the link up, the shared TL1 keeps a PRIO_ANCHOR pointing at
TL2 indices that were just returned to the AF's block-wide TL2 pool and
may be re-allocated to another function.

The commit message says TL1 "is not freed back to the AF by
nix_txschq_free_one()", which rules out re-allocating TL1 itself, but does
it cover the residual anchor pointing into recycled TL2 indices?  Whether
the TL1 arbiter can act on such a stale anchor depends on hardware
semantics that are not visible from the tree, so this may be harmless in
practice, but the set/reset asymmetry looks worth spelling out in the code
rather than only in the changelog.

[Severity: High]
Does this cover the case where the AF, not the driver, releases the QoS
queues?  The reset only runs on the driver's explicit per-schq frees, but
nix_txschq_free() in af/rvu_nix.c returns the same TL4/TL3/TL2 indices to
the shared pool with only PARENT and SCHEDULE cleared:

	nix_reset_tx_schedule(rvu, blkaddr, lvl, schq);
	rvu_free_rsrc(&txsch->schq, schq);
	txsch->pfvf_map[schq] = TXSCH_MAP(0, NIX_TXSCHQ_FREE);

and nix_reset_tx_schedule() writes TLxX_PARENT and TLxX_SCHEDULE only, so
TOPOLOGY (PRIO_ANCHOR/RR_PRIO) stays programmed.

That path is reachable while an HTB qdisc is still offloaded, for example
on a function reset (echo 1 > /sys/bus/pci/devices/<bdf>/reset) or a VF
FLR from a passthrough guest:

rvu_flr_handler() -> rvu_blklf_teardown() -> rvu_nix_lf_teardown() ->
nix_txschq_free()

In that case otx2_qos_free_hw_schq() never runs, so the state the commit
message describes ("PRIO_ANCHOR/RR_PRIO settings can survive in the
shared scheduler pool and affect later allocations") is unchanged.

The allocation side also looks unsanitized.  nix_txschq_alloc() resets
linkcfg, shaping and schedule for every newly allocated schq:

	nix_reset_tx_linkcfg(rvu, blkaddr, lvl, schq);
	nix_reset_tx_shaping(rvu, blkaddr, nixlf, lvl, schq);
	nix_reset_tx_schedule(rvu, blkaddr, lvl, schq);

but not TOPOLOGY, and otx2_txschq_config() never writes TOPOLOGY for
TL2/TL3/TL4, so a new owner inherits whatever anchor was left behind.

Would adding the TLxX_TOPOLOGY register to nix_reset_tx_schedule() be a
better place for this?  That would cover free_one, TXSCHQ_FREE_ALL, FLR
and allocation-time inheritance for all AF clients under rvu->rsrc_lock,
and the driver-side helper added here would not be needed.

[ ... ]

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