[Intel-wired-lan] [PATCH 0/4] igc: Fixes for VLAN priority filtering code

[Intel-wired-lan] [PATCH 0/4] igc: Fixes for VLAN priority filtering code

[Andre Guedes]

Hi,

This patch series provides some fixes and improvements to the VLAN
priority filtering code from IGC driver.

The improvements focuses on aligning the code organization to follow
the same rationale from MAC address filtering code, splitting ethtool
and core logic between igc_ethtool.c and igc_main.c. It also adds some
log messages to help debugging the code.

Best regards,

Andre

Andre Guedes (4):
  igc: Rename IGC_VLAPQF macro
  igc: Dump VLANPQF register
  igc: Return -EOPNOTSUPP when VLAN mask doesn't match
  igc: Refactor VLAN priority filtering code

 drivers/net/ethernet/intel/igc/igc.h         |  3 +
 drivers/net/ethernet/intel/igc/igc_defines.h |  6 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 70 +++++---------------
 drivers/net/ethernet/intel/igc/igc_main.c    | 52 +++++++++++++++
 drivers/net/ethernet/intel/igc/igc_regs.h    |  2 +-
 5 files changed, 76 insertions(+), 57 deletions(-)

-- 
2.26.0

[Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_VLAPQF macro

[Andre Guedes]

This patch renames the IGC_VLAPQF macro to IGC_VLANPQF as well as
related macros so they match the register name and fields described in
the datasheet.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h |  6 +++---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 21 ++++++++++----------
 drivers/net/ethernet/intel/igc/igc_regs.h    |  2 +-
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 63d3d34763da..a4edf344bd63 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -514,9 +514,9 @@
 #define IGC_MAX_MAC_HDR_LEN	127
 #define IGC_MAX_NETWORK_HDR_LEN	511
 
-#define IGC_VLAPQF_QUEUE_SEL(_n, q_idx) ((q_idx) << ((_n) * 4))
-#define IGC_VLAPQF_P_VALID(_n)	(0x1 << (3 + (_n) * 4))
-#define IGC_VLAPQF_QUEUE_MASK	0x03
+#define IGC_VLANPQF_QSEL(_n, q_idx) ((q_idx) << ((_n) * 4))
+#define IGC_VLANPQF_VALID(_n)	(0x1 << (3 + (_n) * 4))
+#define IGC_VLANPQF_QUEUE_MASK	0x03
 
 #define IGC_ADVTXD_MACLEN_SHIFT		9  /* Adv ctxt desc mac len shift */
 #define IGC_ADVTXD_TUCMD_L4T_UDP	0x00000000  /* L4 Packet TYPE of UDP */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 47115d73ea61..9811b26f3c59 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1229,23 +1229,23 @@ static int igc_rxnfc_write_vlan_prio_filter(struct igc_adapter *adapter,
 	u16 queue_index;
 	u32 vlapqf;
 
-	vlapqf = rd32(IGC_VLAPQF);
+	vlapqf = rd32(IGC_VLANPQF);
 	vlan_priority = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
 				>> VLAN_PRIO_SHIFT;
-	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLAPQF_QUEUE_MASK;
+	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLANPQF_QUEUE_MASK;
 
 	/* check whether this vlan prio is already set */
-	if (vlapqf & IGC_VLAPQF_P_VALID(vlan_priority) &&
+	if (vlapqf & IGC_VLANPQF_VALID(vlan_priority) &&
 	    queue_index != input->action) {
 		netdev_err(adapter->netdev,
 			   "ethtool rxnfc set vlan prio filter failed");
 		return -EEXIST;
 	}
 
-	vlapqf |= IGC_VLAPQF_P_VALID(vlan_priority);
-	vlapqf |= IGC_VLAPQF_QUEUE_SEL(vlan_priority, input->action);
+	vlapqf |= IGC_VLANPQF_VALID(vlan_priority);
+	vlapqf |= IGC_VLANPQF_QSEL(vlan_priority, input->action);
 
-	wr32(IGC_VLAPQF, vlapqf);
+	wr32(IGC_VLANPQF, vlapqf);
 
 	return 0;
 }
@@ -1313,12 +1313,11 @@ static void igc_clear_vlan_prio_filter(struct igc_adapter *adapter,
 
 	vlan_priority = (vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
 
-	vlapqf = rd32(IGC_VLAPQF);
-	vlapqf &= ~IGC_VLAPQF_P_VALID(vlan_priority);
-	vlapqf &= ~IGC_VLAPQF_QUEUE_SEL(vlan_priority,
-						IGC_VLAPQF_QUEUE_MASK);
+	vlapqf = rd32(IGC_VLANPQF);
+	vlapqf &= ~IGC_VLANPQF_VALID(vlan_priority);
+	vlapqf &= ~IGC_VLANPQF_QSEL(vlan_priority, IGC_VLANPQF_QUEUE_MASK);
 
-	wr32(IGC_VLAPQF, vlapqf);
+	wr32(IGC_VLANPQF, vlapqf);
 }
 
 int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
index 7bfba20bf207..ab8ac46d381a 100644
--- a/drivers/net/ethernet/intel/igc/igc_regs.h
+++ b/drivers/net/ethernet/intel/igc/igc_regs.h
@@ -123,7 +123,7 @@
 #define IGC_UTA			0x0A000  /* Unicast Table Array - RW */
 #define IGC_RAL(_n)		(0x05400 + ((_n) * 0x08))
 #define IGC_RAH(_n)		(0x05404 + ((_n) * 0x08))
-#define IGC_VLAPQF		0x055B0  /* VLAN Priority Queue Filter VLAPQF */
+#define IGC_VLANPQF		0x055B0  /* VLAN Priority Queue Filter - RW */
 
 /* Transmit Register Descriptions */
 #define IGC_TCTL		0x00400  /* Tx Control - RW */
-- 
2.26.0

[Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_VLAPQF macro

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Friday, April 3, 2020 11:18 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 1/4] igc: Rename IGC_VLAPQF macro
> 
> This patch renames the IGC_VLAPQF macro to IGC_VLANPQF as well as
> related macros so they match the register name and fields described in
> the datasheet.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h |  6 +++---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 21 ++++++++++----------
>  drivers/net/ethernet/intel/igc/igc_regs.h    |  2 +-
>  3 files changed, 14 insertions(+), 15 deletions(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 2/4] igc: Dump VLANPQF register

[Andre Guedes]

This patch adds the VLAN Priority Queue Filter Register (VLANPQF) to the
list of registers dumped by igc_get_regs().

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 9811b26f3c59..dfc21908347d 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -316,6 +316,8 @@ static void igc_get_regs(struct net_device *netdev,
 		regs_buff[172 + i] = rd32(IGC_RAL(i));
 	for (i = 0; i < 16; i++)
 		regs_buff[188 + i] = rd32(IGC_RAH(i));
+
+	regs_buff[204] = rd32(IGC_VLANPQF);
 }
 
 static void igc_get_wol(struct net_device *netdev, struct ethtool_wolinfo *wol)
-- 
2.26.0

[Intel-wired-lan] [PATCH 2/4] igc: Dump VLANPQF register

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Friday, April 3, 2020 11:18 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 2/4] igc: Dump VLANPQF register
> 
> This patch adds the VLAN Priority Queue Filter Register (VLANPQF) to the
> list of registers dumped by igc_get_regs().
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 3/4] igc: Return -EOPNOTSUPP when VLAN mask doesn't match

