[PATCH net-next 0/8] bnxt_en: Driver update for net-next.

[PATCH net-next 0/8] bnxt_en: Driver update for net-next.

[Michael Chan]

This patchset implements ethtool -X to setup user-defined RSS indirection
table.  The new infrastructure also allows the proper logical ring index
to be used to populate the RSS indirection when queried by ethtool -x.
Prior to these patches, we were incorrectly populating the output of
ethtool -x with internal ring IDs which would make no sense to the user.

The last 2 patches add some cleanups to the VLAN acceleration logic
and check the firmware capabilities before allowing VLAN acceleration
offloads.

Edwin Peer (2):
  bnxt_en: clean up VLAN feature bit handling
  bnxt_en: allow firmware to disable VLAN offloads

Michael Chan (6):
  bnxt_en: Set up the chip specific RSS table size.
  bnxt_en: Add logical RSS indirection table structure.
  bnxt_en: Add helper function to return the number of RSS contexts.
  bnxt_en: Fill HW RSS table from the RSS logical indirection table.
  bnxt_en: Return correct RSS indirection table entries to ethtool -x.
  bnxt_en: Implement ethtool -X to set indirection table.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 223 +++++++++++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  20 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  49 ++++-
 3 files changed, 224 insertions(+), 68 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/8] bnxt_en: Set up the chip specific RSS table size.

[Michael Chan]

Currently, we allocate one page for the hardware DMA RSS indirection
table.  While the size is currently big enough for all chips, future
chip variations may support bigger sizes, so it is better to calculate
and store the chip specific size and allocate accordingly.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 12 ++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  7 +++++++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6a884df..4afc1df 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -3538,7 +3538,7 @@ static void bnxt_free_vnic_attributes(struct bnxt *bp)
 		}
 
 		if (vnic->rss_table) {
-			dma_free_coherent(&pdev->dev, PAGE_SIZE,
+			dma_free_coherent(&pdev->dev, vnic->rss_table_size,
 					  vnic->rss_table,
 					  vnic->rss_table_dma_addr);
 			vnic->rss_table = NULL;
@@ -3603,7 +3603,13 @@ static int bnxt_alloc_vnic_attributes(struct bnxt *bp)
 			continue;
 
 		/* Allocate rss table and hash key */
-		vnic->rss_table = dma_alloc_coherent(&pdev->dev, PAGE_SIZE,
+		size = L1_CACHE_ALIGN(HW_HASH_INDEX_SIZE * sizeof(u16));
+		if (bp->flags & BNXT_FLAG_CHIP_P5)
+			size = L1_CACHE_ALIGN(BNXT_MAX_RSS_TABLE_SIZE_P5);
+
+		vnic->rss_table_size = size + HW_HASH_KEY_SIZE;
+		vnic->rss_table = dma_alloc_coherent(&pdev->dev,
+						     vnic->rss_table_size,
 						     &vnic->rss_table_dma_addr,
 						     GFP_KERNEL);
 		if (!vnic->rss_table) {
@@ -3611,8 +3617,6 @@ static int bnxt_alloc_vnic_attributes(struct bnxt *bp)
 			goto out;
 		}
 
-		size = L1_CACHE_ALIGN(HW_HASH_INDEX_SIZE * sizeof(u16));
-
 		vnic->rss_hash_key = ((void *)vnic->rss_table) + size;
 		vnic->rss_hash_key_dma_addr = vnic->rss_table_dma_addr + size;
 	}
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 78e2fd6..5883b246 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1017,6 +1017,13 @@ struct bnxt_vnic_info {
 	__le16		*rss_table;
 	dma_addr_t	rss_hash_key_dma_addr;
 	u64		*rss_hash_key;
+	int		rss_table_size;
+#define BNXT_RSS_TABLE_ENTRIES_P5	64
+#define BNXT_RSS_TABLE_SIZE_P5		(BNXT_RSS_TABLE_ENTRIES_P5 * 4)
+#define BNXT_RSS_TABLE_MAX_TBL_P5	8
+#define BNXT_MAX_RSS_TABLE_SIZE_P5				\
+	(BNXT_RSS_TABLE_SIZE_P5 * BNXT_RSS_TABLE_MAX_TBL_P5)
+
 	u32		rx_mask;
 
 	u8		*mc_list;
-- 
1.8.3.1

[PATCH net-next 2/8] bnxt_en: Add logical RSS indirection table structure.

[Michael Chan]

The driver currently does not keep track of the logical RSS indirection
table.  The hardware RSS table is set up with standard default ring
distribution when initializing the chip.  This makes it difficult to
support user sepcified indirection table entries.  As a first step, add
the logical table in the main bnxt structure and allocate it according
to chip specific table size.  Add a function that sets up default
RSS distribution based on the number of RX rings.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 52 +++++++++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 +++
 2 files changed, 56 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 4afc1df..924bbcc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4830,6 +4830,49 @@ static u16 bnxt_cp_ring_for_tx(struct bnxt *bp, struct bnxt_tx_ring_info *txr)
 	}
 }
 
+static int bnxt_alloc_rss_indir_tbl(struct bnxt *bp)
+{
+	int entries;
+
+	if (bp->flags & BNXT_FLAG_CHIP_P5)
+		entries = BNXT_MAX_RSS_TABLE_ENTRIES_P5;
+	else
+		entries = HW_HASH_INDEX_SIZE;
+
+	bp->rss_indir_tbl_entries = entries;
+	bp->rss_indir_tbl = kcalloc(entries, sizeof(*bp->rss_indir_tbl),
+				    GFP_KERNEL);
+	if (!bp->rss_indir_tbl)
+		return -ENOMEM;
+	return 0;
+}
+
+static void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp)
+{
+	u16 max_rings, max_entries, pad, i;
+
+	if (!bp->rx_nr_rings)
+		return;
+
+	if (BNXT_CHIP_TYPE_NITRO_A0(bp))
+		max_rings = bp->rx_nr_rings - 1;
+	else
+		max_rings = bp->rx_nr_rings;
+
+	if (bp->flags & BNXT_FLAG_CHIP_P5)
+		max_entries = (max_rings + BNXT_RSS_TABLE_ENTRIES_P5 - 1) &
+			      ~(BNXT_RSS_TABLE_ENTRIES_P5 - 1);
+	else
+		max_entries = HW_HASH_INDEX_SIZE;
+
+	for (i = 0; i < max_entries; i++)
+		bp->rss_indir_tbl[i] = i % max_rings;
+
+	pad = bp->rss_indir_tbl_entries - max_entries;
+	if (pad)
+		memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));
+}
+
 static int bnxt_hwrm_vnic_set_rss(struct bnxt *bp, u16 vnic_id, bool set_rss)
 {
 	u32 i, j, max_rings;
@@ -11514,6 +11557,8 @@ static void bnxt_remove_one(struct pci_dev *pdev)
 	bnxt_free_ctx_mem(bp);
 	kfree(bp->ctx);
 	bp->ctx = NULL;
+	kfree(bp->rss_indir_tbl);
+	bp->rss_indir_tbl = NULL;
 	bnxt_free_port_stats(bp);
 	free_netdev(dev);
 }
