[Intel-wired-lan] [PATCH 00/12] igc: Refactor MAC address filtering code

[Intel-wired-lan] [PATCH 00/12] igc: Refactor MAC address filtering code

[Andre Guedes]

Hi all,

The MAC address filtering support from IGC driver has some duplicate code and
convoluted logic, making it harder to read, debug, and maintain.

As a follow-up to the bug fixes already applied to dev-queue, this patchset
refactors the MAC address filtering code to address those issues. It also takes
the opportunity to improve documentation from some key functions and clean up
comments that are not applicable to IGC, and fix minor issues along the way.

In summary, the new MAC address filtering code is organized as follows:

	* Filters are added and deleted only via igc_add_mac_filter() and
	  igc_del_mac_filter() APIs. These APIs manage the adapter->mac_table.
	  They are defined in igc_main.c and used in both igc_main.c and
	  igc_ethtool.c.

	* Filter configuration in hardware is handled by igc_set_mac_filter_
	  hw() and igc_clear_mac_filter_hw() local helpers in igc_main.c.

	* IGC_MAC_STATE_QUEUE_STEERING flag as well as igc_*_mac_steering_
	  filter() are gone.

Best regards,

Andre

Andre Guedes (12):
  igc: Remove duplicate code in MAC filtering logic
  igc: Check unsupported flag in igc_add_mac_filter()
  igc: Change igc_add_mac_filter() returning value
  igc: Fix igc_uc_unsync()
  igc: Refactor igc_rar_set_index()
  igc: Improve address check in igc_del_mac_filter()
  igc: Remove 'queue' check in igc_del_mac_filter()
  igc: Remove IGC_MAC_STATE_QUEUE_STEERING
  igc: Remove igc_*_mac_steering_filter() wrappers
  igc: Refactor igc_mac_entry_can_be_used()
  igc: Refactor igc_del_mac_filter()
  igc: Add debug messages to MAC filter code

 drivers/net/ethernet/intel/igc/igc.h         |  11 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  22 +-
 drivers/net/ethernet/intel/igc/igc_main.c    | 372 ++++++++-----------
 3 files changed, 169 insertions(+), 236 deletions(-)

-- 
2.25.0

[Intel-wired-lan] [PATCH 01/12] igc: Remove duplicate code in MAC filtering logic

[Andre Guedes]

This patch does a code refactoring in the MAC address filtering logic to
get rid of some duplicate code.

IGC driver has two functions to add MAC address filters that are pretty
much the same: igc_add_mac_filter() and igc_add_mac_filter_flags(). The
only difference is that the latter allows the callee to specify the
'flags' parameter while the former has it hardcoded as zero. The same
rationale applies to filter deletion counterparts.

So this patch refactors igc_add_mac_filter() and igc_del_mac_filter() so
they handle the 'flags' parameters, removes the _flags() functions, and
fixes callees accordingly.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index a964ef18ac0c..107e9f86bd0a 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2192,8 +2192,8 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
  * default for the destination address, if matching by source address
  * is desired the flag IGC_MAC_STATE_SRC_ADDR can be used.
  */
-static int igc_add_mac_filter(struct igc_adapter *adapter,
-			      const u8 *addr, const u8 queue)
+static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
+			      const u8 queue, const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2208,12 +2208,12 @@ static int igc_add_mac_filter(struct igc_adapter *adapter,
 	 */
 	for (i = 0; i < rar_entries; i++) {
 		if (!igc_mac_entry_can_be_used(&adapter->mac_table[i],
-					       addr, 0))
+					       addr, flags))
 			continue;
 
 		ether_addr_copy(adapter->mac_table[i].addr, addr);
 		adapter->mac_table[i].queue = queue;
-		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE;
+		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
 
 		igc_rar_set_index(adapter, i);
 		return i;
@@ -2228,8 +2228,8 @@ static int igc_add_mac_filter(struct igc_adapter *adapter,
  * matching by source address is to be removed the flag
  * IGC_MAC_STATE_SRC_ADDR can be used.
  */
-static int igc_del_mac_filter(struct igc_adapter *adapter,
-			      const u8 *addr, const u8 queue)
+static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
+			      const u8 queue, const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2245,7 +2245,7 @@ static int igc_del_mac_filter(struct igc_adapter *adapter,
 	for (i = 0; i < rar_entries; i++) {
 		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
 			continue;
-		if (adapter->mac_table[i].state != 0)
+		if (flags && (adapter->mac_table[i].state & flags) != flags)
 			continue;
 		if (adapter->mac_table[i].queue != queue)
 			continue;
@@ -2277,7 +2277,7 @@ static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 	struct igc_adapter *adapter = netdev_priv(netdev);
 	int ret;
 
-	ret = igc_add_mac_filter(adapter, addr, adapter->num_rx_queues);
+	ret = igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
 
 	return min_t(int, ret, 0);
 }
@@ -2286,7 +2286,7 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	igc_del_mac_filter(adapter, addr, adapter->num_rx_queues);
+	igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
 
 	return 0;
 }
@@ -3723,104 +3723,18 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 	return features;
 }
 
-/* Add a MAC filter for 'addr' directing matching traffic to 'queue',
- * 'flags' is used to indicate what kind of match is made, match is by
- * default for the destination address, if matching by source address
- * is desired the flag IGC_MAC_STATE_SRC_ADDR can be used.
- */
-static int igc_add_mac_filter_flags(struct igc_adapter *adapter,
-				    const u8 *addr, const u8 queue,
-				    const u8 flags)
-{
-	struct igc_hw *hw = &adapter->hw;
-	int rar_entries = hw->mac.rar_entry_count;
-	int i;
-
-	if (is_zero_ether_addr(addr))
-		return -EINVAL;
-
-	/* Search for the first empty entry in the MAC table.
-	 * Do not touch entries at the end of the table reserved for the VF MAC
-	 * addresses.
-	 */
-	for (i = 0; i < rar_entries; i++) {
-		if (!igc_mac_entry_can_be_used(&adapter->mac_table[i],
-					       addr, flags))
-			continue;
-
-		ether_addr_copy(adapter->mac_table[i].addr, addr);
-		adapter->mac_table[i].queue = queue;
-		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
-
-		igc_rar_set_index(adapter, i);
-		return i;
-	}
-
-	return -ENOSPC;
-}
-
 int igc_add_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_add_mac_filter_flags(adapter, addr, queue,
-					IGC_MAC_STATE_QUEUE_STEERING | flags);
-}
-
-/* Remove a MAC filter for 'addr' directing matching traffic to
- * 'queue', 'flags' is used to indicate what kind of match need to be
- * removed, match is by default for the destination address, if
- * matching by source address is to be removed the flag
- * IGC_MAC_STATE_SRC_ADDR can be used.
- */
-static int igc_del_mac_filter_flags(struct igc_adapter *adapter,
-				    const u8 *addr, const u8 queue,
-				    const u8 flags)
-{
-	struct igc_hw *hw = &adapter->hw;
-	int rar_entries = hw->mac.rar_entry_count;
-	int i;
-
-	if (is_zero_ether_addr(addr))
-		return -EINVAL;
-
-	/* Search for matching entry in the MAC table based on given address
-	 * and queue. Do not touch entries at the end of the table reserved
-	 * for the VF MAC addresses.
-	 */
-	for (i = 0; i < rar_entries; i++) {
-		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
-			continue;
-		if ((adapter->mac_table[i].state & flags) != flags)
-			continue;
-		if (adapter->mac_table[i].queue != queue)
-			continue;
-		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
-			continue;
-
-		/* When a filter for the default address is "deleted",
-		 * we return it to its initial configuration
-		 */
-		if (adapter->mac_table[i].state & IGC_MAC_STATE_DEFAULT) {
-			adapter->mac_table[i].state =
-				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
-		} else {
-			adapter->mac_table[i].state = 0;
-			adapter->mac_table[i].queue = 0;
-			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
-		}
-
-		igc_rar_set_index(adapter, i);
-		return 0;
-	}
-
-	return -ENOENT;
+	return igc_add_mac_filter(adapter, addr, queue,
+				  IGC_MAC_STATE_QUEUE_STEERING | flags);
 }
 
 int igc_del_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_del_mac_filter_flags(adapter, addr, queue,
-					IGC_MAC_STATE_QUEUE_STEERING | flags);
+	return igc_del_mac_filter(adapter, addr, queue,
+				  IGC_MAC_STATE_QUEUE_STEERING | flags);
 }
 
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
-- 
2.25.0

[Intel-wired-lan] [PATCH 01/12] igc: Remove duplicate code in MAC filtering logic

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 01/12] igc: Remove duplicate code in MAC
> filtering logic
> 
> This patch does a code refactoring in the MAC address filtering logic to
> get rid of some duplicate code.
> 
> IGC driver has two functions to add MAC address filters that are pretty
> much the same: igc_add_mac_filter() and igc_add_mac_filter_flags(). The
> only difference is that the latter allows the callee to specify the
> 'flags' parameter while the former has it hardcoded as zero. The same
> rationale applies to filter deletion counterparts.
> 
> So this patch refactors igc_add_mac_filter() and igc_del_mac_filter() so
> they handle the 'flags' parameters, removes the _flags() functions, and
> fixes callees accordingly.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 112 +++-------------------
>  1 file changed, 13 insertions(+), 99 deletions(-)

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

[Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in igc_add_mac_filter()

[Andre Guedes]

The IGC_MAC_STATE_SRC_ADDR flags is not supported by igc_add_mac_
filter() so this patch adds a check for it and returns -ENOTSUPP
in case it is set.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 107e9f86bd0a..5360d73d9c63 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2201,6 +2201,8 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 
 	if (is_zero_ether_addr(addr))
 		return -EINVAL;
+	if (flags & IGC_MAC_STATE_SRC_ADDR)
+		return -ENOTSUPP;
 
 	/* Search for the first empty entry in the MAC table.
 	 * Do not touch entries at the end of the table reserved for the VF MAC
-- 
2.25.0

[Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in igc_add_mac_filter()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in
> igc_add_mac_filter()
> 
> The IGC_MAC_STATE_SRC_ADDR flags is not supported by igc_add_mac_
> filter() so this patch adds a check for it and returns -ENOTSUPP
> in case it is set.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
>  1 file changed, 2 insertions(+)

[Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in igc_add_mac_filter()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Brown, Aaron F
> Sent: Tuesday, March 31, 2020 12:59 PM
> To: Guedes, Andre <andre.guedes@intel.com>; intel-wired-lan at lists.osuosl.org
> Subject: Re: [Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in
> igc_add_mac_filter()
> 
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> > Andre Guedes
> > Sent: Wednesday, March 18, 2020 4:01 PM
> > To: intel-wired-lan at lists.osuosl.org
> > Subject: [Intel-wired-lan] [PATCH 02/12] igc: Check unsupported flag in
> > igc_add_mac_filter()
> >
> > The IGC_MAC_STATE_SRC_ADDR flags is not supported by igc_add_mac_
> > filter() so this patch adds a check for it and returns -ENOTSUPP
> > in case it is set.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 2 ++
> >  1 file changed, 2 insertions(+)

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

[Intel-wired-lan] [PATCH 03/12] igc: Change igc_add_mac_filter() returning value

[Andre Guedes]

In case of success, igc_add_mac_filter() returns the index in
adapter->mac_table where the requested filter was added. This
information, however, is not used by any caller of that function.
In fact, callers have extra code just to handle this returning
index as 0 (success).

So this patch changes the function to return 0 on success instead,
and cleans up the extra code.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 925f0979506e..9ef133fe6e40 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1270,7 +1270,6 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 		err = igc_add_mac_steering_filter(adapter,
 						  input->filter.dst_addr,
 						  input->action, 0);
-		err = min_t(int, err, 0);
 		if (err)
 			return err;
 	}
@@ -1280,7 +1279,6 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 						  input->filter.src_addr,
 						  input->action,
 						  IGC_MAC_STATE_SRC_ADDR);
-		err = min_t(int, err, 0);
 		if (err)
 			return err;
 	}
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 5360d73d9c63..ed2648e0fbd8 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2218,7 +2218,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
 
 		igc_rar_set_index(adapter, i);
-		return i;
+		return 0;
 	}
 
 	return -ENOSPC;
@@ -2277,11 +2277,8 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
-	int ret;
-
-	ret = igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
 
-	return min_t(int, ret, 0);
+	return igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
 }
 
 static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
-- 
2.25.0

[Intel-wired-lan] [PATCH 03/12] igc: Change igc_add_mac_filter() returning value

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 03/12] igc: Change igc_add_mac_filter()
> returning value
> 
> In case of success, igc_add_mac_filter() returns the index in
> adapter->mac_table where the requested filter was added. This
> information, however, is not used by any caller of that function.
> In fact, callers have extra code just to handle this returning
> index as 0 (success).
> 
> So this patch changes the function to return 0 on success instead,
> and cleans up the extra code.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 2 --
>  drivers/net/ethernet/intel/igc/igc_main.c    | 7 ++-----
>  2 files changed, 2 insertions(+), 7 deletions(-)

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

[Intel-wired-lan] [PATCH 04/12] igc: Fix igc_uc_unsync()

[Andre Guedes]

In case igc_del_mac_filter() returns error, that error is masked
since the functions always return 0 (success). This patch fixes
igc_uc_unsync() so it returns whatever value igc_del_mac_filter()
returns (0 on success, negative number on error).

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index ed2648e0fbd8..cc1e1b0286b3 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2285,9 +2285,7 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
-
-	return 0;
+	return igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
 }
 
 /**
-- 
2.25.0

[Intel-wired-lan] [PATCH 04/12] igc: Fix igc_uc_unsync()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 04/12] igc: Fix igc_uc_unsync()
> 
> In case igc_del_mac_filter() returns error, that error is masked
> since the functions always return 0 (success). This patch fixes
> igc_uc_unsync() so it returns whatever value igc_del_mac_filter()
> returns (0 on success, negative number on error).
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

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

[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Andre Guedes]

Current igc_rar_set_index() implementation is a bit convoluted so this
patch does some code refactoring to improve it.

The helper igc_rar_set_index() is about writing MAC filter settings into
hardware registers. Logic such as address validation belongs to
functions upper in the call chain such as igc_set_mac() and
igc_add_mac_filter(). So this patch moves the is_valid_ether_addr() call
to igc_add_mac_filter(). No need to touch igc_set_mac() since it already
checks it.

The variables 'rar_low' and 'rar_high' represent the value in registers
RAL and RAH so we rename them to 'ral' and 'rah', respectivelly, to
match the registers names.

To make it explicity, filter settings are passed as arguments to the
function instead of reading them from adapter->mac_table "under the
hood". Also, the function was renamed to igc_set_mac_filter_hw to make
it more clear what it does.

Finally, the patch removes some wrfl() calls and comments not needed.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index cc1e1b0286b3..0ca7afaf6fc7 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -764,43 +764,53 @@ static void igc_setup_tctl(struct igc_adapter *adapter)
 	wr32(IGC_TCTL, tctl);
 }
 
-/**
- * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
- * @adapter: address of board private structure
- * @index: Index of the RAR entry which need to be synced with MAC table
+/* Set MAC address filter in hardware.
+ *
+ * @adapter: Pointer to adapter where the filter should be set.
+ * @index: Filter index.
+ * @addr: Destination MAC address.
+ * @queue: If non-negative, queue assignment feature is enabled and frames
+ * matching the filter are enqueued onto 'queue'. Otherwise, queue assignment
+ * is disabled.
  */
-static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
+static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
+				  const u8 *addr, int queue)
 {
-	u8 *addr = adapter->mac_table[index].addr;
 	struct igc_hw *hw = &adapter->hw;
-	u32 rar_low, rar_high;
+	u32 ral, rah;
 
-	/* HW expects these to be in network order when they are plugged
-	 * into the registers which are little endian.  In order to guarantee
-	 * that ordering we need to do an leXX_to_cpup here in order to be
-	 * ready for the byteswap that occurs with writel
-	 */
-	rar_low = le32_to_cpup((__le32 *)(addr));
-	rar_high = le16_to_cpup((__le16 *)(addr + 4));
+	if (WARN_ON(index >= hw->mac.rar_entry_count))
+		return;
 
-	if (adapter->mac_table[index].state & IGC_MAC_STATE_QUEUE_STEERING) {
-		u8 queue = adapter->mac_table[index].queue;
-		u32 qsel = IGC_RAH_QSEL_MASK & (queue << IGC_RAH_QSEL_SHIFT);
+	ral = le32_to_cpup((__le32 *)(addr));
+	rah = le16_to_cpup((__le16 *)(addr + 4));
 
-		rar_high |= qsel;
-		rar_high |= IGC_RAH_QSEL_ENABLE;
+	if (queue >= 0) {
+		rah &= ~IGC_RAH_QSEL_MASK;
+		rah |= (queue << IGC_RAH_QSEL_SHIFT);
+		rah |= IGC_RAH_QSEL_ENABLE;
 	}
 
-	/* Indicate to hardware the Address is Valid. */
-	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
-		if (is_valid_ether_addr(addr))
-			rar_high |= IGC_RAH_AV;
-	}
+	rah |= IGC_RAH_AV;
 
-	wr32(IGC_RAL(index), rar_low);
-	wrfl();
-	wr32(IGC_RAH(index), rar_high);
-	wrfl();
+	wr32(IGC_RAL(index), ral);
+	wr32(IGC_RAH(index), rah);
+}
+
+/* Clear MAC address filter in hardware.
+ *
+ * @adapter: Pointer to adapter where the filter should be cleared.
+ * @index: Filter index.
+ */
+static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
+{
+	struct igc_hw *hw = &adapter->hw;
+
+	if (WARN_ON(index >= hw->mac.rar_entry_count))
+		return;
+
+	wr32(IGC_RAL(index), 0);
+	wr32(IGC_RAH(index), 0);
 }
 
 /* Set default MAC address for the PF in the first RAR entry */
@@ -811,7 +821,7 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
 	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
 	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
 
