[next-queue PATCH 0/8] igb: offloading of receive filters

[next-queue PATCH 0/8] igb: offloading of receive filters

[Vinicius Costa Gomes]

Hi,

This series enables some ethtool and tc-flower filters to be offloaded
to igb-based network controllers. This is useful when the system
configurator want to steer kinds of traffic to a specific hardware
queue.

The first two commits are bug fixes.

The basis of this series is to export the internal API used to
configure address filters, so they can be used by ethtool, and
extending the functionality so an source address can be handled.

Then, we enable the tc-flower offloading implementation to re-use the
same infrastructure as ethtool, and storing them in the per-adapter
"nfc" (Network Filter Config?) list. But for consistency, for
destructive access they are separated, i.e. an filter added by
tc-flower can only be removed by tc-flower, but ethtool can read them
all.

Only support for VLAN Prio, Source and Destination MAC Address, and
Ethertype are supported for now.

Open question:
  - igb is initialized with the number of traffic classes as 1, if we
  want to use multiple traffic classes we need to increase this value,
  the only way I could find is to use mqprio (for example). Should igb
  be initialized with, say, the number of queues as its "num_tc"?


Vinicius Costa Gomes (8):
  igb: Fix not adding filter elements to the list
  igb: Fix queue selection on MAC filters on i210 and i211
  igb: Enable the hardware traffic class feature bit for igb models
  igb: Add support for MAC address filters specifying source addresses
  igb: Add support for ethtool MAC address filters
  igb: Add the skeletons for tc-flower offloading
  igb: Add support for adding offloaded clsflower filters
  igb: Add support for removing offloaded tc-flower filters

 drivers/net/ethernet/intel/igb/e1000_defines.h |   2 +
 drivers/net/ethernet/intel/igb/igb.h           |  15 ++
 drivers/net/ethernet/intel/igb/igb_ethtool.c   | 125 ++++++++++-
 drivers/net/ethernet/intel/igb/igb_main.c      | 294 +++++++++++++++++++++++--
 4 files changed, 409 insertions(+), 27 deletions(-)

--
2.16.2

[Intel-wired-lan] [next-queue PATCH 0/8] igb: offloading of receive filters

[Vinicius Costa Gomes]

Hi,

This series enables some ethtool and tc-flower filters to be offloaded
to igb-based network controllers. This is useful when the system
configurator want to steer kinds of traffic to a specific hardware
queue.

The first two commits are bug fixes.

The basis of this series is to export the internal API used to
configure address filters, so they can be used by ethtool, and
extending the functionality so an source address can be handled.

Then, we enable the tc-flower offloading implementation to re-use the
same infrastructure as ethtool, and storing them in the per-adapter
"nfc" (Network Filter Config?) list. But for consistency, for
destructive access they are separated, i.e. an filter added by
tc-flower can only be removed by tc-flower, but ethtool can read them
all.

Only support for VLAN Prio, Source and Destination MAC Address, and
Ethertype are supported for now.

Open question:
  - igb is initialized with the number of traffic classes as 1, if we
  want to use multiple traffic classes we need to increase this value,
  the only way I could find is to use mqprio (for example). Should igb
  be initialized with, say, the number of queues as its "num_tc"?


Vinicius Costa Gomes (8):
  igb: Fix not adding filter elements to the list
  igb: Fix queue selection on MAC filters on i210 and i211
  igb: Enable the hardware traffic class feature bit for igb models
  igb: Add support for MAC address filters specifying source addresses
  igb: Add support for ethtool MAC address filters
  igb: Add the skeletons for tc-flower offloading
  igb: Add support for adding offloaded clsflower filters
  igb: Add support for removing offloaded tc-flower filters

 drivers/net/ethernet/intel/igb/e1000_defines.h |   2 +
 drivers/net/ethernet/intel/igb/igb.h           |  15 ++
 drivers/net/ethernet/intel/igb/igb_ethtool.c   | 125 ++++++++++-
 drivers/net/ethernet/intel/igb/igb_main.c      | 294 +++++++++++++++++++++++--
 4 files changed, 409 insertions(+), 27 deletions(-)

--
2.16.2

[next-queue PATCH 1/8] igb: Fix not adding filter elements to the list

[Vinicius Costa Gomes]

Because the order of the parameters passes to 'hlist_add_behind()' was
inverted, the 'parent' node was added "behind" the 'input', as input
is not in the list, this causes the 'input' node to be lost.

Fixes: 0e71def25281 ("igb: add support of RX network flow classification")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 606e6761758f..143f0bb34e4d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2864,7 +2864,7 @@ static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
 
 	/* add filter to the list */
 	if (parent)
-		hlist_add_behind(&parent->nfc_node, &input->nfc_node);
+		hlist_add_behind(&input->nfc_node, &parent->nfc_node);
 	else
 		hlist_add_head(&input->nfc_node, &adapter->nfc_filter_list);
 
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 1/8] igb: Fix not adding filter elements to the list

[Vinicius Costa Gomes]

Because the order of the parameters passes to 'hlist_add_behind()' was
inverted, the 'parent' node was added "behind" the 'input', as input
is not in the list, this causes the 'input' node to be lost.

Fixes: 0e71def25281 ("igb: add support of RX network flow classification")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 606e6761758f..143f0bb34e4d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2864,7 +2864,7 @@ static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
 
 	/* add filter to the list */
 	if (parent)
-		hlist_add_behind(&parent->nfc_node, &input->nfc_node);
+		hlist_add_behind(&input->nfc_node, &parent->nfc_node);
 	else
 		hlist_add_head(&input->nfc_node, &adapter->nfc_filter_list);
 
-- 
2.16.2

[next-queue PATCH 2/8] igb: Fix queue selection on MAC filters on i210 and i211

[Vinicius Costa Gomes]

On the RAH registers there are semantic differences on the meaning of
the "queue" parameter for traffic steering depending on the controller
model: there is the 82575 meaning, which "queue" means a RX Hardware
Queue, and the i350 meaning, where it is a reception pool.

The previous behaviour was having no effect for i210 and i211 based
controllers because the QSEL bit of the RAH register wasn't being set.

This patch separates the condition in discrete cases, so the different
handling is clearer.

Fixes: 83c21335c876 ("igb: improve MAC filter handling")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c      | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 83cabff1e0ab..573bf177fd08 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -490,6 +490,7 @@
  * manageability enabled, allowing us room for 15 multicast addresses.
  */
 #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
+#define E1000_RAH_QSEL_ENABLE 0x10000000
 #define E1000_RAL_MAC_ADDR_LEN 4
 #define E1000_RAH_MAC_ADDR_LEN 2
 #define E1000_RAH_POOL_MASK 0x03FC0000
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b88fae785369..0ea32be07d71 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8741,12 +8741,19 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
 		if (is_valid_ether_addr(addr))
 			rar_high |= E1000_RAH_AV;
 
-		if (hw->mac.type == e1000_82575)
+		switch (hw->mac.type) {
+		case e1000_82575:
+		case e1000_i210:
+		case e1000_i211:
+			rar_high |= E1000_RAH_QSEL_ENABLE;
 			rar_high |= E1000_RAH_POOL_1 *
-				    adapter->mac_table[index].queue;
-		else
+				      adapter->mac_table[index].queue;
+			break;
+		default:
 			rar_high |= E1000_RAH_POOL_1 <<
-				    adapter->mac_table[index].queue;
+				adapter->mac_table[index].queue;
+			break;
+		}
 	}
 
 	wr32(E1000_RAL(index), rar_low);
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 2/8] igb: Fix queue selection on MAC filters on i210 and i211

[Vinicius Costa Gomes]

On the RAH registers there are semantic differences on the meaning of
the "queue" parameter for traffic steering depending on the controller
model: there is the 82575 meaning, which "queue" means a RX Hardware
Queue, and the i350 meaning, where it is a reception pool.

The previous behaviour was having no effect for i210 and i211 based
controllers because the QSEL bit of the RAH register wasn't being set.

This patch separates the condition in discrete cases, so the different
handling is clearer.

Fixes: 83c21335c876 ("igb: improve MAC filter handling")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c      | 15 +++++++++++----
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 83cabff1e0ab..573bf177fd08 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -490,6 +490,7 @@
  * manageability enabled, allowing us room for 15 multicast addresses.
  */
 #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
+#define E1000_RAH_QSEL_ENABLE 0x10000000
 #define E1000_RAL_MAC_ADDR_LEN 4
 #define E1000_RAH_MAC_ADDR_LEN 2
 #define E1000_RAH_POOL_MASK 0x03FC0000
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b88fae785369..0ea32be07d71 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8741,12 +8741,19 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
 		if (is_valid_ether_addr(addr))
 			rar_high |= E1000_RAH_AV;
 
-		if (hw->mac.type == e1000_82575)
+		switch (hw->mac.type) {
+		case e1000_82575:
+		case e1000_i210:
+		case e1000_i211:
+			rar_high |= E1000_RAH_QSEL_ENABLE;
 			rar_high |= E1000_RAH_POOL_1 *
-				    adapter->mac_table[index].queue;
-		else
+				      adapter->mac_table[index].queue;
+			break;
+		default:
 			rar_high |= E1000_RAH_POOL_1 <<
-				    adapter->mac_table[index].queue;
+				adapter->mac_table[index].queue;
+			break;
+		}
 	}
 
 	wr32(E1000_RAL(index), rar_low);
-- 
2.16.2

[next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Vinicius Costa Gomes]

This will allow functionality depending on the hardware being traffic
class aware to work. In particular the tc-flower offloading checks
verifies that this bit is set.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 0ea32be07d71..543aa99892eb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			       NETIF_F_HW_VLAN_CTAG_TX |
 			       NETIF_F_RXALL;
 
-	if (hw->mac.type >= e1000_i350)
-		netdev->hw_features |= NETIF_F_NTUPLE;
+	if (hw->mac.type >= e1000_i350) {
+		netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
+		netdev->features |= NETIF_F_HW_TC;
+	}
 
 	if (pci_using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Vinicius Costa Gomes]

This will allow functionality depending on the hardware being traffic
class aware to work. In particular the tc-flower offloading checks
verifies that this bit is set.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 0ea32be07d71..543aa99892eb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 			       NETIF_F_HW_VLAN_CTAG_TX |
 			       NETIF_F_RXALL;
 
-	if (hw->mac.type >= e1000_i350)
-		netdev->hw_features |= NETIF_F_NTUPLE;
+	if (hw->mac.type >= e1000_i350) {
+		netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
+		netdev->features |= NETIF_F_HW_TC;
+	}
 
 	if (pci_using_dac)
 		netdev->features |= NETIF_F_HIGHDMA;
-- 
2.16.2

[next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Vinicius Costa Gomes]

Makes it possible to direct packets to queues based on their source
address. Documents the expected usage of the 'flags' parameter.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
 drivers/net/ethernet/intel/igb/igb.h           |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 573bf177fd08..c6f552de30dd 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -490,6 +490,7 @@
  * manageability enabled, allowing us room for 15 multicast addresses.
  */
 #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
+#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
 #define E1000_RAH_QSEL_ENABLE 0x10000000
 #define E1000_RAL_MAC_ADDR_LEN 4
 #define E1000_RAH_MAC_ADDR_LEN 2
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 1c6b8d9176a8..d5cd5f6708d9 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -472,6 +472,7 @@ struct igb_mac_addr {
 
 #define IGB_MAC_STATE_DEFAULT	0x1
 #define IGB_MAC_STATE_IN_USE	0x2
+#define IGB_MAC_STATE_SRC_ADDR  0x4
 
 /* board specific private data structure */
 struct igb_adapter {
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 543aa99892eb..db66b697fe3b 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
 	igb_rar_set_index(adapter, 0);
 }
 
+/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
+ */
 static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue)
+			      const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 
 		ether_addr_copy(adapter->mac_table[i].addr, addr);
 		adapter->mac_table[i].queue = queue;
-		adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
+		adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);
 
 		igb_rar_set_index(adapter, i);
 		return i;
@@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
+ * IGB_MAC_STATE_SRC_ADDR can be used.
+ */
 static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue)
+			      const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 	 * for the VF MAC addresses.
 	 */
 	for (i = 0; i < rar_entries; i++) {
-		if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
+		if (!(adapter->mac_table[i].state &
+		      (IGB_MAC_STATE_IN_USE | flags)))
 			continue;
 		if (adapter->mac_table[i].queue != queue)
 			continue;
 		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
 			continue;
 
-		adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
+		adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);
 		memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
 		adapter->mac_table[i].queue = 0;
 
@@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int ret;
 
-	ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
+	ret = igb_add_mac_filter(adapter, addr,
+				 adapter->vfs_allocated_count, 0);
 
 	return min_t(int, ret, 0);
 }
@@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 
-	igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
+	igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
 
 	return 0;
 }
@@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
 			if (entry->vf == vf) {
 				entry->vf = -1;
 				entry->free = true;
-				igb_del_mac_filter(adapter, entry->vf_mac, vf);
+				igb_del_mac_filter(adapter,
+						   entry->vf_mac, vf, 0);
 			}
 		}
 		break;
@@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
 			entry->vf = vf;
 			ether_addr_copy(entry->vf_mac, addr);
 