@@ -12034,6 +12079,11 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	 */
 	bp->tx_nr_rings_per_tc = bp->tx_nr_rings;
 
+	rc = bnxt_alloc_rss_indir_tbl(bp);
+	if (rc)
+		goto init_err_pci_clean;
+	bnxt_set_dflt_rss_indir_tbl(bp);
+
 	if (BNXT_PF(bp)) {
 		if (!bnxt_pf_wq) {
 			bnxt_pf_wq =
@@ -12078,6 +12128,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	bnxt_free_ctx_mem(bp);
 	kfree(bp->ctx);
 	bp->ctx = NULL;
+	kfree(bp->rss_indir_tbl);
+	bp->rss_indir_tbl = NULL;
 
 init_err_free:
 	free_netdev(dev);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 5883b246..6de2813 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1023,6 +1023,8 @@ struct bnxt_vnic_info {
 #define BNXT_RSS_TABLE_MAX_TBL_P5	8
 #define BNXT_MAX_RSS_TABLE_SIZE_P5				\
 	(BNXT_RSS_TABLE_SIZE_P5 * BNXT_RSS_TABLE_MAX_TBL_P5)
+#define BNXT_MAX_RSS_TABLE_ENTRIES_P5				\
+	(BNXT_RSS_TABLE_ENTRIES_P5 * BNXT_RSS_TABLE_MAX_TBL_P5)
 
 	u32		rx_mask;
 
@@ -1655,6 +1657,8 @@ struct bnxt {
 	struct bnxt_ring_grp_info	*grp_info;
 	struct bnxt_vnic_info	*vnic_info;
 	int			nr_vnics;
+	u16			*rss_indir_tbl;
+	u16			rss_indir_tbl_entries;
 	u32			rss_hash_cfg;
 
 	u16			max_mtu;
-- 
1.8.3.1

[PATCH net-next 3/8] bnxt_en: Add helper function to return the number of RSS contexts.

[Michael Chan]

On some chips, this varies based on the number of RX rings.  Add this
helper function and refactor the existing code to use it.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 13 +++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 924bbcc..7bf843d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4873,6 +4873,15 @@ static void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp)
 		memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));
 }
 
