Re: [PATCH net-next 13/15] ice: add VF representors one by one

[Simon Horman <horms@kernel.org>]

On Tue, Nov 14, 2023 at 10:14:33AM -0800, Tony Nguyen wrote:
> From: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> 
> Implement adding representors one by one. Always set switchdev
> environment when first representor is being added and clear environment
> when last one is being removed.
> 
> Basic switchdev configuration remains the same. Code related to creating
> and configuring representor was changed.
> 
> Instead of setting whole representors in one function handle only one
> representor in setup function. The same with removing representors.
> 
> Stop representors when new one is being added or removed. Stop means,
> disabling napi, stopping traffic and removing slow path rule. It is
> needed because ::q_id will change after remapping, so each representor
> will need new rule.
> 
> When representor are stopped rebuild control plane VSI with one more or
> one less queue. One more if new representor is being added, one less if
> representor is being removed.
> 
> Bridge port is removed during unregister_netdev() call on PR, so there
> is no need to call it from driver side.
> 
> After that do remap new queues to correct vector. At the end start all
> representors (napi enable, start queues, add slow path rule).
> 
> Reviewed-by: Piotr Raczynski <piotr.raczynski@intel.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c

...

> +int
> +ice_eswitch_attach(struct ice_pf *pf, struct ice_vf *vf)
> +{
> +	struct ice_repr *repr;
> +	int change = 1;
> +	int err;
> +
> +	if (pf->eswitch_mode == DEVLINK_ESWITCH_MODE_LEGACY)
> +		return 0;
> +
> +	if (xa_empty(&pf->eswitch.reprs)) {
> +		err = ice_eswitch_enable_switchdev(pf);
> +		if (err)
> +			return err;
> +		/* Control plane VSI is created with 1 queue as default */
> +		change = 0;
> +	}
> +
> +	ice_eswitch_stop_reprs(pf);
> +
> +	repr = ice_repr_add_vf(vf);
> +	if (IS_ERR(repr))
> +		goto err_create_repr;

Hi Michal and Tony,

This branch will cause the function to return err,
but err is set to 0 here. Perhaps it should be set to PTR_ERR(repr)
instead?

Flagged by Smatch.

> +
> +	err = ice_eswitch_setup_repr(pf, repr);
> +	if (err)
> +		goto err_setup_repr;
> +
> +	err = xa_alloc(&pf->eswitch.reprs, &repr->id, repr,
> +		       XA_LIMIT(1, INT_MAX), GFP_KERNEL);
> +	if (err)
> +		goto err_xa_alloc;
> +
> +	vf->repr_id = repr->id;
> +
> +	ice_eswitch_cp_change_queues(&pf->eswitch, change);
> +	ice_eswitch_start_reprs(pf);
> +
> +	return 0;
> +
> +err_xa_alloc:
> +	ice_eswitch_release_repr(pf, repr);
> +err_setup_repr:
> +	ice_repr_rem_vf(repr);
> +err_create_repr:
> +	if (xa_empty(&pf->eswitch.reprs))
> +		ice_eswitch_disable_switchdev(pf);
> +	ice_eswitch_start_reprs(pf);
> +
> +	return err;
> +}

...