diff mbox series

[iwl-next,v1,4/8] ice: control default Tx rule in lag

Message ID 20240125125314.852914-5-michal.swiatkowski@linux.intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series ice: use less resources in switchdev | 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: 1077 this patch: 1077
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 0 of 0 maintainers
netdev/build_clang success Errors and warnings before: 1095 this patch: 1095
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: 1095 this patch: 1095
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 96 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest pending net-next-2024-01-25--15-00

Commit Message

Michal Swiatkowski Jan. 25, 2024, 12:53 p.m. UTC 
Tx rule in switchdev was changed to use PF instead of additional control
plane VSI. Because of that during lag we should control it. Control
means to add and remove the default Tx rule during lag active/inactive
switching.

It can be done the same way as default Rx rule.

Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lag.c | 39 ++++++++++++++++++------
 drivers/net/ethernet/intel/ice/ice_lag.h |  3 +-
 2 files changed, 32 insertions(+), 10 deletions(-)

Comments

Simon Horman Jan. 29, 2024, 10:55 a.m. UTC | #1 
On Thu, Jan 25, 2024 at 01:53:10PM +0100, Michal Swiatkowski wrote:
> Tx rule in switchdev was changed to use PF instead of additional control
> plane VSI. Because of that during lag we should control it. Control
> means to add and remove the default Tx rule during lag active/inactive
> switching.
> 
> It can be done the same way as default Rx rule.

Hi Michal,

Can I confirm that LAG TX/RX works both before and after this patch?

> 
> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_lag.c | 39 ++++++++++++++++++------
>  drivers/net/ethernet/intel/ice/ice_lag.h |  3 +-
>  2 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c

...

> @@ -266,9 +274,22 @@ ice_lag_cfg_dflt_fltr(struct ice_lag *lag, bool add)
>  {
>  	u32 act = ICE_SINGLE_ACT_VSI_FORWARDING |
>  		ICE_SINGLE_ACT_VALID_BIT | ICE_SINGLE_ACT_LAN_ENABLE;
> +	int err;
> +
> +	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
> +			       ICE_FLTR_RX, add);
> +	if (err)
> +		return err;
> +
> +	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_tx_rule_id,
> +			       ICE_FLTR_TX, add);
> +	if (err) {
> +		ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
> +				 ICE_FLTR_RX, !add);
> +		return err;
> +	}
>  
> -	return ice_lag_cfg_fltr(lag, act, lag->pf_recipe,
> -				&lag->pf_rule_id, add);
> +	return 0;
>  }

nit: perhaps this could be more idiomatically written using a
     goto to unwind on error.

...
Michal Swiatkowski Feb. 1, 2024, 7:05 a.m. UTC | #2 
On Mon, Jan 29, 2024 at 10:55:41AM +0000, Simon Horman wrote:
> On Thu, Jan 25, 2024 at 01:53:10PM +0100, Michal Swiatkowski wrote:
> > Tx rule in switchdev was changed to use PF instead of additional control
> > plane VSI. Because of that during lag we should control it. Control
> > means to add and remove the default Tx rule during lag active/inactive
> > switching.
> > 
> > It can be done the same way as default Rx rule.
> 
> Hi Michal,
> 
> Can I confirm that LAG TX/RX works both before and after this patch?
> 

Hi Simon,

