|SUCCESS| [2/2] net/ice: fix shaper profile reference count tracking

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163545

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-01

Commit: 8c481c3bb65b ("net/ice: support queue and queue group bandwidth limit")
Author: Mukul Katiyar <mukul at versa-networks.com>
Subject: [PATCH 1/2] net/ice: properly handle TM hierarchy deletion

This series addresses two resource management issues in the Intel ice
driver's traffic management code. The first patch fixes stale hardware
scheduler state when a TM hierarchy is fully removed. The second patch
corrects reference count tracking for shaper profiles.

> diff --git a/.mailmap b/.mailmap
> index 7e23e2f528..e65a0c6b20 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1129,6 +1129,7 @@ Moti Haimovsky <motih at mellanox.com>
>  Muhammad Ahmad <muhammad.ahmad at emumba.com>
>  Muhammad Bilal <m.bilal at emumba.com>
>  Mukesh Dua <mukesh.dua81 at gmail.com>
> +Mukul Katiyar <mukul at versa-networks.com>
>  Murphy Yang <murphyx.yang at intel.com>
>  Murthy NSSR <nidadavolu.murthy at caviumnetworks.com>
>  Muthurajan Jayakumar <muthurajan.jayakumar at intel.com>
> diff --git a/drivers/net/intel/ice/ice_tm.c b/drivers/net/intel/ice/ice_tm.c
> index ff53f2acfd..4dcfb15c27 100644
> --- a/drivers/net/intel/ice/ice_tm.c
> +++ b/drivers/net/intel/ice/ice_tm.c
> @@ -805,6 +805,19 @@ create_sched_node_recursive(struct ice_pf *pf, struct ice_port_info *pi,
>  	return 0;
>  }
>  
> +static void
> +reset_hw_node_recursive(struct ice_hw *hw, struct ice_sched_node *node)
> +{
> +	uint16_t i;
> +
> +	for (i = 0; i < node->num_children; i++) {
> +		reset_hw_node_recursive(hw, node->children[i]);
> +		if (ice_cfg_hw_node(hw, NULL, node->children[i]))
> +			PMD_DRV_LOG(WARNING, "Failed to reset node %u to default configuration",
> +					node->children[i]->info.node_teid);

This function lacks error handling at its call site. If ice_cfg_hw_node()
fails for any child node, does reset_hw_node_recursive() continue
silently? Should this return status propagate up so the caller can take
corrective action?

Also, is there any risk of infinite recursion if the scheduler tree
contains cycles or if node->children[] references become corrupted?

> +	}
> +}
> +
>  static int
>  commit_new_hierarchy(struct rte_eth_dev *dev)
>  {
> @@ -820,8 +833,11 @@ commit_new_hierarchy(struct rte_eth_dev *dev)
>  	struct ice_sched_node *new_vsi_root = hw->vsi_ctx[pf->main_vsi->idx]->sched.vsi_node[0];
>  
>  	if (sw_root == NULL) {
> -		PMD_DRV_LOG(ERR, "No root node defined in TM hierarchy");
> -		return -1;
> +		/* TM hierarchy deleted. Restore default scheduler state. */
> +		reset_hw_node_recursive(hw, hw->vsi_ctx[pf->main_vsi->idx]->sched.vsi_node[0]);
> +		pf->main_vsi->nb_qps = pf->lan_nb_qps;
> +		pf->tm_conf.committed = false;
> +		return ice_alloc_lan_q_ctx(hw, 0, 0, pf->main_vsi->nb_qps);

Does this code properly validate hw->vsi_ctx[pf->main_vsi->idx] and
sched.vsi_node[0] before dereferencing them in
reset_hw_node_recursive()? Can pf->main_vsi or pf->main_vsi->idx ever
be invalid here?

What happens if ice_alloc_lan_q_ctx() fails? The scheduler nodes have
already been reset and pf->tm_conf.committed is already cleared. Does
this leave the device in an inconsistent state?

>  	}
>  
>  	/* handle case where VSI node needs to move DOWN the hierarchy */

[ ... ]

> diff --git a/drivers/net/intel/ice/ice_tm.c b/drivers/net/intel/ice/ice_tm.c
> index 4dcfb15c27..d1f3d80ac7 100644
> --- a/drivers/net/intel/ice/ice_tm.c
> +++ b/drivers/net/intel/ice/ice_tm.c
> @@ -456,6 +456,8 @@ ice_tm_node_add(struct rte_eth_dev *dev, uint32_t node_id,
>  		tm_node->parent = NULL;
>  		tm_node->reference_count = 0;
>  		tm_node->shaper_profile = shaper_profile;
> +		if (shaper_profile != NULL)
> +			shaper_profile->reference_count++;

Does ice_tm_node_add() have error paths after this point? If the
function fails after incrementing shaper_profile->reference_count, does
the caller know to decrement it again?

>  		tm_node->children = RTE_PTR_ADD(tm_node, sizeof(struct ice_tm_node));
>  		tm_node->params = *params;
>  		pf->tm_conf.root = tm_node;
> @@ -518,6 +520,8 @@ ice_tm_node_add(struct rte_eth_dev *dev, uint32_t node_id,
>  	tm_node->parent = parent_node;
>  	tm_node->level = level_id;
>  	tm_node->shaper_profile = shaper_profile;
> +	if (shaper_profile != NULL)
> +		shaper_profile->reference_count++;

Same question here about failure paths. If allocation or validation
fails after this increment, does the reference count get properly
cleaned up?

>  	tm_node->children = RTE_PTR_ADD(tm_node, sizeof(struct ice_tm_node));
>  	tm_node->parent->children[tm_node->parent->reference_count++] = tm_node;
>  	tm_node->params = *params;
> @@ -568,6 +572,8 @@ ice_tm_node_delete(struct rte_eth_dev *dev, uint32_t node_id,
>  
>  	/* root node */
>  	if (tm_node->level == 0) {
> +		if (tm_node->shaper_profile != NULL)
> +			tm_node->shaper_profile->reference_count--;
>  		rte_free(tm_node);
>  		pf->tm_conf.root = NULL;
>  		return 0;
> @@ -582,6 +588,8 @@ ice_tm_node_delete(struct rte_eth_dev *dev, uint32_t node_id,
>  		tm_node->parent->children[j] = tm_node->parent->children[j + 1];
>  
>  	tm_node->parent->reference_count--;
> +	if (tm_node->shaper_profile != NULL)
> +		tm_node->shaper_profile->reference_count--;

Is there any validation that shaper_profile->reference_count doesn't
underflow? Can ice_tm_node_delete() be called multiple times for the
same node, or can the shaper_profile pointer become stale between node
creation and deletion?

>  	rte_free(tm_node);
>  
>  	return 0;