-			ret = igb_add_mac_filter(adapter, addr, vf);
+			ret = igb_add_mac_filter(adapter, addr, vf, 0);
 			ret = min_t(int, ret, 0);
 		} else {
 			ret = -ENOSPC;
@@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
 		if (is_valid_ether_addr(addr))
 			rar_high |= E1000_RAH_AV;
 
+		if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
+			rar_high |= E1000_RAH_ASEL_SRC_ADDR;
+
 		switch (hw->mac.type) {
 		case e1000_82575:
 		case e1000_i210:
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Vinicius Costa Gomes]

Makes it possible to direct packets to queues based on their source
address. Documents the expected usage of the 'flags' parameter.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
 drivers/net/ethernet/intel/igb/igb.h           |  1 +
 drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
index 573bf177fd08..c6f552de30dd 100644
--- a/drivers/net/ethernet/intel/igb/e1000_defines.h
+++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
@@ -490,6 +490,7 @@
  * manageability enabled, allowing us room for 15 multicast addresses.
  */
 #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
+#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
 #define E1000_RAH_QSEL_ENABLE 0x10000000
 #define E1000_RAL_MAC_ADDR_LEN 4
 #define E1000_RAH_MAC_ADDR_LEN 2
diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 1c6b8d9176a8..d5cd5f6708d9 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -472,6 +472,7 @@ struct igb_mac_addr {
 
 #define IGB_MAC_STATE_DEFAULT	0x1
 #define IGB_MAC_STATE_IN_USE	0x2
+#define IGB_MAC_STATE_SRC_ADDR  0x4
 
 /* board specific private data structure */
 struct igb_adapter {
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 543aa99892eb..db66b697fe3b 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
 	igb_rar_set_index(adapter, 0);
 }
 
+/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
+ */
 static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue)
+			      const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 
 		ether_addr_copy(adapter->mac_table[i].addr, addr);
 		adapter->mac_table[i].queue = queue;
-		adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
+		adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);
 
 		igb_rar_set_index(adapter, i);
 		return i;
@@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
+ * IGB_MAC_STATE_SRC_ADDR can be used.
+ */
 static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue)
+			      const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 	 * for the VF MAC addresses.
 	 */
 	for (i = 0; i < rar_entries; i++) {
-		if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
+		if (!(adapter->mac_table[i].state &
+		      (IGB_MAC_STATE_IN_USE | flags)))
 			continue;
 		if (adapter->mac_table[i].queue != queue)
 			continue;
 		if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
 			continue;
 
-		adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
+		adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);
 		memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
 		adapter->mac_table[i].queue = 0;
 
@@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int ret;
 
-	ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
+	ret = igb_add_mac_filter(adapter, addr,
+				 adapter->vfs_allocated_count, 0);
 
 	return min_t(int, ret, 0);
 }
@@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 
-	igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
+	igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
 
 	return 0;
 }
@@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
 			if (entry->vf == vf) {
 				entry->vf = -1;
 				entry->free = true;
-				igb_del_mac_filter(adapter, entry->vf_mac, vf);
+				igb_del_mac_filter(adapter,
+						   entry->vf_mac, vf, 0);
 			}
 		}
 		break;
@@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
 			entry->vf = vf;
 			ether_addr_copy(entry->vf_mac, addr);
 
