> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Wednesday, June 5, 2019 1:59 AM
> To: Rong, Leyi <leyi.r...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>
> Cc: dev@dpdk.org; Raj, Victor <victor....@intel.com>; Stillwell Jr, Paul M
> <paul.m.stillwell...@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....@intel.com>
> > Signed-off-by: Paul M Stillwell Jr <paul.m.stillwell...@intel.com>
> > Signed-off-by: Leyi Rong <leyi.r...@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);
> > }
> > }
> > }
> >
