diff mbox

[1/2] igb: Implement reading of reg RQDPC (Receive Queue Drop Packet Count)

Message ID 1242035422.18487.78.camel@localhost.localdomain
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jesper Dangaard Brouer May 11, 2009, 9:50 a.m. UTC 
On Thu, 2009-05-07 at 10:06 -0600, Williams, Mitch A wrote: 

> NAK.  82575 doesn't have RQDPC registers.  You need to check
> which part you're running on before you read those registers.

I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
working again (This 12 port monster from Hotlava Systems, just needed
more power on PCIe 100Watt)).

I don't see the reason for doing special checks for the 82575. Reading
the RQDPC registers on 82575 always returns 0.  I don't see any harm in
that!?  (it also returns zero in overload situations)

What do you want to redraw your NAK?

I have adjusted the commit message in the patch below...

Comments

Mitch Williams May 13, 2009, 9:07 p.m. UTC | #1 
>-----Original Message-----
>From: Jesper Dangaard Brouer [mailto:hawk@comx.dk] 
>Sent: Monday, May 11, 2009 2:50 AM
>
>I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
>working again (This 12 port monster from Hotlava Systems, just needed
>more power on PCIe 100Watt)).
>
>I don't see the reason for doing special checks for the 82575. Reading
>the RQDPC registers on 82575 always returns 0.  I don't see any harm in
>that!?  (it also returns zero in overload situations)
>
>What do you want to redraw your NAK?
>

Jesper, I still stand by my NAK.  It's never ever a good idea to read
non-existent hardware registers.  You don't know what effect those
reads will have on the hardware.  These addresses may be aliased to other
registers without being documented.  Hardware designers do this all the
time to save a few gates in the address decode logic, and these aliases
may or may not be documented.  If this is the case, reading these
registers will have unintended consequences.  You might be clearing some
other statistic, or worse.

Since we just don't know, it's better to be safe than sorry.  Wrap
those register reads so they only happen on 82576, and I'll happily
ack your patch.

-Mitch--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesper Dangaard Brouer May 14, 2009, 8:03 a.m. UTC | #2 
On Wed, 2009-05-13 at 15:07 -0600, Williams, Mitch A wrote:
>  
> >-----Original Message-----
> >From: Jesper Dangaard Brouer [mailto:hawk@comx.dk] 
> >Sent: Monday, May 11, 2009 2:50 AM
> >
> >I have now tested it on a 82575 chip NIC. (I just got my 82575 NIC
> >working again (This 12 port monster from Hotlava Systems, just needed
> >more power on PCIe 100Watt)).
> >
> >I don't see the reason for doing special checks for the 82575. Reading
> >the RQDPC registers on 82575 always returns 0.  I don't see any harm in
> >that!?  (it also returns zero in overload situations)
> >
> >What do you want to redraw your NAK?
> >
> 
> Jesper, I still stand by my NAK.  It's never ever a good idea to read
> non-existent hardware registers.  You don't know what effect those
> reads will have on the hardware.  These addresses may be aliased to other
> registers without being documented.  Hardware designers do this all the
> time to save a few gates in the address decode logic, and these aliases
> may or may not be documented.  If this is the case, reading these
> registers will have unintended consequences.  You might be clearing some
> other statistic, or worse.
> 
> Since we just don't know, it's better to be safe than sorry.  Wrap
> those register reads so they only happen on 82576, and I'll happily
> ack your patch.

Fine, you argumented well for your case.

I'll repost some patches when time permits...
diff mbox

Patch

diff --git a/drivers/net/igb/e1000_regs.h b/drivers/net/igb/e1000_regs.h
index 0bd7728..85683e2 100644
--- a/drivers/net/igb/e1000_regs.h
+++ b/drivers/net/igb/e1000_regs.h
@@ -165,6 +165,8 @@  enum {
 				    : (0x0C018 + ((_n) * 0x40)))
 #define E1000_RXDCTL(_n)  ((_n) < 4 ? (0x02828 + ((_n) * 0x100)) \
 				    : (0x0C028 + ((_n) * 0x40)))
+#define E1000_RQDPC(_n)   ((_n) < 4 ? (0x02830 + ((_n) * 0x100)) \
+				    : (0x0C030 + ((_n) * 0x40)))
 #define E1000_TDBAL(_n)   ((_n) < 4 ? (0x03800 + ((_n) * 0x100)) \
 				    : (0x0E000 + ((_n) * 0x40)))
 #define E1000_TDBAH(_n)   ((_n) < 4 ? (0x03804 + ((_n) * 0x100)) \
diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index 154c5ac..b2c98de 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -137,11 +137,17 @@  struct igb_buffer {
 	};
 };
 