-			ret = igb_add_mac_filter(adapter, addr, vf);
+			ret = igb_add_mac_filter(adapter, addr, vf, 0);
 			ret = min_t(int, ret, 0);
 		} else {
 			ret = -ENOSPC;
@@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
 		if (is_valid_ether_addr(addr))
 			rar_high |= E1000_RAH_AV;
 
+		if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
+			rar_high |= E1000_RAH_ASEL_SRC_ADDR;
+
 		switch (hw->mac.type) {
 		case e1000_82575:
 		case e1000_i210:
-- 
2.16.2

[next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Vinicius Costa Gomes]

This adds the capability of configuring the queue steering of arriving
packets based on their source and destination MAC addresses.

In practical terms this adds support for the following use cases,
characterized by these examples:

$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
to the RX queue 0)

$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
(this will direct packets with destination address "44:44:44:44:44:44"
to the RX queue 3)

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |   9 +++
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 110 ++++++++++++++++++++++++++-
 drivers/net/ethernet/intel/igb/igb_main.c    |   8 +-
 3 files changed, 119 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index d5cd5f6708d9..e06d6fdcb2ce 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -440,6 +440,8 @@ struct hwmon_buff {
 enum igb_filter_match_flags {
 	IGB_FILTER_FLAG_ETHER_TYPE = 0x1,
 	IGB_FILTER_FLAG_VLAN_TCI   = 0x2,
+	IGB_FILTER_FLAG_SRC_MAC_ADDR   = 0x4,
+	IGB_FILTER_FLAG_DST_MAC_ADDR   = 0x8,
 };
 
 #define IGB_MAX_RXNFC_FILTERS 16
@@ -454,6 +456,8 @@ struct igb_nfc_input {
 	u8 match_flags;
 	__be16 etype;
 	__be16 vlan_tci;
+	u8 src_addr[ETH_ALEN];
+	u8 dst_addr[ETH_ALEN];
 };
 
 struct igb_nfc_filter {
@@ -738,4 +742,9 @@ int igb_add_filter(struct igb_adapter *adapter,
 int igb_erase_filter(struct igb_adapter *adapter,
 		     struct igb_nfc_filter *input);
 
+int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags);
+int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags);
+
 #endif /* _IGB_H_ */
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 143f0bb34e4d..d8686a0f5b5d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -152,6 +152,9 @@ static const char igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
 
 #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
 
+static const u8 broadcast_addr[ETH_ALEN] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+
 static int igb_get_link_ksettings(struct net_device *netdev,
 				  struct ethtool_link_ksettings *cmd)
 {
@@ -2494,6 +2497,25 @@ static int igb_get_ethtool_nfc_entry(struct igb_adapter *adapter,
 			fsp->h_ext.vlan_tci = rule->filter.vlan_tci;
 			fsp->m_ext.vlan_tci = htons(VLAN_PRIO_MASK);
 		}
+		if (rule->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR) {
+			ether_addr_copy(fsp->h_u.ether_spec.h_dest,
+					rule->filter.dst_addr);
+			/* As we only support matching by the full
+			 * mask, return the mask to userspace
+			 */
+			ether_addr_copy(fsp->m_u.ether_spec.h_dest,
+					broadcast_addr);
+		}
+		if (rule->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR) {
+			ether_addr_copy(fsp->h_u.ether_spec.h_source,
+					rule->filter.src_addr);
+			/* As we only support matching by the full
+			 * mask, return the mask to userspace
+			 */
+			ether_addr_copy(fsp->m_u.ether_spec.h_source,
+					broadcast_addr);
+		}
+
 		return 0;
 	}
 	return -EINVAL;
@@ -2698,6 +2720,58 @@ static int igb_set_rss_hash_opt(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_rxnfc_write_dst_mac_filter(struct igb_adapter *adapter,
+					  struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_add_mac_filter(adapter, input->filter.dst_addr,
+				 input->action, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_write_src_mac_filter(struct igb_adapter *adapter,
+					  struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_add_mac_filter(adapter, input->filter.src_addr,
+				 input->action, IGB_MAC_STATE_SRC_ADDR);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_del_dst_mac_filter(struct igb_adapter *adapter,
+					struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_del_mac_filter(adapter, input->filter.dst_addr,
+				 input->action, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_del_src_mac_filter(struct igb_adapter *adapter,
+					struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_del_mac_filter(adapter, input->filter.src_addr,
+				 input->action, IGB_MAC_STATE_SRC_ADDR);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int igb_rxnfc_write_etype_filter(struct igb_adapter *adapter,
 					struct igb_nfc_filter *input)
 {
@@ -2775,6 +2849,18 @@ int igb_add_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 			return err;
 	}
 
+	if (input->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR) {
+		err = igb_rxnfc_write_dst_mac_filter(adapter, input);
+		if (err)
+			return err;
+	}
+
+	if (input->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR) {
+		err = igb_rxnfc_write_src_mac_filter(adapter, input);
+		if (err)
+			return err;
+	}
+
 	if (input->filter.match_flags & IGB_FILTER_FLAG_VLAN_TCI)
 		err = igb_rxnfc_write_vlan_prio_filter(adapter, input);
 
@@ -2823,6 +2909,12 @@ int igb_erase_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 		igb_clear_vlan_prio_filter(adapter,
 					   ntohs(input->filter.vlan_tci));
 
+	if (input->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR)
+		igb_rxnfc_del_src_mac_filter(adapter, input);
+
+	if (input->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR)
+		igb_rxnfc_del_dst_mac_filter(adapter, input);
+
 	return 0;
 }
 
@@ -2904,10 +2996,6 @@ static int igb_add_ethtool_nfc_entry(struct igb_adapter *adapter,
 	if ((fsp->flow_type & ~FLOW_EXT) != ETHER_FLOW)
 		return -EINVAL;
 
-	if (fsp->m_u.ether_spec.h_proto != ETHER_TYPE_FULL_MASK &&
-	    fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK))
-		return -EINVAL;
-
 	input = kzalloc(sizeof(*input), GFP_KERNEL);
 	if (!input)
 		return -ENOMEM;
@@ -2917,6 +3005,20 @@ static int igb_add_ethtool_nfc_entry(struct igb_adapter *adapter,
 		input->filter.match_flags = IGB_FILTER_FLAG_ETHER_TYPE;
 	}
 
+	/* Only support matching addresses by the full mask */
+	if (is_broadcast_ether_addr(fsp->m_u.ether_spec.h_source)) {
+		input->filter.match_flags |= IGB_FILTER_FLAG_SRC_MAC_ADDR;
+		ether_addr_copy(input->filter.src_addr,
+				fsp->h_u.ether_spec.h_source);
+	}
+
+	/* Only support matching addresses by the full mask */
+	if (is_broadcast_ether_addr(fsp->m_u.ether_spec.h_dest)) {
+		input->filter.match_flags |= IGB_FILTER_FLAG_DST_MAC_ADDR;
+		ether_addr_copy(input->filter.dst_addr,
+				fsp->h_u.ether_spec.h_dest);
+	}
+
 	if ((fsp->flow_type & FLOW_EXT) && fsp->m_ext.vlan_tci) {
 		if (fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK)) {
 			err = -EINVAL;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index db66b697fe3b..5dfbdf05f765 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6842,8 +6842,8 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
  * default for the destination address, if matching by source address
  * is desired the flag IGB_MAC_STATE_SRC_ADDR can be used.
  */
-static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6878,8 +6878,8 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
  * matching by source address is to be removed the flag
  * IGB_MAC_STATE_SRC_ADDR can be used.
  */
-static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Vinicius Costa Gomes]

This adds the capability of configuring the queue steering of arriving
packets based on their source and destination MAC addresses.

In practical terms this adds support for the following use cases,
characterized by these examples:

$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
to the RX queue 0)

$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
(this will direct packets with destination address "44:44:44:44:44:44"
to the RX queue 3)

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |   9 +++
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 110 ++++++++++++++++++++++++++-
 drivers/net/ethernet/intel/igb/igb_main.c    |   8 +-
 3 files changed, 119 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index d5cd5f6708d9..e06d6fdcb2ce 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -440,6 +440,8 @@ struct hwmon_buff {
 enum igb_filter_match_flags {
 	IGB_FILTER_FLAG_ETHER_TYPE = 0x1,
 	IGB_FILTER_FLAG_VLAN_TCI   = 0x2,
+	IGB_FILTER_FLAG_SRC_MAC_ADDR   = 0x4,
+	IGB_FILTER_FLAG_DST_MAC_ADDR   = 0x8,
 };
 
 #define IGB_MAX_RXNFC_FILTERS 16
@@ -454,6 +456,8 @@ struct igb_nfc_input {
 	u8 match_flags;
 	__be16 etype;
 	__be16 vlan_tci;
+	u8 src_addr[ETH_ALEN];
+	u8 dst_addr[ETH_ALEN];
 };
 
 struct igb_nfc_filter {
@@ -738,4 +742,9 @@ int igb_add_filter(struct igb_adapter *adapter,
 int igb_erase_filter(struct igb_adapter *adapter,
 		     struct igb_nfc_filter *input);
 
+int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags);
+int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags);
+
 #endif /* _IGB_H_ */
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 143f0bb34e4d..d8686a0f5b5d 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -152,6 +152,9 @@ static const char igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
 
 #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
 
+static const u8 broadcast_addr[ETH_ALEN] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+
 static int igb_get_link_ksettings(struct net_device *netdev,
 				  struct ethtool_link_ksettings *cmd)
 {
@@ -2494,6 +2497,25 @@ static int igb_get_ethtool_nfc_entry(struct igb_adapter *adapter,
 			fsp->h_ext.vlan_tci = rule->filter.vlan_tci;
 			fsp->m_ext.vlan_tci = htons(VLAN_PRIO_MASK);
 		}
+		if (rule->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR) {
+			ether_addr_copy(fsp->h_u.ether_spec.h_dest,
+					rule->filter.dst_addr);
+			/* As we only support matching by the full
+			 * mask, return the mask to userspace
+			 */
+			ether_addr_copy(fsp->m_u.ether_spec.h_dest,
+					broadcast_addr);
+		}
+		if (rule->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR) {
+			ether_addr_copy(fsp->h_u.ether_spec.h_source,
+					rule->filter.src_addr);
+			/* As we only support matching by the full
+			 * mask, return the mask to userspace
+			 */
+			ether_addr_copy(fsp->m_u.ether_spec.h_source,
+					broadcast_addr);
+		}
+
 		return 0;
 	}
 	return -EINVAL;
@@ -2698,6 +2720,58 @@ static int igb_set_rss_hash_opt(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_rxnfc_write_dst_mac_filter(struct igb_adapter *adapter,
+					  struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_add_mac_filter(adapter, input->filter.dst_addr,
+				 input->action, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_write_src_mac_filter(struct igb_adapter *adapter,
+					  struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_add_mac_filter(adapter, input->filter.src_addr,
+				 input->action, IGB_MAC_STATE_SRC_ADDR);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_del_dst_mac_filter(struct igb_adapter *adapter,
+					struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_del_mac_filter(adapter, input->filter.dst_addr,
+				 input->action, 0);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
+static int igb_rxnfc_del_src_mac_filter(struct igb_adapter *adapter,
+					struct igb_nfc_filter *input)
+{
+	int err;
+
+	err = igb_del_mac_filter(adapter, input->filter.src_addr,
+				 input->action, IGB_MAC_STATE_SRC_ADDR);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int igb_rxnfc_write_etype_filter(struct igb_adapter *adapter,
 					struct igb_nfc_filter *input)
 {
@@ -2775,6 +2849,18 @@ int igb_add_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 			return err;
 	}
 
+	if (input->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR) {
+		err = igb_rxnfc_write_dst_mac_filter(adapter, input);
+		if (err)
+			return err;
+	}
+
+	if (input->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR) {
+		err = igb_rxnfc_write_src_mac_filter(adapter, input);
+		if (err)
+			return err;
+	}
+
 	if (input->filter.match_flags & IGB_FILTER_FLAG_VLAN_TCI)
 		err = igb_rxnfc_write_vlan_prio_filter(adapter, input);
 
@@ -2823,6 +2909,12 @@ int igb_erase_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 		igb_clear_vlan_prio_filter(adapter,
 					   ntohs(input->filter.vlan_tci));
 
+	if (input->filter.match_flags & IGB_FILTER_FLAG_SRC_MAC_ADDR)
+		igb_rxnfc_del_src_mac_filter(adapter, input);
+
+	if (input->filter.match_flags & IGB_FILTER_FLAG_DST_MAC_ADDR)
+		igb_rxnfc_del_dst_mac_filter(adapter, input);
+
 	return 0;
 }
 
@@ -2904,10 +2996,6 @@ static int igb_add_ethtool_nfc_entry(struct igb_adapter *adapter,
 	if ((fsp->flow_type & ~FLOW_EXT) != ETHER_FLOW)
 		return -EINVAL;
 
-	if (fsp->m_u.ether_spec.h_proto != ETHER_TYPE_FULL_MASK &&
-	    fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK))
-		return -EINVAL;
-
 	input = kzalloc(sizeof(*input), GFP_KERNEL);
 	if (!input)
 		return -ENOMEM;
@@ -2917,6 +3005,20 @@ static int igb_add_ethtool_nfc_entry(struct igb_adapter *adapter,
 		input->filter.match_flags = IGB_FILTER_FLAG_ETHER_TYPE;
 	}
 
+	/* Only support matching addresses by the full mask */
+	if (is_broadcast_ether_addr(fsp->m_u.ether_spec.h_source)) {
+		input->filter.match_flags |= IGB_FILTER_FLAG_SRC_MAC_ADDR;
+		ether_addr_copy(input->filter.src_addr,
+				fsp->h_u.ether_spec.h_source);
+	}
+
+	/* Only support matching addresses by the full mask */
+	if (is_broadcast_ether_addr(fsp->m_u.ether_spec.h_dest)) {
+		input->filter.match_flags |= IGB_FILTER_FLAG_DST_MAC_ADDR;
+		ether_addr_copy(input->filter.dst_addr,
+				fsp->h_u.ether_spec.h_dest);
+	}
+
 	if ((fsp->flow_type & FLOW_EXT) && fsp->m_ext.vlan_tci) {
 		if (fsp->m_ext.vlan_tci != htons(VLAN_PRIO_MASK)) {
 			err = -EINVAL;
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index db66b697fe3b..5dfbdf05f765 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6842,8 +6842,8 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
  * default for the destination address, if matching by source address
  * is desired the flag IGB_MAC_STATE_SRC_ADDR can be used.
  */
-static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
@@ -6878,8 +6878,8 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
  * matching by source address is to be removed the flag
  * IGB_MAC_STATE_SRC_ADDR can be used.
  */
-static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
-			      const u8 queue, const u8 flags)
+int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
+		       const u8 queue, const u8 flags)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	int rar_entries = hw->mac.rar_entry_count -
-- 
2.16.2

[next-queue PATCH 6/8] igb: Add the skeletons for tc-flower offloading

[Vinicius Costa Gomes]

This adds basic functions needed to implement offloading for filters
created by tc-flower.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 66 +++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5dfbdf05f765..5344261e6f45 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -35,6 +35,7 @@
 #include <net/checksum.h>
 #include <net/ip6_checksum.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <linux/net_tstamp.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
@@ -2496,6 +2497,69 @@ static int igb_offload_cbs(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_configure_clsflower(struct igb_adapter *adapter,
+				   struct tc_cls_flower_offload *cls_flower)
+{
+	return -EOPNOTSUPP;
+}
+
+static int igb_delete_clsflower(struct igb_adapter *adapter,
+				struct tc_cls_flower_offload *cls_flower)
+{
+	return -EOPNOTSUPP;
+}
+
+static int igb_setup_tc_cls_flower(struct igb_adapter *adapter,
+				   struct tc_cls_flower_offload *cls_flower)
+{
+	switch (cls_flower->command) {
+	case TC_CLSFLOWER_REPLACE:
+		return igb_configure_clsflower(adapter, cls_flower);
+	case TC_CLSFLOWER_DESTROY:
+		return igb_delete_clsflower(adapter, cls_flower);
+	case TC_CLSFLOWER_STATS:
+		return -EOPNOTSUPP;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int igb_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
+				 void *cb_priv)
+{
+	struct igb_adapter *adapter = cb_priv;
+
+	if (!tc_cls_can_offload_and_chain0(adapter->netdev, type_data))
+		return -EOPNOTSUPP;
+
+	switch (type) {
+	case TC_SETUP_CLSFLOWER:
+		return igb_setup_tc_cls_flower(adapter, type_data);
+
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int igb_setup_tc_block(struct igb_adapter *adapter,
+			      struct tc_block_offload *f)
+{
+	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
+		return -EOPNOTSUPP;
+
+	switch (f->command) {
+	case TC_BLOCK_BIND:
+		return tcf_block_cb_register(f->block, igb_setup_tc_block_cb,
+					     adapter, adapter);
+	case TC_BLOCK_UNBIND:
+		tcf_block_cb_unregister(f->block, igb_setup_tc_block_cb,
+					adapter);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 			void *type_data)
 {
@@ -2504,6 +2568,8 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	switch (type) {
 	case TC_SETUP_QDISC_CBS:
 		return igb_offload_cbs(adapter, type_data);
+	case TC_SETUP_BLOCK:
+		return igb_setup_tc_block(adapter, type_data);
 
 	default:
 		return -EOPNOTSUPP;
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 6/8] igb: Add the skeletons for tc-flower offloading

[Vinicius Costa Gomes]

This adds basic functions needed to implement offloading for filters
created by tc-flower.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 66 +++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5dfbdf05f765..5344261e6f45 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -35,6 +35,7 @@
 #include <net/checksum.h>
 #include <net/ip6_checksum.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <linux/net_tstamp.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
@@ -2496,6 +2497,69 @@ static int igb_offload_cbs(struct igb_adapter *adapter,
 	return 0;
 }
 
+static int igb_configure_clsflower(struct igb_adapter *adapter,
+				   struct tc_cls_flower_offload *cls_flower)
+{
+	return -EOPNOTSUPP;
+}
+
+static int igb_delete_clsflower(struct igb_adapter *adapter,
+				struct tc_cls_flower_offload *cls_flower)
+{
+	return -EOPNOTSUPP;
+}
+
+static int igb_setup_tc_cls_flower(struct igb_adapter *adapter,
+				   struct tc_cls_flower_offload *cls_flower)
+{
+	switch (cls_flower->command) {
+	case TC_CLSFLOWER_REPLACE:
+		return igb_configure_clsflower(adapter, cls_flower);
+	case TC_CLSFLOWER_DESTROY:
+		return igb_delete_clsflower(adapter, cls_flower);
+	case TC_CLSFLOWER_STATS:
+		return -EOPNOTSUPP;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int igb_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
+				 void *cb_priv)
+{
+	struct igb_adapter *adapter = cb_priv;
+
+	if (!tc_cls_can_offload_and_chain0(adapter->netdev, type_data))
+		return -EOPNOTSUPP;
+
+	switch (type) {
+	case TC_SETUP_CLSFLOWER:
+		return igb_setup_tc_cls_flower(adapter, type_data);
+
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int igb_setup_tc_block(struct igb_adapter *adapter,
+			      struct tc_block_offload *f)
+{
+	if (f->binder_type != TCF_BLOCK_BINDER_TYPE_CLSACT_INGRESS)
+		return -EOPNOTSUPP;
+
+	switch (f->command) {
+	case TC_BLOCK_BIND:
+		return tcf_block_cb_register(f->block, igb_setup_tc_block_cb,
+					     adapter, adapter);
+	case TC_BLOCK_UNBIND:
+		tcf_block_cb_unregister(f->block, igb_setup_tc_block_cb,
+					adapter);
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 			void *type_data)
 {
@@ -2504,6 +2568,8 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	switch (type) {
 	case TC_SETUP_QDISC_CBS:
 		return igb_offload_cbs(adapter, type_data);
+	case TC_SETUP_BLOCK:
+		return igb_setup_tc_block(adapter, type_data);
 
 	default:
 		return -EOPNOTSUPP;
-- 
2.16.2

[next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

This allows filters added by tc-flower and specifying MAC addresses,
Ethernet types, and the VLAN priority field, to be offloaded to the
controller.

This reuses most of the infrastructure used by ethtool, ethtool can be
used to read these filters, but modification and deletion can only be
done via tc-flower.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |   5 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  13 ++-
 drivers/net/ethernet/intel/igb/igb_main.c    | 140 ++++++++++++++++++++++++++-
 3 files changed, 152 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index e06d6fdcb2ce..05d8c827d33e 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -463,6 +463,7 @@ struct igb_nfc_input {
 struct igb_nfc_filter {
 	struct hlist_node nfc_node;
 	struct igb_nfc_input filter;
+	unsigned long cookie;
 	u16 etype_reg_index;
 	u16 sw_idx;
 	u16 action;
@@ -747,4 +748,8 @@ int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 		       const u8 queue, const u8 flags);
 
+int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
+				 struct igb_nfc_filter *input,
+				 u16 sw_idx);
+
 #endif /* _IGB_H_ */
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index d8686a0f5b5d..5386eb68ab15 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2918,9 +2918,9 @@ int igb_erase_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 	return 0;
 }
 
-static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
-					struct igb_nfc_filter *input,
-					u16 sw_idx)
+int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
+				 struct igb_nfc_filter *input,
+				 u16 sw_idx)
 {
 	struct igb_nfc_filter *rule, *parent;
 	int err = -EINVAL;
@@ -2935,8 +2935,11 @@ static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
 		parent = rule;
 	}
 
-	/* if there is an old rule occupying our place remove it */
-	if (rule && (rule->sw_idx == sw_idx)) {
+	/* if there is an old rule occupying our place remove it, also
+	 * only allow rules added by ethtool to be removed, these
+	 * rules don't have a cookie
+	 */
+	if (rule && (!rule->cookie && rule->sw_idx == sw_idx)) {
 		if (!input)
 			err = igb_erase_filter(adapter, rule);
 
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5344261e6f45..b1d401e77d62 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2497,10 +2497,148 @@ static int igb_offload_cbs(struct igb_adapter *adapter,
 	return 0;
 }
 
+#define ETHER_TYPE_FULL_MASK ((__force __be16)~0)
+
+static int igb_parse_cls_flower(struct igb_adapter *adapter,
+				struct tc_cls_flower_offload *f,
+				int traffic_class,
+				struct igb_nfc_filter *input)
+{
+	if (f->dissector->used_keys &
+	    ~(BIT(FLOW_DISSECTOR_KEY_BASIC) |
+	      BIT(FLOW_DISSECTOR_KEY_CONTROL) |
+	      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
+	      BIT(FLOW_DISSECTOR_KEY_VLAN))) {
+		dev_err(&adapter->pdev->dev, "Unsupported key used: 0x%x\n",
+			f->dissector->used_keys);
+		return -EOPNOTSUPP;
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+		struct flow_dissector_key_eth_addrs *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_ETH_ADDRS,
+						  f->key);
+
+		struct flow_dissector_key_eth_addrs *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_ETH_ADDRS,
+						  f->mask);
+
+		if (is_broadcast_ether_addr(mask->dst)) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_DST_MAC_ADDR;
+			ether_addr_copy(input->filter.dst_addr, key->dst);
+		}
+
+		if (is_broadcast_ether_addr(mask->src)) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_SRC_MAC_ADDR;
+			ether_addr_copy(input->filter.src_addr, key->src);
+		}
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
+		struct flow_dissector_key_basic *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_BASIC,
+						  f->key);
+
+		struct flow_dissector_key_basic *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_BASIC,
+						  f->mask);
+
+		if (mask->n_proto == ETHER_TYPE_FULL_MASK) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_ETHER_TYPE;
+			input->filter.etype = key->n_proto;
+		}
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
+		struct flow_dissector_key_vlan *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_VLAN,
+						  f->key);
+		struct flow_dissector_key_vlan *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_VLAN,
+						  f->mask);
+
+		if (mask->vlan_priority) {
+			input->filter.match_flags |= IGB_FILTER_FLAG_VLAN_TCI;
+			input->filter.vlan_tci = key->vlan_priority;
+		}
+	}
+
+	input->action = traffic_class;
+	input->cookie = f->cookie;
+
+	return 0;
+}
+
 static int igb_configure_clsflower(struct igb_adapter *adapter,
 				   struct tc_cls_flower_offload *cls_flower)
 {
-	return -EOPNOTSUPP;
+	struct igb_nfc_filter *input, *rule;
+	u16 location = 0;
+	int err, tc;
+
+	if (!(adapter->netdev->hw_features & NETIF_F_NTUPLE))
+		return -EOPNOTSUPP;
+
+	tc = tc_classid_to_hwtc(adapter->netdev, cls_flower->classid);
+	if (tc < 0) {
+		dev_err(&adapter->pdev->dev, "Invalid traffic class\n");
+		return -EINVAL;
+	}
+
+	input = kzalloc(sizeof(*input), GFP_KERNEL);
+	if (!input)
+		return -ENOMEM;
+
+	err = igb_parse_cls_flower(adapter, cls_flower, tc, input);
+	if (err < 0)
+		goto error_parse;
+
+	spin_lock(&adapter->nfc_lock);
+
+	hlist_for_each_entry(rule, &adapter->nfc_filter_list, nfc_node) {
+		if (!memcmp(&input->filter, &rule->filter,
+			    sizeof(input->filter))) {
+			err = -EEXIST;
+			dev_err(&adapter->pdev->dev,
+				"tc-flower: this filter is already set\n");
+			goto error_locked;
+		}
+
+		if (rule->sw_idx > location)
+			location = rule->sw_idx;
+	}
+
+	if (adapter->nfc_filter_count != 0)
+		location++;
+
+	input->sw_idx = location;
+
+	err = igb_add_filter(adapter, input);
+	if (err < 0)
+		goto error_locked;
+
+	igb_update_ethtool_nfc_entry(adapter, input, location);
+
+	spin_unlock(&adapter->nfc_lock);
+
+	return 0;
+
+error_locked:
+	spin_unlock(&adapter->nfc_lock);
+
+error_parse:
+	kfree(input);
+
+	return err;
 }
 
 static int igb_delete_clsflower(struct igb_adapter *adapter,
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

This allows filters added by tc-flower and specifying MAC addresses,
Ethernet types, and the VLAN priority field, to be offloaded to the
controller.

This reuses most of the infrastructure used by ethtool, ethtool can be
used to read these filters, but modification and deletion can only be
done via tc-flower.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb.h         |   5 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |  13 ++-
 drivers/net/ethernet/intel/igb/igb_main.c    | 140 ++++++++++++++++++++++++++-
 3 files changed, 152 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index e06d6fdcb2ce..05d8c827d33e 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -463,6 +463,7 @@ struct igb_nfc_input {
 struct igb_nfc_filter {
 	struct hlist_node nfc_node;
 	struct igb_nfc_input filter;
+	unsigned long cookie;
 	u16 etype_reg_index;
 	u16 sw_idx;
 	u16 action;
@@ -747,4 +748,8 @@ int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
 		       const u8 queue, const u8 flags);
 
+int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
+				 struct igb_nfc_filter *input,
+				 u16 sw_idx);
+
 #endif /* _IGB_H_ */
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index d8686a0f5b5d..5386eb68ab15 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2918,9 +2918,9 @@ int igb_erase_filter(struct igb_adapter *adapter, struct igb_nfc_filter *input)
 	return 0;
 }
 
-static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
-					struct igb_nfc_filter *input,
-					u16 sw_idx)
+int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
+				 struct igb_nfc_filter *input,
+				 u16 sw_idx)
 {
 	struct igb_nfc_filter *rule, *parent;
 	int err = -EINVAL;
@@ -2935,8 +2935,11 @@ static int igb_update_ethtool_nfc_entry(struct igb_adapter *adapter,
 		parent = rule;
 	}
 
-	/* if there is an old rule occupying our place remove it */
-	if (rule && (rule->sw_idx == sw_idx)) {
+	/* if there is an old rule occupying our place remove it, also
+	 * only allow rules added by ethtool to be removed, these
+	 * rules don't have a cookie
+	 */
+	if (rule && (!rule->cookie && rule->sw_idx == sw_idx)) {
 		if (!input)
 			err = igb_erase_filter(adapter, rule);
 
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5344261e6f45..b1d401e77d62 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2497,10 +2497,148 @@ static int igb_offload_cbs(struct igb_adapter *adapter,
 	return 0;
 }
 
+#define ETHER_TYPE_FULL_MASK ((__force __be16)~0)
+
+static int igb_parse_cls_flower(struct igb_adapter *adapter,
+				struct tc_cls_flower_offload *f,
+				int traffic_class,
+				struct igb_nfc_filter *input)
+{
+	if (f->dissector->used_keys &
+	    ~(BIT(FLOW_DISSECTOR_KEY_BASIC) |
+	      BIT(FLOW_DISSECTOR_KEY_CONTROL) |
+	      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
+	      BIT(FLOW_DISSECTOR_KEY_VLAN))) {
+		dev_err(&adapter->pdev->dev, "Unsupported key used: 0x%x\n",
+			f->dissector->used_keys);
+		return -EOPNOTSUPP;
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
+		struct flow_dissector_key_eth_addrs *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_ETH_ADDRS,
+						  f->key);
+
+		struct flow_dissector_key_eth_addrs *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_ETH_ADDRS,
+						  f->mask);
+
+		if (is_broadcast_ether_addr(mask->dst)) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_DST_MAC_ADDR;
+			ether_addr_copy(input->filter.dst_addr, key->dst);
+		}
+
+		if (is_broadcast_ether_addr(mask->src)) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_SRC_MAC_ADDR;
+			ether_addr_copy(input->filter.src_addr, key->src);
+		}
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
+		struct flow_dissector_key_basic *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_BASIC,
+						  f->key);
+
+		struct flow_dissector_key_basic *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_BASIC,
+						  f->mask);
+
+		if (mask->n_proto == ETHER_TYPE_FULL_MASK) {
+			input->filter.match_flags |=
+				IGB_FILTER_FLAG_ETHER_TYPE;
+			input->filter.etype = key->n_proto;
+		}
+	}
+
+	if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
+		struct flow_dissector_key_vlan *key =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_VLAN,
+						  f->key);
+		struct flow_dissector_key_vlan *mask =
+			skb_flow_dissector_target(f->dissector,
+						  FLOW_DISSECTOR_KEY_VLAN,
+						  f->mask);
+
+		if (mask->vlan_priority) {
+			input->filter.match_flags |= IGB_FILTER_FLAG_VLAN_TCI;
+			input->filter.vlan_tci = key->vlan_priority;
+		}
+	}
+
+	input->action = traffic_class;
+	input->cookie = f->cookie;
+
+	return 0;
+}
+
 static int igb_configure_clsflower(struct igb_adapter *adapter,
 				   struct tc_cls_flower_offload *cls_flower)
 {
-	return -EOPNOTSUPP;
+	struct igb_nfc_filter *input, *rule;
+	u16 location = 0;
+	int err, tc;
+
+	if (!(adapter->netdev->hw_features & NETIF_F_NTUPLE))
+		return -EOPNOTSUPP;
+
+	tc = tc_classid_to_hwtc(adapter->netdev, cls_flower->classid);
+	if (tc < 0) {
+		dev_err(&adapter->pdev->dev, "Invalid traffic class\n");
+		return -EINVAL;
+	}
+
+	input = kzalloc(sizeof(*input), GFP_KERNEL);
+	if (!input)
+		return -ENOMEM;
+
+	err = igb_parse_cls_flower(adapter, cls_flower, tc, input);
+	if (err < 0)
+		goto error_parse;
+
+	spin_lock(&adapter->nfc_lock);
+
+	hlist_for_each_entry(rule, &adapter->nfc_filter_list, nfc_node) {
+		if (!memcmp(&input->filter, &rule->filter,
+			    sizeof(input->filter))) {
+			err = -EEXIST;
+			dev_err(&adapter->pdev->dev,
+				"tc-flower: this filter is already set\n");
+			goto error_locked;
+		}
+
+		if (rule->sw_idx > location)
+			location = rule->sw_idx;
+	}
+
+	if (adapter->nfc_filter_count != 0)
+		location++;
+
+	input->sw_idx = location;
+
+	err = igb_add_filter(adapter, input);
+	if (err < 0)
+		goto error_locked;
+
+	igb_update_ethtool_nfc_entry(adapter, input, location);
+
+	spin_unlock(&adapter->nfc_lock);
+
+	return 0;
+
+error_locked:
+	spin_unlock(&adapter->nfc_lock);
+
+error_parse:
+	kfree(input);
+
+	return err;
 }
 
 static int igb_delete_clsflower(struct igb_adapter *adapter,
-- 
2.16.2

[next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Vinicius Costa Gomes]

This allows tc-flower filters that were offloaded to be removed.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 32 ++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b1d401e77d62..5e0e1df0e941 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2641,10 +2641,40 @@ static int igb_configure_clsflower(struct igb_adapter *adapter,
 	return err;
 }
 
+static int igb_delete_filter_by_cookie(struct igb_adapter *adapter,
+				       unsigned long cookie)
+{
+	struct igb_nfc_filter *filter;
+	int err;
+
+	spin_lock(&adapter->nfc_lock);
+
+	hlist_for_each_entry(filter, &adapter->nfc_filter_list, nfc_node) {
+		if (filter->cookie == cookie)
+			break;
+	}
+
+	if (!filter) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	err = igb_erase_filter(adapter, filter);
+
+	hlist_del(&filter->nfc_node);
+	kfree(filter);
+	adapter->nfc_filter_count--;
+
+out:
+	spin_unlock(&adapter->nfc_lock);
+
+	return err;
+}
+
 static int igb_delete_clsflower(struct igb_adapter *adapter,
 				struct tc_cls_flower_offload *cls_flower)
 {
-	return -EOPNOTSUPP;
+	return igb_delete_filter_by_cookie(adapter, cls_flower->cookie);
 }
 
 static int igb_setup_tc_cls_flower(struct igb_adapter *adapter,
-- 
2.16.2

[Intel-wired-lan] [next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Vinicius Costa Gomes]

This allows tc-flower filters that were offloaded to be removed.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 32 ++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index b1d401e77d62..5e0e1df0e941 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2641,10 +2641,40 @@ static int igb_configure_clsflower(struct igb_adapter *adapter,
 	return err;
 }
 
+static int igb_delete_filter_by_cookie(struct igb_adapter *adapter,
+				       unsigned long cookie)
+{
+	struct igb_nfc_filter *filter;
+	int err;
+
+	spin_lock(&adapter->nfc_lock);
+
+	hlist_for_each_entry(filter, &adapter->nfc_filter_list, nfc_node) {
+		if (filter->cookie == cookie)
+			break;
+	}
+
+	if (!filter) {
+		err = -ENOENT;
+		goto out;
+	}
+
+	err = igb_erase_filter(adapter, filter);
+
+	hlist_del(&filter->nfc_node);
+	kfree(filter);
+	adapter->nfc_filter_count--;
+
+out:
+	spin_unlock(&adapter->nfc_lock);
+
+	return err;
+}
+
 static int igb_delete_clsflower(struct igb_adapter *adapter,
 				struct tc_cls_flower_offload *cls_flower)
 {
-	return -EOPNOTSUPP;
+	return igb_delete_filter_by_cookie(adapter, cls_flower->cookie);
 }
 
 static int igb_setup_tc_cls_flower(struct igb_adapter *adapter,
-- 
2.16.2

Re: [next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Florian Fainelli]

On February 23, 2018 5:20:36 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This allows tc-flower filters that were offloaded to be removed.

This should be squashed into your previous patch, either the functionality is there and you can add/remove or it is not.

-- 
Florian

[Intel-wired-lan] [next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Florian Fainelli]

On February 23, 2018 5:20:36 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This allows tc-flower filters that were offloaded to be removed.

This should be squashed into your previous patch, either the functionality is there and you can add/remove or it is not.

-- 
Florian

Re: [next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Florian Fainelli]

On February 23, 2018 5:20:33 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This adds the capability of configuring the queue steering of arriving
>packets based on their source and destination MAC addresses.
>
>In practical terms this adds support for the following use cases,
>characterized by these examples:
>
>$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
>(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
>to the RX queue 0)
>
>$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
>(this will direct packets with destination address "44:44:44:44:44:44"
>to the RX queue 3)
>
>Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>---

[snip]

>diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>index 143f0bb34e4d..d8686a0f5b5d 100644
>--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>@@ -152,6 +152,9 @@ static const char
>igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
> 
> #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
> 
>+static const u8 broadcast_addr[ETH_ALEN] = {
>+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

This is already defined in an existing header, don't have it handy but likely etherdevice.h.

-- 
Florian

[Intel-wired-lan] [next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Florian Fainelli]

On February 23, 2018 5:20:33 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This adds the capability of configuring the queue steering of arriving
>packets based on their source and destination MAC addresses.
>
>In practical terms this adds support for the following use cases,
>characterized by these examples:
>
>$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
>(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
>to the RX queue 0)
>
>$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
>(this will direct packets with destination address "44:44:44:44:44:44"
>to the RX queue 3)
>
>Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>---

[snip]

>diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>index 143f0bb34e4d..d8686a0f5b5d 100644
>--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>@@ -152,6 +152,9 @@ static const char
>igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
> 
> #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
> 
>+static const u8 broadcast_addr[ETH_ALEN] = {
>+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };

This is already defined in an existing header, don't have it handy but likely etherdevice.h.

-- 
Florian

Re: [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Florian Fainelli]

On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This allows filters added by tc-flower and specifying MAC addresses,
>Ethernet types, and the VLAN priority field, to be offloaded to the
>controller.
>
>This reuses most of the infrastructure used by ethtool, ethtool can be
>used to read these filters, but modification and deletion can only be
>done via tc-flower.

You would want to check what other drivers supporting both ethtool::rxnfc and cls_flower do, but this can be seriously confusing to an user. As an user I would be more comfortable with seeing only rules added through ethtool via ethtool and those added by cls_flower via cls_flower. They will both access a shared set of resources but it seems easier for me to dump rules with both tools to figure out why -ENOSPC was returned rather than seeing something I did not add. Others might see it entirely differently.

If you added the ability for cls_flower to indicate a queue number and either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could that eliminate entirely the need for adding MAC address steering in earlier patches?

-- 
Florian

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Florian Fainelli]

On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>This allows filters added by tc-flower and specifying MAC addresses,
>Ethernet types, and the VLAN priority field, to be offloaded to the
>controller.
>
>This reuses most of the infrastructure used by ethtool, ethtool can be
>used to read these filters, but modification and deletion can only be
>done via tc-flower.

You would want to check what other drivers supporting both ethtool::rxnfc and cls_flower do, but this can be seriously confusing to an user. As an user I would be more comfortable with seeing only rules added through ethtool via ethtool and those added by cls_flower via cls_flower. They will both access a shared set of resources but it seems easier for me to dump rules with both tools to figure out why -ENOSPC was returned rather than seeing something I did not add. Others might see it entirely differently.

If you added the ability for cls_flower to indicate a queue number and either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could that eliminate entirely the need for adding MAC address steering in earlier patches?

-- 
Florian

Re: [Intel-wired-lan] [next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Alexander Duyck]

On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> This will allow functionality depending on the hardware being traffic
> class aware to work. In particular the tc-flower offloading checks
> verifies that this bit is set.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 0ea32be07d71..543aa99892eb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                                NETIF_F_HW_VLAN_CTAG_TX |
>                                NETIF_F_RXALL;
>
> -       if (hw->mac.type >= e1000_i350)
> -               netdev->hw_features |= NETIF_F_NTUPLE;
> +       if (hw->mac.type >= e1000_i350) {
> +               netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
> +               netdev->features |= NETIF_F_HW_TC;

The parens aren't needed.

Also you might consider moving this block up to where we have a
similar one for 82576. Then you wouldn't need to set both features and
hw_features in the case of the HW_TC flag.

> +       }
>
>         if (pci_using_dac)
>                 netdev->features |= NETIF_F_HIGHDMA;
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Alexander Duyck]

On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> This will allow functionality depending on the hardware being traffic
> class aware to work. In particular the tc-flower offloading checks
> verifies that this bit is set.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 0ea32be07d71..543aa99892eb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>                                NETIF_F_HW_VLAN_CTAG_TX |
>                                NETIF_F_RXALL;
>
> -       if (hw->mac.type >= e1000_i350)
> -               netdev->hw_features |= NETIF_F_NTUPLE;
> +       if (hw->mac.type >= e1000_i350) {
> +               netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
> +               netdev->features |= NETIF_F_HW_TC;

The parens aren't needed.

Also you might consider moving this block up to where we have a
similar one for 82576. Then you wouldn't need to set both features and
hw_features in the case of the HW_TC flag.

> +       }
>
>         if (pci_using_dac)
>                 netdev->features |= NETIF_F_HIGHDMA;
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Alexander Duyck]

On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> Makes it possible to direct packets to queues based on their source
> address. Documents the expected usage of the 'flags' parameter.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
>  drivers/net/ethernet/intel/igb/igb.h           |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
>  3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index 573bf177fd08..c6f552de30dd 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -490,6 +490,7 @@
>   * manageability enabled, allowing us room for 15 multicast addresses.
>   */
>  #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
> +#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
>  #define E1000_RAH_QSEL_ENABLE 0x10000000
>  #define E1000_RAL_MAC_ADDR_LEN 4
>  #define E1000_RAH_MAC_ADDR_LEN 2
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 1c6b8d9176a8..d5cd5f6708d9 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -472,6 +472,7 @@ struct igb_mac_addr {
>
>  #define IGB_MAC_STATE_DEFAULT  0x1
>  #define IGB_MAC_STATE_IN_USE   0x2
> +#define IGB_MAC_STATE_SRC_ADDR  0x4
>
>  /* board specific private data structure */
>  struct igb_adapter {
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 543aa99892eb..db66b697fe3b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
>         igb_rar_set_index(adapter, 0);
>  }
>
> +/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>
>                 ether_addr_copy(adapter->mac_table[i].addr, addr);
>                 adapter->mac_table[i].queue = queue;
> -               adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);

More unneeded parenthesis.

>
>                 igb_rar_set_index(adapter, i);
>                 return i;
> @@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
> + * IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>          * for the VF MAC addresses.
>          */
>         for (i = 0; i < rar_entries; i++) {
> -               if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
> +               if (!(adapter->mac_table[i].state &
> +                     (IGB_MAC_STATE_IN_USE | flags)))

Shouldn't these be two separate checks? If the address isn't in use
why would I care what the flags state is? It probably isn't valid.

>                         continue;
>                 if (adapter->mac_table[i].queue != queue)
>                         continue;
>                 if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>                         continue;
>
> -               adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);

Maybe instead of just clearing the specific flags we should just clear
the state and reset it back to 0.

>                 memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>                 adapter->mac_table[i].queue = 0;
>
> @@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
>         struct igb_adapter *adapter = netdev_priv(netdev);
>         int ret;
>
> -       ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       ret = igb_add_mac_filter(adapter, addr,
> +                                adapter->vfs_allocated_count, 0);

Instead of having to add a 0 to all these functions it might be better
to rename the original igb_add_mac_filter, and then add a new function
named the same as the original that just automatically passes the 0.
It will take a lot of noise out of this code.

>
>         return min_t(int, ret, 0);
>  }
> @@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>  {
>         struct igb_adapter *adapter = netdev_priv(netdev);
>
> -       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
>
>         return 0;
>  }
> @@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         if (entry->vf == vf) {
>                                 entry->vf = -1;
>                                 entry->free = true;
> -                               igb_del_mac_filter(adapter, entry->vf_mac, vf);
> +                               igb_del_mac_filter(adapter,
> +                                                  entry->vf_mac, vf, 0);
>                         }
>                 }
>                 break;
> @@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         entry->vf = vf;
>                         ether_addr_copy(entry->vf_mac, addr);
>
> -                       ret = igb_add_mac_filter(adapter, addr, vf);
> +                       ret = igb_add_mac_filter(adapter, addr, vf, 0);
>                         ret = min_t(int, ret, 0);
>                 } else {
>                         ret = -ENOSPC;
> @@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
>                 if (is_valid_ether_addr(addr))
>                         rar_high |= E1000_RAH_AV;
>
> +               if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
> +                       rar_high |= E1000_RAH_ASEL_SRC_ADDR;
> +
>                 switch (hw->mac.type) {
>                 case e1000_82575:
>                 case e1000_i210:
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Alexander Duyck]

On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
> Makes it possible to direct packets to queues based on their source
> address. Documents the expected usage of the 'flags' parameter.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
>  drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
>  drivers/net/ethernet/intel/igb/igb.h           |  1 +
>  drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
>  3 files changed, 28 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
> index 573bf177fd08..c6f552de30dd 100644
> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
> @@ -490,6 +490,7 @@
>   * manageability enabled, allowing us room for 15 multicast addresses.
>   */
>  #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
> +#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
>  #define E1000_RAH_QSEL_ENABLE 0x10000000
>  #define E1000_RAL_MAC_ADDR_LEN 4
>  #define E1000_RAH_MAC_ADDR_LEN 2
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 1c6b8d9176a8..d5cd5f6708d9 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -472,6 +472,7 @@ struct igb_mac_addr {
>
>  #define IGB_MAC_STATE_DEFAULT  0x1
>  #define IGB_MAC_STATE_IN_USE   0x2
> +#define IGB_MAC_STATE_SRC_ADDR  0x4
>
>  /* board specific private data structure */
>  struct igb_adapter {
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 543aa99892eb..db66b697fe3b 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
>         igb_rar_set_index(adapter, 0);
>  }
>
> +/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>
>                 ether_addr_copy(adapter->mac_table[i].addr, addr);
>                 adapter->mac_table[i].queue = queue;
> -               adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);

More unneeded parenthesis.

>
>                 igb_rar_set_index(adapter, i);
>                 return i;
> @@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
> + * IGB_MAC_STATE_SRC_ADDR can be used.
> + */
>  static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
> -                             const u8 queue)
> +                             const u8 queue, const u8 flags)
>  {
>         struct e1000_hw *hw = &adapter->hw;
>         int rar_entries = hw->mac.rar_entry_count -
> @@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>          * for the VF MAC addresses.
>          */
>         for (i = 0; i < rar_entries; i++) {
> -               if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
> +               if (!(adapter->mac_table[i].state &
> +                     (IGB_MAC_STATE_IN_USE | flags)))

Shouldn't these be two separate checks? If the address isn't in use
why would I care what the flags state is? It probably isn't valid.

>                         continue;
>                 if (adapter->mac_table[i].queue != queue)
>                         continue;
>                 if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>                         continue;
>
> -               adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
> +               adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);

Maybe instead of just clearing the specific flags we should just clear
the state and reset it back to 0.

>                 memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>                 adapter->mac_table[i].queue = 0;
>
> @@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
>         struct igb_adapter *adapter = netdev_priv(netdev);
>         int ret;
>
> -       ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       ret = igb_add_mac_filter(adapter, addr,
> +                                adapter->vfs_allocated_count, 0);

Instead of having to add a 0 to all these functions it might be better
to rename the original igb_add_mac_filter, and then add a new function
named the same as the original that just automatically passes the 0.
It will take a lot of noise out of this code.

>
>         return min_t(int, ret, 0);
>  }
> @@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>  {
>         struct igb_adapter *adapter = netdev_priv(netdev);
>
> -       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
> +       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
>
>         return 0;
>  }
> @@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         if (entry->vf == vf) {
>                                 entry->vf = -1;
>                                 entry->free = true;
> -                               igb_del_mac_filter(adapter, entry->vf_mac, vf);
> +                               igb_del_mac_filter(adapter,
> +                                                  entry->vf_mac, vf, 0);
>                         }
>                 }
>                 break;
> @@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>                         entry->vf = vf;
>                         ether_addr_copy(entry->vf_mac, addr);
>
> -                       ret = igb_add_mac_filter(adapter, addr, vf);
> +                       ret = igb_add_mac_filter(adapter, addr, vf, 0);
>                         ret = min_t(int, ret, 0);
>                 } else {
>                         ret = -ENOSPC;
> @@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
>                 if (is_valid_ether_addr(addr))
>                         rar_high |= E1000_RAH_AV;
>
> +               if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
> +                       rar_high |= E1000_RAH_ASEL_SRC_ADDR;
> +
>                 switch (hw->mac.type) {
>                 case e1000_82575:
>                 case e1000_i210:
> --
> 2.16.2
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan at osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[kbuild test robot]

Hi Vinicius,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jkirsher-next-queue/dev-queue]
[also build test WARNING on v4.16-rc2 next-20180223]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vinicius-Costa-Gomes/igb-Fix-not-adding-filter-elements-to-the-list/20180226-053124
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/net/ethernet/intel/igb/igb_main.c:474:25: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:474:25: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:554:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:554:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:560:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:560:33: sparse: cast to restricted __le64
>> drivers/net/ethernet/intel/igb/igb_main.c:2573:48: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 vlan_tci @@ got unsignedrestricted __be16 vlan_tci @@
   drivers/net/ethernet/intel/igb/igb_main.c:2573:48: expected restricted __be16 vlan_tci
   drivers/net/ethernet/intel/igb/igb_main.c:2573:48: got unsigned short vlan_priority:3
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: sparse: incorrect type in argument 2 (different base types) @@ expected restricted __wsum diff @@ got restricted __wsum diff @@
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: expected restricted __wsum diff
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: got restricted __be32 <noident>
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16

vim +2573 drivers/net/ethernet/intel/igb/igb_main.c

  2503	
  2504	static int igb_parse_cls_flower(struct igb_adapter *adapter,
  2505					struct tc_cls_flower_offload *f,
  2506					int traffic_class,
  2507					struct igb_nfc_filter *input)
  2508	{
  2509		if (f->dissector->used_keys &
  2510		    ~(BIT(FLOW_DISSECTOR_KEY_BASIC) |
  2511		      BIT(FLOW_DISSECTOR_KEY_CONTROL) |
  2512		      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
  2513		      BIT(FLOW_DISSECTOR_KEY_VLAN))) {
  2514			dev_err(&adapter->pdev->dev, "Unsupported key used: 0x%x\n",
  2515				f->dissector->used_keys);
  2516			return -EOPNOTSUPP;
  2517		}
  2518	
  2519		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
  2520			struct flow_dissector_key_eth_addrs *key =
  2521				skb_flow_dissector_target(f->dissector,
  2522							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
  2523							  f->key);
  2524	
  2525			struct flow_dissector_key_eth_addrs *mask =
  2526				skb_flow_dissector_target(f->dissector,
  2527							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
  2528							  f->mask);
  2529	
  2530			if (is_broadcast_ether_addr(mask->dst)) {
  2531				input->filter.match_flags |=
  2532					IGB_FILTER_FLAG_DST_MAC_ADDR;
  2533				ether_addr_copy(input->filter.dst_addr, key->dst);
  2534			}
  2535	
  2536			if (is_broadcast_ether_addr(mask->src)) {
  2537				input->filter.match_flags |=
  2538					IGB_FILTER_FLAG_SRC_MAC_ADDR;
  2539				ether_addr_copy(input->filter.src_addr, key->src);
  2540			}
  2541		}
  2542	
  2543		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
  2544			struct flow_dissector_key_basic *key =
  2545				skb_flow_dissector_target(f->dissector,
  2546							  FLOW_DISSECTOR_KEY_BASIC,
  2547							  f->key);
  2548	
  2549			struct flow_dissector_key_basic *mask =
  2550				skb_flow_dissector_target(f->dissector,
  2551							  FLOW_DISSECTOR_KEY_BASIC,
  2552							  f->mask);
  2553	
  2554			if (mask->n_proto == ETHER_TYPE_FULL_MASK) {
  2555				input->filter.match_flags |=
  2556					IGB_FILTER_FLAG_ETHER_TYPE;
  2557				input->filter.etype = key->n_proto;
  2558			}
  2559		}
  2560	
  2561		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
  2562			struct flow_dissector_key_vlan *key =
  2563				skb_flow_dissector_target(f->dissector,
  2564							  FLOW_DISSECTOR_KEY_VLAN,
  2565							  f->key);
  2566			struct flow_dissector_key_vlan *mask =
  2567				skb_flow_dissector_target(f->dissector,
  2568							  FLOW_DISSECTOR_KEY_VLAN,
  2569							  f->mask);
  2570	
  2571			if (mask->vlan_priority) {
  2572				input->filter.match_flags |= IGB_FILTER_FLAG_VLAN_TCI;
> 2573				input->filter.vlan_tci = key->vlan_priority;
  2574			}
  2575		}
  2576	
  2577		input->action = traffic_class;
  2578		input->cookie = f->cookie;
  2579	
  2580		return 0;
  2581	}
  2582	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[kbuild test robot]

Hi Vinicius,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jkirsher-next-queue/dev-queue]
[also build test WARNING on v4.16-rc2 next-20180223]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Vinicius-Costa-Gomes/igb-Fix-not-adding-filter-elements-to-the-list/20180226-053124
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/next-queue.git dev-queue
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/net/ethernet/intel/igb/igb_main.c:474:25: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:474:25: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:554:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:554:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:560:33: sparse: cast to restricted __le64
   drivers/net/ethernet/intel/igb/igb_main.c:560:33: sparse: cast to restricted __le64
>> drivers/net/ethernet/intel/igb/igb_main.c:2573:48: sparse: incorrect type in assignment (different base types) @@ expected restricted __be16 vlan_tci @@ got unsignedrestricted __be16 vlan_tci @@
   drivers/net/ethernet/intel/igb/igb_main.c:2573:48: expected restricted __be16 vlan_tci
   drivers/net/ethernet/intel/igb/igb_main.c:2573:48: got unsigned short vlan_priority:3
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: sparse: incorrect type in argument 2 (different base types) @@ expected restricted __wsum diff @@ got restricted __wsum diff @@
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: expected restricted __wsum diff
   drivers/net/ethernet/intel/igb/igb_main.c:5616:46: got restricted __be32 <noident>
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast to restricted __be16
   drivers/net/ethernet/intel/igb/igb_main.c:8047:31: sparse: cast from restricted __le16

vim +2573 drivers/net/ethernet/intel/igb/igb_main.c

  2503	
  2504	static int igb_parse_cls_flower(struct igb_adapter *adapter,
  2505					struct tc_cls_flower_offload *f,
  2506					int traffic_class,
  2507					struct igb_nfc_filter *input)
  2508	{
  2509		if (f->dissector->used_keys &
  2510		    ~(BIT(FLOW_DISSECTOR_KEY_BASIC) |
  2511		      BIT(FLOW_DISSECTOR_KEY_CONTROL) |
  2512		      BIT(FLOW_DISSECTOR_KEY_ETH_ADDRS) |
  2513		      BIT(FLOW_DISSECTOR_KEY_VLAN))) {
  2514			dev_err(&adapter->pdev->dev, "Unsupported key used: 0x%x\n",
  2515				f->dissector->used_keys);
  2516			return -EOPNOTSUPP;
  2517		}
  2518	
  2519		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
  2520			struct flow_dissector_key_eth_addrs *key =
  2521				skb_flow_dissector_target(f->dissector,
  2522							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
  2523							  f->key);
  2524	
  2525			struct flow_dissector_key_eth_addrs *mask =
  2526				skb_flow_dissector_target(f->dissector,
  2527							  FLOW_DISSECTOR_KEY_ETH_ADDRS,
  2528							  f->mask);
  2529	
  2530			if (is_broadcast_ether_addr(mask->dst)) {
  2531				input->filter.match_flags |=
  2532					IGB_FILTER_FLAG_DST_MAC_ADDR;
  2533				ether_addr_copy(input->filter.dst_addr, key->dst);
  2534			}
  2535	
  2536			if (is_broadcast_ether_addr(mask->src)) {
  2537				input->filter.match_flags |=
  2538					IGB_FILTER_FLAG_SRC_MAC_ADDR;
  2539				ether_addr_copy(input->filter.src_addr, key->src);
  2540			}
  2541		}
  2542	
  2543		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_BASIC)) {
  2544			struct flow_dissector_key_basic *key =
  2545				skb_flow_dissector_target(f->dissector,
  2546							  FLOW_DISSECTOR_KEY_BASIC,
  2547							  f->key);
  2548	
  2549			struct flow_dissector_key_basic *mask =
  2550				skb_flow_dissector_target(f->dissector,
  2551							  FLOW_DISSECTOR_KEY_BASIC,
  2552							  f->mask);
  2553	
  2554			if (mask->n_proto == ETHER_TYPE_FULL_MASK) {
  2555				input->filter.match_flags |=
  2556					IGB_FILTER_FLAG_ETHER_TYPE;
  2557				input->filter.etype = key->n_proto;
  2558			}
  2559		}
  2560	
  2561		if (dissector_uses_key(f->dissector, FLOW_DISSECTOR_KEY_VLAN)) {
  2562			struct flow_dissector_key_vlan *key =
  2563				skb_flow_dissector_target(f->dissector,
  2564							  FLOW_DISSECTOR_KEY_VLAN,
  2565							  f->key);
  2566			struct flow_dissector_key_vlan *mask =
  2567				skb_flow_dissector_target(f->dissector,
  2568							  FLOW_DISSECTOR_KEY_VLAN,
  2569							  f->mask);
  2570	
  2571			if (mask->vlan_priority) {
  2572				input->filter.match_flags |= IGB_FILTER_FLAG_VLAN_TCI;
> 2573				input->filter.vlan_tci = key->vlan_priority;
  2574			}
  2575		}
  2576	
  2577		input->action = traffic_class;
  2578		input->cookie = f->cookie;
  2579	
  2580		return 0;
  2581	}
  2582	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [Intel-wired-lan] [next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Vinicius Costa Gomes]

Hi,

Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> This will allow functionality depending on the hardware being traffic
>> class aware to work. In particular the tc-flower offloading checks
>> verifies that this bit is set.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>  drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 0ea32be07d71..543aa99892eb 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>                                NETIF_F_HW_VLAN_CTAG_TX |
>>                                NETIF_F_RXALL;
>>
>> -       if (hw->mac.type >= e1000_i350)
>> -               netdev->hw_features |= NETIF_F_NTUPLE;
>> +       if (hw->mac.type >= e1000_i350) {
>> +               netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
>> +               netdev->features |= NETIF_F_HW_TC;
>
> The parens aren't needed.
>
> Also you might consider moving this block up to where we have a
> similar one for 82576. Then you wouldn't need to set both features and
> hw_features in the case of the HW_TC flag.

Cool. Will fix.

>
>> +       }
>>
>>         if (pci_using_dac)
>>                 netdev->features |= NETIF_F_HIGHDMA;
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Cheers,
--
Vinicius

[Intel-wired-lan] [next-queue PATCH 3/8] igb: Enable the hardware traffic class feature bit for igb models

[Vinicius Costa Gomes]

Hi,

Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> This will allow functionality depending on the hardware being traffic
>> class aware to work. In particular the tc-flower offloading checks
>> verifies that this bit is set.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>  drivers/net/ethernet/intel/igb/igb_main.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 0ea32be07d71..543aa99892eb 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -2820,8 +2820,10 @@ static int igb_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>                                NETIF_F_HW_VLAN_CTAG_TX |
>>                                NETIF_F_RXALL;
>>
>> -       if (hw->mac.type >= e1000_i350)
>> -               netdev->hw_features |= NETIF_F_NTUPLE;
>> +       if (hw->mac.type >= e1000_i350) {
>> +               netdev->hw_features |= (NETIF_F_NTUPLE | NETIF_F_HW_TC);
>> +               netdev->features |= NETIF_F_HW_TC;
>
> The parens aren't needed.
>
> Also you might consider moving this block up to where we have a
> similar one for 82576. Then you wouldn't need to set both features and
> hw_features in the case of the HW_TC flag.

Cool. Will fix.

>
>> +       }
>>
>>         if (pci_using_dac)
>>                 netdev->features |= NETIF_F_HIGHDMA;
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan at osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan


Cheers,
--
Vinicius

Re: [Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Vinicius Costa Gomes]

Hi,

Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> Makes it possible to direct packets to queues based on their source
>> address. Documents the expected usage of the 'flags' parameter.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>  drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
>>  drivers/net/ethernet/intel/igb/igb.h           |  1 +
>>  drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
>>  3 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
>> index 573bf177fd08..c6f552de30dd 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
>> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
>> @@ -490,6 +490,7 @@
>>   * manageability enabled, allowing us room for 15 multicast addresses.
>>   */
>>  #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
>> +#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
>>  #define E1000_RAH_QSEL_ENABLE 0x10000000
>>  #define E1000_RAL_MAC_ADDR_LEN 4
>>  #define E1000_RAH_MAC_ADDR_LEN 2
>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>> index 1c6b8d9176a8..d5cd5f6708d9 100644
>> --- a/drivers/net/ethernet/intel/igb/igb.h
>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>> @@ -472,6 +472,7 @@ struct igb_mac_addr {
>>
>>  #define IGB_MAC_STATE_DEFAULT  0x1
>>  #define IGB_MAC_STATE_IN_USE   0x2
>> +#define IGB_MAC_STATE_SRC_ADDR  0x4
>>
>>  /* board specific private data structure */
>>  struct igb_adapter {
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 543aa99892eb..db66b697fe3b 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
>>         igb_rar_set_index(adapter, 0);
>>  }
>>
>> +/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
>> + */
>>  static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>> -                             const u8 queue)
>> +                             const u8 queue, const u8 flags)
>>  {
>>         struct e1000_hw *hw = &adapter->hw;
>>         int rar_entries = hw->mac.rar_entry_count -
>> @@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>>
>>                 ether_addr_copy(adapter->mac_table[i].addr, addr);
>>                 adapter->mac_table[i].queue = queue;
>> -               adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
>> +               adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);
>
> More unneeded parenthesis.

Will fix.

>
>>
>>                 igb_rar_set_index(adapter, i);
>>                 return i;
>> @@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
>> + * IGB_MAC_STATE_SRC_ADDR can be used.
>> + */
>>  static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>> -                             const u8 queue)
>> +                             const u8 queue, const u8 flags)
>>  {
>>         struct e1000_hw *hw = &adapter->hw;
>>         int rar_entries = hw->mac.rar_entry_count -
>> @@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>>          * for the VF MAC addresses.
>>          */
>>         for (i = 0; i < rar_entries; i++) {
>> -               if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
>> +               if (!(adapter->mac_table[i].state &
>> +                     (IGB_MAC_STATE_IN_USE | flags)))
>
> Shouldn't these be two separate checks? If the address isn't in use
> why would I care what the flags state is? It probably isn't valid.

Will change.

>
>>                         continue;
>>                 if (adapter->mac_table[i].queue != queue)
>>                         continue;
>>                 if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>>                         continue;
>>
>> -               adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
>> +               adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);
>
> Maybe instead of just clearing the specific flags we should just clear
> the state and reset it back to 0.

Will take another look at the "default" entries, which are the other
users of specific flags, as you put it, and see if I wouldn't break
them.

>
>>                 memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>>                 adapter->mac_table[i].queue = 0;
>>
>> @@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
>>         struct igb_adapter *adapter = netdev_priv(netdev);
>>         int ret;
>>
>> -       ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
>> +       ret = igb_add_mac_filter(adapter, addr,
>> +                                adapter->vfs_allocated_count, 0);
>
> Instead of having to add a 0 to all these functions it might be better
> to rename the original igb_add_mac_filter, and then add a new function
> named the same as the original that just automatically passes the 0.
> It will take a lot of noise out of this code.

Cool. Will do.

>
>>
>>         return min_t(int, ret, 0);
>>  }
>> @@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>>  {
>>         struct igb_adapter *adapter = netdev_priv(netdev);
>>
>> -       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
>> +       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
>>
>>         return 0;
>>  }
>> @@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>>                         if (entry->vf == vf) {
>>                                 entry->vf = -1;
>>                                 entry->free = true;
>> -                               igb_del_mac_filter(adapter, entry->vf_mac, vf);
>> +                               igb_del_mac_filter(adapter,
>> +                                                  entry->vf_mac, vf, 0);
>>                         }
>>                 }
>>                 break;
>> @@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>>                         entry->vf = vf;
>>                         ether_addr_copy(entry->vf_mac, addr);
>>
>> -                       ret = igb_add_mac_filter(adapter, addr, vf);
>> +                       ret = igb_add_mac_filter(adapter, addr, vf, 0);
>>                         ret = min_t(int, ret, 0);
>>                 } else {
>>                         ret = -ENOSPC;
>> @@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
>>                 if (is_valid_ether_addr(addr))
>>                         rar_high |= E1000_RAH_AV;
>>
>> +               if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
>> +                       rar_high |= E1000_RAH_ASEL_SRC_ADDR;
>> +
>>                 switch (hw->mac.type) {
>>                 case e1000_82575:
>>                 case e1000_i210:
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan@osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

[Intel-wired-lan] [next-queue PATCH 4/8] igb: Add support for MAC address filters specifying source addresses

[Vinicius Costa Gomes]

Hi,

Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Feb 23, 2018 at 5:20 PM, Vinicius Costa Gomes
> <vinicius.gomes@intel.com> wrote:
>> Makes it possible to direct packets to queues based on their source
>> address. Documents the expected usage of the 'flags' parameter.
>>
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> ---
>>  drivers/net/ethernet/intel/igb/e1000_defines.h |  1 +
>>  drivers/net/ethernet/intel/igb/igb.h           |  1 +
>>  drivers/net/ethernet/intel/igb/igb_main.c      | 35 +++++++++++++++++++-------
>>  3 files changed, 28 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/e1000_defines.h b/drivers/net/ethernet/intel/igb/e1000_defines.h
>> index 573bf177fd08..c6f552de30dd 100644
>> --- a/drivers/net/ethernet/intel/igb/e1000_defines.h
>> +++ b/drivers/net/ethernet/intel/igb/e1000_defines.h
>> @@ -490,6 +490,7 @@
>>   * manageability enabled, allowing us room for 15 multicast addresses.
>>   */
>>  #define E1000_RAH_AV  0x80000000        /* Receive descriptor valid */
>> +#define E1000_RAH_ASEL_SRC_ADDR 0x00010000
>>  #define E1000_RAH_QSEL_ENABLE 0x10000000
>>  #define E1000_RAL_MAC_ADDR_LEN 4
>>  #define E1000_RAH_MAC_ADDR_LEN 2
>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>> index 1c6b8d9176a8..d5cd5f6708d9 100644
>> --- a/drivers/net/ethernet/intel/igb/igb.h
>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>> @@ -472,6 +472,7 @@ struct igb_mac_addr {
>>
>>  #define IGB_MAC_STATE_DEFAULT  0x1
>>  #define IGB_MAC_STATE_IN_USE   0x2
>> +#define IGB_MAC_STATE_SRC_ADDR  0x4
>>
>>  /* board specific private data structure */
>>  struct igb_adapter {
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index 543aa99892eb..db66b697fe3b 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -6837,8 +6837,13 @@ static void igb_set_default_mac_filter(struct igb_adapter *adapter)
>>         igb_rar_set_index(adapter, 0);
>>  }
>>
>> +/* 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 IGB_MAC_STATE_SRC_ADDR can be used.
>> + */
>>  static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>> -                             const u8 queue)
>> +                             const u8 queue, const u8 flags)
>>  {
>>         struct e1000_hw *hw = &adapter->hw;
>>         int rar_entries = hw->mac.rar_entry_count -
>> @@ -6858,7 +6863,7 @@ static int igb_add_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>>
>>                 ether_addr_copy(adapter->mac_table[i].addr, addr);
>>                 adapter->mac_table[i].queue = queue;
>> -               adapter->mac_table[i].state |= IGB_MAC_STATE_IN_USE;
>> +               adapter->mac_table[i].state |= (IGB_MAC_STATE_IN_USE | flags);
>
> More unneeded parenthesis.

Will fix.

>
>>
>>                 igb_rar_set_index(adapter, i);
>>                 return i;
>> @@ -6867,8 +6872,14 @@ static int igb_add_mac_filter(struct igb_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
>> + * IGB_MAC_STATE_SRC_ADDR can be used.
>> + */
>>  static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>> -                             const u8 queue)
>> +                             const u8 queue, const u8 flags)
>>  {
>>         struct e1000_hw *hw = &adapter->hw;
>>         int rar_entries = hw->mac.rar_entry_count -
>> @@ -6883,14 +6894,15 @@ static int igb_del_mac_filter(struct igb_adapter *adapter, const u8 *addr,
>>          * for the VF MAC addresses.
>>          */
>>         for (i = 0; i < rar_entries; i++) {
>> -               if (!(adapter->mac_table[i].state & IGB_MAC_STATE_IN_USE))
>> +               if (!(adapter->mac_table[i].state &
>> +                     (IGB_MAC_STATE_IN_USE | flags)))
>
> Shouldn't these be two separate checks? If the address isn't in use
> why would I care what the flags state is? It probably isn't valid.

Will change.

>
>>                         continue;
>>                 if (adapter->mac_table[i].queue != queue)
>>                         continue;
>>                 if (!ether_addr_equal(adapter->mac_table[i].addr, addr))
>>                         continue;
>>
>> -               adapter->mac_table[i].state &= ~IGB_MAC_STATE_IN_USE;
>> +               adapter->mac_table[i].state &= ~(IGB_MAC_STATE_IN_USE | flags);
>
> Maybe instead of just clearing the specific flags we should just clear
> the state and reset it back to 0.

Will take another look at the "default" entries, which are the other
users of specific flags, as you put it, and see if I wouldn't break
them.

>
>>                 memset(adapter->mac_table[i].addr, 0, ETH_ALEN);
>>                 adapter->mac_table[i].queue = 0;
>>
>> @@ -6906,7 +6918,8 @@ static int igb_uc_sync(struct net_device *netdev, const unsigned char *addr)
>>         struct igb_adapter *adapter = netdev_priv(netdev);
>>         int ret;
>>
>> -       ret = igb_add_mac_filter(adapter, addr, adapter->vfs_allocated_count);
>> +       ret = igb_add_mac_filter(adapter, addr,
>> +                                adapter->vfs_allocated_count, 0);
>
> Instead of having to add a 0 to all these functions it might be better
> to rename the original igb_add_mac_filter, and then add a new function
> named the same as the original that just automatically passes the 0.
> It will take a lot of noise out of this code.

Cool. Will do.

>
>>
>>         return min_t(int, ret, 0);
>>  }
>> @@ -6915,7 +6928,7 @@ static int igb_uc_unsync(struct net_device *netdev, const unsigned char *addr)
>>  {
>>         struct igb_adapter *adapter = netdev_priv(netdev);
>>
>> -       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count);
>> +       igb_del_mac_filter(adapter, addr, adapter->vfs_allocated_count, 0);
>>
>>         return 0;
>>  }
>> @@ -6937,7 +6950,8 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>>                         if (entry->vf == vf) {
>>                                 entry->vf = -1;
>>                                 entry->free = true;
>> -                               igb_del_mac_filter(adapter, entry->vf_mac, vf);
>> +                               igb_del_mac_filter(adapter,
>> +                                                  entry->vf_mac, vf, 0);
>>                         }
>>                 }
>>                 break;
>> @@ -6968,7 +6982,7 @@ static int igb_set_vf_mac_filter(struct igb_adapter *adapter, const int vf,
>>                         entry->vf = vf;
>>                         ether_addr_copy(entry->vf_mac, addr);
>>
>> -                       ret = igb_add_mac_filter(adapter, addr, vf);
>> +                       ret = igb_add_mac_filter(adapter, addr, vf, 0);
>>                         ret = min_t(int, ret, 0);
>>                 } else {
>>                         ret = -ENOSPC;
>> @@ -8743,6 +8757,9 @@ static void igb_rar_set_index(struct igb_adapter *adapter, u32 index)
>>                 if (is_valid_ether_addr(addr))
>>                         rar_high |= E1000_RAH_AV;
>>
>> +               if (adapter->mac_table[index].state & IGB_MAC_STATE_SRC_ADDR)
>> +                       rar_high |= E1000_RAH_ASEL_SRC_ADDR;
>> +
>>                 switch (hw->mac.type) {
>>                 case e1000_82575:
>>                 case e1000_i210:
>> --
>> 2.16.2
>>
>> _______________________________________________
>> Intel-wired-lan mailing list
>> Intel-wired-lan at osuosl.org
>> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:33 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This adds the capability of configuring the queue steering of arriving
>>packets based on their source and destination MAC addresses.
>>
>>In practical terms this adds support for the following use cases,
>>characterized by these examples:
>>
>>$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
>>(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
>>to the RX queue 0)
>>
>>$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
>>(this will direct packets with destination address "44:44:44:44:44:44"
>>to the RX queue 3)
>>
>>Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>---
>
> [snip]
>
>>diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>index 143f0bb34e4d..d8686a0f5b5d 100644
>>--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>@@ -152,6 +152,9 @@ static const char
>>igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
>> 
>> #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
>> 
>>+static const u8 broadcast_addr[ETH_ALEN] = {
>>+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>
> This is already defined in an existing header, don't have it handy but
> likely etherdevice.h.

Yeah, I didn't find the address definition, but there's a helper to
build a broadcast address, which is just what I need. Thanks.

>
> -- 
> Florian


Cheers,
--
Vinicius

[Intel-wired-lan] [next-queue PATCH 5/8] igb: Add support for ethtool MAC address filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:33 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This adds the capability of configuring the queue steering of arriving
>>packets based on their source and destination MAC addresses.
>>
>>In practical terms this adds support for the following use cases,
>>characterized by these examples:
>>
>>$ ethtool -N eth0 flow-type ether dst aa:aa:aa:aa:aa:aa action 0
>>(this will direct packets with destination address "aa:aa:aa:aa:aa:aa"
>>to the RX queue 0)
>>
>>$ ethtool -N eth0 flow-type ether src 44:44:44:44:44:44 action 3
>>(this will direct packets with destination address "44:44:44:44:44:44"
>>to the RX queue 3)
>>
>>Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>>---
>
> [snip]
>
>>diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>index 143f0bb34e4d..d8686a0f5b5d 100644
>>--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
>>@@ -152,6 +152,9 @@ static const char
>>igb_priv_flags_strings[][ETH_GSTRING_LEN] = {
>> 
>> #define IGB_PRIV_FLAGS_STR_LEN ARRAY_SIZE(igb_priv_flags_strings)
>> 
>>+static const u8 broadcast_addr[ETH_ALEN] = {
>>+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
>
> This is already defined in an existing header, don't have it handy but
> likely etherdevice.h.

Yeah, I didn't find the address definition, but there's a helper to
build a broadcast address, which is just what I need. Thanks.

>
> -- 
> Florian


Cheers,
--
Vinicius

Re: [next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:36 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This allows tc-flower filters that were offloaded to be removed.
>
> This should be squashed into your previous patch, either the
> functionality is there and you can add/remove or it is not.

Will do. Thanks.

>
> -- 
> Florian


Cheers,
--
Vinicius

[Intel-wired-lan] [next-queue PATCH 8/8] igb: Add support for removing offloaded tc-flower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:36 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This allows tc-flower filters that were offloaded to be removed.
>
> This should be squashed into your previous patch, either the
> functionality is there and you can add/remove or it is not.

Will do. Thanks.

>
> -- 
> Florian


Cheers,
--
Vinicius

Re: [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This allows filters added by tc-flower and specifying MAC addresses,
>>Ethernet types, and the VLAN priority field, to be offloaded to the
>>controller.
>>
>>This reuses most of the infrastructure used by ethtool, ethtool can be
>>used to read these filters, but modification and deletion can only be
>>done via tc-flower.
>
> You would want to check what other drivers supporting both
> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
> to an user. As an user I would be more comfortable with seeing only
> rules added through ethtool via ethtool and those added by cls_flower
> via cls_flower. They will both access a shared set of resources but it
> seems easier for me to dump rules with both tools to figure out why
> -ENOSPC was returned rather than seeing something I did not add.
> Others might see it entirely differently.

I took a closer look at mlx5 and i40e, and they seem to use different
hardware capabilities (even incompatible in the case of i40e, which
returns an error when tring to add cls_flower filter when an ethtool
based filter exists) for ethtool and cls_flower. So I don't think the
model applies exactly here.

As they keep the filters separated for the user perspective, I could do
the same, in the name of convention, it's just that the separation is
more "artificial". But I have no strong opinions either way.

>
> If you added the ability for cls_flower to indicate a queue number and
> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
> that eliminate entirely the need for adding MAC address steering in
> earlier patches?

I am not sure that I understand. 'cls_flower' already has support for
indicating a queue number (expressed via the 'hw_tc' parameter to tc)
(commit 384c181e3780 ("net: sched: Identify hardware traffic classes
using classid").

And adding more control for the allocation of indexes for the rules seem
not to help much in reducing the size/complexity of this series. I.e.
this series has 4 parts: bug fixes, adding support for source addresses
for MAC filters, adding ethtool support MAC address filters (as it was
only missing some glue code), and adding offloading for some types of
cls_flower filters. More control for the allocation of rule indexes would
only affect the cls_flower part.

But perhaps I could be missing something here.

>
> -- 
> Florian

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>This allows filters added by tc-flower and specifying MAC addresses,
>>Ethernet types, and the VLAN priority field, to be offloaded to the
>>controller.
>>
>>This reuses most of the infrastructure used by ethtool, ethtool can be
>>used to read these filters, but modification and deletion can only be
>>done via tc-flower.
>
> You would want to check what other drivers supporting both
> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
> to an user. As an user I would be more comfortable with seeing only
> rules added through ethtool via ethtool and those added by cls_flower
> via cls_flower. They will both access a shared set of resources but it
> seems easier for me to dump rules with both tools to figure out why
> -ENOSPC was returned rather than seeing something I did not add.
> Others might see it entirely differently.

I took a closer look at mlx5 and i40e, and they seem to use different
hardware capabilities (even incompatible in the case of i40e, which
returns an error when tring to add cls_flower filter when an ethtool
based filter exists) for ethtool and cls_flower. So I don't think the
model applies exactly here.

As they keep the filters separated for the user perspective, I could do
the same, in the name of convention, it's just that the separation is
more "artificial". But I have no strong opinions either way.

>
> If you added the ability for cls_flower to indicate a queue number and
> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
> that eliminate entirely the need for adding MAC address steering in
> earlier patches?

I am not sure that I understand. 'cls_flower' already has support for
indicating a queue number (expressed via the 'hw_tc' parameter to tc)
(commit 384c181e3780 ("net: sched: Identify hardware traffic classes
using classid").

And adding more control for the allocation of indexes for the rules seem
not to help much in reducing the size/complexity of this series. I.e.
this series has 4 parts: bug fixes, adding support for source addresses
for MAC filters, adding ethtool support MAC address filters (as it was
only missing some glue code), and adding offloading for some types of
cls_flower filters. More control for the allocation of rule indexes would
only affect the cls_flower part.

But perhaps I could be missing something here.

>
> -- 
> Florian

Re: [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Florian Fainelli]

On 02/26/2018 04:40 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>> This allows filters added by tc-flower and specifying MAC addresses,
>>> Ethernet types, and the VLAN priority field, to be offloaded to the
>>> controller.
>>>
>>> This reuses most of the infrastructure used by ethtool, ethtool can be
>>> used to read these filters, but modification and deletion can only be
>>> done via tc-flower.
>>
>> You would want to check what other drivers supporting both
>> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
>> to an user. As an user I would be more comfortable with seeing only
>> rules added through ethtool via ethtool and those added by cls_flower
>> via cls_flower. They will both access a shared set of resources but it
>> seems easier for me to dump rules with both tools to figure out why
>> -ENOSPC was returned rather than seeing something I did not add.
>> Others might see it entirely differently.
> 
> I took a closer look at mlx5 and i40e, and they seem to use different
> hardware capabilities (even incompatible in the case of i40e, which
> returns an error when tring to add cls_flower filter when an ethtool
> based filter exists) for ethtool and cls_flower. So I don't think the
> model applies exactly here.
> 
> As they keep the filters separated for the user perspective, I could do
> the same, in the name of convention, it's just that the separation is
> more "artificial". But I have no strong opinions either way.

True, I would still conform to what these two drivers do since they have
a large user base (so does igb, but not yet for cls_flower yet since you
are obviously working on it).

> 
>>
>> If you added the ability for cls_flower to indicate a queue number and
>> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
>> that eliminate entirely the need for adding MAC address steering in
>> earlier patches?
> 
> I am not sure that I understand. 'cls_flower' already has support for
> indicating a queue number (expressed via the 'hw_tc' parameter to tc)
> (commit 384c181e3780 ("net: sched: Identify hardware traffic classes
> using classid").

I had missed that cls_flower gained the capability to specify a queue
number, that's good. What it still does not support AFAICT that ethtool
does though is either automatically allocating a rule location (Rule ID
shown by ethtool) or allowing placement at a specific location. This can
be important when the rule location can be carried by the hardware on
e.g: a per-packet basis, the hardware that I work with (bcm_sf2_cfp.c)
makes use of that for instance, maybe this is such an isolated case that
I should take care of it at some point if I was remotely serious into
providing tc/cls_flower support for that driver...

> 
> And adding more control for the allocation of indexes for the rules seem
> not to help much in reducing the size/complexity of this series. I.e.
> this series has 4 parts: bug fixes, adding support for source addresses
> for MAC filters, adding ethtool support MAC address filters (as it was
> only missing some glue code), and adding offloading for some types of
> cls_flower filters. More control for the allocation of rule indexes would
> only affect the cls_flower part.
> 
> But perhaps I could be missing something here.

You are absolutely right, it was not so much about trying to reduce the
complexity rather than avoiding having two user interface facilities:
ethtool and tc/cls_flower to do essentailly the same thing, yet, having
some small differences in the offered capabilities, in the case of
tc/cls_flower, lack of specification of rule location.
-- 
Florian

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Florian Fainelli]

On 02/26/2018 04:40 PM, Vinicius Costa Gomes wrote:
> Hi,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>> This allows filters added by tc-flower and specifying MAC addresses,
>>> Ethernet types, and the VLAN priority field, to be offloaded to the
>>> controller.
>>>
>>> This reuses most of the infrastructure used by ethtool, ethtool can be
>>> used to read these filters, but modification and deletion can only be
>>> done via tc-flower.
>>
>> You would want to check what other drivers supporting both
>> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
>> to an user. As an user I would be more comfortable with seeing only
>> rules added through ethtool via ethtool and those added by cls_flower
>> via cls_flower. They will both access a shared set of resources but it
>> seems easier for me to dump rules with both tools to figure out why
>> -ENOSPC was returned rather than seeing something I did not add.
>> Others might see it entirely differently.
> 
> I took a closer look at mlx5 and i40e, and they seem to use different
> hardware capabilities (even incompatible in the case of i40e, which
> returns an error when tring to add cls_flower filter when an ethtool
> based filter exists) for ethtool and cls_flower. So I don't think the
> model applies exactly here.
> 
> As they keep the filters separated for the user perspective, I could do
> the same, in the name of convention, it's just that the separation is
> more "artificial". But I have no strong opinions either way.

True, I would still conform to what these two drivers do since they have
a large user base (so does igb, but not yet for cls_flower yet since you
are obviously working on it).

> 
>>
>> If you added the ability for cls_flower to indicate a queue number and
>> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
>> that eliminate entirely the need for adding MAC address steering in
>> earlier patches?
> 
> I am not sure that I understand. 'cls_flower' already has support for
> indicating a queue number (expressed via the 'hw_tc' parameter to tc)
> (commit 384c181e3780 ("net: sched: Identify hardware traffic classes
> using classid").

I had missed that cls_flower gained the capability to specify a queue
number, that's good. What it still does not support AFAICT that ethtool
does though is either automatically allocating a rule location (Rule ID
shown by ethtool) or allowing placement at a specific location. This can
be important when the rule location can be carried by the hardware on
e.g: a per-packet basis, the hardware that I work with (bcm_sf2_cfp.c)
makes use of that for instance, maybe this is such an isolated case that
I should take care of it at some point if I was remotely serious into
providing tc/cls_flower support for that driver...

> 
> And adding more control for the allocation of indexes for the rules seem
> not to help much in reducing the size/complexity of this series. I.e.
> this series has 4 parts: bug fixes, adding support for source addresses
> for MAC filters, adding ethtool support MAC address filters (as it was
> only missing some glue code), and adding offloading for some types of
> cls_flower filters. More control for the allocation of rule indexes would
> only affect the cls_flower part.
> 
> But perhaps I could be missing something here.

You are absolutely right, it was not so much about trying to reduce the
complexity rather than avoiding having two user interface facilities:
ethtool and tc/cls_flower to do essentailly the same thing, yet, having
some small differences in the offered capabilities, in the case of
tc/cls_flower, lack of specification of rule location.
-- 
Florian

Re: [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 02/26/2018 04:40 PM, Vinicius Costa Gomes wrote:
>> Hi,
>> 
>> Florian Fainelli <f.fainelli@gmail.com> writes:
>> 
>>> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>>> This allows filters added by tc-flower and specifying MAC addresses,
>>>> Ethernet types, and the VLAN priority field, to be offloaded to the
>>>> controller.
>>>>
>>>> This reuses most of the infrastructure used by ethtool, ethtool can be
>>>> used to read these filters, but modification and deletion can only be
>>>> done via tc-flower.
>>>
>>> You would want to check what other drivers supporting both
>>> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
>>> to an user. As an user I would be more comfortable with seeing only
>>> rules added through ethtool via ethtool and those added by cls_flower
>>> via cls_flower. They will both access a shared set of resources but it
>>> seems easier for me to dump rules with both tools to figure out why
>>> -ENOSPC was returned rather than seeing something I did not add.
>>> Others might see it entirely differently.
>> 
>> I took a closer look at mlx5 and i40e, and they seem to use different
>> hardware capabilities (even incompatible in the case of i40e, which
>> returns an error when tring to add cls_flower filter when an ethtool
>> based filter exists) for ethtool and cls_flower. So I don't think the
>> model applies exactly here.
>> 
>> As they keep the filters separated for the user perspective, I could do
>> the same, in the name of convention, it's just that the separation is
>> more "artificial". But I have no strong opinions either way.
>
> True, I would still conform to what these two drivers do since they have
> a large user base (so does igb, but not yet for cls_flower yet since you
> are obviously working on it).

Awesome. Will present them as separate to the user, then.

>
>> 
>>>
>>> If you added the ability for cls_flower to indicate a queue number and
>>> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
>>> that eliminate entirely the need for adding MAC address steering in
>>> earlier patches?
>> 
>> I am not sure that I understand. 'cls_flower' already has support for
>> indicating a queue number (expressed via the 'hw_tc' parameter to tc)
>> (commit 384c181e3780 ("net: sched: Identify hardware traffic classes
>> using classid").
>
> I had missed that cls_flower gained the capability to specify a queue
> number, that's good. What it still does not support AFAICT that ethtool
> does though is either automatically allocating a rule location (Rule ID
> shown by ethtool) or allowing placement at a specific location. This can
> be important when the rule location can be carried by the hardware on
> e.g: a per-packet basis, the hardware that I work with (bcm_sf2_cfp.c)
> makes use of that for instance, maybe this is such an isolated case that
> I should take care of it at some point if I was remotely serious into
> providing tc/cls_flower support for that driver...

Now I am starting to see what you meant about the rules location. In the
igb case of the igb-based controller, only the rule 0 is special (it's
reserved for the local address) which the user wouldn't have no control
over. From what I can see, for the igb driver, the location of rules only
controls the order they are displayed to the user.

>
>> 
>> And adding more control for the allocation of indexes for the rules seem
>> not to help much in reducing the size/complexity of this series. I.e.
>> this series has 4 parts: bug fixes, adding support for source addresses
>> for MAC filters, adding ethtool support MAC address filters (as it was
>> only missing some glue code), and adding offloading for some types of
>> cls_flower filters. More control for the allocation of rule indexes would
>> only affect the cls_flower part.
>> 
>> But perhaps I could be missing something here.
>
> You are absolutely right, it was not so much about trying to reduce the
> complexity rather than avoiding having two user interface facilities:
> ethtool and tc/cls_flower to do essentailly the same thing, yet, having
> some small differences in the offered capabilities, in the case of
> tc/cls_flower, lack of specification of rule location.
> -- 
> Florian


Cheers,
--
Vinicius

[Intel-wired-lan] [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

[Vinicius Costa Gomes]

Hi,

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 02/26/2018 04:40 PM, Vinicius Costa Gomes wrote:
>> Hi,
>> 
>> Florian Fainelli <f.fainelli@gmail.com> writes:
>> 
>>> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@intel.com> wrote:
>>>> This allows filters added by tc-flower and specifying MAC addresses,
>>>> Ethernet types, and the VLAN priority field, to be offloaded to the
>>>> controller.
>>>>
>>>> This reuses most of the infrastructure used by ethtool, ethtool can be
>>>> used to read these filters, but modification and deletion can only be
>>>> done via tc-flower.
>>>
>>> You would want to check what other drivers supporting both
>>> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
>>> to an user. As an user I would be more comfortable with seeing only
>>> rules added through ethtool via ethtool and those added by cls_flower
>>> via cls_flower. They will both access a shared set of resources but it
>>> seems easier for me to dump rules with both tools to figure out why
>>> -ENOSPC was returned rather than seeing something I did not add.
>>> Others might see it entirely differently.
>> 
>> I took a closer look at mlx5 and i40e, and they seem to use different
>> hardware capabilities (even incompatible in the case of i40e, which
>> returns an error when tring to add cls_flower filter when an ethtool
>> based filter exists) for ethtool and cls_flower. So I don't think the
>> model applies exactly here.
>> 
>> As they keep the filters separated for the user perspective, I could do
>> the same, in the name of convention, it's just that the separation is
>> more "artificial". But I have no strong opinions either way.
>
> True, I would still conform to what these two drivers do since they have
> a large user base (so does igb, but not yet for cls_flower yet since you
> are obviously working on it).

Awesome. Will present them as separate to the user, then.

>
>> 
>>>
>>> If you added the ability for cls_flower to indicate a queue number and
>>> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
>>> that eliminate entirely the need for adding MAC address steering in
>>> earlier patches?
>> 
>> I am not sure that I understand. 'cls_flower' already has support for
>> indicating a queue number (expressed via the 'hw_tc' parameter to tc)
>> (commit 384c181e3780 ("net: sched: Identify hardware traffic classes
>> using classid").
>
> I had missed that cls_flower gained the capability to specify a queue
> number, that's good. What it still does not support AFAICT that ethtool
> does though is either automatically allocating a rule location (Rule ID
> shown by ethtool) or allowing placement at a specific location. This can
> be important when the rule location can be carried by the hardware on
> e.g: a per-packet basis, the hardware that I work with (bcm_sf2_cfp.c)
> makes use of that for instance, maybe this is such an isolated case that
> I should take care of it at some point if I was remotely serious into
> providing tc/cls_flower support for that driver...

Now I am starting to see what you meant about the rules location. In the
igb case of the igb-based controller, only the rule 0 is special (it's
reserved for the local address) which the user wouldn't have no control
over. From what I can see, for the igb driver, the location of rules only
controls the order they are displayed to the user.

>
>> 
>> And adding more control for the allocation of indexes for the rules seem
>> not to help much in reducing the size/complexity of this series. I.e.
>> this series has 4 parts: bug fixes, adding support for source addresses
>> for MAC filters, adding ethtool support MAC address filters (as it was
>> only missing some glue code), and adding offloading for some types of
>> cls_flower filters. More control for the allocation of rule indexes would
>> only affect the cls_flower part.
>> 
>> But perhaps I could be missing something here.
>
> You are absolutely right, it was not so much about trying to reduce the
> complexity rather than avoiding having two user interface facilities:
> ethtool and tc/cls_flower to do essentailly the same thing, yet, having
> some small differences in the offered capabilities, in the case of
> tc/cls_flower, lack of specification of rule location.
> -- 
> Florian


Cheers,
--
Vinicius