-	igc_rar_set_index(adapter, 0);
+	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
 }
 
 /**
@@ -2199,7 +2209,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	int rar_entries = hw->mac.rar_entry_count;
 	int i;
 
-	if (is_zero_ether_addr(addr))
+	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 	if (flags & IGC_MAC_STATE_SRC_ADDR)
 		return -ENOTSUPP;
@@ -2217,7 +2227,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		adapter->mac_table[i].queue = queue;
 		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
 
-		igc_rar_set_index(adapter, i);
+		igc_set_mac_filter_hw(adapter, i, addr, queue);
 		return 0;
 	}
 
@@ -2261,13 +2271,14 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 			adapter->mac_table[i].state =
 				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
 			adapter->mac_table[i].queue = 0;
+			igc_set_mac_filter_hw(adapter, 0, addr, -1);
 		} else {
 			adapter->mac_table[i].state = 0;
 			adapter->mac_table[i].queue = 0;
 			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
+			igc_clear_mac_filter_hw(adapter, i);
 		}
 
-		igc_rar_set_index(adapter, i);
 		return 0;
 	}
 
-- 
2.25.0

[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Neftin, Sasha]

On 3/19/2020 01:00, Andre Guedes wrote:
> Current igc_rar_set_index() implementation is a bit convoluted so this
> patch does some code refactoring to improve it.
> 
> The helper igc_rar_set_index() is about writing MAC filter settings into
> hardware registers. Logic such as address validation belongs to
> functions upper in the call chain such as igc_set_mac() and
> igc_add_mac_filter(). So this patch moves the is_valid_ether_addr() call
> to igc_add_mac_filter(). No need to touch igc_set_mac() since it already
> checks it.
> 
> The variables 'rar_low' and 'rar_high' represent the value in registers
> RAL and RAH so we rename them to 'ral' and 'rah', respectivelly, to
> match the registers names.
> 
> To make it explicity, filter settings are passed as arguments to the
> function instead of reading them from adapter->mac_table "under the
> hood". Also, the function was renamed to igc_set_mac_filter_hw to make
> it more clear what it does.
> 
> Finally, the patch removes some wrfl() calls and comments not needed.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 75 +++++++++++++----------
>   1 file changed, 43 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index cc1e1b0286b3..0ca7afaf6fc7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -764,43 +764,53 @@ static void igc_setup_tctl(struct igc_adapter *adapter)
>   	wr32(IGC_TCTL, tctl);
>   }
>   
> -/**
> - * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
> - * @adapter: address of board private structure
> - * @index: Index of the RAR entry which need to be synced with MAC table
> +/* Set MAC address filter in hardware.
Small correction to be consistently. Please, keep /** line above method 
declaration line.
> + *
> + * @adapter: Pointer to adapter where the filter should be set.
> + * @index: Filter index.
> + * @addr: Destination MAC address.
> + * @queue: If non-negative, queue assignment feature is enabled and frames
> + * matching the filter are enqueued onto 'queue'. Otherwise, queue assignment
> + * is disabled.
>    */
> -static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
> +static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
> +				  const u8 *addr, int queue)
>   {
> -	u8 *addr = adapter->mac_table[index].addr;
>   	struct igc_hw *hw = &adapter->hw;
> -	u32 rar_low, rar_high;
> +	u32 ral, rah;
>   
> -	/* HW expects these to be in network order when they are plugged
> -	 * into the registers which are little endian.  In order to guarantee
> -	 * that ordering we need to do an leXX_to_cpup here in order to be
> -	 * ready for the byteswap that occurs with writel
> -	 */
> -	rar_low = le32_to_cpup((__le32 *)(addr));
> -	rar_high = le16_to_cpup((__le16 *)(addr + 4));
> +	if (WARN_ON(index >= hw->mac.rar_entry_count))
> +		return;
>   
> -	if (adapter->mac_table[index].state & IGC_MAC_STATE_QUEUE_STEERING) {
> -		u8 queue = adapter->mac_table[index].queue;
> -		u32 qsel = IGC_RAH_QSEL_MASK & (queue << IGC_RAH_QSEL_SHIFT);
> +	ral = le32_to_cpup((__le32 *)(addr));
> +	rah = le16_to_cpup((__le16 *)(addr + 4));
>   
> -		rar_high |= qsel;
> -		rar_high |= IGC_RAH_QSEL_ENABLE;
> +	if (queue >= 0) {
> +		rah &= ~IGC_RAH_QSEL_MASK;
> +		rah |= (queue << IGC_RAH_QSEL_SHIFT);
> +		rah |= IGC_RAH_QSEL_ENABLE;
>   	}
>   
> -	/* Indicate to hardware the Address is Valid. */
> -	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
> -		if (is_valid_ether_addr(addr))
> -			rar_high |= IGC_RAH_AV;
> -	}
> +	rah |= IGC_RAH_AV;
>   
> -	wr32(IGC_RAL(index), rar_low);
> -	wrfl();
> -	wr32(IGC_RAH(index), rar_high);
> -	wrfl();
> +	wr32(IGC_RAL(index), ral);
> +	wr32(IGC_RAH(index), rah);
> +}
> +
> +/* Clear MAC address filter in hardware.
Same here. Small correction to be consistently. Please, keep /** line 
above method declaration line.
> + *
> + * @adapter: Pointer to adapter where the filter should be cleared.
> + * @index: Filter index.
> + */
> +static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
> +{
> +	struct igc_hw *hw = &adapter->hw;
> +
> +	if (WARN_ON(index >= hw->mac.rar_entry_count))
> +		return;
> +
> +	wr32(IGC_RAL(index), 0);
> +	wr32(IGC_RAH(index), 0);
>   }
>   
>   /* Set default MAC address for the PF in the first RAR entry */
> @@ -811,7 +821,7 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
>   	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
>   	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
>   
> -	igc_rar_set_index(adapter, 0);
> +	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
>   }
>   
>   /**
> @@ -2199,7 +2209,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   	int rar_entries = hw->mac.rar_entry_count;
>   	int i;
>   
> -	if (is_zero_ether_addr(addr))
> +	if (!is_valid_ether_addr(addr))
>   		return -EINVAL;
>   	if (flags & IGC_MAC_STATE_SRC_ADDR)
>   		return -ENOTSUPP;
> @@ -2217,7 +2227,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   		adapter->mac_table[i].queue = queue;
>   		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
>   
> -		igc_rar_set_index(adapter, i);
> +		igc_set_mac_filter_hw(adapter, i, addr, queue);
>   		return 0;
>   	}
>   
> @@ -2261,13 +2271,14 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   			adapter->mac_table[i].state =
>   				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
>   			adapter->mac_table[i].queue = 0;
> +			igc_set_mac_filter_hw(adapter, 0, addr, -1);
>   		} else {
>   			adapter->mac_table[i].state = 0;
>   			adapter->mac_table[i].queue = 0;
>   			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
> +			igc_clear_mac_filter_hw(adapter, i);
>   		}
>   
> -		igc_rar_set_index(adapter, i);
>   		return 0;
>   	}
>   
> 
Thanks Andre - two small nitpicks.

[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Andre Guedes]

Quoting Neftin, Sasha (2020-03-31 04:22:12)
> > -/**
> > - * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
> > - * @adapter: address of board private structure
> > - * @index: Index of the RAR entry which need to be synced with MAC table
> > +/* Set MAC address filter in hardware.
> Small correction to be consistently. Please, keep /** line above method 
> declaration line.

Yes, I can fix that. I thought we utilized kernel-doc style comments only for
APIs, not local helper functions like this.

@Jeff, what works better for your workflow, I send a v2 in-reply-to this patch
or I send a v2 of the whole patchset?

- Andre

[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Andre Guedes]

Hi Jeff,

> > > -/**
> > > - * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
> > > - * @adapter: address of board private structure
> > > - * @index: Index of the RAR entry which need to be synced with MAC table
> > > +/* Set MAC address filter in hardware.
> > Small correction to be consistently. Please, keep /** line above method 
> > declaration line.
> 
> Yes, I can fix that. I thought we utilized kernel-doc style comments only for
> APIs, not local helper functions like this.
> 
> @Jeff, what works better for your workflow, I send a v2 in-reply-to this patch
> or I send a v2 of the whole patchset?

Since the modifications are very trivial and only 3 patches from this series
were actually updated, I sent the v2 patches in-reply-to. Hope that's fine.

- Andre

[Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 05/12] igc: Refactor igc_rar_set_index()
> 
> Current igc_rar_set_index() implementation is a bit convoluted so this
> patch does some code refactoring to improve it.
> 
> The helper igc_rar_set_index() is about writing MAC filter settings into
> hardware registers. Logic such as address validation belongs to
> functions upper in the call chain such as igc_set_mac() and
> igc_add_mac_filter(). So this patch moves the is_valid_ether_addr() call
> to igc_add_mac_filter(). No need to touch igc_set_mac() since it already
> checks it.
> 
> The variables 'rar_low' and 'rar_high' represent the value in registers
> RAL and RAH so we rename them to 'ral' and 'rah', respectivelly, to
> match the registers names.
> 
> To make it explicity, filter settings are passed as arguments to the
> function instead of reading them from adapter->mac_table "under the
> hood". Also, the function was renamed to igc_set_mac_filter_hw to make
> it more clear what it does.
> 
> Finally, the patch removes some wrfl() calls and comments not needed.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 75 +++++++++++++----------
>  1 file changed, 43 insertions(+), 32 deletions(-)

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

[Intel-wired-lan] [PATCH v2 05/12] igc: Refactor igc_rar_set_index()

[Andre Guedes]

Current igc_rar_set_index() implementation is a bit convoluted so this
patch does some code refactoring to improve it.

The helper igc_rar_set_index() is about writing MAC filter settings into
hardware registers. Logic such as address validation belongs to
functions upper in the call chain such as igc_set_mac() and
igc_add_mac_filter(). So this patch moves the is_valid_ether_addr() call
to igc_add_mac_filter(). No need to touch igc_set_mac() since it already
checks it.

The variables 'rar_low' and 'rar_high' represent the value in registers
RAL and RAH so we rename them to 'ral' and 'rah', respectivelly, to
match the registers names.

To make it explicity, filter settings are passed as arguments to the
function instead of reading them from adapter->mac_table "under the
hood". Also, the function was renamed to igc_set_mac_filter_hw to make
it more clear what it does.

Finally, the patch removes some wrfl() calls and comments not needed.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
---
v2: Fix function documentation to follow kernel-doc style.
---
 drivers/net/ethernet/intel/igc/igc_main.c | 73 +++++++++++++----------
 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index cc1e1b0286b3..c94e4e31e04b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -765,42 +765,52 @@ static void igc_setup_tctl(struct igc_adapter *adapter)
 }
 
 /**
- * igc_rar_set_index - Sync RAL[index] and RAH[index] registers with MAC table
- * @adapter: address of board private structure
- * @index: Index of the RAR entry which need to be synced with MAC table
+ * igc_set_mac_filter_hw() - Set MAC address filter in hardware.
+ * @adapter: Pointer to adapter where the filter should be set.
+ * @index: Filter index.
+ * @addr: Destination MAC address.
+ * @queue: If non-negative, queue assignment feature is enabled and frames
+ *         matching the filter are enqueued onto 'queue'. Otherwise, queue
+ *         assignment is disabled.
  */