[Andre Guedes]

The I225 controller supports rx queue assignment based on VLAN priority
only. Other Tag Control Information (TCI) are valid, but not supported
by the driver. So this patch changes the returning code from igc_add_
ethtool_nfc_entry() to -EOPNOTSUPP in order to provide more meaningful
information on why the function failed.

It also adds a debug messages to give the user a hint about what went
wrong with the NFC setup.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index dfc21908347d..036a2244b76c 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1446,7 +1446,8 @@ static int igc_add_ethtool_nfc_entry(struct igc_adapter *adapter,
 
 	if ((fsp->flow_type & FLOW_EXT) && fsp->m_ext.vlan_tci) {
 		if (fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK)) {
-			err = -EINVAL;
+			netdev_dbg(netdev, "VLAN mask not supported");
+			err = -EOPNOTSUPP;
 			goto err_out;
 		}
 		input->filter.vlan_tci = fsp->h_ext.vlan_tci;
-- 
2.26.0

[Intel-wired-lan] [PATCH 3/4] igc: Return -EOPNOTSUPP when VLAN mask doesn't match

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Friday, April 3, 2020 11:18 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 3/4] igc: Return -EOPNOTSUPP when VLAN
> mask doesn't match
> 
> The I225 controller supports rx queue assignment based on VLAN priority
> only. Other Tag Control Information (TCI) are valid, but not supported
> by the driver. So this patch changes the returning code from igc_add_
> ethtool_nfc_entry() to -EOPNOTSUPP in order to provide more meaningful
> information on why the function failed.
> 
> It also adds a debug messages to give the user a hint about what went
> wrong with the NFC setup.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering code

