|SUCCESS| [v2, 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/163786

_AI Code Review_

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

Commit: (sha unavailable - analyzing patch file directly)
Author: Mukul Katiyar <mukul at versa-networks.com>
Subject: net/ice: properly handle TM hierarchy deletion

This patch fixes TM hierarchy deletion to reset hardware scheduler nodes
and corrects shaper profile reference counting.

> diff --git a/.mailmap b/.mailmap
> index 22476860ed..2f96e80f16 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1132,6 +1132,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..2bb684bb02 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);
> +	}
> +}

In reset_hw_node_recursive(), does the function properly handle the case
where node->children[i] is NULL? The code dereferences node->children[i]
after the recursive call without checking if it's valid.

Additionally, the PMD_DRV_LOG() call spans multiple lines with incorrect
indentation. According to the DPDK coding style guide section 3.1, when
breaking lines, the continuation should align with the opening delimiter
of the function call. The second line should align with the first
character after the opening parenthesis of PMD_DRV_LOG.

> +
>  static int
>  commit_new_hierarchy(struct rte_eth_dev *dev)
>  {
> @@ -820,8 +833,15 @@ 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;
> +		if (!pf->tm_conf.committed) {
> +			PMD_DRV_LOG(ERR, "No root node defined in TM hierarchy");
> +			return -EINVAL;
> +		}
> +		/* 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);
>  	}

In commit_new_hierarchy(), when calling reset_hw_node_recursive(), does
the code check whether hw->vsi_ctx[pf->main_vsi->idx]->sched.vsi_node[0]
is NULL before dereferencing? If this pointer is NULL, the call will
cause a null pointer dereference.

Also, does ice_alloc_lan_q_ctx() properly handle failure? The return
value is passed through, but if it fails, pf->tm_conf.committed has
already been set to false and pf->main_vsi->nb_qps has been modified.
Should these be restored to their previous values on failure?

[ ... ]

> diff --git a/drivers/net/intel/ice/ice_tm.c b/drivers/net/intel/ice/ice_tm.c
> index 2bb684bb02..015a827d7a 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++;

According to section 4.3 of the DPDK coding style guide, comparisons
against NULL should be avoided. This should be written as:

		if (shaper_profile)

>  		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 issue here - the explicit NULL comparison should be removed.

>  	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--;

And here as well.

>  		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--;

And here as well.

>  	rte_free(tm_node);
>  
>  	return 0;