diff mbox series

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

Message ID 20240610074434.1962735-4-michal.swiatkowski@linux.intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series ice: prepare representor for SF support | expand

Checks

Context Check Description
netdev/series_format warning Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 866 this patch: 866
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 4 maintainers not CCed: anthony.l.nguyen@intel.com pabeni@redhat.com jesse.brandeburg@intel.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 868 this patch: 868
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 870 this patch: 870
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 118 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 15 this patch: 15
netdev/source_inline success Was 0 now: 0

Commit Message

Michal Swiatkowski June 10, 2024, 7:44 a.m. UTC 
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@linux.intel.com>
---
 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(-)

Comments

Simon Horman June 14, 2024, 12:43 p.m. UTC | #1 
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@linux.intel.com>

Hi Michal,

The nit below notwithstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@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.

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

...
Michal Swiatkowski June 21, 2024, 4:16 a.m. UTC | #2 
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@linux.intel.com>
> 
> Hi Michal,
> 
> The nit below notwithstanding, this looks good to me.
> 
> Reviewed-by: Simon Horman <horms@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;
> > +}
> > +
> 
> ...
diff mbox series

Patch

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);
-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;
+}
+
+/**
+ * ice_eswitch_decfg_vsi - unroll changes done to VSI for switchdev
+ * @vsi: VSI structure of representee
+ * @mac: representee MAC
+ */
+void ice_eswitch_decfg_vsi(struct ice_vsi *vsi, const u8 *mac)
+{
+	ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof);
+	ice_fltr_add_mac_and_broadcast(vsi, mac, ICE_FWD_TO_VSI);
 }
 
 /**
diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.h b/drivers/net/ethernet/intel/ice/ice_eswitch.h
index e2e5c0c75e7d..9a25606e9740 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch.h
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch.h
@@ -28,6 +28,9 @@  netdev_tx_t
 ice_eswitch_port_start_xmit(struct sk_buff *skb, struct net_device *netdev);
 struct net_device *ice_eswitch_get_target(struct ice_rx_ring *rx_ring,
 					  union ice_32b_rx_flex_desc *rx_desc);
+
+int ice_eswitch_cfg_vsi(struct ice_vsi *vsi, const u8 *mac);
+void ice_eswitch_decfg_vsi(struct ice_vsi *vsi, const u8 *mac);
 #else /* CONFIG_ICE_SWITCHDEV */
 static inline void ice_eswitch_detach(struct ice_pf *pf, struct ice_vf *vf) { }
 
@@ -85,5 +88,12 @@  ice_eswitch_get_target(struct ice_rx_ring *rx_ring,
 {
 	return rx_ring->netdev;
 }
+
+static inline int ice_eswitch_cfg_vsi(struct ice_vsi *vsi, const u8 *mac)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void ice_eswitch_decfg_vsi(struct ice_vsi *vsi, const u8 *mac) { }
 #endif /* CONFIG_ICE_SWITCHDEV */
 #endif /* _ICE_ESWITCH_H_ */
diff --git a/drivers/net/ethernet/intel/ice/ice_repr.c b/drivers/net/ethernet/intel/ice/ice_repr.c
index 35a6ac8c0466..bdda3401e343 100644
--- a/drivers/net/ethernet/intel/ice/ice_repr.c
+++ b/drivers/net/ethernet/intel/ice/ice_repr.c
@@ -306,6 +306,7 @@  static void ice_repr_rem(struct ice_repr *repr)
 void ice_repr_rem_vf(struct ice_repr *repr)
 {
 	ice_repr_remove_node(&repr->vf->devlink_port);
+	ice_eswitch_decfg_vsi(repr->src_vsi, repr->parent_mac);
 	unregister_netdev(repr->netdev);
 	ice_devlink_destroy_vf_port(repr->vf);
 	ice_virtchnl_set_dflt_ops(repr->vf);
@@ -401,11 +402,17 @@  struct ice_repr *ice_repr_add_vf(struct ice_vf *vf)
 	if (err)
 		goto err_netdev;
 
+	err = ice_eswitch_cfg_vsi(repr->src_vsi, repr->parent_mac);
+	if (err)
+		goto err_cfg_vsi;
+
 	ice_virtchnl_set_repr_ops(vf);
 	ice_repr_set_tx_topology(vf->pf);
 
 	return repr;
 
+err_cfg_vsi:
+	unregister_netdev(repr->netdev);
 err_netdev:
 	ice_repr_rem(repr);
 err_repr_add: