diff mbox series

[net,1/2] iavf: refactor VLAN filter states

Message ID 20230404172523.451026-2-anthony.l.nguyen@intel.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series iavf: fix racing in VLANs | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 18 this patch: 18
netdev/cc_maintainers success CCed 5 of 5 maintainers
netdev/build_clang success Errors and warnings before: 18 this patch: 18
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: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 154 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tony Nguyen April 4, 2023, 5:25 p.m. UTC 
From: Ahmed Zaki <ahmed.zaki@intel.com>

The VLAN filter states are currently being saved as individual bits.
This is error prone as multiple bits might be mistakenly set.

Fix by replacing the bits with a single state enum. Also, add an
"ACTIVE" state for filters that are accepted by the PF.

Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h        | 15 +++++----
 drivers/net/ethernet/intel/iavf/iavf_main.c   |  8 ++---
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 31 +++++++++----------
 3 files changed, 28 insertions(+), 26 deletions(-)

Comments

Jakub Kicinski April 6, 2023, 12:15 a.m. UTC | #1 
On Tue,  4 Apr 2023 10:25:21 -0700 Tony Nguyen wrote:
> +	__IAVF_VLAN_INVALID,
> +	__IAVF_VLAN_ADD,	/* filter needs to be added */
> +	__IAVF_VLAN_IS_NEW,	/* filter is new, wait for PF answer */
> +	__IAVF_VLAN_ACTIVE,	/* filter is accepted by PF */
> +	__IAVF_VLAN_REMOVE,	/* filter needs to be removed */

Why the leading underscores?
Ahmed Zaki April 6, 2023, 12:50 a.m. UTC | #2 
On 2023-04-05 18:15, Jakub Kicinski wrote:
> On Tue,  4 Apr 2023 10:25:21 -0700 Tony Nguyen wrote:
>> +	__IAVF_VLAN_INVALID,
>> +	__IAVF_VLAN_ADD,	/* filter needs to be added */
>> +	__IAVF_VLAN_IS_NEW,	/* filter is new, wait for PF answer */
>> +	__IAVF_VLAN_ACTIVE,	/* filter is accepted by PF */
>> +	__IAVF_VLAN_REMOVE,	/* filter needs to be removed */
> Why the leading underscores?

Just following the convention. iavf_tc_state_t and 
iavf_cloud_filter_state_t have these underscores. Same for iavf_state_t.
Jakub Kicinski April 6, 2023, 12:59 a.m. UTC | #3 
On Wed, 5 Apr 2023 18:50:55 -0600 Ahmed Zaki wrote:
> On 2023-04-05 18:15, Jakub Kicinski wrote:
> > On Tue,  4 Apr 2023 10:25:21 -0700 Tony Nguyen wrote:  
> >> +	__IAVF_VLAN_INVALID,
> >> +	__IAVF_VLAN_ADD,	/* filter needs to be added */
> >> +	__IAVF_VLAN_IS_NEW,	/* filter is new, wait for PF answer */
> >> +	__IAVF_VLAN_ACTIVE,	/* filter is accepted by PF */
> >> +	__IAVF_VLAN_REMOVE,	/* filter needs to be removed */  
> > Why the leading underscores?  
> 
> Just following the convention. iavf_tc_state_t and 
> iavf_cloud_filter_state_t have these underscores. Same for iavf_state_t.

What is the convention, tho?  Differently put what is the thing
that would be defined with the same names but without the underscores?

My intuition is that we prefix bit numbers with __, 
then the mask (1 << __BIT_NO) does not have a prefix.

But these are not used as bits anywhere, in fact you're going away 
from bits...
Ahmed Zaki April 6, 2023, 6:55 p.m. UTC | #4 
On 2023-04-05 18:59, Jakub Kicinski wrote:

> On Wed, 5 Apr 2023 18:50:55 -0600 Ahmed Zaki wrote:
>> On 2023-04-05 18:15, Jakub Kicinski wrote:
>>> On Tue,  4 Apr 2023 10:25:21 -0700 Tony Nguyen wrote:
>>>> +	__IAVF_VLAN_INVALID,
>>>> +	__IAVF_VLAN_ADD,	/* filter needs to be added */
>>>> +	__IAVF_VLAN_IS_NEW,	/* filter is new, wait for PF answer */
>>>> +	__IAVF_VLAN_ACTIVE,	/* filter is accepted by PF */
>>>> +	__IAVF_VLAN_REMOVE,	/* filter needs to be removed */
>>> Why the leading underscores?
>> Just following the convention. iavf_tc_state_t and
>> iavf_cloud_filter_state_t have these underscores. Same for iavf_state_t.
> What is the convention, tho?  Differently put what is the thing
> that would be defined with the same names but without the underscores?