+int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings)
+{
+	if (bp->flags & BNXT_FLAG_CHIP_P5)
+		return DIV_ROUND_UP(rx_rings, BNXT_RSS_TABLE_ENTRIES_P5);
+	if (BNXT_CHIP_TYPE_NITRO_A0(bp))
+		return 2;
+	return 1;
+}
+
 static int bnxt_hwrm_vnic_set_rss(struct bnxt *bp, u16 vnic_id, bool set_rss)
 {
 	u32 i, j, max_rings;
@@ -4928,7 +4937,7 @@ static int bnxt_hwrm_vnic_set_rss_p5(struct bnxt *bp, u16 vnic_id, bool set_rss)
 	req.hash_mode_flags = VNIC_RSS_CFG_REQ_HASH_MODE_FLAGS_DEFAULT;
 	req.ring_grp_tbl_addr = cpu_to_le64(vnic->rss_table_dma_addr);
 	req.hash_key_tbl_addr = cpu_to_le64(vnic->rss_hash_key_dma_addr);
-	nr_ctxs = DIV_ROUND_UP(bp->rx_nr_rings, 64);
+	nr_ctxs = bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings);
 	for (i = 0, k = 0; i < nr_ctxs; i++) {
 		__le16 *ring_tbl = vnic->rss_table;
 		int rc;
@@ -7681,7 +7690,7 @@ static int __bnxt_setup_vnic_p5(struct bnxt *bp, u16 vnic_id)
 {
 	int rc, i, nr_ctxs;
 
-	nr_ctxs = DIV_ROUND_UP(bp->rx_nr_rings, 64);
+	nr_ctxs = bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings);
 	for (i = 0; i < nr_ctxs; i++) {
 		rc = bnxt_hwrm_vnic_ctx_alloc(bp, vnic_id, i);
 		if (rc) {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 6de2813..5890913 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -2039,6 +2039,7 @@ int bnxt_alloc_rx_data(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 int hwrm_send_message_silent(struct bnxt *, void *, u32, int);
 int bnxt_hwrm_func_drv_rgtr(struct bnxt *bp, unsigned long *bmap,
 			    int bmap_size, bool async_only);
+int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings);
 int bnxt_hwrm_vnic_cfg(struct bnxt *bp, u16 vnic_id);
 int __bnxt_hwrm_get_tx_rings(struct bnxt *bp, u16 fid, int *tx_rings);
 int bnxt_nq_rings_in_use(struct bnxt *bp);
-- 
1.8.3.1

[PATCH net-next 4/8] bnxt_en: Fill HW RSS table from the RSS logical indirection table.

[Michael Chan]

Now that we have the logical table, we can fill the HW RSS table
using the logical table's entries and converting them to the HW
specific format.  Re-initialize the logical table to standard
distribution if the number of RX rings changes during ring reservation.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 89 ++++++++++++++++++-------------
 1 file changed, 53 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 7bf843d..87d37dc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -4882,9 +4882,52 @@ int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings)
 	return 1;
 }
 
+static void __bnxt_fill_hw_rss_tbl(struct bnxt *bp, struct bnxt_vnic_info *vnic)
+{
+	bool no_rss = !(vnic->flags & BNXT_VNIC_RSS_FLAG);
+	u16 i, j;
+
+	/* Fill the RSS indirection table with ring group ids */
+	for (i = 0, j = 0; i < HW_HASH_INDEX_SIZE; i++) {
+		if (!no_rss)
+			j = bp->rss_indir_tbl[i];
+		vnic->rss_table[i] = cpu_to_le16(vnic->fw_grp_ids[j]);
+	}
+}
+
+static void __bnxt_fill_hw_rss_tbl_p5(struct bnxt *bp,
+				      struct bnxt_vnic_info *vnic)
+{
+	__le16 *ring_tbl = vnic->rss_table;
+	struct bnxt_rx_ring_info *rxr;
+	u16 tbl_size, i;
+
+	tbl_size = (bp->rx_nr_rings + BNXT_RSS_TABLE_ENTRIES_P5 - 1) &
+		   ~(BNXT_RSS_TABLE_ENTRIES_P5 - 1);
+
+	for (i = 0; i < tbl_size; i++) {
+		u16 ring_id, j;
+
+		j = bp->rss_indir_tbl[i];
+		rxr = &bp->rx_ring[j];
+
+		ring_id = rxr->rx_ring_struct.fw_ring_id;
+		*ring_tbl++ = cpu_to_le16(ring_id);
+		ring_id = bnxt_cp_ring_for_rx(bp, rxr);
+		*ring_tbl++ = cpu_to_le16(ring_id);
+	}
+}
+
+static void bnxt_fill_hw_rss_tbl(struct bnxt *bp, struct bnxt_vnic_info *vnic)
+{
+	if (bp->flags & BNXT_FLAG_CHIP_P5)
+		__bnxt_fill_hw_rss_tbl_p5(bp, vnic);
+	else
+		__bnxt_fill_hw_rss_tbl(bp, vnic);
+}
+
 static int bnxt_hwrm_vnic_set_rss(struct bnxt *bp, u16 vnic_id, bool set_rss)
 {
-	u32 i, j, max_rings;
 	struct bnxt_vnic_info *vnic = &bp->vnic_info[vnic_id];
 	struct hwrm_vnic_rss_cfg_input req = {0};
 
@@ -4894,24 +4937,9 @@ static int bnxt_hwrm_vnic_set_rss(struct bnxt *bp, u16 vnic_id, bool set_rss)
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_VNIC_RSS_CFG, -1, -1);
 	if (set_rss) {
+		bnxt_fill_hw_rss_tbl(bp, vnic);
 		req.hash_type = cpu_to_le32(bp->rss_hash_cfg);
 		req.hash_mode_flags = VNIC_RSS_CFG_REQ_HASH_MODE_FLAGS_DEFAULT;
-		if (vnic->flags & BNXT_VNIC_RSS_FLAG) {
-			if (BNXT_CHIP_TYPE_NITRO_A0(bp))
-				max_rings = bp->rx_nr_rings - 1;
-			else
-				max_rings = bp->rx_nr_rings;
-		} else {
-			max_rings = 1;
-		}
-
-		/* Fill the RSS indirection table with ring group ids */
-		for (i = 0, j = 0; i < HW_HASH_INDEX_SIZE; i++, j++) {
-			if (j == max_rings)
-				j = 0;
-			vnic->rss_table[i] = cpu_to_le16(vnic->fw_grp_ids[j]);
-		}
-
 		req.ring_grp_tbl_addr = cpu_to_le64(vnic->rss_table_dma_addr);
 		req.hash_key_tbl_addr =
 			cpu_to_le64(vnic->rss_hash_key_dma_addr);
@@ -4923,9 +4951,9 @@ static int bnxt_hwrm_vnic_set_rss(struct bnxt *bp, u16 vnic_id, bool set_rss)
 static int bnxt_hwrm_vnic_set_rss_p5(struct bnxt *bp, u16 vnic_id, bool set_rss)
 {
 	struct bnxt_vnic_info *vnic = &bp->vnic_info[vnic_id];
-	u32 i, j, k, nr_ctxs, max_rings = bp->rx_nr_rings;
-	struct bnxt_rx_ring_info *rxr = &bp->rx_ring[0];
 	struct hwrm_vnic_rss_cfg_input req = {0};
+	dma_addr_t ring_tbl_map;
+	u32 i, nr_ctxs;
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_VNIC_RSS_CFG, -1, -1);
 	req.vnic_id = cpu_to_le16(vnic->fw_vnic_id);
@@ -4933,31 +4961,18 @@ static int bnxt_hwrm_vnic_set_rss_p5(struct bnxt *bp, u16 vnic_id, bool set_rss)
 		hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
 		return 0;
 	}
+	bnxt_fill_hw_rss_tbl(bp, vnic);
 	req.hash_type = cpu_to_le32(bp->rss_hash_cfg);
 	req.hash_mode_flags = VNIC_RSS_CFG_REQ_HASH_MODE_FLAGS_DEFAULT;
-	req.ring_grp_tbl_addr = cpu_to_le64(vnic->rss_table_dma_addr);
 	req.hash_key_tbl_addr = cpu_to_le64(vnic->rss_hash_key_dma_addr);
+	ring_tbl_map = vnic->rss_table_dma_addr;
 	nr_ctxs = bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings);
