[PATCH net 0/5] bnxt_en: Bug fixes.

[PATCH net 0/5] bnxt_en: Bug fixes.

[Michael Chan]

Miscellaneous small bug fixes.

Michael Chan (5):
  bnxt_en: Change sp events definitions to represent bit position.
  bnxt_en: Determine tcp/ipv6 RSS hash type correctly.
  bnxt_en: map CAG_REG_LEGACY_INT_STATUS_MASK to GRC window #4
  bnxt_en: Fix comparison of u16 sw_id against negative value.
  bnxt_en: More robust SRIOV cleanup sequence.

 drivers/net/ethernet/broadcom/bnxt/bnxt.c       | 28 ++++++++++++-----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h       | 26 +++++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 40 +++++++++++++++++--------
 3 files changed, 64 insertions(+), 30 deletions(-)

-- 
1.8.3.1

[PATCH net 1/5] bnxt_en: Change sp events definitions to represent bit position.

[Michael Chan]

From: Jeffrey Huang <huangjw@broadcom.com>

Fix the sp event bits to be bit positions instead of bit values since
the bit helper functions are expecting the former.

Signed-off-by: Jeffrey Huang <huangjw@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 4f2267c..4cae492 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -959,11 +959,11 @@ struct bnxt {
 #define BNXT_RX_MASK_SP_EVENT		0
 #define BNXT_RX_NTP_FLTR_SP_EVENT	1
 #define BNXT_LINK_CHNG_SP_EVENT		2
-#define BNXT_HWRM_EXEC_FWD_REQ_SP_EVENT	4
-#define BNXT_VXLAN_ADD_PORT_SP_EVENT	8
-#define BNXT_VXLAN_DEL_PORT_SP_EVENT	16
-#define BNXT_RESET_TASK_SP_EVENT	32
-#define BNXT_RST_RING_SP_EVENT		64
+#define BNXT_HWRM_EXEC_FWD_REQ_SP_EVENT	3
+#define BNXT_VXLAN_ADD_PORT_SP_EVENT	4
+#define BNXT_VXLAN_DEL_PORT_SP_EVENT	5
+#define BNXT_RESET_TASK_SP_EVENT	6
+#define BNXT_RST_RING_SP_EVENT		7
 
 	struct bnxt_pf_info	pf;
 #ifdef CONFIG_BNXT_SRIOV
-- 
1.8.3.1

[PATCH net 2/5] bnxt_en: Determine tcp/ipv6 RSS hash type correctly.

[Michael Chan]

The profile ID in the completion record needs to be ANDed with the
profile ID mask of 0x1f.  This bug was causing the SKB hash type
and the gso_type to be wrong in some cases.

Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 4cae492..5afe65d 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -166,9 +166,11 @@ struct rx_cmp {
 #define RX_CMP_HASH_VALID(rxcmp)				\
 	((rxcmp)->rx_cmp_len_flags_type & cpu_to_le32(RX_CMP_FLAGS_RSS_VALID))
 
+#define RSS_PROFILE_ID_MASK	0x1f
+
 #define RX_CMP_HASH_TYPE(rxcmp)					\
-	((le32_to_cpu((rxcmp)->rx_cmp_misc_v1) & RX_CMP_RSS_HASH_TYPE) >>\
-	 RX_CMP_RSS_HASH_TYPE_SHIFT)
+	(((le32_to_cpu((rxcmp)->rx_cmp_misc_v1) & RX_CMP_RSS_HASH_TYPE) >>\
+	  RX_CMP_RSS_HASH_TYPE_SHIFT) & RSS_PROFILE_ID_MASK)
 
 struct rx_cmp_ext {
 	__le32 rx_cmp_flags2;
@@ -282,9 +284,9 @@ struct rx_tpa_start_cmp {
 	 cpu_to_le32(RX_TPA_START_CMP_FLAGS_RSS_VALID))
 
 #define TPA_START_HASH_TYPE(rx_tpa_start)				\
-	((le32_to_cpu((rx_tpa_start)->rx_tpa_start_cmp_misc_v1) &	\
-	  RX_TPA_START_CMP_RSS_HASH_TYPE) >>				\
-	 RX_TPA_START_CMP_RSS_HASH_TYPE_SHIFT)
+	(((le32_to_cpu((rx_tpa_start)->rx_tpa_start_cmp_misc_v1) &	\
+	   RX_TPA_START_CMP_RSS_HASH_TYPE) >>				\
+	  RX_TPA_START_CMP_RSS_HASH_TYPE_SHIFT) & RSS_PROFILE_ID_MASK)
 
 #define TPA_START_AGG_ID(rx_tpa_start)					\
 	((le32_to_cpu((rx_tpa_start)->rx_tpa_start_cmp_misc_v1) &	\
-- 
1.8.3.1

[PATCH net 3/5] bnxt_en: map CAG_REG_LEGACY_INT_STATUS_MASK to GRC window #4

[Michael Chan]

From: Jeffrey Huang <huangjw@broadcom.com>

In order to use offset 0x4014 for reading CAG interrupt status,
the actual CAG register must be mapped to GRC bar0 window #4.
Otherwise, the driver is reading garbage. This patch corrects
this issue.

Signed-off-by: Jeffrey Huang <huangjw@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 19 ++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h |  4 ++++
 2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 6c2e0c6..a62deff 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1292,8 +1292,6 @@ static inline int bnxt_has_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr)
 	return TX_CMP_VALID(txcmp, raw_cons);
 }
 
-#define CAG_LEGACY_INT_STATUS	0x2014
-
 static irqreturn_t bnxt_inta(int irq, void *dev_instance)
 {
 	struct bnxt_napi *bnapi = dev_instance;
@@ -1305,7 +1303,7 @@ static irqreturn_t bnxt_inta(int irq, void *dev_instance)
 	prefetch(&cpr->cp_desc_ring[CP_RING(cons)][CP_IDX(cons)]);
 
 	if (!bnxt_has_work(bp, cpr)) {
-		int_status = readl(bp->bar0 + CAG_LEGACY_INT_STATUS);
+		int_status = readl(bp->bar0 + BNXT_CAG_REG_LEGACY_INT_STATUS);
 		/* return if erroneous interrupt */
 		if (!(int_status & (0x10000 << cpr->cp_ring_struct.fw_ring_id)))
 			return IRQ_NONE;
@@ -4527,10 +4525,25 @@ static int bnxt_update_phy_setting(struct bnxt *bp)
 	return rc;
 }
 
+/* Common routine to pre-map certain register block to different GRC window.
+ * A PF has 16 4K windows and a VF has 4 4K windows. However, only 15 windows
+ * in PF and 3 windows in VF that can be customized to map in different
+ * register blocks.
+ */
+static void bnxt_preset_reg_win(struct bnxt *bp)
+{
+	if (BNXT_PF(bp)) {
+		/* CAG registers map to GRC window #4 */
+		writel(BNXT_CAG_REG_BASE,
+		       bp->bar0 + BNXT_GRCPF_REG_WINDOW_BASE_OUT + 12);
+	}
+}
+
 static int __bnxt_open_nic(struct bnxt *bp, bool irq_re_init, bool link_re_init)
 {
 	int rc = 0;
 
+	bnxt_preset_reg_win(bp);
 	netif_carrier_off(bp->dev);
 	if (irq_re_init) {
 		rc = bnxt_setup_int_mode(bp);
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 5afe65d..674bc51 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -841,6 +841,10 @@ struct bnxt_queue_info {
 	u8	queue_profile;
 };
 
+#define BNXT_GRCPF_REG_WINDOW_BASE_OUT	0x400
+#define BNXT_CAG_REG_LEGACY_INT_STATUS	0x4014
+#define BNXT_CAG_REG_BASE		0x300000
+
 struct bnxt {
 	void __iomem		*bar0;
 	void __iomem		*bar1;
-- 
1.8.3.1

[PATCH net 4/5] bnxt_en: Fix comparison of u16 sw_id against negative value.

[Michael Chan]

Assign the return value from bitmap_find_free_region() to an integer
variable and check for negative error codes first, before assigning
the bit ID to the unsigned sw_id field.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index a62deff..db15c5e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -5307,7 +5307,7 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
 	struct bnxt_ntuple_filter *fltr, *new_fltr;
 	struct flow_keys *fkeys;
 	struct ethhdr *eth = (struct ethhdr *)skb_mac_header(skb);
-	int rc = 0, idx;
+	int rc = 0, idx, bit_id;
 	struct hlist_head *head;
 
 	if (skb->encapsulation)
@@ -5345,14 +5345,15 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
 	rcu_read_unlock();
 
 	spin_lock_bh(&bp->ntp_fltr_lock);
-	new_fltr->sw_id = bitmap_find_free_region(bp->ntp_fltr_bmap,
-						  BNXT_NTP_FLTR_MAX_FLTR, 0);
-	if (new_fltr->sw_id < 0) {
+	bit_id = bitmap_find_free_region(bp->ntp_fltr_bmap,
+					 BNXT_NTP_FLTR_MAX_FLTR, 0);
+	if (bit_id < 0) {
 		spin_unlock_bh(&bp->ntp_fltr_lock);
 		rc = -ENOMEM;
 		goto err_free;
 	}
 
+	new_fltr->sw_id = (u16)bit_id;
 	new_fltr->flow_id = flow_id;
 	new_fltr->rxq = rxq_index;
 	hlist_add_head_rcu(&new_fltr->hash, head);
-- 
1.8.3.1

[PATCH net 5/5] bnxt_en: More robust SRIOV cleanup sequence.

[Michael Chan]

From: Jeffrey Huang <huangjw@broadcom.com>

Instead of always calling pci_sriov_disable() in remove_one(),
the driver should detect whether VFs are currently assigned
to the VMs. If the VFs are active in VMs, then it should not
disable SRIOV as it is catastrophic to the VMs. Instead,
it just leaves the VFs alone and continues to unload the PF.
The user can then cleanup the VMs even after the PF driver
has been unloaded.

Signed-off-by: Jeffrey Huang <huangjw@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c | 40 +++++++++++++++++--------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
index 60989e7..f4cf688 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_sriov.c
@@ -258,7 +258,7 @@ static int bnxt_set_vf_attr(struct bnxt *bp, int num_vfs)
 	return 0;
 }
 
-static int bnxt_hwrm_func_vf_resource_free(struct bnxt *bp)
+static int bnxt_hwrm_func_vf_resource_free(struct bnxt *bp, int num_vfs)
 {
 	int i, rc = 0;
 	struct bnxt_pf_info *pf = &bp->pf;
@@ -267,7 +267,7 @@ static int bnxt_hwrm_func_vf_resource_free(struct bnxt *bp)
 	bnxt_hwrm_cmd_hdr_init(bp, &req, HWRM_FUNC_VF_RESC_FREE, -1, -1);
 
 	mutex_lock(&bp->hwrm_cmd_lock);
-	for (i = pf->first_vf_id; i < pf->first_vf_id + pf->active_vfs; i++) {
+	for (i = pf->first_vf_id; i < pf->first_vf_id + num_vfs; i++) {
 		req.vf_id = cpu_to_le16(i);
 		rc = _hwrm_send_message(bp, &req, sizeof(req),
 					HWRM_CMD_TIMEOUT);
@@ -509,7 +509,7 @@ static int bnxt_sriov_enable(struct bnxt *bp, int *num_vfs)
 
 err_out2:
 	/* Free the resources reserved for various VF's */
-	bnxt_hwrm_func_vf_resource_free(bp);
+	bnxt_hwrm_func_vf_resource_free(bp, *num_vfs);
 
 err_out1:
 	bnxt_free_vf_resources(bp);
@@ -519,13 +519,19 @@ err_out1:
 
 void bnxt_sriov_disable(struct bnxt *bp)
 {
-	if (!bp->pf.active_vfs)
-		return;
+	u16 num_vfs = pci_num_vf(bp->pdev);
 
-	pci_disable_sriov(bp->pdev);
+	if (!num_vfs)
+		return;
 
-	/* Free the resources reserved for various VF's */
-	bnxt_hwrm_func_vf_resource_free(bp);
+	if (pci_vfs_assigned(bp->pdev)) {
+		netdev_warn(bp->dev, "Unable to free %d VFs because some are assigned to VMs.\n",
+			    num_vfs);
+	} else {
+		pci_disable_sriov(bp->pdev);
+		/* Free the HW resources reserved for various VF's */
+		bnxt_hwrm_func_vf_resource_free(bp, num_vfs);
+	}
 
 	bnxt_free_vf_resources(bp);
 
@@ -552,17 +558,25 @@ int bnxt_sriov_configure(struct pci_dev *pdev, int num_vfs)
 	}
 	bp->sriov_cfg = true;
 	rtnl_unlock();
-	if (!num_vfs) {
-		bnxt_sriov_disable(bp);
-		return 0;
+
+	if (pci_vfs_assigned(bp->pdev)) {
+		netdev_warn(dev, "Unable to configure SRIOV since some VFs are assigned to VMs.\n");
+		num_vfs = 0;
+		goto sriov_cfg_exit;
 	}
 
 	/* Check if enabled VFs is same as requested */
-	if (num_vfs == bp->pf.active_vfs)
-		return 0;
+	if (num_vfs && num_vfs == bp->pf.active_vfs)
+		goto sriov_cfg_exit;
+
+	/* if there are previous existing VFs, clean them up */
+	bnxt_sriov_disable(bp);
+	if (!num_vfs)
+		goto sriov_cfg_exit;
 
 	bnxt_sriov_enable(bp, &num_vfs);
 
+sriov_cfg_exit:
 	bp->sriov_cfg = false;
 	wake_up(&bp->sriov_cfg_wait);
 
-- 
1.8.3.1

Re: [PATCH net 0/5] bnxt_en: Bug fixes.

[David Miller]

From: Michael Chan <mchan@broadcom.com>
Date: Thu, 5 Nov 2015 16:25:46 -0500

> Miscellaneous small bug fixes.

This looks fine, series applied, thanks.

Re: [PATCH net 4/5] bnxt_en: Fix comparison of u16 sw_id against negative value.

[Sergei Shtylyov]

Hello.

On 11/6/2015 12:25 AM, Michael Chan wrote:

> Assign the return value from bitmap_find_free_region() to an integer
> variable and check for negative error codes first, before assigning
> the bit ID to the unsigned sw_id field.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
>   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index a62deff..db15c5e 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -5307,7 +5307,7 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
>   	struct bnxt_ntuple_filter *fltr, *new_fltr;
>   	struct flow_keys *fkeys;
>   	struct ethhdr *eth = (struct ethhdr *)skb_mac_header(skb);
> -	int rc = 0, idx;
> +	int rc = 0, idx, bit_id;

    I'd use the already declared 'rc' variable.

[...]

MBR, Sergei

Re: [PATCH net 4/5] bnxt_en: Fix comparison of u16 sw_id against negative value.

[Michael Chan]

On Fri, 2015-11-06 at 14:57 +0300, Sergei Shtylyov wrote: 
> Hello.
> 
> On 11/6/2015 12:25 AM, Michael Chan wrote:
> 
> > Assign the return value from bitmap_find_free_region() to an integer
> > variable and check for negative error codes first, before assigning
> > the bit ID to the unsigned sw_id field.
> >
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Cc: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > ---
> >   drivers/net/ethernet/broadcom/bnxt/bnxt.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index a62deff..db15c5e 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -5307,7 +5307,7 @@ static int bnxt_rx_flow_steer(struct net_device *dev, const struct sk_buff *skb,
> >   	struct bnxt_ntuple_filter *fltr, *new_fltr;
> >   	struct flow_keys *fkeys;
> >   	struct ethhdr *eth = (struct ethhdr *)skb_mac_header(skb);
> > -	int rc = 0, idx;
> > +	int rc = 0, idx, bit_id;
> 
>     I'd use the already declared 'rc' variable.
> 

I agree with you.  Normally, rc should only contain 0 or negative error
codes and is returned to the caller.  In this particular case, the
caller expects negative or non negative values and we could have used
rc.

Perhaps I'll make the change in the next round of patches.  Thanks.