Re: [PATCH iwl-next v2 2/6] ice: count number of VSIS in agg_vsi_list

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ice: num_vsis inflated and stale entry left on VSI move failure
--

commit a9d16d503219ec18b5a73776a1a7f4b0b851cd1b
Author: Jacob Keller <jacob.e.keller@intel.com>

ice: count number of VSIS in agg_vsi_list

This patch adds a member num_vsis to the ice_sched_agg_info structure to track
the number of VSIs associated with an aggregator node. The counter is
incremented when adding a VSI to the list and decremented when removing it,
avoiding the need to iterate over the agg_vsi_list.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c b/drivers/net/ethernet/intel/ice/ice_sched.c
> index bcfc9db69e17e..08d0f685f6c45 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c

[ ... ]

> @@ -2915,24 +2920,26 @@ ice_sched_assoc_vsi_to_agg(struct ice_port_info *pi, u32 agg_id,
>  		/* add VSI ID into the aggregator list */
>  		agg_vsi_info->vsi_handle = vsi_handle;
>  		list_add(&agg_vsi_info->list_entry, &agg_info->agg_vsi_list);
> +		agg_info->num_vsis++;
>  	}
>  	/* Move VSI node to new aggregator node for requested TC(s) */
>  	ice_for_each_traffic_class(tc) {
>  		if (!ice_is_tc_ena(*tc_bitmap, tc))
>  			continue;
>  
>  		/* Move VSI to new aggregator */
>  		status = ice_sched_move_vsi_to_agg(pi, vsi_handle, agg_id, tc);
>  		if (status)
>  			break;

[Severity: Medium]
If ice_sched_move_vsi_to_agg() fails and returns an error here, the loop will
break. Should the function roll back the agg_info->num_vsis increment and remove
the newly added agg_vsi_info from agg_info->agg_vsi_list before returning?

Without a rollback, a stale VSI entry with an empty tc_bitmap remains in the
aggregator list, permanently inflating the num_vsis counter. If subsequent code
relies on num_vsis to enforce limits like ICE_MAX_VSIS_IN_AGG_NODE, this could
cause the aggregator to prematurely reject new VSIs.

>  
>  		set_bit(tc, agg_vsi_info->tc_bitmap);
>  		if (old_agg_vsi_info)
>  			clear_bit(tc, old_agg_vsi_info->tc_bitmap);
>  	}
>  	if (old_agg_vsi_info && !old_agg_vsi_info->tc_bitmap[0]) {
>  		list_del(&old_agg_vsi_info->list_entry);
>  		devm_kfree(ice_hw_to_dev(pi->hw), old_agg_vsi_info);
> +		old_agg_info->num_vsis--;
>  	}
>  	return status;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-jk-cleanup-vsi-aggregator-nodes-v2-0-313964c25c7b@intel.com?part=2