netdev - Re: [iwl-next v3 3/4] ice: move VSI configuration outside repr setup

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] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 06:16:27 +0200
From: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
To: Simon Horman <horms@...nel.org>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	jacob.e.keller@...el.com, michal.kubiak@...el.com,
	maciej.fijalkowski@...el.com, sridhar.samudrala@...el.com,
	przemyslaw.kitszel@...el.com, wojciech.drewek@...el.com,
	pio.raczynski@...il.com, jiri@...dia.com,
	mateusz.polchlopek@...el.com, shayd@...dia.com, kuba@...nel.org
Subject: Re: [iwl-next v3 3/4] ice: move VSI configuration outside repr setup

On Fri, Jun 14, 2024 at 01:43:31PM +0100, Simon Horman wrote:
> On Mon, Jun 10, 2024 at 09:44:33AM +0200, Michal Swiatkowski wrote:
> > It is needed because subfunction port representor shouldn't configure
> > the source VSI during representor creation.
> > 
> > Move the code to separate function and call it only in case the VF port
> > representor is being created.
> > 
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>
> 
> Hi Michal,
> 
> The nit below notwithstanding, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@...nel.org>
> 
> > ---
> >  drivers/net/ethernet/intel/ice/ice_eswitch.c | 55 ++++++++++++++------
> >  drivers/net/ethernet/intel/ice/ice_eswitch.h | 10 ++++
> >  drivers/net/ethernet/intel/ice/ice_repr.c    |  7 +++
> >  3 files changed, 57 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > index 7b57a6561a5a..3f73f46111fc 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> > @@ -117,17 +117,10 @@ static int ice_eswitch_setup_repr(struct ice_pf *pf, struct ice_repr *repr)
> >  	struct ice_vsi *vsi = repr->src_vsi;
> >  	struct metadata_dst *dst;
> >  
> > -	ice_remove_vsi_fltr(&pf->hw, vsi->idx);
> >  	repr->dst = metadata_dst_alloc(0, METADATA_HW_PORT_MUX,
> >  				       GFP_KERNEL);
> >  	if (!repr->dst)
> > -		goto err_add_mac_fltr;
> > -
> > -	if (ice_vsi_update_security(vsi, ice_vsi_ctx_clear_antispoof))
> > -		goto err_dst_free;
> > -
> > -	if (ice_vsi_add_vlan_zero(vsi))
> > -		goto err_update_security;
> > +		return -ENOMEM;
> >  
> >  	netif_keep_dst(uplink_vsi->netdev);
> >  
> > @@ -136,16 +129,48 @@ static int ice_eswitch_setup_repr(struct ice_pf *pf, struct ice_repr *repr)
> >  	dst->u.port_info.lower_dev = uplink_vsi->netdev;
> >  
> >  	return 0;
> > +}
> >  
> > -err_update_security:
> > +/**
> > + * ice_eswitch_cfg_vsi - configure VSI to work in slow-path
> > + * @vsi: VSI structure of representee
> > + * @mac: representee MAC
> > + *
> > + * Return: 0 on success, non-zero on error.
> > + */
> > +int ice_eswitch_cfg_vsi(struct ice_vsi *vsi, const u8 *mac)
> > +{
> > +	int err;
> > +
> > +	ice_remove_vsi_fltr(&vsi->back->hw, vsi->idx);
> > +
> > +	err = ice_vsi_update_security(vsi, ice_vsi_ctx_clear_antispoof);
> > +	if (err)
> > +		goto err_update_security;
> > +
> > +	err = ice_vsi_add_vlan_zero(vsi);
> > +	if (err)
> > +		goto err_vlan_zero;
> > +
> > +	return 0;
> > +
> > +err_vlan_zero:
> >  	ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof);
> 
> nit: Please consider continuing the practice, that is used for the labels
>      removed by this patch, of naming labels after what they do rather
>      than what jumps to them.
> 

Ok, I was wondering which approach is better. Probably mixing both is
the worst. I will follow your advice next time, thanks.

> > -err_dst_free:
> > -	metadata_dst_free(repr->dst);
> > -	repr->dst = NULL;
> > -err_add_mac_fltr:
> > -	ice_fltr_add_mac_and_broadcast(vsi, repr->parent_mac, ICE_FWD_TO_VSI);
> > +err_update_security:
> > +	ice_fltr_add_mac_and_broadcast(vsi, mac, ICE_FWD_TO_VSI);
> >  
> > -	return -ENODEV;
> > +	return err;
> > +}
> > +
> 
> ...