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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260828035231.46911-1-rkannoth@xxxxxxxxxxx?part=1
---
octeontx2-pf: reset HTB scheduler topology before freeing queues

This adds otx2_qos_reset_schq_topology() and a wrapper
otx2_qos_free_hw_schq() so that NIX_AF_TLxX_TOPOLOGY is zeroed for TL4
through TL2 before a QoS scheduler queue is returned to the AF.  The new
wrapper replaces otx2_txschq_free_one() in the class node teardown path and
in the cfg rollback path.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> index 69c0911e28e91..ab9661334e6f9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
> @@ -235,6 +235,58 @@ 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;
> +
> +	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);
> +	else
> +		cfg->reg[0] = NIX_AF_TL1X_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);
> +	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)
>  {

[Severity: Low]
The trailing else here cannot be reached.  In the mbox level enum
NIX_TXSCH_LVL_TL4 is 1, TL3 is 2, TL2 is 3 and TL1 is 4, and the guard
at the top of otx2_qos_reset_schq_topology() already bails out with:

	if (lvl < NIX_TXSCH_LVL_TL4 || lvl >= NIX_TXSCH_LVL_TL1)
		return 0;

so only lvl 1, 2 and 3 ever get past it, and those three values are
exhaustively covered by the TL4/TL3/TL2 arms above.  The
NIX_AF_TL1X_TOPOLOGY(schq) assignment is dead code.

Beyond being unreachable, it actively contradicts the commit message,
which says the aggregation level is deliberately skipped because TL1 is
shared with the PF, the default Tx hierarchy and the VFs and is never
returned to the AF by nix_txschq_free_one().  A reader who only sees the
else arm will conclude the opposite, and if the guard is ever relaxed the
function will silently start zeroing a TL1 TOPOLOGY that other users
depend on.

Would you drop the else arm and make the last case an explicit
"else if (lvl == NIX_TXSCH_LVL_TL2)", or convert the chain to a switch
with the TL1 case omitted, so the code and the commit message agree on
which levels are handled?

[Severity: High]
Was otx2_qos_free_hw_node_schq() intentionally left on the bare
otx2_txschq_free_one() path?  It still reads:

drivers/net/ethernet/marvell/octeontx2/nic/qos.c:otx2_qos_free_hw_node_schq() {
	list_for_each_entry_reverse(node, &parent->child_schq_list, list)
		otx2_txschq_free_one(pfvf, node->level, node->schq);
}

These child_schq_list entries are the intermediate chain created by
otx2_qos_alloc_txschq_node() for every level from node->level - 1 down to
MDQ:

	for (lvl = node->level - 1; lvl >= NIX_TXSCH_LVL_MDQ; lvl--) {

so they sit at exactly the TL4/TL3/TL2 levels the commit message says are
sanitized.

They also carry TOPOLOGY, since otx2_qos_txschq_push_cfg_schq() programs it
for each chain entry's parent:

	list_for_each_entry(tmp, &node->child_schq_list, list) {
		ret = otx2_qos_txschq_config(pfvf, tmp);
		if (ret)
			return -EIO;
		ret = otx2_qos_txschq_set_parent_topology(pfvf, tmp->parent);

Two cases where those chain schqs would reach nix_txschq_free_one() with a
non-zero TOPOLOGY:

1) HTB set up while the interface is down.  otx2_qos_push_txschq_cfg() and
   otx2_qos_root_add() only fill the cfg and skip hardware programming:

	if (!(pfvf->netdev->flags & IFF_UP)) {
		otx2_qos_txschq_fill_cfg(pfvf, node, cfg);
		return 0;
	}

   A following "tc qdisc del" while still down goes
   otx2_qos_root_destroy() -> otx2_qos_destroy_node() ->
   otx2_qos_free_hw_cfg() -> otx2_qos_free_hw_node_schq(), returning the
   queue with whatever TOPOLOGY the previous owner left.

2) A partially failed push, for example otx2_qos_txschq_update_config()
   failing in the restore paths of otx2_qos_leaf_to_inner() /
   otx2_qos_leaf_alloc(), or otx2_qos_config_txschq() failing on ifup.  The
   chain is left unprogrammed and later freed through the same helper.

On the AF side nix_txschq_free_one() only touches XOFF, linkcfg, shaping and
schedule:

drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c:nix_txschq_free_one() {
	nix_clear_tx_xoff(rvu, blkaddr, lvl, 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);
}

so a stale PRIO_ANCHOR/RR_PRIO in those chain queues would still survive
into the shared pool, which is the case the commit message describes.

> @@ -252,7 +304,7 @@ static void otx2_qos_free_hw_node(struct otx2_nic *pfvf,
>  	list_for_each_entry_safe(node, tmp, &parent->child_list, list) {
>  		otx2_qos_free_hw_node(pfvf, node);
>  		otx2_qos_free_hw_node_schq(pfvf, node);
> -		otx2_txschq_free_one(pfvf, node->level, node->schq);
> +		otx2_qos_free_hw_schq(pfvf, node->level, node->schq);
>  	}
>  }
>  
> @@ -913,7 +965,7 @@ static void otx2_qos_free_cfg(struct otx2_nic *pfvf, struct otx2_qos_cfg *cfg)
>  	for (lvl = 0; lvl < NIX_TXSCH_LVL_CNT; lvl++) {
>  		for (idx = 0; idx < cfg->schq[lvl]; idx++) {
>  			schq = cfg->schq_list[lvl][idx];
> -			otx2_txschq_free_one(pfvf, lvl, schq);
> +			otx2_qos_free_hw_schq(pfvf, lvl, schq);
>  		}
>  	}
>  

Is the difference between the two paths deliberate?  cfg->schq_list is
filled from child_schq_list by otx2_qos_read_txschq_cfg_schq(), so the
rollback path here does reset TOPOLOGY for the same queues that the
hierarchy teardown in otx2_qos_free_hw_node_schq() frees without a reset.
Would routing the child_schq_list loop through otx2_qos_free_hw_schq() as
well make the two paths symmetric?

[ ... ]