Re: [PATCH iwl-next v2 5/6] ice: remove ice_agg_node wrapper structure

[sashiko-bot@kernel.org]

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 <jacob.e.keller@intel.com>

    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.

> +	}
> +
> +	/* 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?

> +	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.

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

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