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

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

[Ratheesh Kannoth]

HTB offload programs NIX_AF_TLxX_TOPOLOGY on QoS-allocated scheduler
queues via otx2_qos_txschq_set_parent_topology(), but teardown freed
those queues without clearing TOPOLOGY. The AF only restores PARENT and
SCHEDULE on free, so PRIO_ANCHOR/RR_PRIO settings can survive in the
shared scheduler pool and affect later allocations.

Reset TL1 through TL4 TOPOLOGY to zero on each HTB node's schq before
returning it to the AF during hierarchy teardown and cfg rollback.

Fixes: 5e6808b4c68d ("octeontx2-pf: Add support for HTB offload")
Signed-off-by: Ratheesh Kannoth <rkannoth@marvell.com>

---
v1 -> v2: Addressed sashiko comments
	https://sashiko.dev/#/patchset/20260821072812.2890922-1-rkannoth%40marvell.com
---
 .../net/ethernet/marvell/octeontx2/nic/qos.c  | 60 +++++++++++++++++--
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/qos.c b/drivers/net/ethernet/marvell/octeontx2/nic/qos.c
index 69c0911e28e9..100342b68ffa 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)
 {
@@ -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);
 	}
 }
 
@@ -266,7 +318,7 @@ static void otx2_qos_free_hw_cfg(struct otx2_nic *pfvf,
 	otx2_qos_free_hw_node_schq(pfvf, node);
 
 	/* free node hw mappings */
-	otx2_txschq_free_one(pfvf, node->level, node->schq);
+	otx2_qos_free_hw_schq(pfvf, node->level, node->schq);
 
 	mutex_unlock(&pfvf->qos.qos_lock);
 }
@@ -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);
 		}
 	}
 
@@ -921,7 +973,7 @@ static void otx2_qos_free_cfg(struct otx2_nic *pfvf, struct otx2_qos_cfg *cfg)
 		for (idx = 0; idx < cfg->schq_contig[lvl]; idx++) {
 			if (cfg->schq_index_used[lvl][idx]) {
 				schq = cfg->schq_contig_list[lvl][idx];
-				otx2_txschq_free_one(pfvf, lvl, schq);
+				otx2_qos_free_hw_schq(pfvf, lvl, schq);
 			}
 		}
 	}
-- 
2.43.0

Re: [PATCH v2 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/20260824072147.3524339-1-rkannoth@marvell.com?part=1
---
octeontx2-pf: reset HTB scheduler topology before freeing queues

This patch adds otx2_qos_reset_schq_topology() and wraps
otx2_txschq_free_one() in a new otx2_qos_free_hw_schq() helper so that
NIX_AF_TLxX_TOPOLOGY is zeroed for TL1 through TL4 before an HTB scheduler
queue is handed back to the AF. The new helper is used in the hierarchy
teardown paths 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..100342b68ffa2 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;

[Severity: High]

The level guard admits NIX_TXSCH_LVL_TL1, so the trailing else writes
NIX_AF_TL1X_TOPOLOGY(schq) = 0.  Is zero the right value for TL1?

otx2_txschq_config() in otx2_common.c programs it with the aggregation
level RR priority:

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

and otx2_qos_root_add() depends on the same value for the HTB root:

	/* Update TL1 RR PRIO */
	if (root->level == NIX_TXSCH_LVL_TL1) {
		root->child_dwrr_prio = pfvf->hw.txschq_aggr_lvl_rr_prio;

On a PF the HTB root sits at TL1, since otx2_qos_alloc_root() does:

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

so every qdisc delete now reaches the new code with lvl == TL1:

otx2_qos_root_destroy() -> otx2_qos_destroy_node() ->
    otx2_qos_free_hw_cfg() -> otx2_qos_free_hw_schq(pfvf, TL1, root->schq)

TL1 is the per-tx-link queue shared by the PF, its default (non-HTB) Tx
hierarchy and all of its VFs.  nix_txsch_alloc() hands out the same queue
for everyone on that link:

	if (lvl >= hw->cap.nix_tx_aggr_lvl) {
		start = link;
		end = link;

Can zeroing RR_PRIO and PRIO_ANCHOR there disturb DWRR arbitration for the
PF's ordinary traffic and for its VFs, whose TL2 SCHEDULE[PRIO] is
programmed with txschq_aggr_lvl_rr_prio?

The AF does apply the write when it comes from a PF.
rvu_mbox_handler_nix_txschq_cfg() only short-circuits aggregation level
writes issued by VFs:

	if (req->lvl >= hw->cap.nix_tx_aggr_lvl &&
	    pcifunc & RVU_PFVF_FUNC_MASK) {

and nix_tl1_default_cfg() will not put the default back, because
NIX_TXSCHQ_CFG_DONE is already set:

	/* Skip if PF has already done the config */
	if (TXSCH_MAP_FLAGS(pfvf_map[schq]) & NIX_TXSCHQ_CFG_DONE)
		return;

Since otx2_txschq_config() only runs at interface open, does the zeroed TL1
TOPOLOGY then persist until the interface is brought down and up again?

Separately, is the TL1 write useful at all?  nix_txschq_free_one() never
frees an aggregation level queue:

	if (lvl >= hw->cap.nix_tx_aggr_lvl || schq >= txsch->schq.max)
		return 0;

That makes the commit message wording "before returning it to the AF" and
"can survive in the shared scheduler pool and affect later allocations" not
apply at TL1, while the register write still lands.  Would skipping
lvl >= hw->cap.nix_tx_aggr_lvl, or writing
hw->txschq_aggr_lvl_rr_prio << 1 instead of 0, be more appropriate?

> +
> +	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: Low]

This isn't a bug, but doing the sanitization in the PF/VF driver only covers
cooperative in-kernel HTB teardown.

nix_txschq_free() in rvu_nix.c (FLR, NIXLF free, VF removal) frees all of a
pcifunc's schqs through nix_reset_tx_linkcfg(), nix_clear_tx_xoff(),
nix_reset_tx_shaping() and nix_reset_tx_schedule(), and
nix_reset_tx_schedule() touches only PARENT and SCHEDULE:

	if (tlx_parent)
		rvu_write64(rvu, blkaddr, tlx_parent, 0x0);

	if (tlx_schedule)
		rvu_write64(rvu, blkaddr, tlx_schedule, 0x0);

So a PF or VF that is FLR'd while an HTB hierarchy is configured still
returns queues with PRIO_ANCHOR/RR_PRIO programmed, which is the state the
commit message describes.

Would clearing TOPOLOGY inside nix_reset_tx_schedule() cover all callers
instead, and also avoid the extra synchronous mbox message per freed schq
that this patch adds to teardown?

> +	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)
>  {

[ ... ]

> @@ -266,7 +318,7 @@ static void otx2_qos_free_hw_cfg(struct otx2_nic *pfvf,
>  	otx2_qos_free_hw_node_schq(pfvf, node);
>  
>  	/* free node hw mappings */
> -	otx2_txschq_free_one(pfvf, node->level, node->schq);
> +	otx2_qos_free_hw_schq(pfvf, node->level, node->schq);
>  
>  	mutex_unlock(&pfvf->qos.qos_lock);
>  }

[ ... ]