On 10/9/2026 1:26 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s)
> to consider:
> - [High] ice: VF rebuild fails to re-associate VSI with fully populated
> aggregator node
> --
>
> commit aed63df873bf36bec0897ed1209a7e411aadbd13
> Author: Jacob Keller <[email protected]>
>
> ice: remove ice_agg_node wrapper structure
>
> The commit removes the ice_agg_node wrapper structure and replaces it with
> existing scheduler aggregator info. It introduces ice_cfg_vsi_agg() and
> uses it to look up and configure VSIs during initialization and rebuilds.
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sched.c
>> b/drivers/net/ethernet/intel/ice/ice_sched.c
>> index b1c2901b23f12..f0b1924b0411f 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_sched.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_sched.c
> [ ... ]
>> @@ -3082,6 +3082,114 @@ ice_move_vsi_to_agg(struct ice_port_info *pi, u32
>> agg_id, u16 vsi_handle,
>> return status;
>> }
>>
>> +/**
>> + * ice_find_available_agg - Find or create aggregator node
>> + * @pi: port information structure
>> + * @min_id: minimum aggregator node ID to use
>> + * @max_id: maximum aggregator node ID to use
>> + * @agg_type: The aggregator node type
>> + * @tc_bitmap: TC bitmap for a newly created node
>> + *
>> + * Check the existing xarray of aggregator nodes. Find the first one between
>> + * min_id and max_id which has fewer than ICE_MAX_VSIS_IN_AGG_NODE (64) VSIs
>> + * in use. If there is no aggregator node that fits this criteria, create
>> + * a new one between that range.
>> + *
>> + * Context: Must be called while holding the scheduler lock.
>> + *
>> + * Return: A pointer to the aggregator info structure, or an ERR_PTR on
>> + * failure.
>> + */
>> +static struct ice_sched_agg_info *
>> +ice_find_available_agg(struct ice_port_info *pi, u32 min_id, u32 max_id,
>> + enum ice_agg_type agg_type, unsigned long *tc_bitmap)
>> +{
>> + struct ice_sched_agg_info *agg_info;
>> + struct ice_hw *hw = pi->hw;
>> + unsigned long agg_id;
>> + int status;
>> +
>> + xa_for_each_range(&hw->agg_nodes, agg_id, agg_info, min_id, max_id) {
>> + if (agg_info->agg_type != agg_type)
>> + continue;
>> +
>> + if (agg_info->num_vsis < ICE_MAX_VSIS_IN_AGG_NODE)
>> + return agg_info;
>
> [Severity: High]
> Does this check inadvertently reject fully populated aggregators when the
> caller
> is explicitly requesting an existing node during a rebuild?
>
> If ice_vf_rebuild_aggregator_node_cfg() attempts to re-associate the VF
> with an existing perfectly full aggregator (64 VSIs), it seems this check
> evaluates to false because the SW tree structures were preserved across the
> reset and the VF is still accounted for in num_vsis.
>
Two reasons this is not valid:
a) we only call this path on creating a new VSI
b) we already remove aggregator nodes through ice_vsi_decfg()
I believe we will need to investigate the first path and likely need to
recreate aggregator node config after rebuild, but I haven't actually
tracked down what the consequence of the existing logic is yet.
The existing "replay" logic replays any remaining aggregator node config
without going through this "init" path so either way this path is not
supposed to be called in that way.
>> + }
>> +
>> + /* No aggregator node exists with space, so create a new one */
>> + agg_info = ice_alloc_agg_info(hw, min_id, max_id, agg_type);
>
> Since ice_find_available_agg() returns here if no space was found, won't
> this allocation attempt to create a new node with the exact same ID (because
> min_id and max_id are both vsi->agg_id), causing xa_alloc() to fail?
>
it would, but only if its given a single ID, (as in the VF rebuild
case). But in that case, the VF was removed before and is re-added now.
>> + if (IS_ERR(agg_info))
>> + return agg_info;
> [ ... ]
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> index c56a35b8dd48f..a67586f7db8be 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> @@ -471,24 +471,20 @@ static void ice_vf_rebuild_aggregator_node_cfg(struct
>> ice_vsi *vsi)
>> struct device *dev;
>> int status;
>>
>> - if (!vsi->agg_node)
>> - return;
>> -
>> dev = ice_pf_to_dev(pf);
>> - if (vsi->agg_node->num_vsis == ICE_MAX_VSIS_IN_AGG_NODE) {
>> - dev_dbg(dev,
>> - "agg_id %u already has reached max_num_vsis %u\n",
>> - vsi->agg_node->agg_id, vsi->agg_node->num_vsis);
>> +
>> + if (vsi->agg_id < 0 || vsi->agg_id > U32_MAX) {
>> + dev_dbg(dev, "VSI idx %u does not have a valid aggregator node
>> ID stored\n",
>> + vsi->idx);
>> return;
>> }
>>
>> - status = ice_move_vsi_to_agg(pf->hw.port_info, vsi->agg_node->agg_id,
>> - vsi->idx, vsi->tc_cfg.ena_tc);
>> + status = ice_cfg_vsi_agg(pf->hw.port_info, vsi->idx,
>> + (u32)vsi->agg_id, (u32)vsi->agg_id,
>> + (u8)vsi->tc_cfg.ena_tc, NULL);
>
> If ice_cfg_vsi_agg() fails when the original node is fully populated, does
> this break QoS and bandwidth isolation for any VF assigned to that full
> aggregator? The previous logic specifically checked for and gracefully handled
> reaching ICE_MAX_VSIS_IN_AGG_NODE.
>
This shouldn't be reachable. I'll try to investigate how that was
handled before. I don't think it actually *did* gracefully handle this
scenario.
>> if (status)
>> - dev_dbg(dev, "unable to move VSI idx %u into aggregator %u
>> node",
>> - vsi->idx, vsi->agg_node->agg_id);
>> - else
>> - vsi->agg_node->num_vsis++;
>> + dev_dbg(dev, "unable to move VSI idx %u into aggregator node
>> %lld\n",
>> + vsi->idx, vsi->agg_id);
>> }
>
