[PATCH net-next 0/6] qlge: bug fix

[PATCH net-next 0/6] qlge: bug fix

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

Please apply it to net-next.

Thanks,
Jitendra

[PATCH net-next 1/6] qlge: Fixed packet transmit errors due to potential driver errors.

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

qlge driver was acting wrongly when considering TX ring full
as a TX error. TX ring full is expected behavior when NIC is
overwhelmed and is expected to happen, as far as packets are
not lost.

Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_main.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index 09d8d33..cdbc860 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2562,7 +2562,6 @@ static netdev_tx_t qlge_send(struct sk_buff *skb, struct net_device *ndev)
 			   __func__, tx_ring_idx);
 		netif_stop_subqueue(ndev, tx_ring->wq_id);
 		atomic_inc(&tx_ring->queue_stopped);
-		tx_ring->tx_errors++;
 		return NETDEV_TX_BUSY;
 	}
 	tx_ring_desc = &tx_ring->q[tx_ring->prod_idx];
-- 
1.7.1

[PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |   43 ++++++++++++++--------
 1 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
index 8e2c2a7..81672f5 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
@@ -388,10 +388,14 @@ static void ql_get_drvinfo(struct net_device *ndev,
 static void ql_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
 {
 	struct ql_adapter *qdev = netdev_priv(ndev);
-	/* What we support. */
-	wol->supported = WAKE_MAGIC;
-	/* What we've currently got set. */
-	wol->wolopts = qdev->wol;
+
+	if (qdev->pdev->subsystem_device == 0x0068 ||
+			qdev->pdev->subsystem_device == 0x0180) {
+		/* What we support. */
+		wol->supported = WAKE_MAGIC;
+		/* What we've currently got set. */
+		wol->wolopts = qdev->wol;
+	}
 }
 
 static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
@@ -399,19 +403,26 @@ static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
 	struct ql_adapter *qdev = netdev_priv(ndev);
 	int status;
 
-	if (wol->wolopts & ~WAKE_MAGIC)
-		return -EINVAL;
-	qdev->wol = wol->wolopts;
-
-	netif_info(qdev, drv, qdev->ndev, "Set wol option 0x%x\n", qdev->wol);
-	if (!qdev->wol) {
-		u32 wol = 0;
-		status = ql_mb_wol_mode(qdev, wol);
-		netif_err(qdev, drv, qdev->ndev, "WOL %s (wol code 0x%x)\n",
-			  status == 0 ? "cleared successfully" : "clear failed",
-			  wol);
+	if (qdev->pdev->subsystem_device == 0x0068 ||
+			qdev->pdev->subsystem_device == 0x0180) {
+		if (wol->wolopts & ~WAKE_MAGIC)
+			return -EINVAL;
+		qdev->wol = wol->wolopts;
+
+		netif_info(qdev, drv, qdev->ndev,
+				"Set wol option 0x%x\n", qdev->wol);
+		if (!qdev->wol) {
+			u32 wol = 0;
+			status = ql_mb_wol_mode(qdev, wol);
+			netif_err(qdev, drv, qdev->ndev,
+			"WOL %s (wol code 0x%x)\n",
+			status == 0 ? "cleared successfully" : "clear failed",
+			wol);
+		}
+	} else {
+		netif_info(qdev, drv, qdev->ndev,
+				"WOL is not supported on stand-up card\n");
 	}
-
 	return 0;
 }
 
-- 
1.7.1

[PATCH net-next 3/6] qlge: Garbage values shown in extra info during selftest.

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

while running selftest 'ethtool -t' multiple times will get
different values in the 'extra info' section, which was garbage.

Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
index 81672f5..966bd96 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
@@ -248,6 +248,9 @@ static char ql_stats_str_arr[][ETH_GSTRING_LEN] = {
 static void ql_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 {
 	switch (stringset) {
+	case ETH_SS_TEST:
+		memcpy(buf, *ql_gstrings_test, QLGE_TEST_LEN * ETH_GSTRING_LEN);
+		break;
 	case ETH_SS_STATS:
 		memcpy(buf, ql_stats_str_arr, sizeof(ql_stats_str_arr));
 		break;
@@ -539,6 +542,8 @@ static void ql_self_test(struct net_device *ndev,
 {
 	struct ql_adapter *qdev = netdev_priv(ndev);
 
+	memset(data, 0, sizeof(u64) * QLGE_TEST_LEN);
+
 	if (netif_running(ndev)) {
 		set_bit(QL_SELFTEST, &qdev->flags);
 		if (eth_test->flags == ETH_TEST_FL_OFFLINE) {
-- 
1.7.1

[PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge_main.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index cdbc860..aa514c5 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -2701,11 +2701,9 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
 	    pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
 				 &tx_ring->wq_base_dma);
 
-	if ((tx_ring->wq_base == NULL) ||
-	    tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
-		netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
-		return -ENOMEM;
-	}
+	if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
+		goto err;
+
 	tx_ring->q =
 	    kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
 	if (tx_ring->q == NULL)
@@ -2713,8 +2711,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
 
 	return 0;
 err:
-	pci_free_consistent(qdev->pdev, tx_ring->wq_size,
+	if (tx_ring->wq_base) {
+		pci_free_consistent(qdev->pdev, tx_ring->wq_size,
 			    tx_ring->wq_base, tx_ring->wq_base_dma);
+		tx_ring->wq_base = NULL;
+	}
+	netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
 	return -ENOMEM;
 }
 
-- 
1.7.1

[PATCH net-next 5/6] qlge: Categorize receive frame errors from firmware.

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

Signed-off-by: Sritej Velaga <sritej.velaga@qlogic.com>
Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge.h         |    8 ++++
 drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |   14 +++++++
 drivers/net/ethernet/qlogic/qlge/qlge_main.c    |   46 ++++++++++++++++++----
 3 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index 5a639df..e81bbb7 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -1535,6 +1535,14 @@ struct nic_stats {
 	u64 rx_1024_to_1518_pkts;
 	u64 rx_1519_to_max_pkts;
 	u64 rx_len_err_pkts;
+	/* Receive Mac Err stats */
+	u64 rx_code_err;
+	u64 rx_oversize_err;
+	u64 rx_undersize_err;
+	u64 rx_preamble_err;
+	u64 rx_frame_len_err;
+	u64 rx_crc_err;
+	u64 rx_err_count;
 	/*
 	 * These stats come from offset 500h to 5C8h
 	 * in the XGMAC register.
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
index 966bd96..bbc4136 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
@@ -226,6 +226,13 @@ static char ql_stats_str_arr[][ETH_GSTRING_LEN] = {
 	{"rx_1024_to_1518_pkts"},
 	{"rx_1519_to_max_pkts"},
 	{"rx_len_err_pkts"},
+	{"rx_code_err"},
+	{"rx_oversize_err"},
+	{"rx_undersize_err"},
+	{"rx_preamble_err"},
+	{"rx_frame_len_err"},
+	{"rx_crc_err"},
+	{"rx_err_count"},
 	{"tx_cbfc_pause_frames0"},
 	{"tx_cbfc_pause_frames1"},
 	{"tx_cbfc_pause_frames2"},
@@ -320,6 +327,13 @@ ql_get_ethtool_stats(struct net_device *ndev,
 	*data++ = s->rx_1024_to_1518_pkts;
 	*data++ = s->rx_1519_to_max_pkts;
 	*data++ = s->rx_len_err_pkts;
+	*data++ = s->rx_code_err;
+	*data++ = s->rx_oversize_err;
+	*data++ = s->rx_undersize_err;
+	*data++ = s->rx_preamble_err;
+	*data++ = s->rx_frame_len_err;
+	*data++ = s->rx_crc_err;
+	*data++ = s->rx_err_count;
 	*data++ = s->tx_cbfc_pause_frames0;
 	*data++ = s->tx_cbfc_pause_frames1;
 	*data++ = s->tx_cbfc_pause_frames2;
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
index aa514c5..0f56148 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
+++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
@@ -1433,6 +1433,34 @@ map_error:
 	return NETDEV_TX_BUSY;
 }
 
+/* Categorizing receive firmware frame errors */
+static void ql_categorize_rx_err(struct ql_adapter *qdev, u8 rx_err)
+{
+	qdev->nic_stats.rx_err_count++;
+
+	switch (rx_err & IB_MAC_IOCB_RSP_ERR_MASK) {
+	case IB_MAC_IOCB_RSP_ERR_CODE_ERR:
+		qdev->nic_stats.rx_code_err++;
+		break;
+	case IB_MAC_IOCB_RSP_ERR_OVERSIZE:
+		qdev->nic_stats.rx_oversize_err++;
+		break;
+	case IB_MAC_IOCB_RSP_ERR_UNDERSIZE:
+		qdev->nic_stats.rx_undersize_err++;
+		break;
+	case IB_MAC_IOCB_RSP_ERR_PREAMBLE:
+		qdev->nic_stats.rx_preamble_err++;
+		break;
+	case IB_MAC_IOCB_RSP_ERR_FRAME_LEN:
+		qdev->nic_stats.rx_frame_len_err++;
+		break;
+	case IB_MAC_IOCB_RSP_ERR_CRC:
+		qdev->nic_stats.rx_crc_err++;
+	default:
+		break;
+	}
+}
+
 /* Process an inbound completion from an rx ring. */
 static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
 					struct rx_ring *rx_ring,
@@ -1446,6 +1474,12 @@ static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
 
 	napi->dev = qdev->ndev;
 
+	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
+		ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
+		put_page(lbq_desc->p.pg_chunk.page);
+		return;
+	}
+
 	skb = napi_get_frags(napi);
 	if (!skb) {
 		netif_err(qdev, drv, qdev->ndev,
@@ -1502,9 +1536,7 @@ static void ql_process_mac_rx_page(struct ql_adapter *qdev,
 
 	/* Frame error, so drop the packet. */
 	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
-		netif_info(qdev, drv, qdev->ndev,
-			  "Receive error, flags2 = 0x%x\n", ib_mac_rsp->flags2);
-		rx_ring->rx_errors++;
+		ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
 		goto err_out;
 	}
 
@@ -1595,10 +1627,8 @@ static void ql_process_mac_rx_skb(struct ql_adapter *qdev,
 
 	/* Frame error, so drop the packet. */
 	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
-		netif_info(qdev, drv, qdev->ndev,
-			  "Receive error, flags2 = 0x%x\n", ib_mac_rsp->flags2);
+		ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
 		dev_kfree_skb_any(skb);
-		rx_ring->rx_errors++;
 		return;
 	}
 
@@ -1910,10 +1940,8 @@ static void ql_process_mac_split_rx_intr(struct ql_adapter *qdev,
 
 	/* Frame error, so drop the packet. */
 	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
-		netif_info(qdev, drv, qdev->ndev,
-			  "Receive error, flags2 = 0x%x\n", ib_mac_rsp->flags2);
+		ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
 		dev_kfree_skb_any(skb);
-		rx_ring->rx_errors++;
 		return;
 	}
 
-- 
1.7.1

[PATCH net-next 6/6] qlge: Bumped driver version to 1.00.00.31

[Jitendra Kalsaria]

From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>

Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
---
 drivers/net/ethernet/qlogic/qlge/qlge.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
index e81bbb7..5a8c00c 100644
--- a/drivers/net/ethernet/qlogic/qlge/qlge.h
+++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
@@ -18,7 +18,7 @@
  */
 #define DRV_NAME  	"qlge"
 #define DRV_STRING 	"QLogic 10 Gigabit PCI-E Ethernet Driver "
-#define DRV_VERSION	"v1.00.00.30.00.00-01"
+#define DRV_VERSION	"v1.00.00.31"
 
 #define WQ_ADDR_ALIGN	0x3	/* 4 byte alignment */
 
-- 
1.7.1

Re: [PATCH net-next 0/6] qlge: bug fix

[Francois Romieu]

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> 
> Please apply it to net-next.

Bug fixes are supposed to be applied to current net, and fast-tracked
to stable.

Why net-next ?

-- 
Ueimor

Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.

[Francois Romieu]

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> 
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
>  drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |   43 ++++++++++++++--------
>  1 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 8e2c2a7..81672f5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -388,10 +388,14 @@ static void ql_get_drvinfo(struct net_device *ndev,
>  static void ql_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  {
>  	struct ql_adapter *qdev = netdev_priv(ndev);
> -	/* What we support. */
> -	wol->supported = WAKE_MAGIC;
> -	/* What we've currently got set. */
> -	wol->wolopts = qdev->wol;
> +
> +	if (qdev->pdev->subsystem_device == 0x0068 ||
> +			qdev->pdev->subsystem_device == 0x0180) {
> +		/* What we support. */
> +		wol->supported = WAKE_MAGIC;
> +		/* What we've currently got set. */
> +		wol->wolopts = qdev->wol;
> +	}

	unsigned short ssys_dev = qdev->pdev->subsystem_device;

	if (ssys_dev == 0x0068 || ssys_dev == 0x0180) {
		wol->supported = WAKE_MAGIC;
		wol->wolopts = qdev->wol;
	}
>  }
>  
>  static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> @@ -399,19 +403,26 @@ static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  	struct ql_adapter *qdev = netdev_priv(ndev);
>  	int status;
>  
> -	if (wol->wolopts & ~WAKE_MAGIC)
> -		return -EINVAL;
> -	qdev->wol = wol->wolopts;
> -
> -	netif_info(qdev, drv, qdev->ndev, "Set wol option 0x%x\n", qdev->wol);
> -	if (!qdev->wol) {
> -		u32 wol = 0;
> -		status = ql_mb_wol_mode(qdev, wol);
> -		netif_err(qdev, drv, qdev->ndev, "WOL %s (wol code 0x%x)\n",
> -			  status == 0 ? "cleared successfully" : "clear failed",
> -			  wol);
> +	if (qdev->pdev->subsystem_device == 0x0068 ||
> +			qdev->pdev->subsystem_device == 0x0180) {

See above.

> +		if (wol->wolopts & ~WAKE_MAGIC)
> +			return -EINVAL;
> +		qdev->wol = wol->wolopts;
> +
> +		netif_info(qdev, drv, qdev->ndev,
> +				"Set wol option 0x%x\n", qdev->wol);

qdev->ndev == ndev, right ?

> +		if (!qdev->wol) {
> +			u32 wol = 0;
> +			status = ql_mb_wol_mode(qdev, wol);

Missing empty line.

status should be declared here.

> +			netif_err(qdev, drv, qdev->ndev,
> +			"WOL %s (wol code 0x%x)\n",
> +			status == 0 ? "cleared successfully" : "clear failed",
> +			wol);

- broken indent
- netif_err(... "cleared successfully" ...)

> +		}
> +	} else {
> +		netif_info(qdev, drv, qdev->ndev,
> +				"WOL is not supported on stand-up card\n");
>  	}
> -
>  	return 0;

If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
difference on the status code. It should be fixed.

-- 
Ueimor

Re: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.

[Francois Romieu]

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index cdbc860..aa514c5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -2701,11 +2701,9 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>  	    pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
>  				 &tx_ring->wq_base_dma);
>  
> -	if ((tx_ring->wq_base == NULL) ||
> -	    tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
> -		netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
> -		return -ENOMEM;
> -	}
> +	if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
> +		goto err;
> +
>  	tx_ring->q =
>  	    kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
>  	if (tx_ring->q == NULL)
> @@ -2713,8 +2711,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>  
>  	return 0;
>  err:
> -	pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> +	if (tx_ring->wq_base) {
> +		pci_free_consistent(qdev->pdev, tx_ring->wq_size,
>  			    tx_ring->wq_base, tx_ring->wq_base_dma);
> +		tx_ring->wq_base = NULL;
> +	}

You do not need a test: use a second label + goto.

Nit: You can replace 'if ((tx_ring->wq_base == NULL)' with
'if (!tx_ring->wq_base' and add a local variable for qdev->pdev.

You may consider replacing pci_alloc_consistent with dma_alloc_coherent
in a future patch.

-- 
Ueimor

Re: [PATCH net-next 5/6] qlge: Categorize receive frame errors from firmware.

[Francois Romieu]

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> 
> Signed-off-by: Sritej Velaga <sritej.velaga@qlogic.com>
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
>  drivers/net/ethernet/qlogic/qlge/qlge.h         |    8 ++++
>  drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |   14 +++++++
>  drivers/net/ethernet/qlogic/qlge/qlge_main.c    |   46 ++++++++++++++++++----
>  3 files changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 5a639df..e81bbb7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1535,6 +1535,14 @@ struct nic_stats {
>  	u64 rx_1024_to_1518_pkts;
>  	u64 rx_1519_to_max_pkts;
>  	u64 rx_len_err_pkts;
> +	/* Receive Mac Err stats */
> +	u64 rx_code_err;
> +	u64 rx_oversize_err;
> +	u64 rx_undersize_err;
> +	u64 rx_preamble_err;
> +	u64 rx_frame_len_err;
> +	u64 rx_crc_err;
> +	u64 rx_err_count;
>  	/*
>  	 * These stats come from offset 500h to 5C8h
>  	 * in the XGMAC register.
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 966bd96..bbc4136 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -226,6 +226,13 @@ static char ql_stats_str_arr[][ETH_GSTRING_LEN] = {
>  	{"rx_1024_to_1518_pkts"},
>  	{"rx_1519_to_max_pkts"},
>  	{"rx_len_err_pkts"},
> +	{"rx_code_err"},
> +	{"rx_oversize_err"},
> +	{"rx_undersize_err"},
> +	{"rx_preamble_err"},
> +	{"rx_frame_len_err"},
> +	{"rx_crc_err"},
> +	{"rx_err_count"},
>  	{"tx_cbfc_pause_frames0"},
>  	{"tx_cbfc_pause_frames1"},
>  	{"tx_cbfc_pause_frames2"},
> @@ -320,6 +327,13 @@ ql_get_ethtool_stats(struct net_device *ndev,
>  	*data++ = s->rx_1024_to_1518_pkts;
>  	*data++ = s->rx_1519_to_max_pkts;
>  	*data++ = s->rx_len_err_pkts;
> +	*data++ = s->rx_code_err;
> +	*data++ = s->rx_oversize_err;
> +	*data++ = s->rx_undersize_err;
> +	*data++ = s->rx_preamble_err;
> +	*data++ = s->rx_frame_len_err;
> +	*data++ = s->rx_crc_err;
> +	*data++ = s->rx_err_count;
>  	*data++ = s->tx_cbfc_pause_frames0;
>  	*data++ = s->tx_cbfc_pause_frames1;
>  	*data++ = s->tx_cbfc_pause_frames2;

The stats seem to be 64 bits integers only. Couldn't you use a plain loop
with ARRAY_SIZE(ql_stats_str_arr) ?

> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index aa514c5..0f56148 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1433,6 +1433,34 @@ map_error:
>  	return NETDEV_TX_BUSY;
>  }
>  
> +/* Categorizing receive firmware frame errors */
> +static void ql_categorize_rx_err(struct ql_adapter *qdev, u8 rx_err)
> +{
> +	qdev->nic_stats.rx_err_count++;

Use a local variable for &qdev->nic_stats ?

> +
> +	switch (rx_err & IB_MAC_IOCB_RSP_ERR_MASK) {
> +	case IB_MAC_IOCB_RSP_ERR_CODE_ERR:
> +		qdev->nic_stats.rx_code_err++;
> +		break;
> +	case IB_MAC_IOCB_RSP_ERR_OVERSIZE:
> +		qdev->nic_stats.rx_oversize_err++;
> +		break;
> +	case IB_MAC_IOCB_RSP_ERR_UNDERSIZE:
> +		qdev->nic_stats.rx_undersize_err++;
> +		break;
> +	case IB_MAC_IOCB_RSP_ERR_PREAMBLE:
> +		qdev->nic_stats.rx_preamble_err++;
> +		break;
> +	case IB_MAC_IOCB_RSP_ERR_FRAME_LEN:
> +		qdev->nic_stats.rx_frame_len_err++;
> +		break;
> +	case IB_MAC_IOCB_RSP_ERR_CRC:
> +		qdev->nic_stats.rx_crc_err++;
> +	default:
> +		break;
> +	}
> +}
> +
>  /* Process an inbound completion from an rx ring. */
>  static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
>  					struct rx_ring *rx_ring,
> @@ -1446,6 +1474,12 @@ static void ql_process_mac_rx_gro_page(struct ql_adapter *qdev,
>  
>  	napi->dev = qdev->ndev;
>  
> +	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_ERR_MASK) {
> +		ql_categorize_rx_err(qdev, ib_mac_rsp->flags2);
> +		put_page(lbq_desc->p.pg_chunk.page);
> +		return;
> +	}

I did not expect a put_page here from the commit description.

-- 
Ueimor

Re: [PATCH net-next 0/6] qlge: bug fix

[Francois Romieu]

Please fix your Cc:.

Ron@linux-zupk.site does not exist.

-- 
Ueimor

RE: [PATCH net-next 0/6] qlge: bug fix

[Jitendra Kalsaria]

Francois,

I have already fixed my cc list. Will resubmit to net as major of them where bug fixes and thanks for your suggestion.

Jiten

-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com] 
Sent: Friday, June 29, 2012 2:40 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 0/6] qlge: bug fix

Please fix your Cc:.

Ron@linux-zupk.site does not exist.

-- 
Ueimor

RE: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.

[Jitendra Kalsaria]

Francois,

See comments inline.

Jiten

-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com] 
Sent: Friday, June 29, 2012 2:38 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Ron@linux-zupk.site; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
> From: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> 
> Signed-off-by: Jitendra Kalsaria <jitendra.kalsaria@qlogic.com>
> ---
>  drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c |   43 ++++++++++++++--------
>  1 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> index 8e2c2a7..81672f5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_ethtool.c
> @@ -388,10 +388,14 @@ static void ql_get_drvinfo(struct net_device *ndev,
>  static void ql_get_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  {
>  	struct ql_adapter *qdev = netdev_priv(ndev);
> -	/* What we support. */
> -	wol->supported = WAKE_MAGIC;
> -	/* What we've currently got set. */
> -	wol->wolopts = qdev->wol;
> +
> +	if (qdev->pdev->subsystem_device == 0x0068 ||
> +			qdev->pdev->subsystem_device == 0x0180) {
> +		/* What we support. */
> +		wol->supported = WAKE_MAGIC;
> +		/* What we've currently got set. */
> +		wol->wolopts = qdev->wol;
> +	}

	unsigned short ssys_dev = qdev->pdev->subsystem_device;

	if (ssys_dev == 0x0068 || ssys_dev == 0x0180) {
		wol->supported = WAKE_MAGIC;
		wol->wolopts = qdev->wol;
	}
[JK] Will do this.
>  }
>  
>  static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
> @@ -399,19 +403,26 @@ static int ql_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
>  	struct ql_adapter *qdev = netdev_priv(ndev);
>  	int status;
>  
> -	if (wol->wolopts & ~WAKE_MAGIC)
> -		return -EINVAL;
> -	qdev->wol = wol->wolopts;
> -
> -	netif_info(qdev, drv, qdev->ndev, "Set wol option 0x%x\n", qdev->wol);
> -	if (!qdev->wol) {
> -		u32 wol = 0;
> -		status = ql_mb_wol_mode(qdev, wol);
> -		netif_err(qdev, drv, qdev->ndev, "WOL %s (wol code 0x%x)\n",
> -			  status == 0 ? "cleared successfully" : "clear failed",
> -			  wol);
> +	if (qdev->pdev->subsystem_device == 0x0068 ||
> +			qdev->pdev->subsystem_device == 0x0180) {

See above.

> +		if (wol->wolopts & ~WAKE_MAGIC)
> +			return -EINVAL;
> +		qdev->wol = wol->wolopts;
> +
> +		netif_info(qdev, drv, qdev->ndev,
> +				"Set wol option 0x%x\n", qdev->wol);

qdev->ndev == ndev, right ?
[JK] Yes
> +		if (!qdev->wol) {
> +			u32 wol = 0;
> +			status = ql_mb_wol_mode(qdev, wol);

Missing empty line.

status should be declared here.
[JK] Will fix this
> +			netif_err(qdev, drv, qdev->ndev,
> +			"WOL %s (wol code 0x%x)\n",
> +			status == 0 ? "cleared successfully" : "clear failed",
> +			wol);

- broken indent
- netif_err(... "cleared successfully" ...)
[JK] I don't think so, %s will be replace with "cleared successfully" or with "clear failed" depend upon status value.
> +		}
> +	} else {
> +		netif_info(qdev, drv, qdev->ndev,
> +				"WOL is not supported on stand-up card\n");
>  	}
> -
>  	return 0;

If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
difference on the status code. It should be fixed.

[JK] You right, here we are not clearing WAKE_MAGIC but we are making sure that wol is disable when no option is set.
-- 
Ueimor

RE: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.

[Jitendra Kalsaria]

Francois,

See comments inline.

Jiten

-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com] 
Sent: Friday, June 29, 2012 2:38 PM
To: Jitendra Kalsaria
Cc: David Miller; netdev; Ron@linux-zupk.site; Dept-NX Linux NIC Driver
Subject: Re: [PATCH net-next 4/6] qlge: Fixed double pci free upon tx_ring->q allocation failure.

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index cdbc860..aa514c5 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -2701,11 +2701,9 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>  	    pci_alloc_consistent(qdev->pdev, tx_ring->wq_size,
>  				 &tx_ring->wq_base_dma);
>  
> -	if ((tx_ring->wq_base == NULL) ||
> -	    tx_ring->wq_base_dma & WQ_ADDR_ALIGN) {
> -		netif_err(qdev, ifup, qdev->ndev, "tx_ring alloc failed.\n");
> -		return -ENOMEM;
> -	}
> +	if ((tx_ring->wq_base == NULL) || tx_ring->wq_base_dma & WQ_ADDR_ALIGN)
> +		goto err;
> +
>  	tx_ring->q =
>  	    kmalloc(tx_ring->wq_len * sizeof(struct tx_ring_desc), GFP_KERNEL);
>  	if (tx_ring->q == NULL)
> @@ -2713,8 +2711,12 @@ static int ql_alloc_tx_resources(struct ql_adapter *qdev,
>  
>  	return 0;
>  err:
> -	pci_free_consistent(qdev->pdev, tx_ring->wq_size,
> +	if (tx_ring->wq_base) {
> +		pci_free_consistent(qdev->pdev, tx_ring->wq_size,
>  			    tx_ring->wq_base, tx_ring->wq_base_dma);
> +		tx_ring->wq_base = NULL;
> +	}

You do not need a test: use a second label + goto.

Nit: You can replace 'if ((tx_ring->wq_base == NULL)' with
'if (!tx_ring->wq_base' and add a local variable for qdev->pdev.

You may consider replacing pci_alloc_consistent with dma_alloc_coherent
in a future patch.

[JK] Will, definitely take care of this in future patch.

-- 
Ueimor

Re: [PATCH net-next 2/6] qlge: Stand-up card should not report supporting wol.

[Francois Romieu]

(linux-zupk.site removed)

Jitendra Kalsaria <jitendra.kalsaria@qlogic.com> :
[...]
> - broken indent
> - netif_err(... "cleared successfully" ...)
> [JK] I don't think so, %s will be replace with "cleared successfully" or with "clear failed" depend upon status value.

I would not netif_err when everything is ok.

[...]
> If I read things correctly, WAKE_MAGIC can not be cleared and it makes no
> difference on the status code. It should be fixed.
> 
> [JK] You right, here we are not clearing WAKE_MAGIC but we are making sure that wol is disable when no option is set.

Imho wol should be disabled as well when neither WAKE_MAGIC nor any option
is set at all.

-- 
Ueimor