-static void igc_rar_set_index(struct igc_adapter *adapter, u32 index)
+static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
+				  const u8 *addr, int queue)
 {
-	u8 *addr = adapter->mac_table[index].addr;
 	struct igc_hw *hw = &adapter->hw;
-	u32 rar_low, rar_high;
+	u32 ral, rah;
 
-	/* HW expects these to be in network order when they are plugged
-	 * into the registers which are little endian.  In order to guarantee
-	 * that ordering we need to do an leXX_to_cpup here in order to be
-	 * ready for the byteswap that occurs with writel
-	 */
-	rar_low = le32_to_cpup((__le32 *)(addr));
-	rar_high = le16_to_cpup((__le16 *)(addr + 4));
+	if (WARN_ON(index >= hw->mac.rar_entry_count))
+		return;
 
-	if (adapter->mac_table[index].state & IGC_MAC_STATE_QUEUE_STEERING) {
-		u8 queue = adapter->mac_table[index].queue;
-		u32 qsel = IGC_RAH_QSEL_MASK & (queue << IGC_RAH_QSEL_SHIFT);
+	ral = le32_to_cpup((__le32 *)(addr));
+	rah = le16_to_cpup((__le16 *)(addr + 4));
 
-		rar_high |= qsel;
-		rar_high |= IGC_RAH_QSEL_ENABLE;
+	if (queue >= 0) {
+		rah &= ~IGC_RAH_QSEL_MASK;
+		rah |= (queue << IGC_RAH_QSEL_SHIFT);
+		rah |= IGC_RAH_QSEL_ENABLE;
 	}
 
-	/* Indicate to hardware the Address is Valid. */
-	if (adapter->mac_table[index].state & IGC_MAC_STATE_IN_USE) {
-		if (is_valid_ether_addr(addr))
-			rar_high |= IGC_RAH_AV;
-	}
+	rah |= IGC_RAH_AV;
 
-	wr32(IGC_RAL(index), rar_low);
-	wrfl();
-	wr32(IGC_RAH(index), rar_high);
-	wrfl();
+	wr32(IGC_RAL(index), ral);
+	wr32(IGC_RAH(index), rah);
+}
+
+/**
+ * igc_clear_mac_filter_hw() - Clear MAC address filter in hardware.
+ * @adapter: Pointer to adapter where the filter should be cleared.
+ * @index: Filter index.
+ */
+static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
+{
+	struct igc_hw *hw = &adapter->hw;
+
+	if (WARN_ON(index >= hw->mac.rar_entry_count))
+		return;
+
+	wr32(IGC_RAL(index), 0);
+	wr32(IGC_RAH(index), 0);
 }
 
 /* Set default MAC address for the PF in the first RAR entry */
@@ -811,7 +821,7 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
 	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
 	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
 
-	igc_rar_set_index(adapter, 0);
+	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
 }
 
 /**
@@ -2199,7 +2209,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	int rar_entries = hw->mac.rar_entry_count;
 	int i;
 
-	if (is_zero_ether_addr(addr))
+	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 	if (flags & IGC_MAC_STATE_SRC_ADDR)
 		return -ENOTSUPP;
@@ -2217,7 +2227,7 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		adapter->mac_table[i].queue = queue;
 		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
 
-		igc_rar_set_index(adapter, i);
+		igc_set_mac_filter_hw(adapter, i, addr, queue);
 		return 0;
 	}
 
@@ -2261,13 +2271,14 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 			adapter->mac_table[i].state =
 				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
 			adapter->mac_table[i].queue = 0;
+			igc_set_mac_filter_hw(adapter, 0, addr, -1);
 		} else {
 			adapter->mac_table[i].state = 0;
 			adapter->mac_table[i].queue = 0;
 			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
+			igc_clear_mac_filter_hw(adapter, i);
 		}
 
-		igc_rar_set_index(adapter, i);
 		return 0;
 	}
 
-- 
2.26.0

[Intel-wired-lan] [PATCH 06/12] igc: Improve address check in igc_del_mac_filter()

[Andre Guedes]

igc_add_mac_filter() doesn't allow filters with invalid MAC address to
be added to adapter->mac_table so, in igc_del_mac_filter(), we can early
return if MAC address is invalid. No need to traverse the table.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 0ca7afaf6fc7..a71f1a5ca27c 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2247,7 +2247,7 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	int rar_entries = hw->mac.rar_entry_count;
 	int i;
 
-	if (is_zero_ether_addr(addr))
+	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 
 	/* Search for matching entry in the MAC table based on given address
-- 
2.25.0

[Intel-wired-lan] [PATCH 06/12] igc: Improve address check in igc_del_mac_filter()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 06/12] igc: Improve address check in
> igc_del_mac_filter()
> 
> igc_add_mac_filter() doesn't allow filters with invalid MAC address to
> be added to adapter->mac_table so, in igc_del_mac_filter(), we can early
> return if MAC address is invalid. No need to traverse the table.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in igc_del_mac_filter()

[Andre Guedes]

igc_add_mac_filter() doesn't allow us to have more than one entry with
the same address and address type in adapter->mac_table so checking if
'queue' matches in igc_del_mac_filter() isn't necessary. This patch
removes that check.

This patch also takes the opportunity to improve the igc_del_mac_filter
documentation and remove comment which is not applicable to this I225
controller.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index a71f1a5ca27c..8a3cae2367d4 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2234,14 +2234,17 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	return -ENOSPC;
 }
 
-/* Remove a MAC filter for 'addr' directing matching traffic to
- * 'queue', 'flags' is used to indicate what kind of match need to be
- * removed, match is by default for the destination address, if
- * matching by source address is to be removed the flag
- * IGC_MAC_STATE_SRC_ADDR can be used.
- */
+/* Delete MAC address filter from adapter.
+ *
+ * @adapter: Pointer to adapter where the filter should be deleted from.
+ * @addr: MAC address.
+ * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
+ * address.
+ *
+ * In case of success, returns 0. Otherwise, it returns a negative errno code.
+  */
 static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+			      const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2250,17 +2253,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 
