From: Mandeep Baines <[email protected]>
Rx byte count was off; instead use the hardware's count. Tx packet
count was counting pre-TSO packets; instead count on-the-wire packets.
Report hardware dropped frame count as rx_fifo_errors.
- The count of transmitted packets reported by the forcedeth driver
reports pre-TSO (TCP Segmentation Offload) packet counts and not the
count of the number of packets sent on the wire. This change fixes
the forcedeth driver to report the correct count. Fixed the code by
copying the count stored in the NIC H/W to the value reported by the
driver.
- Count rx_drop_frame errors as rx_fifo_errors:
We see a lot of rx_drop_frame errors if we disable the rx bottom-halves
for too long. Normally, rx_fifo_errors would be counted in this case.
The rx_drop_frame error count is private to forcedeth and is not
reported by ifconfig or sysfs. The rx_fifo_errors count is currently
unused in the forcedeth driver. It is reported by ifconfig as overruns.
This change reports rx_drop_frame errors as rx_fifo_errors.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index d09e8b0..895471d 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -1684,6 +1684,7 @@ static void nv_get_hw_stats(struct net_device *dev)
np->estats.tx_pause += readl(base + NvRegTxPause);
np->estats.rx_pause += readl(base + NvRegRxPause);
np->estats.rx_drop_frame += readl(base + NvRegRxDropFrame);
+ np->estats.rx_errors_total += np->estats.rx_drop_frame;
}
if (np->driver_data & DEV_HAS_STATISTICS_V3) {
@@ -1708,11 +1709,14 @@ static struct net_device_stats *nv_get_stats(struct net_device *dev)
nv_get_hw_stats(dev);
/* copy to net_device stats */
+ dev->stats.tx_packets = np->estats.tx_packets;
+ dev->stats.rx_bytes = np->estats.rx_bytes;
dev->stats.tx_bytes = np->estats.tx_bytes;
dev->stats.tx_fifo_errors = np->estats.tx_fifo_errors;
dev->stats.tx_carrier_errors = np->estats.tx_carrier_errors;
dev->stats.rx_crc_errors = np->estats.rx_crc_errors;
dev->stats.rx_over_errors = np->estats.rx_over_errors;
+ dev->stats.rx_fifo_errors = np->estats.rx_drop_frame;
dev->stats.rx_errors = np->estats.rx_errors_total;
dev->stats.tx_errors = np->estats.tx_errors_total;
}
--
1.7.3.1
From: Sameer Nanda <[email protected]>
This change publishes a new ethtool stats: tx_timeout that counts the
number of times the tx_timeout callback was triggered.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 895471d..112dc0b 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -632,6 +632,7 @@ static const struct nv_ethtool_str nv_estats_str[] = {
{ "rx_packets" },
{ "rx_errors_total" },
{ "tx_errors_total" },
+ { "tx_timeout" },
/* version 2 stats */
{ "tx_deferral" },
@@ -672,6 +673,7 @@ struct nv_ethtool_stats {
u64 rx_packets;
u64 rx_errors_total;
u64 tx_errors_total;
+ u64 tx_timeout;
/* version 2 stats */
u64 tx_deferral;
@@ -2526,6 +2528,8 @@ static void nv_tx_timeout(struct net_device *dev)
spin_lock_irq(&np->lock);
+ np->estats.tx_timeout++;
+
/* 1) stop tx engine */
nv_stop_tx(dev);
--
1.7.3.1
From: Sameer Nanda <[email protected]>
This adds a new module parameter "debug_tx_timeout" to silence most
debug messages in case of TX timeout. These messages don't provide a
signal/noise ratio high enough for production systems and, with ~30kB
logged each time, they tend to add to a cascade effect if the system
is already under stress (memory pressure, disk, etc.).
By default, the parameter is clear, meaning the debug messages are not
displayed.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 91 ++++++++++++++++++++++++++--------------------
1 files changed, 51 insertions(+), 40 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 112dc0b..7a6aa08 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -896,6 +896,11 @@ enum {
static int dma_64bit = NV_DMA_64BIT_ENABLED;
/*
+ * Debug output control for tx_timeout
+ */
+static bool debug_tx_timeout = false;
+
+/*
* Crossover Detection
* Realtek 8201 phy + some OEM boards do not work properly.
*/
@@ -2473,7 +2478,6 @@ static void nv_tx_timeout(struct net_device *dev)
u32 status;
union ring_type put_tx;
int saved_tx_limit;
- int i;
if (np->msi_flags & NV_MSI_X_ENABLED)
status = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQSTAT_MASK;
@@ -2482,47 +2486,51 @@ static void nv_tx_timeout(struct net_device *dev)
netdev_info(dev, "Got tx_timeout. irq: %08x\n", status);
- netdev_info(dev, "Ring at %lx\n", (unsigned long)np->ring_addr);
- netdev_info(dev, "Dumping tx registers\n");
- for (i = 0; i <= np->register_size; i += 32) {
- netdev_info(dev,
- "%3x: %08x %08x %08x %08x %08x %08x %08x %08x\n",
- i,
- readl(base + i + 0), readl(base + i + 4),
- readl(base + i + 8), readl(base + i + 12),
- readl(base + i + 16), readl(base + i + 20),
- readl(base + i + 24), readl(base + i + 28));
- }
- netdev_info(dev, "Dumping tx ring\n");
- for (i = 0; i < np->tx_ring_size; i += 4) {
- if (!nv_optimized(np)) {
- netdev_info(dev,
- "%03x: %08x %08x // %08x %08x // %08x %08x // %08x %08x\n",
- i,
- le32_to_cpu(np->tx_ring.orig[i].buf),
- le32_to_cpu(np->tx_ring.orig[i].flaglen),
- le32_to_cpu(np->tx_ring.orig[i+1].buf),
- le32_to_cpu(np->tx_ring.orig[i+1].flaglen),
- le32_to_cpu(np->tx_ring.orig[i+2].buf),
- le32_to_cpu(np->tx_ring.orig[i+2].flaglen),
- le32_to_cpu(np->tx_ring.orig[i+3].buf),
- le32_to_cpu(np->tx_ring.orig[i+3].flaglen));
- } else {
+ if (unlikely(debug_tx_timeout)) {
+ int i;
+
+ netdev_info(dev, "Ring at %lx\n", (unsigned long)np->ring_addr);
+ netdev_info(dev, "Dumping tx registers\n");
+ for (i = 0; i <= np->register_size; i += 32) {
netdev_info(dev,
- "%03x: %08x %08x %08x // %08x %08x %08x // %08x %08x %08x // %08x %08x %08x\n",
+ "%3x: %08x %08x %08x %08x %08x %08x %08x %08x\n",
i,
- le32_to_cpu(np->tx_ring.ex[i].bufhigh),
- le32_to_cpu(np->tx_ring.ex[i].buflow),
- le32_to_cpu(np->tx_ring.ex[i].flaglen),
- le32_to_cpu(np->tx_ring.ex[i+1].bufhigh),
- le32_to_cpu(np->tx_ring.ex[i+1].buflow),
- le32_to_cpu(np->tx_ring.ex[i+1].flaglen),
- le32_to_cpu(np->tx_ring.ex[i+2].bufhigh),
- le32_to_cpu(np->tx_ring.ex[i+2].buflow),
- le32_to_cpu(np->tx_ring.ex[i+2].flaglen),
- le32_to_cpu(np->tx_ring.ex[i+3].bufhigh),
- le32_to_cpu(np->tx_ring.ex[i+3].buflow),
- le32_to_cpu(np->tx_ring.ex[i+3].flaglen));
+ readl(base + i + 0), readl(base + i + 4),
+ readl(base + i + 8), readl(base + i + 12),
+ readl(base + i + 16), readl(base + i + 20),
+ readl(base + i + 24), readl(base + i + 28));
+ }
+ netdev_info(dev, "Dumping tx ring\n");
+ for (i = 0; i < np->tx_ring_size; i += 4) {
+ if (!nv_optimized(np)) {
+ netdev_info(dev,
+ "%03x: %08x %08x // %08x %08x // %08x %08x // %08x %08x\n",
+ i,
+ le32_to_cpu(np->tx_ring.orig[i].buf),
+ le32_to_cpu(np->tx_ring.orig[i].flaglen),
+ le32_to_cpu(np->tx_ring.orig[i+1].buf),
+ le32_to_cpu(np->tx_ring.orig[i+1].flaglen),
+ le32_to_cpu(np->tx_ring.orig[i+2].buf),
+ le32_to_cpu(np->tx_ring.orig[i+2].flaglen),
+ le32_to_cpu(np->tx_ring.orig[i+3].buf),
+ le32_to_cpu(np->tx_ring.orig[i+3].flaglen));
+ } else {
+ netdev_info(dev,
+ "%03x: %08x %08x %08x // %08x %08x %08x // %08x %08x %08x // %08x %08x %08x\n",
+ i,
+ le32_to_cpu(np->tx_ring.ex[i].bufhigh),
+ le32_to_cpu(np->tx_ring.ex[i].buflow),
+ le32_to_cpu(np->tx_ring.ex[i].flaglen),
+ le32_to_cpu(np->tx_ring.ex[i+1].bufhigh),
+ le32_to_cpu(np->tx_ring.ex[i+1].buflow),
+ le32_to_cpu(np->tx_ring.ex[i+1].flaglen),
+ le32_to_cpu(np->tx_ring.ex[i+2].bufhigh),
+ le32_to_cpu(np->tx_ring.ex[i+2].buflow),
+ le32_to_cpu(np->tx_ring.ex[i+2].flaglen),
+ le32_to_cpu(np->tx_ring.ex[i+3].bufhigh),
+ le32_to_cpu(np->tx_ring.ex[i+3].buflow),
+ le32_to_cpu(np->tx_ring.ex[i+3].flaglen));
+ }
}
}
@@ -6006,6 +6014,9 @@ module_param(phy_cross, int, 0);
MODULE_PARM_DESC(phy_cross, "Phy crossover detection for Realtek 8201 phy is enabled by setting to 1 and disabled by setting to 0.");
module_param(phy_power_down, int, 0);
MODULE_PARM_DESC(phy_power_down, "Power down phy and disable link when interface is down (1), or leave phy powered up (0).");
+module_param(debug_tx_timeout, bool, false);
+MODULE_PARM_DESC(debug_tx_timeout,
+ "Dump tx related registers and ring when tx_timeout happens");
MODULE_AUTHOR("Manfred Spraul <[email protected]>");
MODULE_DESCRIPTION("Reverse Engineered nForce ethernet driver");
--
1.7.3.1
From: Mike Ditto <[email protected]>
This is to avoid a race, accidentally acknowledging an interrupt that
we didn't notice and won't immediately process. This is based solely
on code inspection; it is not known if there was an actual bug here.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 13 ++++++++-----
1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 7a6aa08..17e79de 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -3403,7 +3403,8 @@ static irqreturn_t nv_nic_irq_tx(int foo, void *data)
for (i = 0;; i++) {
events = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQ_TX_ALL;
- writel(NVREG_IRQ_TX_ALL, base + NvRegMSIXIrqStatus);
+ writel(events, base + NvRegMSIXIrqStatus);
+ netdev_dbg(dev, "%s: tx irq: %08x\n", dev->name, events);
if (!(events & np->irqmask))
break;
@@ -3514,7 +3515,8 @@ static irqreturn_t nv_nic_irq_rx(int foo, void *data)
for (i = 0;; i++) {
events = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQ_RX_ALL;
- writel(NVREG_IRQ_RX_ALL, base + NvRegMSIXIrqStatus);
+ writel(events, base + NvRegMSIXIrqStatus);
+ netdev_dbg(dev, "%s: rx irq: %08x\n", dev->name, events);
if (!(events & np->irqmask))
break;
@@ -3558,7 +3560,8 @@ static irqreturn_t nv_nic_irq_other(int foo, void *data)
for (i = 0;; i++) {
events = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQ_OTHER;
- writel(NVREG_IRQ_OTHER, base + NvRegMSIXIrqStatus);
+ writel(events, base + NvRegMSIXIrqStatus);
+ netdev_dbg(dev, "%s: irq: %08x\n", dev->name, events);
if (!(events & np->irqmask))
break;
@@ -3622,10 +3625,10 @@ static irqreturn_t nv_nic_irq_test(int foo, void *data)
if (!(np->msi_flags & NV_MSI_X_ENABLED)) {
events = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK;
- writel(NVREG_IRQ_TIMER, base + NvRegIrqStatus);
+ writel(events & NVREG_IRQ_TIMER, base + NvRegIrqStatus);
} else {
events = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQSTAT_MASK;
- writel(NVREG_IRQ_TIMER, base + NvRegMSIXIrqStatus);
+ writel(events & NVREG_IRQ_TIMER, base + NvRegMSIXIrqStatus);
}
pci_push(base);
if (!(events & NVREG_IRQ_TIMER))
--
1.7.3.1
From: Mike Ditto <[email protected]>
This adds a few debug messages to indicate whether PCIe interrupts are
signaled with MSI or MSI-X.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 17e79de..2712ddc 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -3745,6 +3745,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
writel(0, base + NvRegMSIXMap0);
writel(0, base + NvRegMSIXMap1);
}
+ netdev_info(dev, "forcedeth: MSI-X enabled\n");
}
}
if (ret != 0 && np->msi_flags & NV_MSI_CAPABLE) {
@@ -3766,6 +3767,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
writel(0, base + NvRegMSIMap1);
/* enable msi vector 0 */
writel(NVREG_MSI_VECTOR_0_ENABLED, base + NvRegMSIIrqMask);
+ netdev_info(dev, "forcedeth: MSI enabled\n");
}
}
if (ret != 0) {
--
1.7.3.1
From: Salman Qazi <[email protected]>
The race was between del_timer_sync and nv_do_stats_poll called through
nv_get_ethtool_stats. To prevent this, we have to introduce mutual
exclusion between nv_get_ethtool_stats and del_timer_sync. Notice
that we don't put the mutual exclusion in nv_do_stats_poll. That's
because doing so would result in a deadlock, since it is a timer
callback and hence already waited for by timer deletion.
Signed-off-by: David Decotigny <[email protected]>
---
drivers/net/forcedeth.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 2712ddc..2121cea 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -3921,6 +3921,10 @@ static void nv_poll_controller(struct net_device *dev)
}
#endif
+/* No locking is needed as long as this is in the timer
+ * callback. However, any other callers must call this
+ * function with np->lock held.
+ */
static void nv_do_stats_poll(unsigned long data)
{
struct net_device *dev = (struct net_device *) data;
@@ -4553,12 +4557,17 @@ static int nv_get_sset_count(struct net_device *dev, int sset)
static void nv_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *estats, u64 *buffer)
{
+ unsigned long flags;
struct fe_priv *np = netdev_priv(dev);
+ spin_lock_irqsave(&np->lock, flags);
+
/* update stats */
nv_do_stats_poll((unsigned long)dev);
memcpy(buffer, &np->estats, nv_get_sset_count(dev, ETH_SS_STATS)*sizeof(u64));
+
+ spin_unlock_irqrestore(&np->lock, flags);
}
static int nv_link_test(struct net_device *dev)
@@ -5176,13 +5185,13 @@ static int nv_close(struct net_device *dev)
spin_lock_irq(&np->lock);
np->in_shutdown = 1;
+ del_timer_sync(&np->stats_poll);
spin_unlock_irq(&np->lock);
nv_napi_disable(dev);
synchronize_irq(np->pci_dev->irq);
del_timer_sync(&np->oom_kick);
del_timer_sync(&np->nic_poll);
- del_timer_sync(&np->stats_poll);
netif_stop_queue(dev);
spin_lock_irq(&np->lock);
--
1.7.3.1
On Wed, 18 May 2011 17:14:36 -0700
David Decotigny <[email protected]> wrote:
> From: Sameer Nanda <[email protected]>
>
> This change publishes a new ethtool stats: tx_timeout that counts the
> number of times the tx_timeout callback was triggered.
>
>
> Signed-off-by: David Decotigny <[email protected]>
Since this is generic, maybe should be done that way not through ethtool
that way tools and administrators don't have to look for something special.
Something like:
--- a/include/linux/netdevice.h 2011-05-18 17:40:15.901691265 -0700
+++ b/include/linux/netdevice.h 2011-05-18 17:56:11.731742792 -0700
@@ -571,6 +571,8 @@ struct netdev_queue {
* please use this field instead of dev->trans_start
*/
unsigned long trans_start;
+
+ unsigned long trans_timeout;
} ____cacheline_aligned_in_smp;
static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)
--- a/net/core/net-sysfs.c 2011-05-18 17:50:54.540403456 -0700
+++ b/net/core/net-sysfs.c 2011-05-18 17:57:47.136747867 -0700
@@ -788,7 +788,6 @@ net_rx_queue_update_kobjects(struct net_
#endif
}
-#ifdef CONFIG_XPS
/*
* netdev_queue sysfs structures and functions.
*/
@@ -834,6 +833,17 @@ static const struct sysfs_ops netdev_que
.store = netdev_queue_attr_store,
};
+static ssize_t show_trans_timeout(struct netdev_queue *queue,
+ struct netdev_queue_attribute *attribute,
+ char *buf)
+{
+ return sprintf(buf, "%lu", queue->trans_timeout);
+}
+
+static struct netdev_queue_attribute queue_trans_timeout =
+ __ATTR(tx_timeout, S_IRUGO, show_trans_timeout, NULL);
+
+#ifdef CONFIG_XPS
static inline unsigned int get_netdev_queue_index(struct netdev_queue *queue)
{
struct net_device *dev = queue->dev;
@@ -1043,9 +1053,13 @@ error:
static struct netdev_queue_attribute xps_cpus_attribute =
__ATTR(xps_cpus, S_IRUGO | S_IWUSR, show_xps_map, store_xps_map);
+#endif /* CONFIG_XPS */
static struct attribute *netdev_queue_default_attrs[] = {
+ &queue_trans_timeout.attr,
+#ifdef CONFIG_XPS
&xps_cpus_attribute.attr,
+#endif
NULL
};
@@ -1125,7 +1139,6 @@ static int netdev_queue_add_kobject(stru
return error;
}
-#endif /* CONFIG_XPS */
int
netdev_queue_update_kobjects(struct net_device *net, int old_num, int new_num)
--- a/net/sched/sch_generic.c 2011-05-18 17:45:07.740756564 -0700
+++ b/net/sched/sch_generic.c 2011-05-18 17:48:18.474761735 -0700
@@ -245,6 +245,7 @@ static void dev_watchdog(unsigned long a
if (netif_tx_queue_stopped(txq) &&
time_after(jiffies, (trans_start +
dev->watchdog_timeo))) {
+ ++txq->trans_timeout;
some_queue_timedout = 1;
break;
}
On Wed, 2011-05-18 at 17:14 -0700, David Decotigny wrote:
> From: Mike Ditto <[email protected]>
>
> This adds a few debug messages to indicate whether PCIe interrupts are
> signaled with MSI or MSI-X.
>
>
> Signed-off-by: David Decotigny <[email protected]>
> ---
> drivers/net/forcedeth.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> index 17e79de..2712ddc 100644
> --- a/drivers/net/forcedeth.c
> +++ b/drivers/net/forcedeth.c
> @@ -3745,6 +3745,7 @@ static int nv_request_irq(struct net_device *dev, int intr_test)
> writel(0, base + NvRegMSIXMap0);
> writel(0, base + NvRegMSIXMap1);
> }
> + netdev_info(dev, "forcedeth: MSI-X enabled\n");
[...]
netdev_info() and similar logging functions already include the driver
name.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
On Wed, 2011-05-18 at 17:14 -0700, David Decotigny wrote:
> From: Mike Ditto <[email protected]>
>
> This is to avoid a race, accidentally acknowledging an interrupt that
> we didn't notice and won't immediately process. This is based solely
> on code inspection; it is not known if there was an actual bug here.
>
>
> Signed-off-by: David Decotigny <[email protected]>
> ---
> drivers/net/forcedeth.c | 13 ++++++++-----
> 1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
> index 7a6aa08..17e79de 100644
> --- a/drivers/net/forcedeth.c
> +++ b/drivers/net/forcedeth.c
> @@ -3403,7 +3403,8 @@ static irqreturn_t nv_nic_irq_tx(int foo, void *data)
>
> for (i = 0;; i++) {
> events = readl(base + NvRegMSIXIrqStatus) & NVREG_IRQ_TX_ALL;
> - writel(NVREG_IRQ_TX_ALL, base + NvRegMSIXIrqStatus);
> + writel(events, base + NvRegMSIXIrqStatus);
> + netdev_dbg(dev, "%s: tx irq: %08x\n", dev->name, events);
[...]
netdev_dbg() and related logging functions already include the device
name, too. :-)
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.