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

[Jacob Keller <jacob.e.keller@intel.com>]

On 10/9/2026 1:27 PM, sashiko-bot@kernel.org wrote:
> 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.
> 

I'll have to check this but cleaning up seems reasonable if it can be
done properly.

Honestly the entire logic here feels like the data types and
abstractions are not very good.. but its been difficult to try and take
incremental steps forward on fixing it :\

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