-	/* Search for matching entry in the MAC table based on given address
-	 * and queue. Do not touch entries at the end of the table reserved
-	 * for the VF MAC addresses.
-	 */
 	for (i = 0; i < rar_entries; i++) {
 		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
 			continue;
 		if (flags && (adapter->mac_table[i].state & flags) != flags)
 			continue;
-		if (adapter->mac_table[i].queue != queue)
-			continue;
 		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
 			continue;
 
@@ -2296,7 +2293,7 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	return igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
+	return igc_del_mac_filter(adapter, addr, 0);
 }
 
 /**
@@ -3741,7 +3738,7 @@ int igc_add_mac_steering_filter(struct igc_adapter *adapter,
 int igc_del_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_del_mac_filter(adapter, addr, queue,
+	return igc_del_mac_filter(adapter, addr,
 				  IGC_MAC_STATE_QUEUE_STEERING | flags);
 }
 
-- 
2.25.0

[Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in igc_del_mac_filter()

[Neftin, Sasha]

On 3/19/2020 01:00, Andre Guedes wrote:
> igc_add_mac_filter() doesn't allow us to have more than one entry with
> the same address and address type in adapter->mac_table so checking if
> 'queue' matches in igc_del_mac_filter() isn't necessary. This patch
> removes that check.
> 
> This patch also takes the opportunity to improve the igc_del_mac_filter
> documentation and remove comment which is not applicable to this I225
> controller.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc_main.c | 27 ++++++++++-------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index a71f1a5ca27c..8a3cae2367d4 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2234,14 +2234,17 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   	return -ENOSPC;
>   }
>   
> -/* Remove a MAC filter for 'addr' directing matching traffic to
> - * 'queue', 'flags' is used to indicate what kind of match need to be
> - * removed, match is by default for the destination address, if
> - * matching by source address is to be removed the flag
> - * IGC_MAC_STATE_SRC_ADDR can be used.
> - */
> +/* Delete MAC address filter from adapter.
> + *
> + * @adapter: Pointer to adapter where the filter should be deleted from.
> + * @addr: MAC address.
> + * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
> + * address.
> + *
> + * In case of success, returns 0. Otherwise, it returns a negative errno code.
Block comments should align the * on each line (please, remove one space 
before *)
> +  */ >   static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 
*addr,
> -			      const u8 queue, const u8 flags)
> +			      const u8 flags)
>   {
>   	struct igc_hw *hw = &adapter->hw;
>   	int rar_entries = hw->mac.rar_entry_count;
> @@ -2250,17 +2253,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   	if (!is_valid_ether_addr(addr))
>   		return -EINVAL;
>   
> -	/* Search for matching entry in the MAC table based on given address
> -	 * and queue. Do not touch entries at the end of the table reserved
> -	 * for the VF MAC addresses.
> -	 */
>   	for (i = 0; i < rar_entries; i++) {
>   		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
>   			continue;
>   		if (flags && (adapter->mac_table[i].state & flags) != flags)
>   			continue;
> -		if (adapter->mac_table[i].queue != queue)
> -			continue;
>   		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>   			continue;
>   
> @@ -2296,7 +2293,7 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>   {
>   	struct igc_adapter *adapter = netdev_priv(netdev);
>   
> -	return igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
> +	return igc_del_mac_filter(adapter, addr, 0);
>   }
>   
>   /**
> @@ -3741,7 +3738,7 @@ int igc_add_mac_steering_filter(struct igc_adapter *adapter,
>   int igc_del_mac_steering_filter(struct igc_adapter *adapter,
>   				const u8 *addr, u8 queue, u8 flags)
>   {
> -	return igc_del_mac_filter(adapter, addr, queue,
> +	return igc_del_mac_filter(adapter, addr,
>   				  IGC_MAC_STATE_QUEUE_STEERING | flags);
>   }
>   
> 

[Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in igc_del_mac_filter()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Neftin, Sasha
> Sent: Tuesday, March 31, 2020 8:53 AM
> To: intel-wired-lan at osuosl.org; Lifshits, Vitaly <vitaly.lifshits@intel.com>; Avivi,
> Amir <amir.avivi@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in
> igc_del_mac_filter()
> 
> On 3/19/2020 01:00, Andre Guedes wrote:
> > igc_add_mac_filter() doesn't allow us to have more than one entry with
> > the same address and address type in adapter->mac_table so checking if
> > 'queue' matches in igc_del_mac_filter() isn't necessary. This patch
> > removes that check.
> >
> > This patch also takes the opportunity to improve the igc_del_mac_filter
> > documentation and remove comment which is not applicable to this I225
> > controller.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > ---
> >   drivers/net/ethernet/intel/igc/igc_main.c | 27 ++++++++++-------------
> >   1 file changed, 12 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> > index a71f1a5ca27c..8a3cae2367d4 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -2234,14 +2234,17 @@ static int igc_add_mac_filter(struct igc_adapter
> *adapter, const u8 *addr,
> >   	return -ENOSPC;
> >   }
> >
> > -/* Remove a MAC filter for 'addr' directing matching traffic to
> > - * 'queue', 'flags' is used to indicate what kind of match need to be
> > - * removed, match is by default for the destination address, if
> > - * matching by source address is to be removed the flag
> > - * IGC_MAC_STATE_SRC_ADDR can be used.
> > - */
> > +/* Delete MAC address filter from adapter.
> > + *
> > + * @adapter: Pointer to adapter where the filter should be deleted from.
> > + * @addr: MAC address.
> > + * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a
> source
> > + * address.
> > + *
> > + * In case of success, returns 0. Otherwise, it returns a negative errno code.
> Block comments should align the * on each line (please, remove one space
> before *)
> > +  */ >   static int igc_del_mac_filter(struct igc_adapter *adapter, const u8
> *addr, 

Yes, that comment block throws a checkpatch warning:
-------------------
u1322:[1]/usr/src/kernels/next-queue> git format-patch $item -1 --stdout|./scripts/checkpatch.pl -
WARNING: Block comments should align the * on each line
#42: FILE: drivers/net/ethernet/intel/igc/igc_main.c:2245:
+ * In case of success, returns 0. Otherwise, it returns a negative errno code.
+  */

total: 0 errors, 1 warnings, 0 checks, 57 lines checked

Aside from that:
Tested-by: Aaron Brown <aaron.f.brown

[Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in igc_del_mac_filter()

[Kirsher, Jeffrey T]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Brown, Aaron F
> Sent: Tuesday, March 31, 2020 13:48
> To: Neftin, Sasha <sasha.neftin@intel.com>; intel-wired-lan at osuosl.org;
> Lifshits, Vitaly <vitaly.lifshits@intel.com>; Avivi, Amir <amir.avivi@intel.com>;
> Guedes, Andre <andre.guedes@intel.com>
> Subject: Re: [Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check in
> igc_del_mac_filter()
> 
> > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> > Of Neftin, Sasha
> > Sent: Tuesday, March 31, 2020 8:53 AM
> > To: intel-wired-lan at osuosl.org; Lifshits, Vitaly
> > <vitaly.lifshits@intel.com>; Avivi, Amir <amir.avivi@intel.com>
> > Subject: Re: [Intel-wired-lan] [PATCH 07/12] igc: Remove 'queue' check
> > in
> > igc_del_mac_filter()
> >
> > On 3/19/2020 01:00, Andre Guedes wrote:
> > > igc_add_mac_filter() doesn't allow us to have more than one entry
> > > with the same address and address type in adapter->mac_table so
> > > checking if 'queue' matches in igc_del_mac_filter() isn't necessary.
> > > This patch removes that check.
> > >
> > > This patch also takes the opportunity to improve the
> > > igc_del_mac_filter documentation and remove comment which is not
> > > applicable to this I225 controller.
> > >
> > > Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> > > ---
> > >   drivers/net/ethernet/intel/igc/igc_main.c | 27 ++++++++++-------------
> > >   1 file changed, 12 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c
> > b/drivers/net/ethernet/intel/igc/igc_main.c
> > > index a71f1a5ca27c..8a3cae2367d4 100644
> > > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > > @@ -2234,14 +2234,17 @@ static int igc_add_mac_filter(struct
> > > igc_adapter
> > *adapter, const u8 *addr,
> > >   	return -ENOSPC;
> > >   }
> > >
> > > -/* Remove a MAC filter for 'addr' directing matching traffic to
> > > - * 'queue', 'flags' is used to indicate what kind of match need to
> > > be
> > > - * removed, match is by default for the destination address, if
> > > - * matching by source address is to be removed the flag
> > > - * IGC_MAC_STATE_SRC_ADDR can be used.
> > > - */
> > > +/* Delete MAC address filter from adapter.
> > > + *
> > > + * @adapter: Pointer to adapter where the filter should be deleted from.
> > > + * @addr: MAC address.
> > > + * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a
> > source
> > > + * address.
> > > + *
> > > + * In case of success, returns 0. Otherwise, it returns a negative errno
> code.
> > Block comments should align the * on each line (please, remove one
> > space before *)
> > > +  */ >   static int igc_del_mac_filter(struct igc_adapter *adapter, const u8
> > *addr,
> 
> Yes, that comment block throws a checkpatch warning:
> -------------------
> u1322:[1]/usr/src/kernels/next-queue> git format-patch $item -1 --
> stdout|./scripts/checkpatch.pl -
> WARNING: Block comments should align the * on each line
> #42: FILE: drivers/net/ethernet/intel/igc/igc_main.c:2245:
> + * In case of success, returns 0. Otherwise, it returns a negative errno code.
> +  */
> 
> total: 0 errors, 1 warnings, 0 checks, 57 lines checked
>

[Kirsher, Jeffrey T] 
Yeah, Andre let me know and I will fix it when I go to push the patch upstream.
 
> Aside from that:
> Tested-by: Aaron Brown <aaron.f.brown
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [PATCH v2 07/12] igc: Remove 'queue' check in igc_del_mac_filter()

[Andre Guedes]

igc_add_mac_filter() doesn't allow us to have more than one entry with
the same address and address type in adapter->mac_table so checking if
'queue' matches in igc_del_mac_filter() isn't necessary. This patch
removes that check.

This patch also takes the opportunity to improve the igc_del_mac_filter
documentation and remove comment which is not applicable to this I225
controller.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
---
v2: Fix function documentation to follow kernel-doc style.
---
 drivers/net/ethernet/intel/igc/igc_main.c | 25 ++++++++++-------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 37c8980a43a0..961a7c642e44 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2234,14 +2234,17 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	return -ENOSPC;
 }
 
-/* Remove a MAC filter for 'addr' directing matching traffic to
- * 'queue', 'flags' is used to indicate what kind of match need to be
- * removed, match is by default for the destination address, if
- * matching by source address is to be removed the flag
- * IGC_MAC_STATE_SRC_ADDR can be used.
+/**
+ * igc_del_mac_filter() - Delete MAC address filter.
+ * @adapter: Pointer to adapter where the filter should be deleted from.
+ * @addr: MAC address.
+ * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
+ *         address.
+ *
+ * Return: 0 in case of success, negative errno code otherwise.
  */
 static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+			      const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2250,17 +2253,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 
-	/* Search for matching entry in the MAC table based on given address
-	 * and queue. Do not touch entries at the end of the table reserved
-	 * for the VF MAC addresses.
-	 */
 	for (i = 0; i < rar_entries; i++) {
 		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
 			continue;
 		if (flags && (adapter->mac_table[i].state & flags) != flags)
 			continue;
-		if (adapter->mac_table[i].queue != queue)
-			continue;
 		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
 			continue;
 
@@ -2296,7 +2293,7 @@ static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	return igc_del_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
+	return igc_del_mac_filter(adapter, addr, 0);
 }
 
 /**
@@ -3741,7 +3738,7 @@ int igc_add_mac_steering_filter(struct igc_adapter *adapter,
 int igc_del_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_del_mac_filter(adapter, addr, queue,
+	return igc_del_mac_filter(adapter, addr,
 				  IGC_MAC_STATE_QUEUE_STEERING | flags);
 }
 
-- 
2.26.0

[Intel-wired-lan] [PATCH 08/12] igc: Remove IGC_MAC_STATE_QUEUE_STEERING

[Andre Guedes]

The IGC_MAC_STATE_QUEUE_STEERING bit in mac_table[i].state is
utilized to indicate that frames matching the filter are assigned to
mac_table[i].queue. This bit is not strictly necessary since we can
convey the same information as follows: queue == -1 means queue
assignment is disabled, otherwise it is enabled.

In addition to make the code simpler, this change fixes some awkward
situations where we pass a complete misleading 'queue' value such as in
igc_uc_sync().

So this patch removes IGC_MAC_STATE_QUEUE_STEERING and also takes the
opportunity to improve the igc_add_mac_filter documentation.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h      |  3 +-
 drivers/net/ethernet/intel/igc/igc_main.c | 34 +++++++++++++----------
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index e743f92a27c6..192cef07bdf7 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -470,14 +470,13 @@ struct igc_nfc_filter {
 
 struct igc_mac_addr {
 	u8 addr[ETH_ALEN];
-	u8 queue;
+	s8 queue;
 	u8 state; /* bitmask */
 };
 
 #define IGC_MAC_STATE_DEFAULT		0x1
 #define IGC_MAC_STATE_IN_USE		0x2
 #define IGC_MAC_STATE_SRC_ADDR		0x4
-#define IGC_MAC_STATE_QUEUE_STEERING	0x8
 
 #define IGC_MAX_RXNFC_FILTERS		16
 
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 8a3cae2367d4..273817252823 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -820,8 +820,9 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
 
 	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
 	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
+	mac_table->queue = -1;
 
-	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
+	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, mac_table->queue);
 }
 
 /**
@@ -2197,13 +2198,20 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
 	return true;
 }
 
-/* Add a MAC filter for 'addr' directing matching traffic to 'queue',
- * 'flags' is used to indicate what kind of match is made, match is by
- * default for the destination address, if matching by source address
- * is desired the flag IGC_MAC_STATE_SRC_ADDR can be used.
- */
+/* Add MAC address filter to adapter.
+ *
+ * @adapter: Pointer to adapter where the filter should be added.
+ * @addr: MAC address.
+ * @queue: If non-negative, queue assignment feature is enabled and frames
+ * matching the filter are enqueued onto 'queue'. Otherwise, queue assignment
+ * is disabled.
+ * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
+ * address.
+ *
+ * In case of success, returns 0. Otherwise, it returns a negative errno code.
+  */
 static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+			      const s8 queue, const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2267,11 +2275,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		if (adapter->mac_table[i].state & IGC_MAC_STATE_DEFAULT) {
 			adapter->mac_table[i].state =
 				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
-			adapter->mac_table[i].queue = 0;
+			adapter->mac_table[i].queue = -1;
 			igc_set_mac_filter_hw(adapter, 0, addr, -1);
 		} else {
 			adapter->mac_table[i].state = 0;
-			adapter->mac_table[i].queue = 0;
+			adapter->mac_table[i].queue = -1;
 			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
 			igc_clear_mac_filter_hw(adapter, i);
 		}
@@ -2286,7 +2294,7 @@ static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	return igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
+	return igc_add_mac_filter(adapter, addr, -1, 0);
 }
 
 static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
@@ -3731,15 +3739,13 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 int igc_add_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_add_mac_filter(adapter, addr, queue,
-				  IGC_MAC_STATE_QUEUE_STEERING | flags);
+	return igc_add_mac_filter(adapter, addr, queue, flags);
 }
 
 int igc_del_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_del_mac_filter(adapter, addr,
-				  IGC_MAC_STATE_QUEUE_STEERING | flags);
+	return igc_del_mac_filter(adapter, addr, flags);
 }
 
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
-- 
2.25.0