This part of LAG code is related to the LAG + switchdev feature (it
isn't chaning LAG core code). Hope that normal LAG also works well. This
is the scenario when you have PF in switchdev, bond created of two PFs
connected to the bridge with representors. Switching between interfaces
from bond needs to add default Rx rule, and after my changes also
default Tx rule.

Do you think I should add this description to commit message?

Thanks,
Michal

> > 
> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
> > Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> > Signed-off-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_lag.c | 39 ++++++++++++++++++------
> >  drivers/net/ethernet/intel/ice/ice_lag.h |  3 +-
> >  2 files changed, 32 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
> 
> ...
> 
> > @@ -266,9 +274,22 @@ ice_lag_cfg_dflt_fltr(struct ice_lag *lag, bool add)
> >  {
> >  	u32 act = ICE_SINGLE_ACT_VSI_FORWARDING |
> >  		ICE_SINGLE_ACT_VALID_BIT | ICE_SINGLE_ACT_LAN_ENABLE;
> > +	int err;
> > +
> > +	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
> > +			       ICE_FLTR_RX, add);
> > +	if (err)
> > +		return err;
> > +
> > +	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_tx_rule_id,
> > +			       ICE_FLTR_TX, add);
> > +	if (err) {
> > +		ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
> > +				 ICE_FLTR_RX, !add);
> > +		return err;
> > +	}
> >  
> > -	return ice_lag_cfg_fltr(lag, act, lag->pf_recipe,
> > -				&lag->pf_rule_id, add);
> > +	return 0;
> >  }
> 
> nit: perhaps this could be more idiomatically written using a
>      goto to unwind on error.
>
Thanks, I will rewrite it.

> ...
Simon Horman Feb. 2, 2024, 5:18 p.m. UTC | #3 
On Thu, Feb 01, 2024 at 08:05:06AM +0100, Michal Swiatkowski wrote:
> On Mon, Jan 29, 2024 at 10:55:41AM +0000, Simon Horman wrote:
> > On Thu, Jan 25, 2024 at 01:53:10PM +0100, Michal Swiatkowski wrote:
> > > Tx rule in switchdev was changed to use PF instead of additional control
> > > plane VSI. Because of that during lag we should control it. Control
> > > means to add and remove the default Tx rule during lag active/inactive
> > > switching.
> > > 
> > > It can be done the same way as default Rx rule.
> > 
> > Hi Michal,
> > 
> > Can I confirm that LAG TX/RX works both before and after this patch?
> > 
> 
> Hi Simon,
> 
> This part of LAG code is related to the LAG + switchdev feature (it
> isn't chaning LAG core code). Hope that normal LAG also works well. This
> is the scenario when you have PF in switchdev, bond created of two PFs
> connected to the bridge with representors. Switching between interfaces
> from bond needs to add default Rx rule, and after my changes also
> default Tx rule.
> 
> Do you think I should add this description to commit message?

Thanks Michal,

I think that might be a good idea.

But my question was a bit different: I'm asking if this patch addresses
a regression introduced earlier in the series. Sorry for not being clearer
the first time around.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
index 467372d541d2..efe882a46fd3 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.c
+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
@@ -202,11 +202,12 @@  static struct ice_lag *ice_lag_find_primary(struct ice_lag *lag)
  * @act: rule action
  * @recipe_id: recipe id for the new rule
  * @rule_idx: pointer to rule index
+ * @direction: ICE_FLTR_RX or ICE_FLTR_TX
  * @add: boolean on whether we are adding filters
  */
 static int
 ice_lag_cfg_fltr(struct ice_lag *lag, u32 act, u16 recipe_id, u16 *rule_idx,
-		 bool add)
+		 u8 direction, bool add)
 {
 	struct ice_sw_rule_lkup_rx_tx *s_rule;
 	u16 s_rule_sz, vsi_num;
@@ -231,9 +232,16 @@  ice_lag_cfg_fltr(struct ice_lag *lag, u32 act, u16 recipe_id, u16 *rule_idx,
 
 		act |= FIELD_PREP(ICE_SINGLE_ACT_VSI_ID_M, vsi_num);
 
-		s_rule->hdr.type = cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
 		s_rule->recipe_id = cpu_to_le16(recipe_id);
-		s_rule->src = cpu_to_le16(hw->port_info->lport);
+		if (direction == ICE_FLTR_RX) {
+			s_rule->hdr.type =
+				cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_RX);
+			s_rule->src = cpu_to_le16(hw->port_info->lport);
+		} else {
+			s_rule->hdr.type =
+				cpu_to_le16(ICE_AQC_SW_RULES_T_LKUP_TX);
+			s_rule->src = cpu_to_le16(vsi_num);
+		}
 		s_rule->act = cpu_to_le32(act);
 		s_rule->hdr_len = cpu_to_le16(DUMMY_ETH_HDR_LEN);
 		opc = ice_aqc_opc_add_sw_rules;
@@ -266,9 +274,22 @@  ice_lag_cfg_dflt_fltr(struct ice_lag *lag, bool add)
 {
 	u32 act = ICE_SINGLE_ACT_VSI_FORWARDING |
 		ICE_SINGLE_ACT_VALID_BIT | ICE_SINGLE_ACT_LAN_ENABLE;
+	int err;
+
+	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
+			       ICE_FLTR_RX, add);
+	if (err)
+		return err;
+
+	err = ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_tx_rule_id,
+			       ICE_FLTR_TX, add);
+	if (err) {
+		ice_lag_cfg_fltr(lag, act, lag->pf_recipe, &lag->pf_rx_rule_id,
+				 ICE_FLTR_RX, !add);
+		return err;
+	}
 