Nothing.

>
> My intuition is that we prefix bit numbers with __,
> then the mask (1 << __BIT_NO) does not have a prefix.
>
> But these are not used as bits anywhere, in fact you're going away
> from bits...

Ok, how about sending v2 without these underscores, then send another 
patch to net-next fixing the rest of states?
Jakub Kicinski April 6, 2023, 8:04 p.m. UTC | #5 
On Thu, 6 Apr 2023 12:55:43 -0600 Ahmed Zaki wrote:
> > My intuition is that we prefix bit numbers with __,
> > then the mask (1 << __BIT_NO) does not have a prefix.
> >
> > But these are not used as bits anywhere, in fact you're going away
> > from bits...  
> 
> Ok, how about sending v2 without these underscores, then send another 
> patch to net-next fixing the rest of states?

Definitely SGTM, thanks.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 232bc61d9eee..dd3b1c0fec4e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -158,15 +158,18 @@  struct iavf_vlan {
 	u16 tpid;
 };
 
+enum iavf_vlan_state_t {
+	__IAVF_VLAN_INVALID,
+	__IAVF_VLAN_ADD,	/* filter needs to be added */
+	__IAVF_VLAN_IS_NEW,	/* filter is new, wait for PF answer */
+	__IAVF_VLAN_ACTIVE,	/* filter is accepted by PF */
+	__IAVF_VLAN_REMOVE,	/* filter needs to be removed */
+};
+
 struct iavf_vlan_filter {
 	struct list_head list;
 	struct iavf_vlan vlan;
-	struct {
-		u8 is_new_vlan:1;	/* filter is new, wait for PF answer */
-		u8 remove:1;		/* filter needs to be removed */
-		u8 add:1;		/* filter needs to be added */
-		u8 padding:5;
-	};
+	enum iavf_vlan_state_t state;
 };
 
 #define IAVF_MAX_TRAFFIC_CLASS	4
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 095201e83c9d..5e3429677daa 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -791,7 +791,7 @@  iavf_vlan_filter *iavf_add_vlan(struct iavf_adapter *adapter,
 		f->vlan = vlan;
 
 		list_add_tail(&f->list, &adapter->vlan_filter_list);
-		f->add = true;
+		f->state = __IAVF_VLAN_ADD;
 		adapter->aq_required |= IAVF_FLAG_AQ_ADD_VLAN_FILTER;
 	}
 
@@ -813,7 +813,7 @@  static void iavf_del_vlan(struct iavf_adapter *adapter, struct iavf_vlan vlan)
 
 	f = iavf_find_vlan(adapter, vlan);
 	if (f) {
-		f->remove = true;
+		f->state = __IAVF_VLAN_REMOVE;
 		adapter->aq_required |= IAVF_FLAG_AQ_DEL_VLAN_FILTER;
 	}
 