-struct igb_queue_stats {
+struct igb_tx_queue_stats {
 	u64 packets;
 	u64 bytes;
 };
 
+struct igb_rx_queue_stats {
+	u64 packets;
+	u64 bytes;
+	u64 drops;
+};
+
 struct igb_ring {
 	struct igb_adapter *adapter; /* backlink */
 	void *desc;                  /* descriptor ring memory */
@@ -167,12 +173,13 @@  struct igb_ring {
 	union {
 		/* TX */
 		struct {
-			struct igb_queue_stats tx_stats;
+			struct igb_tx_queue_stats tx_stats;
 			bool detect_tx_hung;
 		};
 		/* RX */
 		struct {
-			struct igb_queue_stats rx_stats;
+			struct igb_rx_queue_stats rx_stats;
+			u64 rx_queue_drops;
 			struct napi_struct napi;
 			int set_itr;
 			struct igb_ring *buddy;
diff --git a/drivers/net/igb/igb_ethtool.c b/drivers/net/igb/igb_ethtool.c
index b8551a5..f0bf6f1 100644
--- a/drivers/net/igb/igb_ethtool.c
+++ b/drivers/net/igb/igb_ethtool.c
@@ -96,9 +96,10 @@  static const struct igb_stats igb_gstrings_stats[] = {
 };
 
 #define IGB_QUEUE_STATS_LEN \
-	((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues + \
-	 ((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
-	(sizeof(struct igb_queue_stats) / sizeof(u64)))
+	(((((struct igb_adapter *)netdev_priv(netdev))->num_rx_queues) * \
+	  (sizeof(struct igb_rx_queue_stats) / sizeof(u64))) +		 \
+	 ((((struct igb_adapter *)netdev_priv(netdev))->num_tx_queues) * \
+	  (sizeof(struct igb_tx_queue_stats) / sizeof(u64))))
 #define IGB_GLOBAL_STATS_LEN	\
 	sizeof(igb_gstrings_stats) / sizeof(struct igb_stats)
 #define IGB_STATS_LEN (IGB_GLOBAL_STATS_LEN + IGB_QUEUE_STATS_LEN)
@@ -1960,7 +1961,8 @@  static void igb_get_ethtool_stats(struct net_device *netdev,
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	u64 *queue_stat;
-	int stat_count = sizeof(struct igb_queue_stats) / sizeof(u64);
+	int stat_count_tx = sizeof(struct igb_tx_queue_stats) / sizeof(u64);
+	int stat_count_rx = sizeof(struct igb_rx_queue_stats) / sizeof(u64);
 	int j;
 	int i;
 
@@ -1973,14 +1975,14 @@  static void igb_get_ethtool_stats(struct net_device *netdev,
 	for (j = 0; j < adapter->num_tx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->tx_ring[j].tx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_tx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
 	for (j = 0; j < adapter->num_rx_queues; j++) {
 		int k;
 		queue_stat = (u64 *)&adapter->rx_ring[j].rx_stats;
-		for (k = 0; k < stat_count; k++)
+		for (k = 0; k < stat_count_rx; k++)
 			data[i + k] = queue_stat[k];
 		i += k;
 	}
@@ -2014,6 +2016,8 @@  static void igb_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 			p += ETH_GSTRING_LEN;
 			sprintf(p, "rx_queue_%u_bytes", i);
 			p += ETH_GSTRING_LEN;
+			sprintf(p, "rx_queue_%u_drops", i);
+			p += ETH_GSTRING_LEN;
 		}
 /*		BUG_ON(p - data != IGB_STATS_LEN * ETH_GSTRING_LEN); */
 		break;
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 8de8629..06b01fc 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3495,6 +3495,8 @@  void igb_update_stats(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	struct pci_dev *pdev = adapter->pdev;
 	u16 phy_tmp;
+	u32 rqdpc_tmp;
+	int i;
 
 #define PHY_IDLE_ERROR_COUNT_MASK 0x00FF
 
@@ -3586,6 +3588,18 @@  void igb_update_stats(struct igb_adapter *adapter)
 
 	/* Rx Errors */
 
+	/* Read out drops stats per RX queue. Notice RQDPC (Receive
+	 * Queue Drop Packet Count) stats only gets incremented, if
+	 * the DROP_EN bit it set (in the SRRCTL register for that
+	 * queue). If DROP_EN bit is NOT set, then the some what
+	 * equivalent count is stored in RNBC (not per queue basis).
+	 * Also note the drop count is due to lack of available descriptors.
+	 */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		rqdpc_tmp = rd32(E1000_RQDPC(i)) & 0xFFF;
+		adapter->rx_ring[i].rx_stats.drops += rqdpc_tmp;
+	}
+
 	/* RLEC on some newer hardware can be incorrect so build
 	* our own version based on RUC and ROC */
 	adapter->net_stats.rx_errors = adapter->stats.rxerrc +