[Andre Guedes]

The whole VLAN priority filtering code is implemented in igc_ethtool.c
and mixes logic from ethtool and core parts. This patch refactors it so
core logic is moved to igc_main.c, aligning the VLAN priority filtering
code organization with the MAC address filtering code.

This patch also takes the opportunity to add some log messages to ease
debugging.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  3 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
 drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
 3 files changed, 68 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 1b07e8b870c2..48eb9c00a44d 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -235,6 +235,9 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const s8 queue, const u8 flags);
 int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const u8 flags);
+int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio,
+			     int queue);
+void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio);
 void igc_update_stats(struct igc_adapter *adapter);
 
 /* igc_dump declarations */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 036a2244b76c..35bc125183a0 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1223,35 +1223,6 @@ static int igc_rxnfc_write_etype_filter(struct igc_adapter *adapter,
 	return 0;
 }
 
-static int igc_rxnfc_write_vlan_prio_filter(struct igc_adapter *adapter,
-					    struct igc_nfc_filter *input)
-{
-	struct igc_hw *hw = &adapter->hw;
-	u8 vlan_priority;
-	u16 queue_index;
-	u32 vlapqf;
-
-	vlapqf = rd32(IGC_VLANPQF);
-	vlan_priority = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
-				>> VLAN_PRIO_SHIFT;
-	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLANPQF_QUEUE_MASK;
-
-	/* check whether this vlan prio is already set */
-	if (vlapqf & IGC_VLANPQF_VALID(vlan_priority) &&
-	    queue_index != input->action) {
-		netdev_err(adapter->netdev,
-			   "ethtool rxnfc set vlan prio filter failed");
-		return -EEXIST;
-	}
-
-	vlapqf |= IGC_VLANPQF_VALID(vlan_priority);
-	vlapqf |= IGC_VLANPQF_QSEL(vlan_priority, input->action);
-
-	wr32(IGC_VLANPQF, vlapqf);
-
-	return 0;
-}
-
 int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 {
 	struct igc_hw *hw = &adapter->hw;
@@ -1285,10 +1256,15 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 			return err;
 	}
 
-	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
-		err = igc_rxnfc_write_vlan_prio_filter(adapter, input);
+	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
+		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
+				>> VLAN_PRIO_SHIFT;
+		err = igc_add_vlan_prio_filter(adapter, prio, input->action);
+		if (err)
+			return err;
+	}
 
-	return err;
+	return 0;
 }
 
 static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
@@ -1306,31 +1282,17 @@ static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
 	adapter->etype_bitmap[reg_index] = false;
 }
 