@@ -1296,11 +1296,11 @@  static void iavf_clear_mac_vlan_filters(struct iavf_adapter *adapter)
 	/* remove all VLAN filters */
 	list_for_each_entry_safe(vlf, vlftmp, &adapter->vlan_filter_list,
 				 list) {
-		if (vlf->add) {
+		if (vlf->state == __IAVF_VLAN_ADD) {
 			list_del(&vlf->list);
 			kfree(vlf);
 		} else {
-			vlf->remove = true;
+			vlf->state = __IAVF_VLAN_REMOVE;
 		}
 	}
 	spin_unlock_bh(&adapter->mac_vlan_list_lock);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 4e17d006c52d..9ba83f0d212e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -642,7 +642,7 @@  static void iavf_vlan_add_reject(struct iavf_adapter *adapter)
 
 	spin_lock_bh(&adapter->mac_vlan_list_lock);
 	list_for_each_entry_safe(f, ftmp, &adapter->vlan_filter_list, list) {
-		if (f->is_new_vlan) {
+		if (f->state == __IAVF_VLAN_IS_NEW) {
 			if (f->vlan.tpid == ETH_P_8021Q)
 				clear_bit(f->vlan.vid,
 					  adapter->vsi.active_cvlans);
@@ -679,7 +679,7 @@  void iavf_add_vlans(struct iavf_adapter *adapter)
 	spin_lock_bh(&adapter->mac_vlan_list_lock);
 
 	list_for_each_entry(f, &adapter->vlan_filter_list, list) {
-		if (f->add)
+		if (f->state == __IAVF_VLAN_ADD)
 			count++;
 	}
 	if (!count || !VLAN_FILTERING_ALLOWED(adapter)) {
@@ -710,11 +710,10 @@  void iavf_add_vlans(struct iavf_adapter *adapter)
 		vvfl->vsi_id = adapter->vsi_res->vsi_id;
 		vvfl->num_elements = count;
 		list_for_each_entry(f, &adapter->vlan_filter_list, list) {
-			if (f->add) {
+			if (f->state == __IAVF_VLAN_ADD) {
 				vvfl->vlan_id[i] = f->vlan.vid;
 				i++;
-				f->add = false;
-				f->is_new_vlan = true;
+				f->state = __IAVF_VLAN_IS_NEW;
 				if (i == count)
 					break;
 			}
@@ -760,7 +759,7 @@  void iavf_add_vlans(struct iavf_adapter *adapter)
 		vvfl_v2->vport_id = adapter->vsi_res->vsi_id;
 		vvfl_v2->num_elements = count;
 		list_for_each_entry(f, &adapter->vlan_filter_list, list) {
-			if (f->add) {
+			if (f->state == __IAVF_VLAN_ADD) {
 				struct virtchnl_vlan_supported_caps *filtering_support =
 					&adapter->vlan_v2_caps.filtering.filtering_support;
 				struct virtchnl_vlan *vlan;
@@ -778,8 +777,7 @@  void iavf_add_vlans(struct iavf_adapter *adapter)
 				vlan->tpid = f->vlan.tpid;
 
 				i++;
-				f->add = false;
-				f->is_new_vlan = true;
+				f->state = __IAVF_VLAN_IS_NEW;
 			}
 		}
 
@@ -822,10 +820,11 @@  void iavf_del_vlans(struct iavf_adapter *adapter)
 		 * filters marked for removal to enable bailing out before
 		 * sending a virtchnl message
 		 */
-		if (f->remove && !VLAN_FILTERING_ALLOWED(adapter)) {
+		if (f->state == __IAVF_VLAN_REMOVE &&
+		    !VLAN_FILTERING_ALLOWED(adapter)) {
 			list_del(&f->list);
 			kfree(f);
-		} else if (f->remove) {
+		} else if (f->state == __IAVF_VLAN_REMOVE) {
 			count++;
 		}
 	}
@@ -857,7 +856,7 @@  void iavf_del_vlans(struct iavf_adapter *adapter)
 		vvfl->vsi_id = adapter->vsi_res->vsi_id;
 		vvfl->num_elements = count;
 		list_for_each_entry_safe(f, ftmp, &adapter->vlan_filter_list, list) {
-			if (f->remove) {
+			if (f->state == __IAVF_VLAN_REMOVE) {
 				vvfl->vlan_id[i] = f->vlan.vid;
 				i++;
 				list_del(&f->list);
@@ -901,7 +900,7 @@  void iavf_del_vlans(struct iavf_adapter *adapter)
 		vvfl_v2->vport_id = adapter->vsi_res->vsi_id;
 		vvfl_v2->num_elements = count;
 		list_for_each_entry_safe(f, ftmp, &adapter->vlan_filter_list, list) {
-			if (f->remove) {
+			if (f->state == __IAVF_VLAN_REMOVE) {
 				struct virtchnl_vlan_supported_caps *filtering_support =
 					&adapter->vlan_v2_caps.filtering.filtering_support;
 				struct virtchnl_vlan *vlan;
@@ -2192,7 +2191,7 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 				list_for_each_entry(vlf,
 						    &adapter->vlan_filter_list,
 						    list)
-					vlf->add = true;
+					vlf->state = __IAVF_VLAN_ADD;
 
 				adapter->aq_required |=
 					IAVF_FLAG_AQ_ADD_VLAN_FILTER;
@@ -2260,7 +2259,7 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 				list_for_each_entry(vlf,
 						    &adapter->vlan_filter_list,
 						    list)
-					vlf->add = true;
+					vlf->state = __IAVF_VLAN_ADD;
 
 				aq_required |= IAVF_FLAG_AQ_ADD_VLAN_FILTER;
 			}
@@ -2444,8 +2443,8 @@  void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 
 		spin_lock_bh(&adapter->mac_vlan_list_lock);
 		list_for_each_entry(f, &adapter->vlan_filter_list, list) {
-			if (f->is_new_vlan) {
-				f->is_new_vlan = false;
+			if (f->state == __IAVF_VLAN_IS_NEW) {
+				f->state = __IAVF_VLAN_ACTIVE;
 				if (f->vlan.tpid == ETH_P_8021Q)
 					set_bit(f->vlan.vid,
 						adapter->vsi.active_cvlans);