Checks
Context | Check | Description |
---|---|---|
ci/checkpatch | success | coding style OK |
ci/Intel-compilation | fail | Compilation issues |
Commit Message
Leyi Rong
June 4, 2019, 5:42 a.m. UTC
Code added to replay the advanced rule per VSI basis and remove the advanced rule information from shared code recipe list. Signed-off-by: Victor Raj <victor.raj@intel.com> Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com> Signed-off-by: Leyi Rong <leyi.rong@intel.com> --- drivers/net/ice/base/ice_switch.c | 81 ++++++++++++++++++++++++++----- 1 file changed, 69 insertions(+), 12 deletions(-)
Comments
On 6/4/19 7:42 AM, Leyi Rong wrote: > Code added to replay the advanced rule per VSI basis and remove the > advanced rule information from shared code recipe list. > > Signed-off-by: Victor Raj <victor.raj@intel.com> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com> > Signed-off-by: Leyi Rong <leyi.rong@intel.com> > --- > drivers/net/ice/base/ice_switch.c | 81 ++++++++++++++++++++++++++----- > 1 file changed, 69 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ice/base/ice_switch.c b/drivers/net/ice/base/ice_switch.c > index c53021aed..ca0497ca7 100644 > --- a/drivers/net/ice/base/ice_switch.c > +++ b/drivers/net/ice/base/ice_switch.c > @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, struct LIST_HEAD_TYPE *rule_head) > } > } > > +/** > + * ice_rem_adv_rule_info > + * @hw: pointer to the hardware structure > + * @rule_head: pointer to the switch list structure that we want to delete > + */ > +static void > +ice_rem_adv_rule_info(struct ice_hw *hw, struct LIST_HEAD_TYPE *rule_head) > +{ > + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; > + struct ice_adv_fltr_mgmt_list_entry *lst_itr; > + > + if (LIST_EMPTY(rule_head)) > + return; Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not loop and status will be returned: #define LIST_FOR_EACH_ENTRY_SAFE(pos, tmp, head, type, member) \ LIST_FOR_EACH_ENTRY(pos, head, type, member) with: #define LIST_FOR_EACH_ENTRY(pos, head, type, member) \ for ((pos) = (head)->lh_first ? \ container_of((head)->lh_first, struct type, member) : \ 0; \ (pos); \ (pos) = (pos)->member.next.le_next ? \ container_of((pos)->member.next.le_next, struct type, \ member) : \ 0) > + LIST_FOR_EACH_ENTRY_SAFE(lst_itr, tmp_entry, rule_head, > + ice_adv_fltr_mgmt_list_entry, list_entry) { > + LIST_DEL(&lst_itr->list_entry); > + ice_free(hw, lst_itr->lkups); > + ice_free(hw, lst_itr); > + } > +} > > /** > * ice_rem_all_sw_rules_info > @@ -3049,6 +3070,8 @@ void ice_rem_all_sw_rules_info(struct ice_hw *hw) > rule_head = &sw->recp_list[i].filt_rules; > if (!sw->recp_list[i].adv_rule) > ice_rem_sw_rule_info(hw, rule_head); > + else > + ice_rem_adv_rule_info(hw, rule_head); > } > } > > @@ -5687,6 +5710,38 @@ ice_replay_vsi_fltr(struct ice_hw *hw, u16 vsi_handle, u8 recp_id, > return status; > } > > +/** > + * ice_replay_vsi_adv_rule - Replay advanced rule for requested VSI > + * @hw: pointer to the hardware structure > + * @vsi_handle: driver VSI handle > + * @list_head: list for which filters need to be replayed > + * > + * Replay the advanced rule for the given VSI. > + */ > +static enum ice_status > +ice_replay_vsi_adv_rule(struct ice_hw *hw, u16 vsi_handle, > + struct LIST_HEAD_TYPE *list_head) > +{ > + struct ice_rule_query_data added_entry = { 0 }; > + struct ice_adv_fltr_mgmt_list_entry *adv_fltr; > + enum ice_status status = ICE_SUCCESS; > + > + if (LIST_EMPTY(list_head)) > + return status; Ditto, it should be removed. > + LIST_FOR_EACH_ENTRY(adv_fltr, list_head, ice_adv_fltr_mgmt_list_entry, > + list_entry) { > + struct ice_adv_rule_info *rinfo = &adv_fltr->rule_info; > + u16 lk_cnt = adv_fltr->lkups_cnt; > + > + if (vsi_handle != rinfo->sw_act.vsi_handle) > + continue; > + status = ice_add_adv_rule(hw, adv_fltr->lkups, lk_cnt, rinfo, > + &added_entry); > + if (status) > + break; > + } > + return status; > +} > > /** > * ice_replay_vsi_all_fltr - replay all filters stored in bookkeeping lists > @@ -5698,23 +5753,23 @@ ice_replay_vsi_fltr(struct ice_hw *hw, u16 vsi_handle, u8 recp_id, > enum ice_status ice_replay_vsi_all_fltr(struct ice_hw *hw, u16 vsi_handle) > { > struct ice_switch_info *sw = hw->switch_info; > - enum ice_status status = ICE_SUCCESS; > + enum ice_status status; > u8 i; > > + /* Update the recipes that were created */ > for (i = 0; i < ICE_MAX_NUM_RECIPES; i++) { > - /* Update the default recipe lines and ones that were created */ > - if (i < ICE_MAX_NUM_RECIPES || sw->recp_list[i].recp_created) { > - struct LIST_HEAD_TYPE *head; > + struct LIST_HEAD_TYPE *head; > > - head = &sw->recp_list[i].filt_replay_rules; > - if (!sw->recp_list[i].adv_rule) > - status = ice_replay_vsi_fltr(hw, vsi_handle, i, > - head); > - if (status != ICE_SUCCESS) > - return status; > - } > + head = &sw->recp_list[i].filt_replay_rules; > + if (!sw->recp_list[i].adv_rule) > + status = ice_replay_vsi_fltr(hw, vsi_handle, i, head); > + else > + status = ice_replay_vsi_adv_rule(hw, vsi_handle, head); > + if (status != ICE_SUCCESS) > + return status; > } > - return status; > + > + return ICE_SUCCESS; > } > > /** > @@ -5738,6 +5793,8 @@ void ice_rm_all_sw_replay_rule_info(struct ice_hw *hw) > l_head = &sw->recp_list[i].filt_replay_rules; > if (!sw->recp_list[i].adv_rule) > ice_rem_sw_rule_info(hw, l_head); > + else > + ice_rem_adv_rule_info(hw, l_head); > } > } > } >
> -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, June 5, 2019 1:59 AM > To: Rong, Leyi <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > Cc: dev@dpdk.org; Raj, Victor <victor.raj@intel.com>; Stillwell Jr, Paul M > <paul.m.stillwell.jr@intel.com> > Subject: Re: [dpdk-dev] [PATCH 07/49] net/ice/base: replay advanced rule > after reset > > > > On 6/4/19 7:42 AM, Leyi Rong wrote: > > Code added to replay the advanced rule per VSI basis and remove the > > advanced rule information from shared code recipe list. > > > > Signed-off-by: Victor Raj <victor.raj@intel.com> > > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell.jr@intel.com> > > Signed-off-by: Leyi Rong <leyi.rong@intel.com> > > --- > > drivers/net/ice/base/ice_switch.c | 81 ++++++++++++++++++++++++++- > ---- > > 1 file changed, 69 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ice/base/ice_switch.c > > b/drivers/net/ice/base/ice_switch.c > > index c53021aed..ca0497ca7 100644 > > --- a/drivers/net/ice/base/ice_switch.c > > +++ b/drivers/net/ice/base/ice_switch.c > > @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, > struct LIST_HEAD_TYPE *rule_head) > > } > > } > > > > +/** > > + * ice_rem_adv_rule_info > > + * @hw: pointer to the hardware structure > > + * @rule_head: pointer to the switch list structure that we want to > > +delete */ static void ice_rem_adv_rule_info(struct ice_hw *hw, > > +struct LIST_HEAD_TYPE *rule_head) { > > + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; > > + struct ice_adv_fltr_mgmt_list_entry *lst_itr; > > + > > + if (LIST_EMPTY(rule_head)) > > + return; > > > Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not loop and > status will be returned: > Yes, the check is necessary. This code gets consumed by multiple different OSs and not all OSs implement the LIST_FOR_EACH_ENTRY_SAFE in the way that DPDK did. For example, if I'm understanding the Linux code correctly, the list_for_each_entry_safe code in Linux would not work correctly without checking LIST_EMPTY since the Linux implementation doesn't have a check for null in it's implementation of list_for_each_entry_safe. So what the DPDK code should be doing is something more like the Linux implementation (since it is the most restrictive use) and not check for null in LIST_FOR_EACH_ENTRY_SAFE and assume the list is not empty, thus the check would be necessary in DPDK also. The change to the DPDK implementation of LIST_FOR_EACH_ENTRY_SAFE would need to be a separate patch though so I think for now we should leave this code and work on a patch to change the DPDK implementation. > #define LIST_FOR_EACH_ENTRY_SAFE(pos, tmp, head, type, member) \ > LIST_FOR_EACH_ENTRY(pos, head, type, member) > > with: > > #define LIST_FOR_EACH_ENTRY(pos, head, type, member) > \ > for ((pos) = (head)->lh_first ? \ > container_of((head)->lh_first, struct type, member) : \ > 0; \ > (pos); \ > (pos) = (pos)->member.next.le_next ? \ > container_of((pos)->member.next.le_next, struct type, > \ > member) : \ > 0) > > > + LIST_FOR_EACH_ENTRY_SAFE(lst_itr, tmp_entry, rule_head, > > > + ice_adv_fltr_mgmt_list_entry, list_entry) { > > + LIST_DEL(&lst_itr->list_entry); > > + ice_free(hw, lst_itr->lkups); > > + ice_free(hw, lst_itr); > > + } > > +} > > > > /** > > * ice_rem_all_sw_rules_info > > @@ -3049,6 +3070,8 @@ void ice_rem_all_sw_rules_info(struct ice_hw > *hw) > > rule_head = &sw->recp_list[i].filt_rules; > > if (!sw->recp_list[i].adv_rule) > > ice_rem_sw_rule_info(hw, rule_head); > > + else > > + ice_rem_adv_rule_info(hw, rule_head); > > } > > } > > > > @@ -5687,6 +5710,38 @@ ice_replay_vsi_fltr(struct ice_hw *hw, u16 > vsi_handle, u8 recp_id, > > return status; > > } > > > > +/** > > + * ice_replay_vsi_adv_rule - Replay advanced rule for requested VSI > > + * @hw: pointer to the hardware structure > > + * @vsi_handle: driver VSI handle > > + * @list_head: list for which filters need to be replayed > > + * > > + * Replay the advanced rule for the given VSI. > > + */ > > +static enum ice_status > > +ice_replay_vsi_adv_rule(struct ice_hw *hw, u16 vsi_handle, > > + struct LIST_HEAD_TYPE *list_head) > > +{ > > + struct ice_rule_query_data added_entry = { 0 }; > > + struct ice_adv_fltr_mgmt_list_entry *adv_fltr; > > + enum ice_status status = ICE_SUCCESS; > > + > > + if (LIST_EMPTY(list_head)) > > + return status; > > Ditto, it should be removed. > > > + LIST_FOR_EACH_ENTRY(adv_fltr, list_head, > ice_adv_fltr_mgmt_list_entry, > > + list_entry) { > > + struct ice_adv_rule_info *rinfo = &adv_fltr->rule_info; > > + u16 lk_cnt = adv_fltr->lkups_cnt; > > + > > + if (vsi_handle != rinfo->sw_act.vsi_handle) > > + continue; > > + status = ice_add_adv_rule(hw, adv_fltr->lkups, lk_cnt, rinfo, > > + &added_entry); > > + if (status) > > + break; > > + } > > + return status; > > +} > > > > /** > > * ice_replay_vsi_all_fltr - replay all filters stored in > > bookkeeping lists @@ -5698,23 +5753,23 @@ ice_replay_vsi_fltr(struct > ice_hw *hw, u16 vsi_handle, u8 recp_id, > > enum ice_status ice_replay_vsi_all_fltr(struct ice_hw *hw, u16 > vsi_handle) > > { > > struct ice_switch_info *sw = hw->switch_info; > > - enum ice_status status = ICE_SUCCESS; > > + enum ice_status status; > > u8 i; > > > > + /* Update the recipes that were created */ > > for (i = 0; i < ICE_MAX_NUM_RECIPES; i++) { > > - /* Update the default recipe lines and ones that were > created */ > > - if (i < ICE_MAX_NUM_RECIPES || sw- > >recp_list[i].recp_created) { > > - struct LIST_HEAD_TYPE *head; > > + struct LIST_HEAD_TYPE *head; > > > > - head = &sw->recp_list[i].filt_replay_rules; > > - if (!sw->recp_list[i].adv_rule) > > - status = ice_replay_vsi_fltr(hw, vsi_handle, i, > > - head); > > - if (status != ICE_SUCCESS) > > - return status; > > - } > > + head = &sw->recp_list[i].filt_replay_rules; > > + if (!sw->recp_list[i].adv_rule) > > + status = ice_replay_vsi_fltr(hw, vsi_handle, i, head); > > + else > > + status = ice_replay_vsi_adv_rule(hw, vsi_handle, > head); > > + if (status != ICE_SUCCESS) > > + return status; > > } > > - return status; > > + > > + return ICE_SUCCESS; > > } > > > > /** > > @@ -5738,6 +5793,8 @@ void ice_rm_all_sw_replay_rule_info(struct > ice_hw *hw) > > l_head = &sw->recp_list[i].filt_replay_rules; > > if (!sw->recp_list[i].adv_rule) > > ice_rem_sw_rule_info(hw, l_head); > > + else > > + ice_rem_adv_rule_info(hw, l_head); > > } > > } > > } > >
On 6/5/19 5:53 PM, Stillwell Jr, Paul M wrote: >>> diff --git a/drivers/net/ice/base/ice_switch.c >>> b/drivers/net/ice/base/ice_switch.c >>> index c53021aed..ca0497ca7 100644 >>> --- a/drivers/net/ice/base/ice_switch.c >>> +++ b/drivers/net/ice/base/ice_switch.c >>> @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, >> struct LIST_HEAD_TYPE *rule_head) >>> } >>> } >>> >>> +/** >>> + * ice_rem_adv_rule_info >>> + * @hw: pointer to the hardware structure >>> + * @rule_head: pointer to the switch list structure that we want to >>> +delete */ static void ice_rem_adv_rule_info(struct ice_hw *hw, >>> +struct LIST_HEAD_TYPE *rule_head) { >>> + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; >>> + struct ice_adv_fltr_mgmt_list_entry *lst_itr; >>> + >>> + if (LIST_EMPTY(rule_head)) >>> + return; >> >> Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not loop and >> status will be returned: >> > Yes, the check is necessary. This code gets consumed by multiple different OSs and not all OSs implement the LIST_FOR_EACH_ENTRY_SAFE in the way that DPDK did. For example, if I'm understanding the Linux code correctly, the list_for_each_entry_safe code in Linux would not work correctly without checking LIST_EMPTY since the Linux implementation doesn't have a check for null in it's implementation of list_for_each_entry_safe. Do you mean the same patch is upstreamed into Linux Kernel without any adaptations?
> -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, June 5, 2019 8:59 AM > To: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>; Rong, Leyi > <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > Cc: dev@dpdk.org; Raj, Victor <victor.raj@intel.com> > Subject: Re: [dpdk-dev] [PATCH 07/49] net/ice/base: replay advanced rule > after reset > > > > On 6/5/19 5:53 PM, Stillwell Jr, Paul M wrote: > >>> diff --git a/drivers/net/ice/base/ice_switch.c > >>> b/drivers/net/ice/base/ice_switch.c > >>> index c53021aed..ca0497ca7 100644 > >>> --- a/drivers/net/ice/base/ice_switch.c > >>> +++ b/drivers/net/ice/base/ice_switch.c > >>> @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, > >> struct LIST_HEAD_TYPE *rule_head) > >>> } > >>> } > >>> > >>> +/** > >>> + * ice_rem_adv_rule_info > >>> + * @hw: pointer to the hardware structure > >>> + * @rule_head: pointer to the switch list structure that we want to > >>> +delete */ static void ice_rem_adv_rule_info(struct ice_hw *hw, > >>> +struct LIST_HEAD_TYPE *rule_head) { > >>> + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; > >>> + struct ice_adv_fltr_mgmt_list_entry *lst_itr; > >>> + > >>> + if (LIST_EMPTY(rule_head)) > >>> + return; > >> > >> Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not > >> loop and status will be returned: > >> > > Yes, the check is necessary. This code gets consumed by multiple different > OSs and not all OSs implement the LIST_FOR_EACH_ENTRY_SAFE in the way > that DPDK did. For example, if I'm understanding the Linux code correctly, > the list_for_each_entry_safe code in Linux would not work correctly without > checking LIST_EMPTY since the Linux implementation doesn't have a check > for null in it's implementation of list_for_each_entry_safe. > > Do you mean the same patch is upstreamed into Linux Kernel without any > adaptations? The same patch is planned to be upstreamed in the Linux kernel without any adaptations. Like I said, for Linux you have to check for LIST_EMPTY since the implementation of list_for_each_entry_safe doesn't check for NULL.
On 6/5/19 6:16 PM, Stillwell Jr, Paul M wrote: >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Wednesday, June 5, 2019 8:59 AM >> To: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>; Rong, Leyi >> <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> >> Cc: dev@dpdk.org; Raj, Victor <victor.raj@intel.com> >> Subject: Re: [dpdk-dev] [PATCH 07/49] net/ice/base: replay advanced rule >> after reset >> >> >> >> On 6/5/19 5:53 PM, Stillwell Jr, Paul M wrote: >>>>> diff --git a/drivers/net/ice/base/ice_switch.c >>>>> b/drivers/net/ice/base/ice_switch.c >>>>> index c53021aed..ca0497ca7 100644 >>>>> --- a/drivers/net/ice/base/ice_switch.c >>>>> +++ b/drivers/net/ice/base/ice_switch.c >>>>> @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, >>>> struct LIST_HEAD_TYPE *rule_head) >>>>> } >>>>> } >>>>> >>>>> +/** >>>>> + * ice_rem_adv_rule_info >>>>> + * @hw: pointer to the hardware structure >>>>> + * @rule_head: pointer to the switch list structure that we want to >>>>> +delete */ static void ice_rem_adv_rule_info(struct ice_hw *hw, >>>>> +struct LIST_HEAD_TYPE *rule_head) { >>>>> + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; >>>>> + struct ice_adv_fltr_mgmt_list_entry *lst_itr; >>>>> + >>>>> + if (LIST_EMPTY(rule_head)) >>>>> + return; >>>> >>>> Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not >>>> loop and status will be returned: >>>> >>> Yes, the check is necessary. This code gets consumed by multiple different >> OSs and not all OSs implement the LIST_FOR_EACH_ENTRY_SAFE in the way >> that DPDK did. For example, if I'm understanding the Linux code correctly, >> the list_for_each_entry_safe code in Linux would not work correctly without >> checking LIST_EMPTY since the Linux implementation doesn't have a check >> for null in it's implementation of list_for_each_entry_safe. >> >> Do you mean the same patch is upstreamed into Linux Kernel without any >> adaptations? > > The same patch is planned to be upstreamed in the Linux kernel without any adaptations. Like I said, for Linux you have to check for LIST_EMPTY since the implementation of list_for_each_entry_safe doesn't check for NULL. > OK, thanks for the clarification. That's a surprise to me that OS abstraction layers are now accepted in upstream kernel (Like ice_acquire_lock for instance). Let's drop my comments about LIST_EMPTY then. Maxime
> -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Wednesday, June 5, 2019 9:28 AM > To: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>; Rong, Leyi > <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > Cc: dev@dpdk.org; Raj, Victor <victor.raj@intel.com> > Subject: Re: [dpdk-dev] [PATCH 07/49] net/ice/base: replay advanced rule > after reset > > > > On 6/5/19 6:16 PM, Stillwell Jr, Paul M wrote: > >> -----Original Message----- > >> From: Maxime Coquelin <maxime.coquelin@redhat.com> > >> Sent: Wednesday, June 5, 2019 8:59 AM > >> To: Stillwell Jr, Paul M <paul.m.stillwell.jr@intel.com>; Rong, Leyi > >> <leyi.rong@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com> > >> Cc: dev@dpdk.org; Raj, Victor <victor.raj@intel.com> > >> Subject: Re: [dpdk-dev] [PATCH 07/49] net/ice/base: replay advanced > >> rule after reset > >> > >> > >> > >> On 6/5/19 5:53 PM, Stillwell Jr, Paul M wrote: > >>>>> diff --git a/drivers/net/ice/base/ice_switch.c > >>>>> b/drivers/net/ice/base/ice_switch.c > >>>>> index c53021aed..ca0497ca7 100644 > >>>>> --- a/drivers/net/ice/base/ice_switch.c > >>>>> +++ b/drivers/net/ice/base/ice_switch.c > >>>>> @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, > >>>> struct LIST_HEAD_TYPE *rule_head) > >>>>> } > >>>>> } > >>>>> > >>>>> +/** > >>>>> + * ice_rem_adv_rule_info > >>>>> + * @hw: pointer to the hardware structure > >>>>> + * @rule_head: pointer to the switch list structure that we want > >>>>> +to delete */ static void ice_rem_adv_rule_info(struct ice_hw > >>>>> +*hw, struct LIST_HEAD_TYPE *rule_head) { > >>>>> + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; > >>>>> + struct ice_adv_fltr_mgmt_list_entry *lst_itr; > >>>>> + > >>>>> + if (LIST_EMPTY(rule_head)) > >>>>> + return; > >>>> > >>>> Is it necessary? If the list is empty, LIST_FOR_EACH_ENTRY will not > >>>> loop and status will be returned: > >>>> > >>> Yes, the check is necessary. This code gets consumed by multiple > >>> different > >> OSs and not all OSs implement the LIST_FOR_EACH_ENTRY_SAFE in the > way > >> that DPDK did. For example, if I'm understanding the Linux code > >> correctly, the list_for_each_entry_safe code in Linux would not work > >> correctly without checking LIST_EMPTY since the Linux implementation > >> doesn't have a check for null in it's implementation of > list_for_each_entry_safe. > >> > >> Do you mean the same patch is upstreamed into Linux Kernel without > >> any adaptations? > > > > The same patch is planned to be upstreamed in the Linux kernel without > any adaptations. Like I said, for Linux you have to check for LIST_EMPTY since > the implementation of list_for_each_entry_safe doesn't check for NULL. > > > > OK, thanks for the clarification. > That's a surprise to me that OS abstraction layers are now accepted in > upstream kernel (Like ice_acquire_lock for instance). > Just to further clarify, when the patch goes upstream in Linux the LIST_FOR_EACH_ENTRY_SAFE and all the other LIST_* macros in the code will get replaced with the corresponding list_* that is in the linux kernel. We have a coccinelle entry to replace these in Linux. > Let's drop my comments about LIST_EMPTY then. > > Maxime
diff --git a/drivers/net/ice/base/ice_switch.c b/drivers/net/ice/base/ice_switch.c index c53021aed..ca0497ca7 100644 --- a/drivers/net/ice/base/ice_switch.c +++ b/drivers/net/ice/base/ice_switch.c @@ -3033,6 +3033,27 @@ ice_rem_sw_rule_info(struct ice_hw *hw, struct LIST_HEAD_TYPE *rule_head) } } +/** + * ice_rem_adv_rule_info + * @hw: pointer to the hardware structure + * @rule_head: pointer to the switch list structure that we want to delete + */ +static void +ice_rem_adv_rule_info(struct ice_hw *hw, struct LIST_HEAD_TYPE *rule_head) +{ + struct ice_adv_fltr_mgmt_list_entry *tmp_entry; + struct ice_adv_fltr_mgmt_list_entry *lst_itr; + + if (LIST_EMPTY(rule_head)) + return; + + LIST_FOR_EACH_ENTRY_SAFE(lst_itr, tmp_entry, rule_head, + ice_adv_fltr_mgmt_list_entry, list_entry) { + LIST_DEL(&lst_itr->list_entry); + ice_free(hw, lst_itr->lkups); + ice_free(hw, lst_itr); + } +} /** * ice_rem_all_sw_rules_info @@ -3049,6 +3070,8 @@ void ice_rem_all_sw_rules_info(struct ice_hw *hw) rule_head = &sw->recp_list[i].filt_rules; if (!sw->recp_list[i].adv_rule) ice_rem_sw_rule_info(hw, rule_head); + else + ice_rem_adv_rule_info(hw, rule_head); } } @@ -5687,6 +5710,38 @@ ice_replay_vsi_fltr(struct ice_hw *hw, u16 vsi_handle, u8 recp_id, return status; } +/** + * ice_replay_vsi_adv_rule - Replay advanced rule for requested VSI + * @hw: pointer to the hardware structure + * @vsi_handle: driver VSI handle + * @list_head: list for which filters need to be replayed + * + * Replay the advanced rule for the given VSI. + */ +static enum ice_status +ice_replay_vsi_adv_rule(struct ice_hw *hw, u16 vsi_handle, + struct LIST_HEAD_TYPE *list_head) +{ + struct ice_rule_query_data added_entry = { 0 }; + struct ice_adv_fltr_mgmt_list_entry *adv_fltr; + enum ice_status status = ICE_SUCCESS; + + if (LIST_EMPTY(list_head)) + return status; + LIST_FOR_EACH_ENTRY(adv_fltr, list_head, ice_adv_fltr_mgmt_list_entry, + list_entry) { + struct ice_adv_rule_info *rinfo = &adv_fltr->rule_info; + u16 lk_cnt = adv_fltr->lkups_cnt; + + if (vsi_handle != rinfo->sw_act.vsi_handle) + continue; + status = ice_add_adv_rule(hw, adv_fltr->lkups, lk_cnt, rinfo, + &added_entry); + if (status) + break; + } + return status; +} /** * ice_replay_vsi_all_fltr - replay all filters stored in bookkeeping lists @@ -5698,23 +5753,23 @@ ice_replay_vsi_fltr(struct ice_hw *hw, u16 vsi_handle, u8 recp_id, enum ice_status ice_replay_vsi_all_fltr(struct ice_hw *hw, u16 vsi_handle) { struct ice_switch_info *sw = hw->switch_info; - enum ice_status status = ICE_SUCCESS; + enum ice_status status; u8 i; + /* Update the recipes that were created */ for (i = 0; i < ICE_MAX_NUM_RECIPES; i++) { - /* Update the default recipe lines and ones that were created */ - if (i < ICE_MAX_NUM_RECIPES || sw->recp_list[i].recp_created) { - struct LIST_HEAD_TYPE *head; + struct LIST_HEAD_TYPE *head; - head = &sw->recp_list[i].filt_replay_rules; - if (!sw->recp_list[i].adv_rule) - status = ice_replay_vsi_fltr(hw, vsi_handle, i, - head); - if (status != ICE_SUCCESS) - return status; - } + head = &sw->recp_list[i].filt_replay_rules; + if (!sw->recp_list[i].adv_rule) + status = ice_replay_vsi_fltr(hw, vsi_handle, i, head); + else + status = ice_replay_vsi_adv_rule(hw, vsi_handle, head); + if (status != ICE_SUCCESS) + return status; } - return status; + + return ICE_SUCCESS; } /** @@ -5738,6 +5793,8 @@ void ice_rm_all_sw_replay_rule_info(struct ice_hw *hw) l_head = &sw->recp_list[i].filt_replay_rules; if (!sw->recp_list[i].adv_rule) ice_rem_sw_rule_info(hw, l_head); + else + ice_rem_adv_rule_info(hw, l_head); } } }