-static void igc_clear_vlan_prio_filter(struct igc_adapter *adapter,
-				       u16 vlan_tci)
-{
-	struct igc_hw *hw = &adapter->hw;
-	u8 vlan_priority;
-	u32 vlapqf;
-
-	vlan_priority = (vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
-
-	vlapqf = rd32(IGC_VLANPQF);
-	vlapqf &= ~IGC_VLANPQF_VALID(vlan_priority);
-	vlapqf &= ~IGC_VLANPQF_QSEL(vlan_priority, IGC_VLANPQF_QUEUE_MASK);
-
-	wr32(IGC_VLANPQF, vlapqf);
-}
-
 int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 {
 	if (input->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
 		igc_clear_etype_filter_regs(adapter,
 					    input->etype_reg_index);
 
-	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
-		igc_clear_vlan_prio_filter(adapter,
-					   ntohs(input->filter.vlan_tci));
+	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
+		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
+				>> VLAN_PRIO_SHIFT;
+		igc_del_vlan_prio_filter(adapter, prio);
+	}
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
 		igc_del_mac_filter(adapter, input->filter.src_addr,
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index c3555148ca0e..70f861b418b2 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2315,6 +2315,58 @@ int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	return 0;
 }
 
+/**
+ * igc_add_vlan_prio_filter() - Add VLAN priority filter.
+ * @adapter: Pointer to adapter where the filter should be added.
+ * @prio: VLAN priority value.
+ * @queue: Queue number which matching frames are assigned to.
+ *
+ * Return: 0 in case of success, negative errno code otherwise.
+ */
+int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio, int queue)
+{
+	struct net_device *dev = adapter->netdev;
+	struct igc_hw *hw = &adapter->hw;
+	u32 vlanpqf;
+
+	vlanpqf = rd32(IGC_VLANPQF);
+
+	if (vlanpqf & IGC_VLANPQF_VALID(prio)) {
+		netdev_dbg(dev, "VLAN priority filter already in use");
+		return -EEXIST;
+	}
+
+	vlanpqf |= IGC_VLANPQF_QSEL(prio, queue);
+	vlanpqf |= IGC_VLANPQF_VALID(prio);
+
+	wr32(IGC_VLANPQF, vlanpqf);
+
+	netdev_dbg(dev, "Add VLAN priority filter: prio %d queue %d",
+		   prio, queue);
+	return 0;
+}
+
+/**
+ * igc_del_vlan_prio_filter() - Delete VLAN priority filter.
+ * @adapter: Pointer to adapter where the filter should be deleted from.
+ * @prio: VLAN priority value.
+ */
+void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 vlanpqf;
+
+	vlanpqf = rd32(IGC_VLANPQF);
+
+	vlanpqf &= ~IGC_VLANPQF_VALID(prio);
+	vlanpqf |= ~IGC_VLANPQF_QSEL(prio, 0);
+
+	wr32(IGC_VLANPQF, vlanpqf);
+
+	netdev_dbg(adapter->netdev, "Delete VLAN priority filter: prio %d",
+		   prio);
+}
+
 static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
-- 
2.26.0

[Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering code

[Vinicius Costa Gomes]

Hi,

Andre Guedes <andre.guedes@intel.com> writes:

> The whole VLAN priority filtering code is implemented in igc_ethtool.c
> and mixes logic from ethtool and core parts. This patch refactors it so
> core logic is moved to igc_main.c, aligning the VLAN priority filtering
> code organization with the MAC address filtering code.
>
> This patch also takes the opportunity to add some log messages to ease
> debugging.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         |  3 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
>  drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
>  3 files changed, 68 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index 1b07e8b870c2..48eb9c00a44d 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -235,6 +235,9 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  		       const s8 queue, const u8 flags);
>  int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  		       const u8 flags);
> +int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio,
> +			     int queue);
> +void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio);
>  void igc_update_stats(struct igc_adapter *adapter);
>  
>  /* igc_dump declarations */
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 036a2244b76c..35bc125183a0 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1223,35 +1223,6 @@ static int igc_rxnfc_write_etype_filter(struct igc_adapter *adapter,
>  	return 0;
>  }
>  
> -static int igc_rxnfc_write_vlan_prio_filter(struct igc_adapter *adapter,
> -					    struct igc_nfc_filter *input)
> -{
> -	struct igc_hw *hw = &adapter->hw;
> -	u8 vlan_priority;
> -	u16 queue_index;
> -	u32 vlapqf;
> -
> -	vlapqf = rd32(IGC_VLANPQF);
> -	vlan_priority = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> -				>> VLAN_PRIO_SHIFT;
> -	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLANPQF_QUEUE_MASK;
> -
> -	/* check whether this vlan prio is already set */
> -	if (vlapqf & IGC_VLANPQF_VALID(vlan_priority) &&
> -	    queue_index != input->action) {
> -		netdev_err(adapter->netdev,
> -			   "ethtool rxnfc set vlan prio filter failed");
> -		return -EEXIST;
> -	}
> -
> -	vlapqf |= IGC_VLANPQF_VALID(vlan_priority);
> -	vlapqf |= IGC_VLANPQF_QSEL(vlan_priority, input->action);
> -
> -	wr32(IGC_VLANPQF, vlapqf);
> -
> -	return 0;
> -}
> -
>  int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  {
>  	struct igc_hw *hw = &adapter->hw;
> @@ -1285,10 +1256,15 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  			return err;
>  	}
>  
> -	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
> -		err = igc_rxnfc_write_vlan_prio_filter(adapter, input);
> +	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
> +		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> +				>> VLAN_PRIO_SHIFT;
> +		err = igc_add_vlan_prio_filter(adapter, prio, input->action);
> +		if (err)
> +			return err;
> +	}
>  
> -	return err;
> +	return 0;
>  }
>  
>  static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
> @@ -1306,31 +1282,17 @@ static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
>  	adapter->etype_bitmap[reg_index] = false;
>  }
>  
> -static void igc_clear_vlan_prio_filter(struct igc_adapter *adapter,
> -				       u16 vlan_tci)
> -{
> -	struct igc_hw *hw = &adapter->hw;
> -	u8 vlan_priority;
> -	u32 vlapqf;
> -
> -	vlan_priority = (vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> -
> -	vlapqf = rd32(IGC_VLANPQF);
> -	vlapqf &= ~IGC_VLANPQF_VALID(vlan_priority);
> -	vlapqf &= ~IGC_VLANPQF_QSEL(vlan_priority, IGC_VLANPQF_QUEUE_MASK);
> -
> -	wr32(IGC_VLANPQF, vlapqf);
> -}
> -
>  int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
>  {
>  	if (input->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
>  		igc_clear_etype_filter_regs(adapter,
>  					    input->etype_reg_index);
>  
> -	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
> -		igc_clear_vlan_prio_filter(adapter,
> -					   ntohs(input->filter.vlan_tci));
> +	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
> +		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> +				>> VLAN_PRIO_SHIFT;

As you are refactoring this, one suggestion/question: is it possible to
move the ntohs() and friends closer to writing/reading to/from the
registers?

> +		igc_del_vlan_prio_filter(adapter, prio);
> +	}
>  
>  	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
>  		igc_del_mac_filter(adapter, input->filter.src_addr,
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index c3555148ca0e..70f861b418b2 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2315,6 +2315,58 @@ int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>  	return 0;
>  }
>  
> +/**
> + * igc_add_vlan_prio_filter() - Add VLAN priority filter.
> + * @adapter: Pointer to adapter where the filter should be added.
> + * @prio: VLAN priority value.
> + * @queue: Queue number which matching frames are assigned to.
> + *
> + * Return: 0 in case of success, negative errno code otherwise.
> + */
> +int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio, int queue)
> +{
> +	struct net_device *dev = adapter->netdev;
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 vlanpqf;
> +
> +	vlanpqf = rd32(IGC_VLANPQF);
> +
> +	if (vlanpqf & IGC_VLANPQF_VALID(prio)) {
> +		netdev_dbg(dev, "VLAN priority filter already in use");
> +		return -EEXIST;
> +	}
> +
> +	vlanpqf |= IGC_VLANPQF_QSEL(prio, queue);
> +	vlanpqf |= IGC_VLANPQF_VALID(prio);
> +
> +	wr32(IGC_VLANPQF, vlanpqf);
> +
> +	netdev_dbg(dev, "Add VLAN priority filter: prio %d queue %d",
> +		   prio, queue);

As you added a way to dump this register, I don't think this debug
statement, and the one below, are that useful.

> +	return 0;
> +}
> +
> +/**
> + * igc_del_vlan_prio_filter() - Delete VLAN priority filter.
> + * @adapter: Pointer to adapter where the filter should be deleted from.
> + * @prio: VLAN priority value.
> + */
> +void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +	u32 vlanpqf;
> +
> +	vlanpqf = rd32(IGC_VLANPQF);
> +
> +	vlanpqf &= ~IGC_VLANPQF_VALID(prio);
> +	vlanpqf |= ~IGC_VLANPQF_QSEL(prio, 0);
> +
> +	wr32(IGC_VLANPQF, vlanpqf);
> +
> +	netdev_dbg(adapter->netdev, "Delete VLAN priority filter: prio %d",
> +		   prio);
> +}
> +
>  static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
>  {
>  	struct igc_adapter *adapter = netdev_priv(netdev);
> -- 
> 2.26.0
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

-- 
Vinicius

[Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering code

[Andre Guedes]

Hi Vinicius,

> >  int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
> >  {
> >       if (input->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
> >               igc_clear_etype_filter_regs(adapter,
> >                                           input->etype_reg_index);
> >  
> > -     if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
> > -             igc_clear_vlan_prio_filter(adapter,
> > -                                        ntohs(input->filter.vlan_tci));
> > +     if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
> > +             int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
> > +                             >> VLAN_PRIO_SHIFT;
> 
> As you are refactoring this, one suggestion/question: is it possible to
> move the ntohs() and friends closer to writing/reading to/from the
> registers?

I left this ntohs() here on purpose. The reason why we need it is due to the
ethtool byte order convention for the vlan_tci, not the controller. So it makes
more sense to have it in igc_ethtool.c instead of igc_main.c.

> > +int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio, int queue)
> > +{
> > +     struct net_device *dev = adapter->netdev;
> > +     struct igc_hw *hw = &adapter->hw;
> > +     u32 vlanpqf;
> > +
> > +     vlanpqf = rd32(IGC_VLANPQF);
> > +
> > +     if (vlanpqf & IGC_VLANPQF_VALID(prio)) {
> > +             netdev_dbg(dev, "VLAN priority filter already in use");
> > +             return -EEXIST;
> > +     }
> > +
> > +     vlanpqf |= IGC_VLANPQF_QSEL(prio, queue);
> > +     vlanpqf |= IGC_VLANPQF_VALID(prio);
> > +
> > +     wr32(IGC_VLANPQF, vlanpqf);
> > +
> > +     netdev_dbg(dev, "Add VLAN priority filter: prio %d queue %d",
> > +                prio, queue);
> 
> As you added a way to dump this register, I don't think this debug
> statement, and the one below, are that useful.

Even considering that we are now able to dump the VLANPQF register, these debug
messages are still useful for logging purposes. We want to know when VLAN
priority filters are added or deleted. The register dump gives us a snapshot
of the register value, not the sequence of events that happened to get there.

At least they have been useful to me while debugging this code.

- Andre

[Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering code

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Friday, April 3, 2020 11:18 AM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 4/4] igc: Refactor VLAN priority filtering
> code
> 
> The whole VLAN priority filtering code is implemented in igc_ethtool.c
> and mixes logic from ethtool and core parts. This patch refactors it so
> core logic is moved to igc_main.c, aligning the VLAN priority filtering
> code organization with the MAC address filtering code.
> 
> This patch also takes the opportunity to add some log messages to ease
> debugging.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         |  3 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
>  drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
>  3 files changed, 68 insertions(+), 51 deletions(-)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

[Intel-wired-lan] [PATCH v2 4/4] igc: Refactor VLAN priority filtering code

[Andre Guedes]

The whole VLAN priority filtering code is implemented in igc_ethtool.c
and mixes logic from ethtool and core parts. This patch refactors it so
core logic is moved to igc_main.c, aligning the VLAN priority filtering
code organization with the MAC address filtering code.

This patch also takes the opportunity to add some log messages to ease
debugging.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
v2: Previous version had a bug when clearing IGC_VLANPQF_QSEL bits in
    igc_del_vlan_prio_filter().
---
 drivers/net/ethernet/intel/igc/igc.h         |  3 +
 drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
 drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
 3 files changed, 68 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 1b07e8b870c2..48eb9c00a44d 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -235,6 +235,9 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const s8 queue, const u8 flags);
 int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const u8 flags);
+int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio,
+			     int queue);
+void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio);
 void igc_update_stats(struct igc_adapter *adapter);
 
 /* igc_dump declarations */
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 036a2244b76c..35bc125183a0 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1223,35 +1223,6 @@ static int igc_rxnfc_write_etype_filter(struct igc_adapter *adapter,
 	return 0;
 }
 
