mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v3 net] octeontx2-pf: reset HTB scheduler topology before freeing queues
@ 2026-08-28  3:52 Ratheesh Kannoth
  2026-09-02 17:24 ` Simon Horman
  0 siblings, 1 reply; 3+ messages in thread
From: Ratheesh Kannoth @ 2026-08-28  3:52 UTC (permalink / raw)
  To: davem, hkelam, linux-kernel, naveenm, netdev, sgoutham
  Cc: andrew+netdev, edumazet, kuba, pabeni, 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.

Add otx2_qos_reset_schq_topology() and otx2_qos_free_hw_schq() to zero
TL4 through TL2 TOPOLOGY before each schq is returned to the AF during
hierarchy teardown and cfg rollback.  Skip the aggregation level (TL1):
it is a per-tx-link queue shared by the PF, default Tx hierarchy and VFs,
and is not freed back to the AF by nix_txschq_free_one().

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

---
v2 -> v3: Addressed AI review comments.
	https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824072147.3524339-1-rkannoth@marvell.com?part=1
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..ab9661334e6f 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 net] octeontx2-pf: reset HTB scheduler topology before freeing queues
  2026-08-28  3:52 [PATCH v3 net] octeontx2-pf: reset HTB scheduler topology before freeing queues Ratheesh Kannoth
@ 2026-09-02 17:24 ` Simon Horman
  2026-09-03  2:28   ` Ratheesh Kannoth
  0 siblings, 1 reply; 3+ messages in thread
From: Simon Horman @ 2026-09-02 17:24 UTC (permalink / raw)
  To: rkannoth
  Cc: Simon Horman, davem, hkelam, linux-kernel, naveenm, netdev,
	sgoutham, andrew+netdev, edumazet, kuba, pabeni

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@marvell.com?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?

[ ... ]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v3 net] octeontx2-pf: reset HTB scheduler topology before freeing queues
  2026-09-02 17:24 ` Simon Horman
@ 2026-09-03  2:28   ` Ratheesh Kannoth
  0 siblings, 0 replies; 3+ messages in thread
From: Ratheesh Kannoth @ 2026-09-03  2:28 UTC (permalink / raw)
  To: Simon Horman
  Cc: davem, hkelam, linux-kernel, naveenm, netdev, sgoutham,
	andrew+netdev, edumazet, kuba, pabeni

On 2026-09-02 at 22:54:28, Simon Horman (horms@kernel.org) 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@marvell.com?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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-09-03  2:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-28  3:52 [PATCH v3 net] octeontx2-pf: reset HTB scheduler topology before freeing queues Ratheesh Kannoth
2026-09-02 17:24 ` Simon Horman
2026-09-03  2:28   ` Ratheesh Kannoth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®