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.swiatkow...@linux.intel.com> > > Hi Michal, > > The nit below notwithstanding, this looks good to me. > > Reviewed-by: Simon Horman <ho...@kernel.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; > > +} > > + > > ...