-	for (i = 0, k = 0; i < nr_ctxs; i++) {
-		__le16 *ring_tbl = vnic->rss_table;
+	for (i = 0; i < nr_ctxs; ring_tbl_map += BNXT_RSS_TABLE_SIZE_P5, i++) {
 		int rc;
 
+		req.ring_grp_tbl_addr = cpu_to_le64(ring_tbl_map);
 		req.ring_table_pair_index = i;
 		req.rss_ctx_idx = cpu_to_le16(vnic->fw_rss_cos_lb_ctx[i]);
-		for (j = 0; j < 64; j++) {
-			u16 ring_id;
-
-			ring_id = rxr->rx_ring_struct.fw_ring_id;
-			*ring_tbl++ = cpu_to_le16(ring_id);
-			ring_id = bnxt_cp_ring_for_rx(bp, rxr);
-			*ring_tbl++ = cpu_to_le16(ring_id);
-			rxr++;
-			k++;
-			if (k == max_rings) {
-				k = 0;
-				rxr = &bp->rx_ring[0];
-			}
-		}
 		rc = hwrm_send_message(bp, &req, sizeof(req), HWRM_CMD_TIMEOUT);
 		if (rc)
 			return rc;
@@ -8252,6 +8267,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 			rc = bnxt_init_int_mode(bp);
 		bnxt_ulp_irq_restart(bp, rc);
 	}
+	bnxt_set_dflt_rss_indir_tbl(bp);
+
 	if (rc) {
 		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
 		return rc;
-- 
1.8.3.1

[PATCH net-next 5/8] bnxt_en: Return correct RSS indirection table entries to ethtool -x.

[Michael Chan]

Now that we have the logical indirection table, we can return these
proper logical indices directly to ethtool -x instead of the physical
IDs.

Reported-by: Jakub Kicinski <kicinski@fb.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 6b88143..46f3978 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -1275,6 +1275,12 @@ static int bnxt_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 
 static u32 bnxt_get_rxfh_indir_size(struct net_device *dev)
 {
+	struct bnxt *bp = netdev_priv(dev);
+
+	if (bp->flags & BNXT_FLAG_CHIP_P5) {
+		return (bp->rx_nr_rings + BNXT_RSS_TABLE_ENTRIES_P5 - 1) &
+		       ~(BNXT_RSS_TABLE_ENTRIES_P5 - 1);
+	}
 	return HW_HASH_INDEX_SIZE;
 }
 
@@ -1288,7 +1294,7 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 {
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_vnic_info *vnic;
-	int i = 0;
+	u32 i, tbl_size;
 
 	if (hfunc)
 		*hfunc = ETH_RSS_HASH_TOP;
@@ -1297,9 +1303,10 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 		return 0;
 
 	vnic = &bp->vnic_info[0];
-	if (indir && vnic->rss_table) {
-		for (i = 0; i < HW_HASH_INDEX_SIZE; i++)
-			indir[i] = le16_to_cpu(vnic->rss_table[i]);
+	if (indir && bp->rss_indir_tbl) {
+		tbl_size = bnxt_get_rxfh_indir_size(dev);
+		for (i = 0; i < tbl_size; i++)
+			indir[i] = bp->rss_indir_tbl[i];
 	}
 
 	if (key && vnic->rss_hash_key)
-- 
1.8.3.1

[PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Michael Chan]

With the new infrastructure in place, we can now support the setting of
the indirection table from ethtool.

The user-configured indirection table will need to be reset to default
if we are unable to reserve the requested number of RX rings or if the
RSS table size changes.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  7 ++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 34 +++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 87d37dc..eb7f2d4 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -6063,6 +6063,10 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
 		rx = rx_rings << 1;
 	cp = sh ? max_t(int, tx, rx_rings) : tx + rx_rings;
 	bp->tx_nr_rings = tx;
+
+	/* Reset the RSS indirection if we cannot reserve all the RX rings */
+	if (rx_rings != bp->rx_nr_rings)
+		bp->dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
 	bp->rx_nr_rings = rx_rings;
 	bp->cp_nr_rings = cp;
 
@@ -8267,7 +8271,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
 			rc = bnxt_init_int_mode(bp);
 		bnxt_ulp_irq_restart(bp, rc);
 	}
-	bnxt_set_dflt_rss_indir_tbl(bp);
+	if (!netif_is_rxfh_configured(bp->dev))
+		bnxt_set_dflt_rss_indir_tbl(bp);
 
 	if (rc) {
 		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 46f3978..ae10ebd 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -926,6 +926,10 @@ static int bnxt_set_channels(struct net_device *dev,
 		return rc;
 	}
 
+	if (bnxt_get_nr_rss_ctxs(bp, req_rx_rings) !=
+	    bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings))
+		bp->dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
+
 	if (netif_running(dev)) {
 		if (BNXT_PF(bp)) {
 			/* TODO CHIMP_FW: Send message to all VF's
@@ -1315,6 +1319,35 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
 	return 0;
 }
 
+static int bnxt_set_rxfh(struct net_device *dev, const u32 *indir,
+			 const u8 *key, const u8 hfunc)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	int rc = 0;
+
+	if (hfunc && hfunc != ETH_RSS_HASH_TOP)
+		return -EOPNOTSUPP;
+
+	if (key)
+		return -EOPNOTSUPP;
+
+	if (indir) {
+		u32 i, pad, tbl_size = bnxt_get_rxfh_indir_size(dev);
+
+		for (i = 0; i < tbl_size; i++)
+			bp->rss_indir_tbl[i] = indir[i];
+		pad = bp->rss_indir_tbl_entries - tbl_size;
+		if (pad)
+			memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));
+	}
+
+	if (netif_running(bp->dev)) {
+		bnxt_close_nic(bp, false, false);
+		rc = bnxt_open_nic(bp, false, false);
+	}
+	return rc;
+}
+
 static void bnxt_get_drvinfo(struct net_device *dev,
 			     struct ethtool_drvinfo *info)
 {
@@ -3621,6 +3654,7 @@ void bnxt_ethtool_free(struct bnxt *bp)
 	.get_rxfh_indir_size    = bnxt_get_rxfh_indir_size,
 	.get_rxfh_key_size      = bnxt_get_rxfh_key_size,
 	.get_rxfh               = bnxt_get_rxfh,
+	.set_rxfh		= bnxt_set_rxfh,
 	.flash_device		= bnxt_flash_device,
 	.get_eeprom_len         = bnxt_get_eeprom_len,
 	.get_eeprom             = bnxt_get_eeprom,
-- 
1.8.3.1

[PATCH net-next 7/8] bnxt_en: clean up VLAN feature bit handling

[Michael Chan]

From: Edwin Peer <edwin.peer@broadcom.com>

The hardware VLAN offload feature on our NIC does not have separate
knobs for handling customer and service tags on RX. Either offloading
of both must be enabled or both must be disabled. Introduce definitions
for the combined feature set in order to clean up the code and make
this constraint more clear. Technically these features can be separately
enabled on TX, however, since the default is to turn both on, the
combined TX feature set is also introduced for code consistency.

Signed-off-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 34 ++++++++++++-------------------
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  5 +++++
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index eb7f2d4..4f8fc28 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1614,7 +1614,7 @@ static inline struct sk_buff *bnxt_tpa_end(struct bnxt *bp,
 		skb_set_hash(skb, tpa_info->rss_hash, tpa_info->hash_type);
 
 	if ((tpa_info->flags2 & RX_CMP_FLAGS2_META_FORMAT_VLAN) &&
-	    (skb->dev->features & NETIF_F_HW_VLAN_CTAG_RX)) {
+	    (skb->dev->features & BNXT_HW_FEATURE_VLAN_ALL_RX)) {
 		u16 vlan_proto = tpa_info->metadata >>
 			RX_CMP_FLAGS2_METADATA_TPID_SFT;
 		u16 vtag = tpa_info->metadata & RX_CMP_FLAGS2_METADATA_TCI_MASK;
@@ -1832,7 +1832,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 
 	if ((rxcmp1->rx_cmp_flags2 &
 	     cpu_to_le32(RX_CMP_FLAGS2_META_FORMAT_VLAN)) &&
-	    (skb->dev->features & NETIF_F_HW_VLAN_CTAG_RX)) {
+	    (skb->dev->features & BNXT_HW_FEATURE_VLAN_ALL_RX)) {
 		u32 meta_data = le32_to_cpu(rxcmp1->rx_cmp_meta_data);
 		u16 vtag = meta_data & RX_CMP_FLAGS2_METADATA_TCI_MASK;
 		u16 vlan_proto = meta_data >> RX_CMP_FLAGS2_METADATA_TPID_SFT;
@@ -9913,24 +9913,16 @@ static netdev_features_t bnxt_fix_features(struct net_device *dev,
 	/* Both CTAG and STAG VLAN accelaration on the RX side have to be
 	 * turned on or off together.
 	 */
-	vlan_features = features & (NETIF_F_HW_VLAN_CTAG_RX |
-				    NETIF_F_HW_VLAN_STAG_RX);
-	if (vlan_features != (NETIF_F_HW_VLAN_CTAG_RX |
-			      NETIF_F_HW_VLAN_STAG_RX)) {
-		if (dev->features & NETIF_F_HW_VLAN_CTAG_RX)
-			features &= ~(NETIF_F_HW_VLAN_CTAG_RX |
-				      NETIF_F_HW_VLAN_STAG_RX);
+	vlan_features = features & BNXT_HW_FEATURE_VLAN_ALL_RX;
+	if (vlan_features != BNXT_HW_FEATURE_VLAN_ALL_RX) {
+		if (dev->features & BNXT_HW_FEATURE_VLAN_ALL_RX)
+			features &= ~BNXT_HW_FEATURE_VLAN_ALL_RX;
 		else if (vlan_features)
-			features |= NETIF_F_HW_VLAN_CTAG_RX |
-				    NETIF_F_HW_VLAN_STAG_RX;
+			features |= BNXT_HW_FEATURE_VLAN_ALL_RX;
 	}
 #ifdef CONFIG_BNXT_SRIOV
-	if (BNXT_VF(bp)) {
-		if (bp->vf.vlan) {
-			features &= ~(NETIF_F_HW_VLAN_CTAG_RX |
-				      NETIF_F_HW_VLAN_STAG_RX);
-		}
-	}
+	if (BNXT_VF(bp) && bp->vf.vlan)
+		features &= ~BNXT_HW_FEATURE_VLAN_ALL_RX;
 #endif
 	return features;
 }
@@ -9953,7 +9945,7 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 	if (bp->flags & BNXT_FLAG_NO_AGG_RINGS)
 		flags &= ~BNXT_FLAG_TPA;
 
-	if (features & NETIF_F_HW_VLAN_CTAG_RX)
+	if (features & BNXT_HW_FEATURE_VLAN_ALL_RX)
 		flags |= BNXT_FLAG_STRIP_VLAN;
 
 	if (features & NETIF_F_NTUPLE)
@@ -12041,8 +12033,8 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM |
 				    NETIF_F_GSO_GRE_CSUM;
 	dev->vlan_features = dev->hw_features | NETIF_F_HIGHDMA;
-	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX |
-			    NETIF_F_HW_VLAN_STAG_RX | NETIF_F_HW_VLAN_STAG_TX;
+	dev->hw_features |= BNXT_HW_FEATURE_VLAN_ALL_RX |
+			    BNXT_HW_FEATURE_VLAN_ALL_TX;
 	if (BNXT_SUPPORTS_TPA(bp))
 		dev->hw_features |= NETIF_F_GRO_HW;
 	dev->features |= dev->hw_features | NETIF_F_HIGHDMA;
@@ -12098,7 +12090,7 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	bnxt_fw_init_one_p3(bp);
 
-	if (dev->hw_features & NETIF_F_HW_VLAN_CTAG_RX)
+	if (dev->hw_features & BNXT_HW_FEATURE_VLAN_ALL_RX)
 		bp->flags |= BNXT_FLAG_STRIP_VLAN;
 
 	rc = bnxt_init_int_mode(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 5890913..13c4064 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1906,6 +1906,11 @@ struct bnxt {
 #define BNXT_PCIE_STATS_OFFSET(counter)			\
 	(offsetof(struct pcie_ctx_hw_stats, counter) / 8)
 
+#define BNXT_HW_FEATURE_VLAN_ALL_RX				\
+	(NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX)
+#define BNXT_HW_FEATURE_VLAN_ALL_TX				\
+	(NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX)
+
 #define I2C_DEV_ADDR_A0				0xa0
 #define I2C_DEV_ADDR_A2				0xa2
 #define SFF_DIAG_SUPPORT_OFFSET			0x5c
-- 
1.8.3.1

[PATCH net-next 8/8] bnxt_en: allow firmware to disable VLAN offloads

[Michael Chan]

From: Edwin Peer <edwin.peer@broadcom.com>

Bare-metal use cases require giving firmware and the embedded
application processor control over VLAN offloads. The driver should
not attempt to override or utilize this feature in such scenarios
since it will not work as expected.

Signed-off-by: Edwin Peer <edwin.peer@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 22 +++++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  3 +++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 4f8fc28..451e0ef 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5210,6 +5210,14 @@ static int bnxt_hwrm_vnic_qcaps(struct bnxt *bp)
 		if (flags &
 		    VNIC_QCAPS_RESP_FLAGS_ROCE_MIRRORING_CAPABLE_VNIC_CAP)
 			bp->flags |= BNXT_FLAG_ROCE_MIRROR_CAP;
+
+		/* Older P5 fw before EXT_HW_STATS support did not set
+		 * VLAN_STRIP_CAP properly.
+		 */
+		if ((flags & VNIC_QCAPS_RESP_FLAGS_VLAN_STRIP_CAP) ||
+		    ((bp->flags & BNXT_FLAG_CHIP_P5) &&
+		     !(bp->fw_cap & BNXT_FW_CAP_EXT_HW_STATS_SUPPORTED)))
+			bp->fw_cap |= BNXT_FW_CAP_VLAN_RX_STRIP;
 		bp->max_tpa_v2 = le16_to_cpu(resp->max_aggs_supported);
 		if (bp->max_tpa_v2)
 			bp->hw_ring_stats_size =
@@ -7030,7 +7038,7 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 	struct hwrm_func_qcaps_input req = {0};
 	struct hwrm_func_qcaps_output *resp = bp->hwrm_cmd_resp_addr;
 	struct bnxt_hw_resc *hw_resc = &bp->hw_resc;
-	u32 flags;
+	u32 flags, flags_ext;
 
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_QCAPS, -1, -1);
 	req.fid = cpu_to_le16(0xffff);
@@ -7055,6 +7063,12 @@ static int __bnxt_hwrm_func_qcaps(struct bnxt *bp)
 		bp->fw_cap |= BNXT_FW_CAP_ERROR_RECOVERY;
 	if (flags & FUNC_QCAPS_RESP_FLAGS_ERR_RECOVER_RELOAD)
 		bp->fw_cap |= BNXT_FW_CAP_ERR_RECOVER_RELOAD;
+	if (!(flags & FUNC_QCAPS_RESP_FLAGS_VLAN_ACCELERATION_TX_DISABLED))
+		bp->fw_cap |= BNXT_FW_CAP_VLAN_TX_INSERT;
+
+	flags_ext = le32_to_cpu(resp->flags_ext);
+	if (flags_ext & FUNC_QCAPS_RESP_FLAGS_EXT_EXT_HW_STATS_SUPPORTED)
+		bp->fw_cap |= BNXT_FW_CAP_EXT_HW_STATS_SUPPORTED;
 
 	bp->tx_push_thresh = 0;
 	if ((flags & FUNC_QCAPS_RESP_FLAGS_PUSH_MODE_SUPPORTED) &&
@@ -12033,8 +12047,10 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->gso_partial_features = NETIF_F_GSO_UDP_TUNNEL_CSUM |
 				    NETIF_F_GSO_GRE_CSUM;
 	dev->vlan_features = dev->hw_features | NETIF_F_HIGHDMA;
-	dev->hw_features |= BNXT_HW_FEATURE_VLAN_ALL_RX |
-			    BNXT_HW_FEATURE_VLAN_ALL_TX;
+	if (bp->fw_cap & BNXT_FW_CAP_VLAN_RX_STRIP)
+		dev->hw_features |= BNXT_HW_FEATURE_VLAN_ALL_RX;
+	if (bp->fw_cap & BNXT_FW_CAP_VLAN_TX_INSERT)
+		dev->hw_features |= BNXT_HW_FEATURE_VLAN_ALL_TX;
 	if (BNXT_SUPPORTS_TPA(bp))
 		dev->hw_features |= NETIF_F_GRO_HW;
 	dev->features |= dev->hw_features | NETIF_F_HIGHDMA;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 13c4064..d556e56 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -1716,6 +1716,9 @@ struct bnxt {
 	#define BNXT_FW_CAP_ERR_RECOVER_RELOAD		0x00100000
 	#define BNXT_FW_CAP_HOT_RESET			0x00200000
 	#define BNXT_FW_CAP_SHARED_PORT_CFG		0x00400000
+	#define BNXT_FW_CAP_VLAN_RX_STRIP		0x01000000
+	#define BNXT_FW_CAP_VLAN_TX_INSERT		0x02000000
+	#define BNXT_FW_CAP_EXT_HW_STATS_SUPPORTED	0x04000000
 
 #define BNXT_NEW_RM(bp)		((bp)->fw_cap & BNXT_FW_CAP_NEW_RM)
 	u32			hwrm_spec_code;
-- 
1.8.3.1

Re: [PATCH net-next 2/8] bnxt_en: Add logical RSS indirection table structure.

[Jakub Kicinski]

On Mon, 29 Jun 2020 02:34:18 -0400 Michael Chan wrote:
> +	bp->rss_indir_tbl_entries = entries;
> +	bp->rss_indir_tbl = kcalloc(entries, sizeof(*bp->rss_indir_tbl),
> +				    GFP_KERNEL);

nit: kmalloc_array() ? I think you init all elements below.

> +	if (!bp->rss_indir_tbl)
> +		return -ENOMEM;
> +	return 0;
> +}
> +
> +static void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp)
> +{
> +	u16 max_rings, max_entries, pad, i;
> +
> +	if (!bp->rx_nr_rings)
> +		return;
> +
> +	if (BNXT_CHIP_TYPE_NITRO_A0(bp))
> +		max_rings = bp->rx_nr_rings - 1;
> +	else
> +		max_rings = bp->rx_nr_rings;
> +
> +	if (bp->flags & BNXT_FLAG_CHIP_P5)
> +		max_entries = (max_rings + BNXT_RSS_TABLE_ENTRIES_P5 - 1) &
> +			      ~(BNXT_RSS_TABLE_ENTRIES_P5 - 1);
> +	else
> +		max_entries = HW_HASH_INDEX_SIZE;
> +
> +	for (i = 0; i < max_entries; i++)
> +		bp->rss_indir_tbl[i] = i % max_rings;

nit: ethtool_rxfh_indir_default()

> +	pad = bp->rss_indir_tbl_entries - max_entries;
> +	if (pad)
> +		memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));

Re: [PATCH net-next 4/8] bnxt_en: Fill HW RSS table from the RSS logical indirection table.

[Jakub Kicinski]

On Mon, 29 Jun 2020 02:34:20 -0400 Michael Chan wrote:
> Now that we have the logical table, we can fill the HW RSS table
> using the logical table's entries and converting them to the HW
> specific format.  Re-initialize the logical table to standard
> distribution if the number of RX rings changes during ring reservation.
> 
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 89 ++++++++++++++++++-------------
>  1 file changed, 53 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 7bf843d..87d37dc 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -4882,9 +4882,52 @@ int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings)
>  	return 1;
>  }
>  
> +static void __bnxt_fill_hw_rss_tbl(struct bnxt *bp, struct bnxt_vnic_info *vnic)
> +{
> +	bool no_rss = !(vnic->flags & BNXT_VNIC_RSS_FLAG);
> +	u16 i, j;
> +
> +	/* Fill the RSS indirection table with ring group ids */
> +	for (i = 0, j = 0; i < HW_HASH_INDEX_SIZE; i++) {
> +		if (!no_rss)
> +			j = bp->rss_indir_tbl[i];
> +		vnic->rss_table[i] = cpu_to_le16(vnic->fw_grp_ids[j]);
> +	}
> +}
> +
> +static void __bnxt_fill_hw_rss_tbl_p5(struct bnxt *bp,
> +				      struct bnxt_vnic_info *vnic)
> +{
> +	__le16 *ring_tbl = vnic->rss_table;
> +	struct bnxt_rx_ring_info *rxr;
> +	u16 tbl_size, i;
> +
> +	tbl_size = (bp->rx_nr_rings + BNXT_RSS_TABLE_ENTRIES_P5 - 1) &
> +		   ~(BNXT_RSS_TABLE_ENTRIES_P5 - 1);

nit: DIV_ROUND_UP() ?

> +	for (i = 0; i < tbl_size; i++) {
> +		u16 ring_id, j;
> +
> +		j = bp->rss_indir_tbl[i];
> +		rxr = &bp->rx_ring[j];
> +
> +		ring_id = rxr->rx_ring_struct.fw_ring_id;
> +		*ring_tbl++ = cpu_to_le16(ring_id);
> +		ring_id = bnxt_cp_ring_for_rx(bp, rxr);
> +		*ring_tbl++ = cpu_to_le16(ring_id);
> +	}
> +}
> +

> @@ -8252,6 +8267,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
>  	}

if (!netif_is_rxfh_configured(nn->dp.netdev))

> +	bnxt_set_dflt_rss_indir_tbl(bp);
> +
>  	if (rc) {
>  		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
>  		return rc;

Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Jakub Kicinski]

On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> With the new infrastructure in place, we can now support the setting of
> the indirection table from ethtool.
> 
> The user-configured indirection table will need to be reset to default
> if we are unable to reserve the requested number of RX rings or if the
> RSS table size changes.
> 
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
requested a RSS mapping, if it can't be maintained driver should 
return an error from the operation which attempts to change the ring
count.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 87d37dc..eb7f2d4 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -6063,6 +6063,10 @@ static int __bnxt_reserve_rings(struct bnxt *bp)
>  		rx = rx_rings << 1;
>  	cp = sh ? max_t(int, tx, rx_rings) : tx + rx_rings;
>  	bp->tx_nr_rings = tx;
> +
> +	/* Reset the RSS indirection if we cannot reserve all the RX rings */
> +	if (rx_rings != bp->rx_nr_rings)
> +		bp->dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
>  	bp->rx_nr_rings = rx_rings;
>  	bp->cp_nr_rings = cp;
>  
> @@ -8267,7 +8271,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
>  			rc = bnxt_init_int_mode(bp);
>  		bnxt_ulp_irq_restart(bp, rc);
>  	}
> -	bnxt_set_dflt_rss_indir_tbl(bp);
> +	if (!netif_is_rxfh_configured(bp->dev))
> +		bnxt_set_dflt_rss_indir_tbl(bp);
>  
>  	if (rc) {
>  		netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index 46f3978..ae10ebd 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -926,6 +926,10 @@ static int bnxt_set_channels(struct net_device *dev,
>  		return rc;
>  	}
>  
> +	if (bnxt_get_nr_rss_ctxs(bp, req_rx_rings) !=
> +	    bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings))
> +		bp->dev->priv_flags &= ~IFF_RXFH_CONFIGURED;
> +
>  	if (netif_running(dev)) {
>  		if (BNXT_PF(bp)) {
>  			/* TODO CHIMP_FW: Send message to all VF's
> @@ -1315,6 +1319,35 @@ static int bnxt_get_rxfh(struct net_device *dev, u32 *indir, u8 *key,
>  	return 0;
>  }
>  
> +static int bnxt_set_rxfh(struct net_device *dev, const u32 *indir,
> +			 const u8 *key, const u8 hfunc)
> +{
> +	struct bnxt *bp = netdev_priv(dev);
> +	int rc = 0;
> +
> +	if (hfunc && hfunc != ETH_RSS_HASH_TOP)
> +		return -EOPNOTSUPP;
> +
> +	if (key)
> +		return -EOPNOTSUPP;
> +
> +	if (indir) {
> +		u32 i, pad, tbl_size = bnxt_get_rxfh_indir_size(dev);
> +
> +		for (i = 0; i < tbl_size; i++)
> +			bp->rss_indir_tbl[i] = indir[i];
> +		pad = bp->rss_indir_tbl_entries - tbl_size;
> +		if (pad)
> +			memset(&bp->rss_indir_tbl[i], 0, pad * sizeof(u16));
> +	}
> +
> +	if (netif_running(bp->dev)) {
> +		bnxt_close_nic(bp, false, false);
> +		rc = bnxt_open_nic(bp, false, false);

Ah, FWIW this makes the feature far less useful for use cases which try
to work around the inability to create/take down queues without
impacting all traffic, but it's probably still useful for asymmetric
traffic distribution.

> +	}
> +	return rc;
> +}
> +
>  static void bnxt_get_drvinfo(struct net_device *dev,
>  			     struct ethtool_drvinfo *info)
>  {
> @@ -3621,6 +3654,7 @@ void bnxt_ethtool_free(struct bnxt *bp)
>  	.get_rxfh_indir_size    = bnxt_get_rxfh_indir_size,
>  	.get_rxfh_key_size      = bnxt_get_rxfh_key_size,
>  	.get_rxfh               = bnxt_get_rxfh,
> +	.set_rxfh		= bnxt_set_rxfh,
>  	.flash_device		= bnxt_flash_device,
>  	.get_eeprom_len         = bnxt_get_eeprom_len,
>  	.get_eeprom             = bnxt_get_eeprom,

Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Michael Chan]

On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > With the new infrastructure in place, we can now support the setting of
> > the indirection table from ethtool.
> >
> > The user-configured indirection table will need to be reset to default
> > if we are unable to reserve the requested number of RX rings or if the
> > RSS table size changes.
> >
> > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
>
> Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> requested a RSS mapping, if it can't be maintained driver should
> return an error from the operation which attempts to change the ring
> count.

Right.  In this case the user has requested non default RSS map and is
now attempting to change rings.  We have a first level check by
calling bnxt_check_rings().  Firmware will tell us if the requested
rings are available or not.  If not, we will return error and the
existing rings and RSS map will be kept.  This should be the expected
outcome in most cases.

In rare cases, firmware can return success during bnxt_check_rings()
but during the actual ring reservation, it fails to reserve all the
rings it promised were available earlier.  In this case, we fall back
and accept the fewer rings and set the RSS map to default.  I have
never seen this scenario but we need to put the code in just in case
it happens.  It should be rare.

Re: [PATCH net-next 4/8] bnxt_en: Fill HW RSS table from the RSS logical indirection table.

[Michael Chan]

On Mon, Jun 29, 2020 at 4:58 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 29 Jun 2020 02:34:20 -0400 Michael Chan wrote:
> > @@ -8252,6 +8267,8 @@ int bnxt_reserve_rings(struct bnxt *bp, bool irq_re_init)
> >                       rc = bnxt_init_int_mode(bp);
> >               bnxt_ulp_irq_restart(bp, rc);
> >       }
>
> if (!netif_is_rxfh_configured(nn->dp.netdev))

Patch #6 adds this line together with the ability to set the RSS map.
At this point, we are only displaying the RSS map and we always set it
to the default map if we need to reserve rings.

>
> > +     bnxt_set_dflt_rss_indir_tbl(bp);
> > +
> >       if (rc) {
> >               netdev_err(bp->dev, "ring reservation/IRQ init failure rc: %d\n", rc);
> >               return rc;
>

Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Jakub Kicinski]

On Mon, 29 Jun 2020 17:38:33 -0700 Michael Chan wrote:
> On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > > With the new infrastructure in place, we can now support the setting of
> > > the indirection table from ethtool.
> > >
> > > The user-configured indirection table will need to be reset to default
> > > if we are unable to reserve the requested number of RX rings or if the
> > > RSS table size changes.
> > >
> > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>  
> >
> > Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> > requested a RSS mapping, if it can't be maintained driver should
> > return an error from the operation which attempts to change the ring
> > count.  
> 
> Right.  In this case the user has requested non default RSS map and is
> now attempting to change rings.  We have a first level check by
> calling bnxt_check_rings().  Firmware will tell us if the requested
> rings are available or not.  If not, we will return error and the
> existing rings and RSS map will be kept.  This should be the expected
> outcome in most cases.
> 
> In rare cases, firmware can return success during bnxt_check_rings()
> but during the actual ring reservation, it fails to reserve all the
> rings it promised were available earlier.  In this case, we fall back
> and accept the fewer rings and set the RSS map to default.  I have
> never seen this scenario but we need to put the code in just in case
> it happens.  It should be rare.

What's the expected application flow? Every time the application 
makes a change to NIC settings it has to re-validate that some of 
the previous configuration didn't get lost? I don't see the driver
returning the error if FW gave it less rings than requested. There 
isn't even a warning printed..

I'd prefer if the driver wrapped the rss indexes, and printed a
warning, but left the config intact. And IMO set_channels should 
return an error.

Re: [PATCH net-next 6/8] bnxt_en: Implement ethtool -X to set indirection table.

[Michael Chan]

On Tue, Jun 30, 2020 at 12:05 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 29 Jun 2020 17:38:33 -0700 Michael Chan wrote:
> > On Mon, Jun 29, 2020 at 5:06 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > On Mon, 29 Jun 2020 02:34:22 -0400 Michael Chan wrote:
> > > > With the new infrastructure in place, we can now support the setting of
> > > > the indirection table from ethtool.
> > > >
> > > > The user-configured indirection table will need to be reset to default
> > > > if we are unable to reserve the requested number of RX rings or if the
> > > > RSS table size changes.
> > > >
> > > > Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> > >
> > > Hm. Clearing IFF_RXFH_CONFIGURED seems wrong. The user has clearly
> > > requested a RSS mapping, if it can't be maintained driver should
> > > return an error from the operation which attempts to change the ring
> > > count.
> >
> > Right.  In this case the user has requested non default RSS map and is
> > now attempting to change rings.  We have a first level check by
> > calling bnxt_check_rings().  Firmware will tell us if the requested
> > rings are available or not.  If not, we will return error and the
> > existing rings and RSS map will be kept.  This should be the expected
> > outcome in most cases.
> >
> > In rare cases, firmware can return success during bnxt_check_rings()
> > but during the actual ring reservation, it fails to reserve all the
> > rings it promised were available earlier.  In this case, we fall back
> > and accept the fewer rings and set the RSS map to default.  I have
> > never seen this scenario but we need to put the code in just in case
> > it happens.  It should be rare.
>
> What's the expected application flow? Every time the application
> makes a change to NIC settings it has to re-validate that some of
> the previous configuration didn't get lost? I don't see the driver
> returning the error if FW gave it less rings than requested. There
> isn't even a warning printed..

In bnxt_set_channels(), if bnxt_check_rings() returns error, we will
print a warning and return error.  This applies whether we have a user
defined RSS map or not.

If the RSS table size changes (only newer chips will change the RSS
table size when the rings cross some thresholds), the current code
always goes back to default RSS map.  But I think you prefer to keep
the user-defined RSS map and the rings and return error from
bnxt_set_channels().  This I can easily change.

I think I misunderstood your original question.  There is another code
path that will change the RSS map to default if we cannot reserve the
number of rings from firmware that were promised earlier from
bnxt_check_rings().  I was referring to this code path earlier.

>
> I'd prefer if the driver wrapped the rss indexes, and printed a
> warning, but left the config intact. And IMO set_channels should
> return an error.

Right.  I think it is clear now.  I will make this change in v2 plus
the other feedback you provided.  Thanks.