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

[netdev-bot+sashiko]

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.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> index 69c0911e28e9..f160b1618efa 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> @@ -235,13 +235,63 @@ static int otx2_qos_txschq_set_parent_topology(struct otx2_nic *pfvf,
>  	return rc;
>  }
>  
> +static int otx2_qos_reset_schq_topology(struct otx2_nic *pfvf, u16 lvl,
> +					u16 schq)
> +{
> +	struct mbox *mbox = &pfvf->mbox;
> +	struct nix_txschq_config *cfg;
> +	int rc;
> +
> +	if (lvl < NIX_TXSCH_LVL_TL4 || lvl >= NIX_TXSCH_LVL_TL1)
> +		return 0;

[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.

> +
> +	mutex_lock(&mbox->lock);
> +
> +	cfg = otx2_mbox_alloc_msg_nix_txschq_cfg(mbox);
> +	if (!cfg) {
> +		mutex_unlock(&mbox->lock);
> +		return -ENOMEM;
> +	}
> +
> +	cfg->lvl = lvl;
> +	cfg->num_regs = 1;
> +
> +	if (lvl == NIX_TXSCH_LVL_TL4)
> +		cfg->reg[0] = NIX_AF_TL4X_TOPOLOGY(schq);
> +	else if (lvl == NIX_TXSCH_LVL_TL3)
> +		cfg->reg[0] = NIX_AF_TL3X_TOPOLOGY(schq);
> +	else if (lvl == NIX_TXSCH_LVL_TL2)
> +		cfg->reg[0] = NIX_AF_TL2X_TOPOLOGY(schq);
> +
> +	cfg->regval[0] = 0;
> +
> +	rc = otx2_sync_mbox_msg(mbox);
> +
> +	mutex_unlock(&mbox->lock);
> +
> +	return rc;
> +}
> +
> +static void otx2_qos_free_hw_schq(struct otx2_nic *pfvf, u16 lvl, u16 schq)
> +{
> +	int err;
> +
> +	err = otx2_qos_reset_schq_topology(pfvf, lvl, schq);

[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.

> +	if (err)
> +		netdev_warn(pfvf->netdev,
> +			    "QoS: failed to reset topology for schq %u at level %u: %d\n",
> +			    schq, lvl, err);
> +
> +	otx2_txschq_free_one(pfvf, lvl, schq);
> +}
> +
>  static void otx2_qos_free_hw_node_schq(struct otx2_nic *pfvf,
>  				       struct otx2_qos_node *parent)
>  {
>  	struct otx2_qos_node *node;
>  
>  	list_for_each_entry_reverse(node, &parent->child_schq_list, list)
> -		otx2_txschq_free_one(pfvf, node->level, node->schq);
> +		otx2_qos_free_hw_schq(pfvf, node->level, node->schq);
>  }

[ ... ]

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