diff mbox series

[07/49] net/ice/base: replay advanced rule after reset

Message ID 20190604054248.68510-8-leyi.rong@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series shared code update |

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

Maxime Coquelin June 5, 2019, 8:58 a.m. UTC | #1 
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);
>   		}
>   	}
>   }
>
Stillwell Jr, Paul M June 5, 2019, 3:53 p.m. UTC | #2 
> -----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);
> >   		}
> >   	}
> >   }
> >
Maxime Coquelin June 5, 2019, 3:59 p.m. UTC | #3 
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?
Stillwell Jr, Paul M June 5, 2019, 4:16 p.m. UTC | #4 
> -----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.
Maxime Coquelin June 5, 2019, 4:28 p.m. UTC | #5 
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
Stillwell Jr, Paul M June 5, 2019, 4:31 p.m. UTC | #6 
> -----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 mbox series

Patch

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