* [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads
@ 2021-09-21 12:04 Wojciech Drewek
2021-09-21 13:00 ` Paul Menzel
0 siblings, 1 reply; 3+ messages in thread
From: Wojciech Drewek @ 2021-09-21 12:04 UTC (permalink / raw)
To: intel-wired-lan
Currently it is not possible to set/unset lb_en and lan_en flags
for advanced rules during their creation. Both flags are enabled
by default. In case of switchdev offloads for egress traffic we
need lb_en to be disabled. Because of that, we work around it by
updating the rule immediately after its creation.
This change allows us to set/unset those flags right away and it
gets rid of old workaround as well. Using ice_adv_rule_flags_info
structure we can pass info about flags we want to be set for
a given advanced rule. Flags are stored in flags_info.act.
Values from act would be used only if act_valid was set to true,
otherwise default values would be used.
Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
---
drivers/net/ethernet/intel/ice/ice_fltr.c | 127 --------------------
drivers/net/ethernet/intel/ice/ice_fltr.h | 4 -
drivers/net/ethernet/intel/ice/ice_switch.c | 9 +-
drivers/net/ethernet/intel/ice/ice_switch.h | 11 ++
drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 +-
5 files changed, 21 insertions(+), 138 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.c b/drivers/net/ethernet/intel/ice/ice_fltr.c
index f22ca4354f84..e49378893390 100644
--- a/drivers/net/ethernet/intel/ice/ice_fltr.c
+++ b/drivers/net/ethernet/intel/ice/ice_fltr.c
@@ -509,133 +509,6 @@ static u32 ice_fltr_build_action(u16 vsi_id)
ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_VALID_BIT;
}
-/**
- * ice_fltr_find_adv_entry - find advanced rule
- * @rules: list of rules
- * @rule_id: id of wanted rule
- */
-static struct ice_adv_fltr_mgmt_list_entry *
-ice_fltr_find_adv_entry(struct list_head *rules, u16 rule_id)
-{
- struct ice_adv_fltr_mgmt_list_entry *entry;
-
- list_for_each_entry(entry, rules, list_entry) {
- if (entry->rule_info.fltr_rule_id == rule_id)
- return entry;
- }
-
- return NULL;
-}
-
-/**
- * ice_fltr_update_adv_rule_flags - update flags on advanced rule
- * @vsi: pointer to VSI
- * @recipe_id: id of recipe
- * @entry: advanced rule entry
- * @new_flags: flags to update
- */
-static enum ice_status
-ice_fltr_update_adv_rule_flags(struct ice_vsi *vsi, u16 recipe_id,
- struct ice_adv_fltr_mgmt_list_entry *entry,
- u32 new_flags)
-{
- struct ice_adv_rule_info *info = &entry->rule_info;
- struct ice_sw_act_ctrl *act = &info->sw_act;
- u32 action;
-
- if (act->fltr_act != ICE_FWD_TO_VSI)
- return ICE_ERR_NOT_SUPPORTED;
-
- action = ice_fltr_build_action(act->fwd_id.hw_vsi_id);
-
- return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
- recipe_id, action, info->sw_act.flag,
- act->src, new_flags);
-}
-
-/**
- * ice_fltr_find_regular_entry - find regular rule
- * @rules: list of rules
- * @rule_id: id of wanted rule
- */
-static struct ice_fltr_mgmt_list_entry *
-ice_fltr_find_regular_entry(struct list_head *rules, u16 rule_id)
-{
- struct ice_fltr_mgmt_list_entry *entry;
-
- list_for_each_entry(entry, rules, list_entry) {
- if (entry->fltr_info.fltr_rule_id == rule_id)
- return entry;
- }
-
- return NULL;
-}
-
-/**
- * ice_fltr_update_regular_rule - update flags on regular rule
- * @vsi: pointer to VSI
- * @recipe_id: id of recipe
- * @entry: regular rule entry
- * @new_flags: flags to update
- */
-static enum ice_status
-ice_fltr_update_regular_rule(struct ice_vsi *vsi, u16 recipe_id,
- struct ice_fltr_mgmt_list_entry *entry,
- u32 new_flags)
-{
- struct ice_fltr_info *info = &entry->fltr_info;
- u32 action;
-
- if (info->fltr_act != ICE_FWD_TO_VSI)
- return ICE_ERR_NOT_SUPPORTED;
-
- action = ice_fltr_build_action(info->fwd_id.hw_vsi_id);
-
- return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
- recipe_id, action, info->flag,
- info->src, new_flags);
-}
-
-/**
- * ice_fltr_update_flags - update flags on rule
- * @vsi: pointer to VSI
- * @rule_id: id of rule
- * @recipe_id: id of recipe
- * @new_flags: flags to update
- *
- * Function updates flags on regular and advance rule.
- *
- * Flags should be a combination of ICE_SINGLE_ACT_LB_ENABLE and
- * ICE_SINGLE_ACT_LAN_ENABLE.
- */
-enum ice_status
-ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
- u32 new_flags)
-{
- struct ice_adv_fltr_mgmt_list_entry *adv_entry;
- struct ice_fltr_mgmt_list_entry *regular_entry;
- struct ice_hw *hw = &vsi->back->hw;
- struct ice_sw_recipe *recp_list;
- struct list_head *fltr_rules;
-
- recp_list = &hw->switch_info->recp_list[recipe_id];
- if (!recp_list)
- return ICE_ERR_DOES_NOT_EXIST;
-
- fltr_rules = &recp_list->filt_rules;
- regular_entry = ice_fltr_find_regular_entry(fltr_rules, rule_id);
- if (regular_entry)
- return ice_fltr_update_regular_rule(vsi, recipe_id,
- regular_entry, new_flags);
-
- adv_entry = ice_fltr_find_adv_entry(fltr_rules, rule_id);
- if (adv_entry)
- return ice_fltr_update_adv_rule_flags(vsi, recipe_id,
- adv_entry, new_flags);
-
- return ICE_ERR_DOES_NOT_EXIST;
-}
-
/**
* ice_fltr_update_flags_dflt_rule - update flags on default rule
* @vsi: pointer to VSI
diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.h b/drivers/net/ethernet/intel/ice/ice_fltr.h
index 339da5fef577..db59cbbeda32 100644
--- a/drivers/net/ethernet/intel/ice/ice_fltr.h
+++ b/drivers/net/ethernet/intel/ice/ice_fltr.h
@@ -46,10 +46,6 @@ enum ice_status
ice_fltr_remove_eth(struct ice_vsi *vsi, u16 ethertype, u16 flag,
enum ice_sw_fwd_act_type action);
void ice_fltr_remove_all(struct ice_vsi *vsi);
-
-enum ice_status
-ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
- u32 new_flags);
enum ice_status
ice_fltr_update_flags_dflt_rule(struct ice_vsi *vsi, u16 rule_id, u8 direction,
u32 new_flags);
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
index ca039a4c3a79..1bbb52bf2279 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.c
+++ b/drivers/net/ethernet/intel/ice/ice_switch.c
@@ -4841,7 +4841,14 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
s_rule = kzalloc(rule_buf_sz, GFP_KERNEL);
if (!s_rule)
return ICE_ERR_NO_MEMORY;
- act |= ICE_SINGLE_ACT_LB_ENABLE | ICE_SINGLE_ACT_LAN_ENABLE;
+ if (!rinfo->flags_info.act_valid) {
+ act |= ICE_SINGLE_ACT_LAN_ENABLE;
+ act |= ICE_SINGLE_ACT_LB_ENABLE;
+ } else {
+ act |= rinfo->flags_info.act & (ICE_SINGLE_ACT_LAN_ENABLE |
+ ICE_SINGLE_ACT_LB_ENABLE);
+ }
+
switch (rinfo->sw_act.fltr_act) {
case ICE_FWD_TO_VSI:
act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
index cd5421a41002..f8e7f0b41631 100644
--- a/drivers/net/ethernet/intel/ice/ice_switch.h
+++ b/drivers/net/ethernet/intel/ice/ice_switch.h
@@ -169,11 +169,22 @@ struct ice_rule_query_data {
u16 vsi_handle;
};
+/* This structure allows to pass info about lb_en and lan_en
+ * flags to ice_add_adv_rule. Values in act would be used
+ * only if act_valid was set to true, otherwise dflt
+ * values would be used.
+ */
+struct ice_adv_rule_flags_info {
+ u32 act;
+ u8 act_valid; /* indicate if flags in act are valid */
+};
+
struct ice_adv_rule_info {
struct ice_sw_act_ctrl sw_act;
u32 priority;
u8 rx; /* true means LOOKUP_RX otherwise LOOKUP_TX */
u16 fltr_rule_id;
+ struct ice_adv_rule_flags_info flags_info;
};
/* A collection of one or more four word recipe */
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index e01b16419923..5e132832044d 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -274,6 +274,8 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
rule_info.sw_act.flag |= ICE_FLTR_TX;
rule_info.sw_act.src = vsi->idx;
rule_info.rx = false;
+ rule_info.flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
+ rule_info.flags_info.act_valid = true;
}
/* specify the cookie as filter_rule_id */
@@ -296,12 +298,6 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
fltr->rid = rule_added.rid;
fltr->rule_id = rule_added.rule_id;
- if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
- if (ice_fltr_update_flags(vsi, fltr->rule_id, fltr->rid,
- ICE_SINGLE_ACT_LAN_ENABLE))
- ice_rem_adv_rule_by_id(hw, &rule_added);
- }
-
exit:
kfree(list);
return ret;
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads
2021-09-21 12:04 [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads Wojciech Drewek
@ 2021-09-21 13:00 ` Paul Menzel
2021-09-21 14:32 ` Drewek, Wojciech
0 siblings, 1 reply; 3+ messages in thread
From: Paul Menzel @ 2021-09-21 13:00 UTC (permalink / raw)
To: intel-wired-lan
Dear Wojciech,
Am 21.09.21 um 14:04 schrieb Wojciech Drewek:
> Currently it is not possible to set/unset lb_en and lan_en flags
> for advanced rules during their creation. Both flags are enabled
> by default. In case of switchdev offloads for egress traffic we
> need lb_en to be disabled. Because of that, we work around it by
> updating the rule immediately after its creation.
>
> This change allows us to set/unset those flags right away and it
> gets rid of old workaround as well. Using ice_adv_rule_flags_info
> structure we can pass info about flags we want to be set for
> a given advanced rule. Flags are stored in flags_info.act.
> Values from act would be used only if act_valid was set to true,
> otherwise default values would be used.
>
> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> ---
> drivers/net/ethernet/intel/ice/ice_fltr.c | 127 --------------------
> drivers/net/ethernet/intel/ice/ice_fltr.h | 4 -
> drivers/net/ethernet/intel/ice/ice_switch.c | 9 +-
> drivers/net/ethernet/intel/ice/ice_switch.h | 11 ++
> drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 +-
> 5 files changed, 21 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.c b/drivers/net/ethernet/intel/ice/ice_fltr.c
> index f22ca4354f84..e49378893390 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fltr.c
> +++ b/drivers/net/ethernet/intel/ice/ice_fltr.c
> @@ -509,133 +509,6 @@ static u32 ice_fltr_build_action(u16 vsi_id)
> ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_VALID_BIT;
> }
>
> -/**
> - * ice_fltr_find_adv_entry - find advanced rule
> - * @rules: list of rules
> - * @rule_id: id of wanted rule
> - */
> -static struct ice_adv_fltr_mgmt_list_entry *
> -ice_fltr_find_adv_entry(struct list_head *rules, u16 rule_id)
> -{
> - struct ice_adv_fltr_mgmt_list_entry *entry;
> -
> - list_for_each_entry(entry, rules, list_entry) {
> - if (entry->rule_info.fltr_rule_id == rule_id)
> - return entry;
> - }
> -
> - return NULL;
> -}
> -
> -/**
> - * ice_fltr_update_adv_rule_flags - update flags on advanced rule
> - * @vsi: pointer to VSI
> - * @recipe_id: id of recipe
> - * @entry: advanced rule entry
> - * @new_flags: flags to update
> - */
> -static enum ice_status
> -ice_fltr_update_adv_rule_flags(struct ice_vsi *vsi, u16 recipe_id,
> - struct ice_adv_fltr_mgmt_list_entry *entry,
> - u32 new_flags)
> -{
> - struct ice_adv_rule_info *info = &entry->rule_info;
> - struct ice_sw_act_ctrl *act = &info->sw_act;
> - u32 action;
> -
> - if (act->fltr_act != ICE_FWD_TO_VSI)
> - return ICE_ERR_NOT_SUPPORTED;
> -
> - action = ice_fltr_build_action(act->fwd_id.hw_vsi_id);
> -
> - return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
> - recipe_id, action, info->sw_act.flag,
> - act->src, new_flags);
> -}
> -
> -/**
> - * ice_fltr_find_regular_entry - find regular rule
> - * @rules: list of rules
> - * @rule_id: id of wanted rule
> - */
> -static struct ice_fltr_mgmt_list_entry *
> -ice_fltr_find_regular_entry(struct list_head *rules, u16 rule_id)
> -{
> - struct ice_fltr_mgmt_list_entry *entry;
> -
> - list_for_each_entry(entry, rules, list_entry) {
> - if (entry->fltr_info.fltr_rule_id == rule_id)
> - return entry;
> - }
> -
> - return NULL;
> -}
> -
> -/**
> - * ice_fltr_update_regular_rule - update flags on regular rule
> - * @vsi: pointer to VSI
> - * @recipe_id: id of recipe
> - * @entry: regular rule entry
> - * @new_flags: flags to update
> - */
> -static enum ice_status
> -ice_fltr_update_regular_rule(struct ice_vsi *vsi, u16 recipe_id,
> - struct ice_fltr_mgmt_list_entry *entry,
> - u32 new_flags)
> -{
> - struct ice_fltr_info *info = &entry->fltr_info;
> - u32 action;
> -
> - if (info->fltr_act != ICE_FWD_TO_VSI)
> - return ICE_ERR_NOT_SUPPORTED;
> -
> - action = ice_fltr_build_action(info->fwd_id.hw_vsi_id);
> -
> - return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
> - recipe_id, action, info->flag,
> - info->src, new_flags);
> -}
> -
> -/**
> - * ice_fltr_update_flags - update flags on rule
> - * @vsi: pointer to VSI
> - * @rule_id: id of rule
> - * @recipe_id: id of recipe
> - * @new_flags: flags to update
> - *
> - * Function updates flags on regular and advance rule.
> - *
> - * Flags should be a combination of ICE_SINGLE_ACT_LB_ENABLE and
> - * ICE_SINGLE_ACT_LAN_ENABLE.
> - */
> -enum ice_status
> -ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
> - u32 new_flags)
> -{
> - struct ice_adv_fltr_mgmt_list_entry *adv_entry;
> - struct ice_fltr_mgmt_list_entry *regular_entry;
> - struct ice_hw *hw = &vsi->back->hw;
> - struct ice_sw_recipe *recp_list;
> - struct list_head *fltr_rules;
> -
> - recp_list = &hw->switch_info->recp_list[recipe_id];
> - if (!recp_list)
> - return ICE_ERR_DOES_NOT_EXIST;
> -
> - fltr_rules = &recp_list->filt_rules;
> - regular_entry = ice_fltr_find_regular_entry(fltr_rules, rule_id);
> - if (regular_entry)
> - return ice_fltr_update_regular_rule(vsi, recipe_id,
> - regular_entry, new_flags);
> -
> - adv_entry = ice_fltr_find_adv_entry(fltr_rules, rule_id);
> - if (adv_entry)
> - return ice_fltr_update_adv_rule_flags(vsi, recipe_id,
> - adv_entry, new_flags);
> -
> - return ICE_ERR_DOES_NOT_EXIST;
> -}
> -
> /**
> * ice_fltr_update_flags_dflt_rule - update flags on default rule
> * @vsi: pointer to VSI
> diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.h b/drivers/net/ethernet/intel/ice/ice_fltr.h
> index 339da5fef577..db59cbbeda32 100644
> --- a/drivers/net/ethernet/intel/ice/ice_fltr.h
> +++ b/drivers/net/ethernet/intel/ice/ice_fltr.h
> @@ -46,10 +46,6 @@ enum ice_status
> ice_fltr_remove_eth(struct ice_vsi *vsi, u16 ethertype, u16 flag,
> enum ice_sw_fwd_act_type action);
> void ice_fltr_remove_all(struct ice_vsi *vsi);
> -
> -enum ice_status
> -ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
> - u32 new_flags);
> enum ice_status
> ice_fltr_update_flags_dflt_rule(struct ice_vsi *vsi, u16 rule_id, u8 direction,
> u32 new_flags);
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> index ca039a4c3a79..1bbb52bf2279 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> @@ -4841,7 +4841,14 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> s_rule = kzalloc(rule_buf_sz, GFP_KERNEL);
> if (!s_rule)
> return ICE_ERR_NO_MEMORY;
> - act |= ICE_SINGLE_ACT_LB_ENABLE | ICE_SINGLE_ACT_LAN_ENABLE;
> + if (!rinfo->flags_info.act_valid) {
> + act |= ICE_SINGLE_ACT_LAN_ENABLE;
> + act |= ICE_SINGLE_ACT_LB_ENABLE;
> + } else {
> + act |= rinfo->flags_info.act & (ICE_SINGLE_ACT_LAN_ENABLE |
> + ICE_SINGLE_ACT_LB_ENABLE);
> + }
> +
It?s all subjective, but a temporary variable could be used:
u32 flags = ICE_SINGLE_ACT_LAN_ENABLE | ICE_SINGLE_ACT_LB_ENABLE;
if (rinfo->flags_info.act_valid)
flags &= rinfo->flags_info.act;
act |= flags
But not that important, I guess.
> switch (rinfo->sw_act.fltr_act) {
> case ICE_FWD_TO_VSI:
> act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
> diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> index cd5421a41002..f8e7f0b41631 100644
> --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> @@ -169,11 +169,22 @@ struct ice_rule_query_data {
> u16 vsi_handle;
> };
>
> +/* This structure allows to pass info about lb_en and lan_en
> + * flags to ice_add_adv_rule. Values in act would be used
> + * only if act_valid was set to true, otherwise dflt
Maybe you missed it, in my first reply, but I?d go with ?default? comments.
> + * values would be used.
> + */
> +struct ice_adv_rule_flags_info {
> + u32 act;
> + u8 act_valid; /* indicate if flags in act are valid */
> +};
> +
> struct ice_adv_rule_info {
> struct ice_sw_act_ctrl sw_act;
> u32 priority;
> u8 rx; /* true means LOOKUP_RX otherwise LOOKUP_TX */
> u16 fltr_rule_id;
> + struct ice_adv_rule_flags_info flags_info;
> };
>
> /* A collection of one or more four word recipe */
> diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> index e01b16419923..5e132832044d 100644
> --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> @@ -274,6 +274,8 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> rule_info.sw_act.flag |= ICE_FLTR_TX;
> rule_info.sw_act.src = vsi->idx;
> rule_info.rx = false;
> + rule_info.flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
> + rule_info.flags_info.act_valid = true;
> }
>
> /* specify the cookie as filter_rule_id */
> @@ -296,12 +298,6 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> fltr->rid = rule_added.rid;
> fltr->rule_id = rule_added.rule_id;
>
> - if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> - if (ice_fltr_update_flags(vsi, fltr->rule_id, fltr->rid,
> - ICE_SINGLE_ACT_LAN_ENABLE))
> - ice_rem_adv_rule_by_id(hw, &rule_added);
> - }
> -
> exit:
> kfree(list);
> return ret;
>
Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads
2021-09-21 13:00 ` Paul Menzel
@ 2021-09-21 14:32 ` Drewek, Wojciech
0 siblings, 0 replies; 3+ messages in thread
From: Drewek, Wojciech @ 2021-09-21 14:32 UTC (permalink / raw)
To: intel-wired-lan
Hi Paul
> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: wtorek, 21 wrze?nia 2021 15:00
> To: Drewek, Wojciech <wojciech.drewek@intel.com>
> Cc: intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads
>
> Dear Wojciech,
>
>
> Am 21.09.21 um 14:04 schrieb Wojciech Drewek:
> > Currently it is not possible to set/unset lb_en and lan_en flags
> > for advanced rules during their creation. Both flags are enabled
> > by default. In case of switchdev offloads for egress traffic we
> > need lb_en to be disabled. Because of that, we work around it by
> > updating the rule immediately after its creation.
> >
> > This change allows us to set/unset those flags right away and it
> > gets rid of old workaround as well. Using ice_adv_rule_flags_info
> > structure we can pass info about flags we want to be set for
> > a given advanced rule. Flags are stored in flags_info.act.
> > Values from act would be used only if act_valid was set to true,
> > otherwise default values would be used.
> >
> > Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> > ---
> > drivers/net/ethernet/intel/ice/ice_fltr.c | 127 --------------------
> > drivers/net/ethernet/intel/ice/ice_fltr.h | 4 -
> > drivers/net/ethernet/intel/ice/ice_switch.c | 9 +-
> > drivers/net/ethernet/intel/ice/ice_switch.h | 11 ++
> > drivers/net/ethernet/intel/ice/ice_tc_lib.c | 8 +-
> > 5 files changed, 21 insertions(+), 138 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.c b/drivers/net/ethernet/intel/ice/ice_fltr.c
> > index f22ca4354f84..e49378893390 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_fltr.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_fltr.c
> > @@ -509,133 +509,6 @@ static u32 ice_fltr_build_action(u16 vsi_id)
> > ICE_SINGLE_ACT_VSI_FORWARDING | ICE_SINGLE_ACT_VALID_BIT;
> > }
> >
> > -/**
> > - * ice_fltr_find_adv_entry - find advanced rule
> > - * @rules: list of rules
> > - * @rule_id: id of wanted rule
> > - */
> > -static struct ice_adv_fltr_mgmt_list_entry *
> > -ice_fltr_find_adv_entry(struct list_head *rules, u16 rule_id)
> > -{
> > - struct ice_adv_fltr_mgmt_list_entry *entry;
> > -
> > - list_for_each_entry(entry, rules, list_entry) {
> > - if (entry->rule_info.fltr_rule_id == rule_id)
> > - return entry;
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > -/**
> > - * ice_fltr_update_adv_rule_flags - update flags on advanced rule
> > - * @vsi: pointer to VSI
> > - * @recipe_id: id of recipe
> > - * @entry: advanced rule entry
> > - * @new_flags: flags to update
> > - */
> > -static enum ice_status
> > -ice_fltr_update_adv_rule_flags(struct ice_vsi *vsi, u16 recipe_id,
> > - struct ice_adv_fltr_mgmt_list_entry *entry,
> > - u32 new_flags)
> > -{
> > - struct ice_adv_rule_info *info = &entry->rule_info;
> > - struct ice_sw_act_ctrl *act = &info->sw_act;
> > - u32 action;
> > -
> > - if (act->fltr_act != ICE_FWD_TO_VSI)
> > - return ICE_ERR_NOT_SUPPORTED;
> > -
> > - action = ice_fltr_build_action(act->fwd_id.hw_vsi_id);
> > -
> > - return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
> > - recipe_id, action, info->sw_act.flag,
> > - act->src, new_flags);
> > -}
> > -
> > -/**
> > - * ice_fltr_find_regular_entry - find regular rule
> > - * @rules: list of rules
> > - * @rule_id: id of wanted rule
> > - */
> > -static struct ice_fltr_mgmt_list_entry *
> > -ice_fltr_find_regular_entry(struct list_head *rules, u16 rule_id)
> > -{
> > - struct ice_fltr_mgmt_list_entry *entry;
> > -
> > - list_for_each_entry(entry, rules, list_entry) {
> > - if (entry->fltr_info.fltr_rule_id == rule_id)
> > - return entry;
> > - }
> > -
> > - return NULL;
> > -}
> > -
> > -/**
> > - * ice_fltr_update_regular_rule - update flags on regular rule
> > - * @vsi: pointer to VSI
> > - * @recipe_id: id of recipe
> > - * @entry: regular rule entry
> > - * @new_flags: flags to update
> > - */
> > -static enum ice_status
> > -ice_fltr_update_regular_rule(struct ice_vsi *vsi, u16 recipe_id,
> > - struct ice_fltr_mgmt_list_entry *entry,
> > - u32 new_flags)
> > -{
> > - struct ice_fltr_info *info = &entry->fltr_info;
> > - u32 action;
> > -
> > - if (info->fltr_act != ICE_FWD_TO_VSI)
> > - return ICE_ERR_NOT_SUPPORTED;
> > -
> > - action = ice_fltr_build_action(info->fwd_id.hw_vsi_id);
> > -
> > - return ice_fltr_update_rule_flags(&vsi->back->hw, info->fltr_rule_id,
> > - recipe_id, action, info->flag,
> > - info->src, new_flags);
> > -}
> > -
> > -/**
> > - * ice_fltr_update_flags - update flags on rule
> > - * @vsi: pointer to VSI
> > - * @rule_id: id of rule
> > - * @recipe_id: id of recipe
> > - * @new_flags: flags to update
> > - *
> > - * Function updates flags on regular and advance rule.
> > - *
> > - * Flags should be a combination of ICE_SINGLE_ACT_LB_ENABLE and
> > - * ICE_SINGLE_ACT_LAN_ENABLE.
> > - */
> > -enum ice_status
> > -ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
> > - u32 new_flags)
> > -{
> > - struct ice_adv_fltr_mgmt_list_entry *adv_entry;
> > - struct ice_fltr_mgmt_list_entry *regular_entry;
> > - struct ice_hw *hw = &vsi->back->hw;
> > - struct ice_sw_recipe *recp_list;
> > - struct list_head *fltr_rules;
> > -
> > - recp_list = &hw->switch_info->recp_list[recipe_id];
> > - if (!recp_list)
> > - return ICE_ERR_DOES_NOT_EXIST;
> > -
> > - fltr_rules = &recp_list->filt_rules;
> > - regular_entry = ice_fltr_find_regular_entry(fltr_rules, rule_id);
> > - if (regular_entry)
> > - return ice_fltr_update_regular_rule(vsi, recipe_id,
> > - regular_entry, new_flags);
> > -
> > - adv_entry = ice_fltr_find_adv_entry(fltr_rules, rule_id);
> > - if (adv_entry)
> > - return ice_fltr_update_adv_rule_flags(vsi, recipe_id,
> > - adv_entry, new_flags);
> > -
> > - return ICE_ERR_DOES_NOT_EXIST;
> > -}
> > -
> > /**
> > * ice_fltr_update_flags_dflt_rule - update flags on default rule
> > * @vsi: pointer to VSI
> > diff --git a/drivers/net/ethernet/intel/ice/ice_fltr.h b/drivers/net/ethernet/intel/ice/ice_fltr.h
> > index 339da5fef577..db59cbbeda32 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_fltr.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_fltr.h
> > @@ -46,10 +46,6 @@ enum ice_status
> > ice_fltr_remove_eth(struct ice_vsi *vsi, u16 ethertype, u16 flag,
> > enum ice_sw_fwd_act_type action);
> > void ice_fltr_remove_all(struct ice_vsi *vsi);
> > -
> > -enum ice_status
> > -ice_fltr_update_flags(struct ice_vsi *vsi, u16 rule_id, u16 recipe_id,
> > - u32 new_flags);
> > enum ice_status
> > ice_fltr_update_flags_dflt_rule(struct ice_vsi *vsi, u16 rule_id, u8 direction,
> > u32 new_flags);
> > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c b/drivers/net/ethernet/intel/ice/ice_switch.c
> > index ca039a4c3a79..1bbb52bf2279 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c
> > @@ -4841,7 +4841,14 @@ ice_add_adv_rule(struct ice_hw *hw, struct ice_adv_lkup_elem *lkups,
> > s_rule = kzalloc(rule_buf_sz, GFP_KERNEL);
> > if (!s_rule)
> > return ICE_ERR_NO_MEMORY;
> > - act |= ICE_SINGLE_ACT_LB_ENABLE | ICE_SINGLE_ACT_LAN_ENABLE;
> > + if (!rinfo->flags_info.act_valid) {
> > + act |= ICE_SINGLE_ACT_LAN_ENABLE;
> > + act |= ICE_SINGLE_ACT_LB_ENABLE;
> > + } else {
> > + act |= rinfo->flags_info.act & (ICE_SINGLE_ACT_LAN_ENABLE |
> > + ICE_SINGLE_ACT_LB_ENABLE);
> > + }
> > +
>
> It?s all subjective, but a temporary variable could be used:
>
> u32 flags = ICE_SINGLE_ACT_LAN_ENABLE | ICE_SINGLE_ACT_LB_ENABLE;
> if (rinfo->flags_info.act_valid)
> flags &= rinfo->flags_info.act;
>
> act |= flags
>
> But not that important, I guess.
Thx for the suggestion, will take a look on it.
>
> > switch (rinfo->sw_act.fltr_act) {
> > case ICE_FWD_TO_VSI:
> > act |= (rinfo->sw_act.fwd_id.hw_vsi_id <<
> > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.h b/drivers/net/ethernet/intel/ice/ice_switch.h
> > index cd5421a41002..f8e7f0b41631 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_switch.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_switch.h
> > @@ -169,11 +169,22 @@ struct ice_rule_query_data {
> > u16 vsi_handle;
> > };
> >
> > +/* This structure allows to pass info about lb_en and lan_en
> > + * flags to ice_add_adv_rule. Values in act would be used
> > + * only if act_valid was set to true, otherwise dflt
>
> Maybe you missed it, in my first reply, but I?d go with ?default? comments.
Indeed I missed that one.
>
> > + * values would be used.
> > + */
> > +struct ice_adv_rule_flags_info {
> > + u32 act;
> > + u8 act_valid; /* indicate if flags in act are valid */
> > +};
> > +
> > struct ice_adv_rule_info {
> > struct ice_sw_act_ctrl sw_act;
> > u32 priority;
> > u8 rx; /* true means LOOKUP_RX otherwise LOOKUP_TX */
> > u16 fltr_rule_id;
> > + struct ice_adv_rule_flags_info flags_info;
> > };
> >
> > /* A collection of one or more four word recipe */
> > diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > index e01b16419923..5e132832044d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
> > @@ -274,6 +274,8 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> > rule_info.sw_act.flag |= ICE_FLTR_TX;
> > rule_info.sw_act.src = vsi->idx;
> > rule_info.rx = false;
> > + rule_info.flags_info.act = ICE_SINGLE_ACT_LAN_ENABLE;
> > + rule_info.flags_info.act_valid = true;
> > }
> >
> > /* specify the cookie as filter_rule_id */
> > @@ -296,12 +298,6 @@ ice_eswitch_add_tc_fltr(struct ice_vsi *vsi, struct ice_tc_flower_fltr *fltr)
> > fltr->rid = rule_added.rid;
> > fltr->rule_id = rule_added.rule_id;
> >
> > - if (fltr->direction == ICE_ESWITCH_FLTR_EGRESS) {
> > - if (ice_fltr_update_flags(vsi, fltr->rule_id, fltr->rid,
> > - ICE_SINGLE_ACT_LAN_ENABLE))
> > - ice_rem_adv_rule_by_id(hw, &rule_added);
> > - }
> > -
> > exit:
> > kfree(list);
> > return ret;
> >
>
> Acked-by: Paul Menzel <pmenzel@molgen.mpg.de>
>
>
> Kind regards,
>
> Paul
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-21 14:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 12:04 [Intel-wired-lan] [PATCH net-next v2] ice: Manage act flags for switchdev offloads Wojciech Drewek
2021-09-21 13:00 ` Paul Menzel
2021-09-21 14:32 ` Drewek, Wojciech
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.