[Intel-wired-lan] [PATCH 08/12] igc: Remove IGC_MAC_STATE_QUEUE_STEERING

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 08/12] igc: Remove
> IGC_MAC_STATE_QUEUE_STEERING
> 
> The IGC_MAC_STATE_QUEUE_STEERING bit in mac_table[i].state is
> utilized to indicate that frames matching the filter are assigned to
> mac_table[i].queue. This bit is not strictly necessary since we can
> convey the same information as follows: queue == -1 means queue
> assignment is disabled, otherwise it is enabled.
> 
> In addition to make the code simpler, this change fixes some awkward
> situations where we pass a complete misleading 'queue' value such as in
> igc_uc_sync().
> 
> So this patch removes IGC_MAC_STATE_QUEUE_STEERING and also takes the
> opportunity to improve the igc_add_mac_filter documentation.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h      |  3 +-
>  drivers/net/ethernet/intel/igc/igc_main.c | 34 +++++++++++++----------
>  2 files changed, 21 insertions(+), 16 deletions(-)

This one also throws the same basic checkpatch warning as the last:
------------------------------------
u1322:[1]/usr/src/kernels/next-queue> git format-patch $commit -1 --stdout|./scripts/checkpatch.pl -
WARNING: Block comments should align the * on each line
#42: FILE: drivers/net/ethernet/intel/igc/igc_main.c:2245:
+ * In case of success, returns 0. Otherwise, it returns a negative errno code.
+  */

total: 0 errors, 1 warnings, 0 checks, 57 lines checked
-----------------------------------
But otherwise:
Tested-by: Aaron Brown <aaron.f.brown@intel.com>

Jeff if you fix up the last patch go ahead and fix this one up to.

[Intel-wired-lan] [PATCH 08/12] igc: Remove IGC_MAC_STATE_QUEUE_STEERING

[Neftin, Sasha]

On 3/19/2020 01:00, Andre Guedes wrote:
> The IGC_MAC_STATE_QUEUE_STEERING bit in mac_table[i].state is
> utilized to indicate that frames matching the filter are assigned to
> mac_table[i].queue. This bit is not strictly necessary since we can
> convey the same information as follows: queue == -1 means queue
> assignment is disabled, otherwise it is enabled.
> 
> In addition to make the code simpler, this change fixes some awkward
> situations where we pass a complete misleading 'queue' value such as in
> igc_uc_sync().
> 
> So this patch removes IGC_MAC_STATE_QUEUE_STEERING and also takes the
> opportunity to improve the igc_add_mac_filter documentation.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>   drivers/net/ethernet/intel/igc/igc.h      |  3 +-
>   drivers/net/ethernet/intel/igc/igc_main.c | 34 +++++++++++++----------
>   2 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> index e743f92a27c6..192cef07bdf7 100644
> --- a/drivers/net/ethernet/intel/igc/igc.h
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -470,14 +470,13 @@ struct igc_nfc_filter {
>   
>   struct igc_mac_addr {
>   	u8 addr[ETH_ALEN];
> -	u8 queue;
> +	s8 queue;
>   	u8 state; /* bitmask */
>   };
>   
>   #define IGC_MAC_STATE_DEFAULT		0x1
>   #define IGC_MAC_STATE_IN_USE		0x2
>   #define IGC_MAC_STATE_SRC_ADDR		0x4
> -#define IGC_MAC_STATE_QUEUE_STEERING	0x8
>   
>   #define IGC_MAX_RXNFC_FILTERS		16
>   
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 8a3cae2367d4..273817252823 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -820,8 +820,9 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
>   
>   	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
>   	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
> +	mac_table->queue = -1;
>   
> -	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
> +	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, mac_table->queue);
>   }
>   
>   /**
> @@ -2197,13 +2198,20 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
>   	return true;
>   }
>   
> -/* Add a MAC filter for 'addr' directing matching traffic to 'queue',
> - * 'flags' is used to indicate what kind of match is made, match is by
> - * default for the destination address, if matching by source address
> - * is desired the flag IGC_MAC_STATE_SRC_ADDR can be used.
> - */
Please, add /** before declaration (as we discussed in prev patches review)
> +/* Add MAC address filter to adapter.
> + *
> + * @adapter: Pointer to adapter where the filter should be added.
> + * @addr: MAC address.
> + * @queue: If non-negative, queue assignment feature is enabled and frames
> + * matching the filter are enqueued onto 'queue'. Otherwise, queue assignment
> + * is disabled.
> + * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
> + * address.
> + *
> + * In case of success, returns 0. Otherwise, it returns a negative errno code.
> +  */
>   static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
> -			      const u8 queue, const u8 flags)
> +			      const s8 queue, const u8 flags)
>   {
>   	struct igc_hw *hw = &adapter->hw;
>   	int rar_entries = hw->mac.rar_entry_count;
> @@ -2267,11 +2275,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
>   		if (adapter->mac_table[i].state & IGC_MAC_STATE_DEFAULT) {
>   			adapter->mac_table[i].state =
>   				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
> -			adapter->mac_table[i].queue = 0;
> +			adapter->mac_table[i].queue = -1;
>   			igc_set_mac_filter_hw(adapter, 0, addr, -1);
>   		} else {
>   			adapter->mac_table[i].state = 0;
> -			adapter->mac_table[i].queue = 0;
> +			adapter->mac_table[i].queue = -1;
>   			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>   			igc_clear_mac_filter_hw(adapter, i);
>   		}
> @@ -2286,7 +2294,7 @@ static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
>   {
>   	struct igc_adapter *adapter = netdev_priv(netdev);
>   
> -	return igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
> +	return igc_add_mac_filter(adapter, addr, -1, 0);
>   }
>   
>   static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
> @@ -3731,15 +3739,13 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
>   int igc_add_mac_steering_filter(struct igc_adapter *adapter,
>   				const u8 *addr, u8 queue, u8 flags)
>   {
> -	return igc_add_mac_filter(adapter, addr, queue,
> -				  IGC_MAC_STATE_QUEUE_STEERING | flags);
> +	return igc_add_mac_filter(adapter, addr, queue, flags);
>   }
>   
>   int igc_del_mac_steering_filter(struct igc_adapter *adapter,
>   				const u8 *addr, u8 queue, u8 flags)
>   {
> -	return igc_del_mac_filter(adapter, addr,
> -				  IGC_MAC_STATE_QUEUE_STEERING | flags);
> +	return igc_del_mac_filter(adapter, addr, flags);
>   }
>   
>   static void igc_tsync_interrupt(struct igc_adapter *adapter)
> 

[Intel-wired-lan] [PATCH v2 08/12] igc: Remove IGC_MAC_STATE_QUEUE_STEERING

[Andre Guedes]

The IGC_MAC_STATE_QUEUE_STEERING bit in mac_table[i].state is
utilized to indicate that frames matching the filter are assigned to
mac_table[i].queue. This bit is not strictly necessary since we can
convey the same information as follows: queue == -1 means queue
assignment is disabled, otherwise it is enabled.

In addition to make the code simpler, this change fixes some awkward
situations where we pass a complete misleading 'queue' value such as in
igc_uc_sync().

So this patch removes IGC_MAC_STATE_QUEUE_STEERING and also takes the
opportunity to improve the igc_add_mac_filter documentation.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
Tested-by: Aaron Brown <aaron.f.brown@intel.com>
---
v2: Fix function documentation to follow kernel-doc style.
---
 drivers/net/ethernet/intel/igc/igc.h      |  3 +--
 drivers/net/ethernet/intel/igc/igc_main.c | 32 ++++++++++++++---------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index e743f92a27c6..192cef07bdf7 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -470,14 +470,13 @@ struct igc_nfc_filter {
 
 struct igc_mac_addr {
 	u8 addr[ETH_ALEN];
-	u8 queue;
+	s8 queue;
 	u8 state; /* bitmask */
 };
 
 #define IGC_MAC_STATE_DEFAULT		0x1
 #define IGC_MAC_STATE_IN_USE		0x2
 #define IGC_MAC_STATE_SRC_ADDR		0x4
-#define IGC_MAC_STATE_QUEUE_STEERING	0x8
 
 #define IGC_MAX_RXNFC_FILTERS		16
 
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 961a7c642e44..332ad5841a9f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -820,8 +820,9 @@ static void igc_set_default_mac_filter(struct igc_adapter *adapter)
 
 	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
 	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
+	mac_table->queue = -1;
 
-	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, -1);
+	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, mac_table->queue);
 }
 
 /**
@@ -2197,13 +2198,20 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
 	return true;
 }
 
-/* Add a MAC filter for 'addr' directing matching traffic to 'queue',
- * 'flags' is used to indicate what kind of match is made, match is by
- * default for the destination address, if matching by source address
- * is desired the flag IGC_MAC_STATE_SRC_ADDR can be used.
+/**
+ * igc_add_mac_filter() - Add MAC address filter.
+ * @adapter: Pointer to adapter where the filter should be added.
+ * @addr: MAC address.
+ * @queue: If non-negative, queue assignment feature is enabled and frames
+ *         matching the filter are enqueued onto 'queue'. Otherwise, queue
+ *         assignment is disabled.
+ * @flags: Set IGC_MAC_STATE_SRC_ADDR bit to indicate @address is a source
+ *         address.
+ *
+ * Return: 0 in case of success, negative errno code otherwise.
  */
 static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+			      const s8 queue, const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2267,11 +2275,11 @@ static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		if (adapter->mac_table[i].state & IGC_MAC_STATE_DEFAULT) {
 			adapter->mac_table[i].state =
 				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
-			adapter->mac_table[i].queue = 0;
+			adapter->mac_table[i].queue = -1;
 			igc_set_mac_filter_hw(adapter, 0, addr, -1);
 		} else {
 			adapter->mac_table[i].state = 0;
-			adapter->mac_table[i].queue = 0;
+			adapter->mac_table[i].queue = -1;
 			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
 			igc_clear_mac_filter_hw(adapter, i);
 		}
