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

[Ratheesh Kannoth]

On 2026-09-02 at 22:54:28, Simon Horman (horms@xxxxxxxxxx) wrote:
> 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?
>
> [ ... ]

ACK.

pw-bot: changes-requested