-static int igc_rxnfc_write_vlan_prio_filter(struct igc_adapter *adapter,
-					    struct igc_nfc_filter *input)
-{
-	struct igc_hw *hw = &adapter->hw;
-	u8 vlan_priority;
-	u16 queue_index;
-	u32 vlapqf;
-
-	vlapqf = rd32(IGC_VLANPQF);
-	vlan_priority = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
-				>> VLAN_PRIO_SHIFT;
-	queue_index = (vlapqf >> (vlan_priority * 4)) & IGC_VLANPQF_QUEUE_MASK;
-
-	/* check whether this vlan prio is already set */
-	if (vlapqf & IGC_VLANPQF_VALID(vlan_priority) &&
-	    queue_index != input->action) {
-		netdev_err(adapter->netdev,
-			   "ethtool rxnfc set vlan prio filter failed");
-		return -EEXIST;
-	}
-
-	vlapqf |= IGC_VLANPQF_VALID(vlan_priority);
-	vlapqf |= IGC_VLANPQF_QSEL(vlan_priority, input->action);
-
-	wr32(IGC_VLANPQF, vlapqf);
-
-	return 0;
-}
-
 int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 {
 	struct igc_hw *hw = &adapter->hw;
@@ -1285,10 +1256,15 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 			return err;
 	}
 