-	return ice_lag_cfg_fltr(lag, act, lag->pf_recipe,
-				&lag->pf_rule_id, add);
+	return 0;
 }
 
 /**
@@ -284,7 +305,7 @@  ice_lag_cfg_drop_fltr(struct ice_lag *lag, bool add)
 		  ICE_SINGLE_ACT_DROP;
 
 	return ice_lag_cfg_fltr(lag, act, lag->lport_recipe,
-				&lag->lport_rule_idx, add);
+				&lag->lport_rule_idx, ICE_FLTR_RX, add);
 }
 
 /**
@@ -310,7 +331,7 @@  ice_lag_cfg_pf_fltrs(struct ice_lag *lag, void *ptr)
 	dev = ice_pf_to_dev(lag->pf);
 
 	/* interface not active - remove old default VSI rule */
-	if (bonding_info->slave.state && lag->pf_rule_id) {
+	if (bonding_info->slave.state && lag->pf_rx_rule_id) {
 		if (ice_lag_cfg_dflt_fltr(lag, false))
 			dev_err(dev, "Error removing old default VSI filter\n");
 		if (ice_lag_cfg_drop_fltr(lag, true))
@@ -319,7 +340,7 @@  ice_lag_cfg_pf_fltrs(struct ice_lag *lag, void *ptr)
 	}
 
 	/* interface becoming active - add new default VSI rule */
-	if (!bonding_info->slave.state && !lag->pf_rule_id) {
+	if (!bonding_info->slave.state && !lag->pf_rx_rule_id) {
 		if (ice_lag_cfg_dflt_fltr(lag, true))
 			dev_err(dev, "Error adding new default VSI filter\n");
 		if (lag->lport_rule_idx && ice_lag_cfg_drop_fltr(lag, false))
@@ -2149,7 +2170,7 @@  void ice_lag_rebuild(struct ice_pf *pf)
 
 	ice_lag_cfg_cp_fltr(lag, true);
 
-	if (lag->pf_rule_id)
+	if (lag->pf_rx_rule_id)
 		if (ice_lag_cfg_dflt_fltr(lag, true))
 			dev_err(ice_pf_to_dev(pf), "Error adding default VSI rule in rebuild\n");
 
diff --git a/drivers/net/ethernet/intel/ice/ice_lag.h b/drivers/net/ethernet/intel/ice/ice_lag.h
index 183b38792ef2..bab2c83142a1 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.h
+++ b/drivers/net/ethernet/intel/ice/ice_lag.h
@@ -43,7 +43,8 @@  struct ice_lag {
 	u8 primary:1; /* this is primary */
 	u16 pf_recipe;
 	u16 lport_recipe;
-	u16 pf_rule_id;
+	u16 pf_rx_rule_id;
+	u16 pf_tx_rule_id;
 	u16 cp_rule_idx;
 	u16 lport_rule_idx;
 	u8 role;