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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20231120195447.GJ245676@kernel.org>
Date: Mon, 20 Nov 2023 19:54:47 +0000
From: Simon Horman <horms@...nel.org>
To: Tony Nguyen <anthony.l.nguyen@...el.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, netdev@...r.kernel.org,
	Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
	wojciech.drewek@...el.com, marcin.szycik@...el.com,
	piotr.raczynski@...el.com,
	Sujai Buvaneswaran <sujai.buvaneswaran@...el.com>
Subject: Re: [PATCH net-next 13/15] ice: add VF representors one by one

On Tue, Nov 14, 2023 at 10:14:33AM -0800, Tony Nguyen wrote:
> From: Michal Swiatkowski <michal.swiatkowski@...ux.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@...el.com>
> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@...el.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@...el.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;
> +}

...