-	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
-		err = igc_rxnfc_write_vlan_prio_filter(adapter, input);
+	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
+		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
+				>> VLAN_PRIO_SHIFT;
+		err = igc_add_vlan_prio_filter(adapter, prio, input->action);
+		if (err)
+			return err;
+	}
 
-	return err;
+	return 0;
 }
 
 static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
@@ -1306,31 +1282,17 @@ static void igc_clear_etype_filter_regs(struct igc_adapter *adapter,
 	adapter->etype_bitmap[reg_index] = false;
 }
 
-static void igc_clear_vlan_prio_filter(struct igc_adapter *adapter,
-				       u16 vlan_tci)
-{
-	struct igc_hw *hw = &adapter->hw;
-	u8 vlan_priority;
-	u32 vlapqf;
-
-	vlan_priority = (vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
-
-	vlapqf = rd32(IGC_VLANPQF);
-	vlapqf &= ~IGC_VLANPQF_VALID(vlan_priority);
-	vlapqf &= ~IGC_VLANPQF_QSEL(vlan_priority, IGC_VLANPQF_QUEUE_MASK);
-
-	wr32(IGC_VLANPQF, vlapqf);
-}
-
 int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 {
 	if (input->filter.match_flags & IGC_FILTER_FLAG_ETHER_TYPE)
 		igc_clear_etype_filter_regs(adapter,
 					    input->etype_reg_index);
 
-	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI)
-		igc_clear_vlan_prio_filter(adapter,
-					   ntohs(input->filter.vlan_tci));
+	if (input->filter.match_flags & IGC_FILTER_FLAG_VLAN_TCI) {
+		int prio = (ntohs(input->filter.vlan_tci) & VLAN_PRIO_MASK)
+				>> VLAN_PRIO_SHIFT;
+		igc_del_vlan_prio_filter(adapter, prio);
+	}
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
 		igc_del_mac_filter(adapter, input->filter.src_addr,
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index c3555148ca0e..7ae5bc476bdb 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2315,6 +2315,58 @@ int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	return 0;
 }
 
+/**
+ * igc_add_vlan_prio_filter() - Add VLAN priority filter.
+ * @adapter: Pointer to adapter where the filter should be added.
+ * @prio: VLAN priority value.
+ * @queue: Queue number which matching frames are assigned to.
+ *
+ * Return: 0 in case of success, negative errno code otherwise.
+ */
+int igc_add_vlan_prio_filter(struct igc_adapter *adapter, int prio, int queue)
+{
+	struct net_device *dev = adapter->netdev;
+	struct igc_hw *hw = &adapter->hw;
+	u32 vlanpqf;
+
+	vlanpqf = rd32(IGC_VLANPQF);
+
+	if (vlanpqf & IGC_VLANPQF_VALID(prio)) {
+		netdev_dbg(dev, "VLAN priority filter already in use");
+		return -EEXIST;
+	}
+
+	vlanpqf |= IGC_VLANPQF_QSEL(prio, queue);
+	vlanpqf |= IGC_VLANPQF_VALID(prio);
+
+	wr32(IGC_VLANPQF, vlanpqf);
+
+	netdev_dbg(dev, "Add VLAN priority filter: prio %d queue %d",
+		   prio, queue);
+	return 0;
+}
+
+/**
+ * igc_del_vlan_prio_filter() - Delete VLAN priority filter.
+ * @adapter: Pointer to adapter where the filter should be deleted from.
+ * @prio: VLAN priority value.
+ */
+void igc_del_vlan_prio_filter(struct igc_adapter *adapter, int prio)
+{
+	struct igc_hw *hw = &adapter->hw;
+	u32 vlanpqf;
+
+	vlanpqf = rd32(IGC_VLANPQF);
+
+	vlanpqf &= ~IGC_VLANPQF_VALID(prio);
+	vlanpqf &= ~IGC_VLANPQF_QSEL(prio, IGC_VLANPQF_QUEUE_MASK);
+
+	wr32(IGC_VLANPQF, vlanpqf);
+
+	netdev_dbg(adapter->netdev, "Delete VLAN priority filter: prio %d",
+		   prio);
+}
+
 static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
-- 
2.26.0

[Intel-wired-lan] [PATCH v2 4/4] igc: Refactor VLAN priority filtering code

[Brown, Aaron F]

> From: Guedes, Andre <andre.guedes@intel.com>
> Sent: Thursday, April 23, 2020 6:11 PM
> To: intel-wired-lan at lists.osuosl.org; Brown, Aaron F
> <aaron.f.brown@intel.com>; Kirsher, Jeffrey T <jeffrey.t.kirsher@intel.com>
> Subject: [PATCH v2 4/4] igc: Refactor VLAN priority filtering code
> 
> The whole VLAN priority filtering code is implemented in igc_ethtool.c
> and mixes logic from ethtool and core parts. This patch refactors it so
> core logic is moved to igc_main.c, aligning the VLAN priority filtering
> code organization with the MAC address filtering code.
> 
> This patch also takes the opportunity to add some log messages to ease
> debugging.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
> v2: Previous version had a bug when clearing IGC_VLANPQF_QSEL bits in
>     igc_del_vlan_prio_filter().
> ---
>  drivers/net/ethernet/intel/igc/igc.h         |  3 +
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 64 ++++----------------
>  drivers/net/ethernet/intel/igc/igc_main.c    | 52 ++++++++++++++++
>  3 files changed, 68 insertions(+), 51 deletions(-)
> 

Tested-by: Aaron Brown <aaron.f.brown@intel.com>