@@ -2286,7 +2294,7 @@ static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igc_adapter *adapter = netdev_priv(netdev);
 
-	return igc_add_mac_filter(adapter, addr, adapter->num_rx_queues, 0);
+	return igc_add_mac_filter(adapter, addr, -1, 0);
 }
 
 static int igc_uc_unsync(struct net_device *netdev, const unsigned char *addr)
@@ -3731,15 +3739,13 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 int igc_add_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_add_mac_filter(adapter, addr, queue,
-				  IGC_MAC_STATE_QUEUE_STEERING | flags);
+	return igc_add_mac_filter(adapter, addr, queue, flags);
 }
 
 int igc_del_mac_steering_filter(struct igc_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags)
 {
-	return igc_del_mac_filter(adapter, addr,
-				  IGC_MAC_STATE_QUEUE_STEERING | flags);
+	return igc_del_mac_filter(adapter, addr, flags);
 }
 
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
-- 
2.26.0

[Intel-wired-lan] [PATCH 09/12] igc: Remove igc_*_mac_steering_filter() wrappers

[Andre Guedes]

With the previous two patches, igc_add_mac_steering_filter() and
igc_del_mac_steering_filter() became a pointless wrapper of
igc_add_mac_filter() and igc_del_mac_filter().

This patch removes these wrappers and update callers to call
igc_add_mac_filter() and igc_del_mac_filter() directly.

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

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 192cef07bdf7..6c18c74a68fb 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -231,10 +231,10 @@ void igc_write_rss_indir_tbl(struct igc_adapter *adapter);
 bool igc_has_link(struct igc_adapter *adapter);
 void igc_reset(struct igc_adapter *adapter);
 int igc_set_spd_dplx(struct igc_adapter *adapter, u32 spd, u8 dplx);
-int igc_add_mac_steering_filter(struct igc_adapter *adapter,
-				const u8 *addr, u8 queue, u8 flags);
-int igc_del_mac_steering_filter(struct igc_adapter *adapter,
-				const u8 *addr, u8 queue, u8 flags);
+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);
 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 9ef133fe6e40..93608bd57c29 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1267,18 +1267,16 @@ int igc_add_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 	}
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_DST_MAC_ADDR) {
-		err = igc_add_mac_steering_filter(adapter,
-						  input->filter.dst_addr,
-						  input->action, 0);
+		err = igc_add_mac_filter(adapter, input->filter.dst_addr,
+					 input->action, 0);
 		if (err)
 			return err;
 	}
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR) {
-		err = igc_add_mac_steering_filter(adapter,
-						  input->filter.src_addr,
-						  input->action,
-						  IGC_MAC_STATE_SRC_ADDR);
+		err = igc_add_mac_filter(adapter, input->filter.src_addr,
+					 input->action,
+					 IGC_MAC_STATE_SRC_ADDR);
 		if (err)
 			return err;
 	}
@@ -1332,13 +1330,11 @@ int igc_erase_filter(struct igc_adapter *adapter, struct igc_nfc_filter *input)
 					   ntohs(input->filter.vlan_tci));
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_SRC_MAC_ADDR)
-		igc_del_mac_steering_filter(adapter, input->filter.src_addr,
-					    input->action,
-					    IGC_MAC_STATE_SRC_ADDR);
+		igc_del_mac_filter(adapter, input->filter.src_addr,
+				   IGC_MAC_STATE_SRC_ADDR);
 
 	if (input->filter.match_flags & IGC_FILTER_FLAG_DST_MAC_ADDR)
-		igc_del_mac_steering_filter(adapter, input->filter.dst_addr,
-					    input->action, 0);
+		igc_del_mac_filter(adapter, input->filter.dst_addr, 0);
 
 	return 0;
 }
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 273817252823..25d0431f4f4a 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2210,8 +2210,8 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
  *
  * In case of success, returns 0. Otherwise, it returns a negative errno code.
   */
-static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const s8 queue, const u8 flags)
+int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
+		       const s8 queue, const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -2251,8 +2251,8 @@ static int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
  *
  * In case of success, returns 0. Otherwise, it returns a negative errno code.
   */
-static int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
-			      const u8 flags)
+int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
+		       const u8 flags)
 {
 	struct igc_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count;
@@ -3736,18 +3736,6 @@ igc_features_check(struct sk_buff *skb, struct net_device *dev,
 	return features;
 }
 
-int igc_add_mac_steering_filter(struct igc_adapter *adapter,
-				const u8 *addr, u8 queue, u8 flags)
-{
-	return igc_add_mac_filter(adapter, addr, queue, flags);
-}
-
-int igc_del_mac_steering_filter(struct igc_adapter *adapter,
-				const u8 *addr, u8 queue, u8 flags)
-{
-	return igc_del_mac_filter(adapter, addr, flags);
-}
-
 static void igc_tsync_interrupt(struct igc_adapter *adapter)
 {
 	struct igc_hw *hw = &adapter->hw;
-- 
2.25.0

[Intel-wired-lan] [PATCH 09/12] igc: Remove igc_*_mac_steering_filter() wrappers

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 09/12] igc: Remove
> igc_*_mac_steering_filter() wrappers
> 
> With the previous two patches, igc_add_mac_steering_filter() and
> igc_del_mac_steering_filter() became a pointless wrapper of
> igc_add_mac_filter() and igc_del_mac_filter().
> 
> This patch removes these wrappers and update callers to call
> igc_add_mac_filter() and igc_del_mac_filter() directly.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc.h         |  8 ++++----
>  drivers/net/ethernet/intel/igc/igc_ethtool.c | 20 ++++++++------------
>  drivers/net/ethernet/intel/igc/igc_main.c    | 20 ++++----------------
>  3 files changed, 16 insertions(+), 32 deletions(-)
> 

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

[Intel-wired-lan] [PATCH 10/12] igc: Refactor igc_mac_entry_can_be_used()

[Andre Guedes]

The helper igc_mac_entry_can_be_used() implementation is a bit
convoluted since it does two different things: find a not-in-use slot
in mac_table or find an in-use slot where the address and address type
match. This patch does a code refactoring and break it up into two
helper functions.

With this patch we might traverse mac_table twice in some situations,
but this is not harmful performance-wise (mac_table has only 16 entries
and adding mac filters is not hot-path), and it improves igc_add_mac_
filter() readability considerably.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 25d0431f4f4a..0264e424bd07 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2177,25 +2177,44 @@ static void igc_nfc_filter_restore(struct igc_adapter *adapter)
 	spin_unlock(&adapter->nfc_lock);
 }
 
-/* If the filter to be added and an already existing filter express
- * the same address and address type, it should be possible to only
- * override the other configurations, for example the queue to steer
- * traffic.
- */
-static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
-				      const u8 *addr, const u8 flags)
+static int igc_find_mac_filter(struct igc_adapter *adapter, const u8 *addr,
+			       u8 flags)
 {
-	if (!(entry->state & IGC_MAC_STATE_IN_USE))
-		return true;
+	int max_entries = adapter->hw.mac.rar_entry_count;
+	struct igc_mac_addr *entry;
+	int i;
 
-	if ((entry->state & IGC_MAC_STATE_SRC_ADDR) !=
-	    (flags & IGC_MAC_STATE_SRC_ADDR))
-		return false;
+	for (i = 0; i < max_entries; i++) {
+		entry = &adapter->mac_table[i];
 
-	if (!ether_addr_equal(addr, entry->addr))
-		return false;
+		if (!(entry->state & IGC_MAC_STATE_IN_USE))
+			continue;
+		if (!ether_addr_equal(addr, entry->addr))
+			continue;
+		if ((entry->state & IGC_MAC_STATE_SRC_ADDR) !=
+		    (flags & IGC_MAC_STATE_SRC_ADDR))
+			continue;
 
-	return true;
+		return i;
+	}
+
+	return -1;
+}
+
+static int igc_get_avail_mac_filter_slot(struct igc_adapter *adapter)
+{
+	int max_entries = adapter->hw.mac.rar_entry_count;
+	struct igc_mac_addr *entry;
+	int i;
+
+	for (i = 0; i < max_entries; i++) {
+		entry = &adapter->mac_table[i];
+
+		if (!(entry->state & IGC_MAC_STATE_IN_USE))
+			return i;
+	}
+
+	return -1;
 }
 
 /* Add MAC address filter to adapter.
@@ -2213,33 +2232,28 @@ static bool igc_mac_entry_can_be_used(const struct igc_mac_addr *entry,
 int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const s8 queue, const u8 flags)
 {
-	struct igc_hw *hw = &adapter->hw;
-	int rar_entries = hw->mac.rar_entry_count;
-	int i;
+	int index;
 
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 	if (flags & IGC_MAC_STATE_SRC_ADDR)
 		return -ENOTSUPP;
 
-	/* Search for the first empty entry in the MAC table.
-	 * Do not touch entries at the end of the table reserved for the VF MAC
-	 * addresses.
-	 */
-	for (i = 0; i < rar_entries; i++) {
-		if (!igc_mac_entry_can_be_used(&adapter->mac_table[i],
-					       addr, flags))
-			continue;
+	index = igc_find_mac_filter(adapter, addr, flags);
+	if (index >= 0)
+		goto update_queue_assignment;
 
-		ether_addr_copy(adapter->mac_table[i].addr, addr);
-		adapter->mac_table[i].queue = queue;
-		adapter->mac_table[i].state |= IGC_MAC_STATE_IN_USE | flags;
+	index = igc_get_avail_mac_filter_slot(adapter);
+	if (index < 0)
+		return -ENOSPC;
 
-		igc_set_mac_filter_hw(adapter, i, addr, queue);
-		return 0;
-	}
+	ether_addr_copy(adapter->mac_table[index].addr, addr);
+	adapter->mac_table[index].state |= IGC_MAC_STATE_IN_USE | flags;
+update_queue_assignment:
+	adapter->mac_table[index].queue = queue;
 
-	return -ENOSPC;
+	igc_set_mac_filter_hw(adapter, index, addr, queue);
+	return 0;
 }
 
 /* Delete MAC address filter from adapter.
-- 
2.25.0

[Intel-wired-lan] [PATCH 10/12] igc: Refactor igc_mac_entry_can_be_used()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 10/12] igc: Refactor
> igc_mac_entry_can_be_used()
> 
> The helper igc_mac_entry_can_be_used() implementation is a bit
> convoluted since it does two different things: find a not-in-use slot
> in mac_table or find an in-use slot where the address and address type
> match. This patch does a code refactoring and break it up into two
> helper functions.
> 
> With this patch we might traverse mac_table twice in some situations,
> but this is not harmful performance-wise (mac_table has only 16 entries
> and adding mac filters is not hot-path), and it improves igc_add_mac_
> filter() readability considerably.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 80 +++++++++++++----------
>  1 file changed, 47 insertions(+), 33 deletions(-)
> 

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

[Intel-wired-lan] [PATCH 11/12] igc: Refactor igc_del_mac_filter()

[Andre Guedes]

This patch does a code refactoring in igc_del_mac_filter() so it uses
the new helper igc_find_mac_filter() and improves the comment about the
special handling when deleting the default filter.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 0264e424bd07..fa7cf33c58a9 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2268,40 +2268,33 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const u8 flags)
 {
-	struct igc_hw *hw = &adapter->hw;
-	int rar_entries = hw->mac.rar_entry_count;
-	int i;
+	struct igc_mac_addr *entry;
+	int index;
 
 	if (!is_valid_ether_addr(addr))
 		return -EINVAL;
 
-	for (i = 0; i < rar_entries; i++) {
-		if (!(adapter->mac_table[i].state & IGC_MAC_STATE_IN_USE))
-			continue;
-		if (flags && (adapter->mac_table[i].state & flags) != flags)
-			continue;
-		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
-			continue;
+	index = igc_find_mac_filter(adapter, addr, flags);
+	if (index < 0)
+		return -ENOENT;
 
-		/* When a filter for the default address is "deleted",
-		 * we return it to its initial configuration
-		 */
-		if (adapter->mac_table[i].state & IGC_MAC_STATE_DEFAULT) {
-			adapter->mac_table[i].state =
-				IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
-			adapter->mac_table[i].queue = -1;
-			igc_set_mac_filter_hw(adapter, 0, addr, -1);
-		} else {
-			adapter->mac_table[i].state = 0;
-			adapter->mac_table[i].queue = -1;
-			memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
-			igc_clear_mac_filter_hw(adapter, i);
-		}
+	entry = &adapter->mac_table[index];
 
-		return 0;
+	if (entry->state & IGC_MAC_STATE_DEFAULT) {
+		/* If this is the default filter, we don't actually delete it.
+		 * We just reset to its default value i.e. disable queue
+		 * assignment.
+		 */
+		entry->queue = -1;
+		igc_set_mac_filter_hw(adapter, 0, addr, entry->queue);
+	} else {
+		entry->state = 0;
+		entry->queue = -1;
+		memset(entry->addr, 0, ETH_ALEN);
+		igc_clear_mac_filter_hw(adapter, index);
 	}
 
-	return -ENOENT;
+	return 0;
 }
 
 static int igc_uc_sync(struct net_device *netdev, const unsigned char *addr)
-- 
2.25.0

[Intel-wired-lan] [PATCH 11/12] igc: Refactor igc_del_mac_filter()

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 11/12] igc: Refactor igc_del_mac_filter()
> 
> This patch does a code refactoring in igc_del_mac_filter() so it uses
> the new helper igc_find_mac_filter() and improves the comment about the
> special handling when deleting the default filter.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 45 ++++++++++-------------
>  1 file changed, 19 insertions(+), 26 deletions(-)
> 

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

[Intel-wired-lan] [PATCH 12/12] igc: Add debug messages to MAC filter code

[Andre Guedes]

This patch adds log messages to functions related to the MAC address
filtering code to ease debugging.

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

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index fa7cf33c58a9..abbe0e1e0cf5 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -776,6 +776,7 @@ static void igc_setup_tctl(struct igc_adapter *adapter)
 static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
 				  const u8 *addr, int queue)
 {
+	struct net_device *dev = adapter->netdev;
 	struct igc_hw *hw = &adapter->hw;
 	u32 ral, rah;
 
@@ -795,6 +796,8 @@ static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
 
 	wr32(IGC_RAL(index), ral);
 	wr32(IGC_RAH(index), rah);
+
+	netdev_dbg(dev, "MAC address filter set in HW: index %d", index);
 }
 
 /* Clear MAC address filter in hardware.
@@ -804,6 +807,7 @@ static void igc_set_mac_filter_hw(struct igc_adapter *adapter, int index,
  */
 static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
 {
+	struct net_device *dev = adapter->netdev;
 	struct igc_hw *hw = &adapter->hw;
 
 	if (WARN_ON(index >= hw->mac.rar_entry_count))
@@ -811,18 +815,24 @@ static void igc_clear_mac_filter_hw(struct igc_adapter *adapter, int index)
 
 	wr32(IGC_RAL(index), 0);
 	wr32(IGC_RAH(index), 0);
+
+	netdev_dbg(dev, "MAC address filter cleared in HW: index %d", index);
 }
 
 /* Set default MAC address for the PF in the first RAR entry */
 static void igc_set_default_mac_filter(struct igc_adapter *adapter)
 {
 	struct igc_mac_addr *mac_table = &adapter->mac_table[0];
+	struct net_device *dev = adapter->netdev;
+	u8 *addr = adapter->hw.mac.addr;
+
+	netdev_dbg(dev, "Set default MAC address filter: address %pM", addr);
 
-	ether_addr_copy(mac_table->addr, adapter->hw.mac.addr);
+	ether_addr_copy(mac_table->addr, addr);
 	mac_table->state = IGC_MAC_STATE_DEFAULT | IGC_MAC_STATE_IN_USE;
 	mac_table->queue = -1;
 
-	igc_set_mac_filter_hw(adapter, 0, mac_table->addr, mac_table->queue);
+	igc_set_mac_filter_hw(adapter, 0, addr, mac_table->queue);
 }
 
 /**
@@ -2232,6 +2242,7 @@ static int igc_get_avail_mac_filter_slot(struct igc_adapter *adapter)
 int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const s8 queue, const u8 flags)
 {
+	struct net_device *dev = adapter->netdev;
 	int index;
 
 	if (!is_valid_ether_addr(addr))
@@ -2247,6 +2258,9 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 	if (index < 0)
 		return -ENOSPC;
 
+	netdev_dbg(dev, "Add MAC address filter: index %d address %pM queue %d",
+		   index, addr, queue);
+
 	ether_addr_copy(adapter->mac_table[index].addr, addr);
 	adapter->mac_table[index].state |= IGC_MAC_STATE_IN_USE | flags;
 update_queue_assignment:
@@ -2268,6 +2282,7 @@ int igc_add_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		       const u8 flags)
 {
+	struct net_device *dev = adapter->netdev;
 	struct igc_mac_addr *entry;
 	int index;
 
@@ -2285,9 +2300,14 @@ int igc_del_mac_filter(struct igc_adapter *adapter, const u8 *addr,
 		 * We just reset to its default value i.e. disable queue
 		 * assignment.
 		 */
+		netdev_dbg(dev, "Disable default MAC filter queue assignment");
+
 		entry->queue = -1;
 		igc_set_mac_filter_hw(adapter, 0, addr, entry->queue);
 	} else {
+		netdev_dbg(dev, "Delete MAC address filter: index %d address %pM",
+			   index, addr);
+
 		entry->state = 0;
 		entry->queue = -1;
 		memset(entry->addr, 0, ETH_ALEN);
-- 
2.25.0

[Intel-wired-lan] [PATCH 12/12] igc: Add debug messages to MAC filter code

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Wednesday, March 18, 2020 4:01 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 12/12] igc: Add debug messages to MAC filter
> code
> 
> This patch adds log messages to functions related to the MAC address
> filtering code to ease debugging.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 24 +++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 

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