[PATCH 1/2] forcedeth: make module parameters readable in /sys/module

[PATCH 1/2] forcedeth: make module parameters readable in /sys/module

[David Decotigny]

This change allows to publish the values of the module parameters in
/sys/module.


Signed-off-by: David Decotigny <decot@google.com>
---
 drivers/net/forcedeth.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 112dc0b..9566567 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -5990,21 +5990,21 @@ static void __exit exit_nic(void)
 	pci_unregister_driver(&driver);
 }
 
-module_param(max_interrupt_work, int, 0);
+module_param(max_interrupt_work, int, S_IRUGO);
 MODULE_PARM_DESC(max_interrupt_work, "forcedeth maximum events handled per interrupt");
-module_param(optimization_mode, int, 0);
+module_param(optimization_mode, int, S_IRUGO);
 MODULE_PARM_DESC(optimization_mode, "In throughput mode (0), every tx & rx packet will generate an interrupt. In CPU mode (1), interrupts are controlled by a timer. In dynamic mode (2), the mode toggles between throughput and CPU mode based on network load.");
-module_param(poll_interval, int, 0);
+module_param(poll_interval, int, S_IRUGO);
 MODULE_PARM_DESC(poll_interval, "Interval determines how frequent timer interrupt is generated by [(time_in_micro_secs * 100) / (2^10)]. Min is 0 and Max is 65535.");
-module_param(msi, int, 0);
+module_param(msi, int, S_IRUGO);
 MODULE_PARM_DESC(msi, "MSI interrupts are enabled by setting to 1 and disabled by setting to 0.");
-module_param(msix, int, 0);
+module_param(msix, int, S_IRUGO);
 MODULE_PARM_DESC(msix, "MSIX interrupts are enabled by setting to 1 and disabled by setting to 0.");
-module_param(dma_64bit, int, 0);
+module_param(dma_64bit, int, S_IRUGO);
 MODULE_PARM_DESC(dma_64bit, "High DMA is enabled by setting to 1 and disabled by setting to 0.");
-module_param(phy_cross, int, 0);
+module_param(phy_cross, int, S_IRUGO);
 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_param(phy_power_down, int, S_IRUGO);
 MODULE_PARM_DESC(phy_power_down, "Power down phy and disable link when interface is down (1), or leave phy powered up (0).");
 
 MODULE_AUTHOR("Manfred Spraul <manfred@colorfullife.com>");
-- 
1.7.3.1

[PATCH 2/2] forcedeth: allow to silence tx_timeout debug messages

[David Decotigny]

From: Sameer Nanda <snanda@google.com>

This change allows to silence most debug messages in case of TX
timeout. These messages don't provide a signare/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 debug messages are not displayed but this can be
overriden by setting the debug_tx_timeout module parameter.


Signed-off-by: David Decotigny <decot@google.com>
---
 drivers/net/forcedeth.c |   93 ++++++++++++++++++++++++++---------------------
 1 files changed, 52 insertions(+), 41 deletions(-)

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 9566567..2c176ff 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,56 +2478,59 @@ 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;
 	else
 		status = readl(base + NvRegIrqStatus) & NVREG_IRQSTAT_MASK;
 
-	netdev_info(dev, "Got tx_timeout. irq: %08x\n", status);
+	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,
-			    "%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 {
+		netdev_warn(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,
-				    "%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, S_IRUGO);
 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, S_IRUGO);
 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, S_IRUGO);
+MODULE_PARM_DESC(debug_tx_timeout,
+		 "Dump tx related registers and ring when tx_timeout happens");
 
 MODULE_AUTHOR("Manfred Spraul <manfred@colorfullife.com>");
 MODULE_DESCRIPTION("Reverse Engineered nForce ethernet driver");
-- 
1.7.3.1

Re: [PATCH 2/2] forcedeth: allow to silence tx_timeout debug messages

[David Miller]

From: David Decotigny <decot@google.com>
Date: Wed, 18 May 2011 14:10:00 -0700

> From: Sameer Nanda <snanda@google.com>
> 
> This change allows to silence most debug messages in case of TX
> timeout. These messages don't provide a signare/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 debug messages are not displayed but this can be
> overriden by setting the debug_tx_timeout module parameter.
> 
> 
> Signed-off-by: David Decotigny <decot@google.com>

I would rather you make the messages less verbose, instead of
having it say absolutely nothing when this happens as it is
a serious problem.

You can add a knob which when enabled gives the old verbosity
back for diagnostic purposes.

Re: [PATCH 2/2] forcedeth: allow to silence tx_timeout debug messages

[David Decotigny]

David,

No problem, I will send the patch series correctly numbered. Sorry for that.

Regarding your comment about this debug info change:

On Wed, May 18, 2011 at 2:16 PM, David Miller <davem@davemloft.net> wrote:
> You can add a knob which when enabled gives the old verbosity
> back for diagnostic purposes.

That was the intent of this patch: it adds a debug_tx_timeout module
parameter to act as the knob. I can rephrase the description of the
patch, it didn't make this so clear.
Or are you suggesting I should implement this with another kind of knob?

Thanks!

Re: [PATCH 2/2] forcedeth: allow to silence tx_timeout debug messages

[David Miller]

From: David Decotigny <decot@google.com>
Date: Wed, 18 May 2011 14:43:58 -0700

> On Wed, May 18, 2011 at 2:16 PM, David Miller <davem@davemloft.net> wrote:
>> You can add a knob which when enabled gives the old verbosity
>> back for diagnostic purposes.
> 
> That was the intent of this patch: it adds a debug_tx_timeout module
> parameter to act as the knob. I can rephrase the description of the
> patch, it didn't make this so clear.
> Or are you suggesting I should implement this with another kind of knob?

I'm saying implement things differently.

Reduce the verbosity of the TX timeout message but still print some
amount of information, but provide a new knob that turns on the
existing verbose messages.

Re: [PATCH 1/2] forcedeth: make module parameters readable in /sys/module

[Stephen Hemminger]

On Wed, 18 May 2011 14:09:59 -0700
David Decotigny <decot@google.com> wrote:

> This change allows to publish the values of the module parameters in
> /sys/module.
> 
>

Although this makes more info for developer, it also means more
stuff in sysfs taking more memory and not providing any real value
that can't be found by looking at the /etc/modprobe.d for any parameters
user might have set.

Re: [PATCH 1/2] forcedeth: make module parameters readable in /sys/module

[David Decotigny]

Hi Stephen,

On Wed, May 18, 2011 at 3:03 PM, Stephen Hemminger
<shemminger@vyatta.com> wrote:
> Although this makes more info for developer, it also means more
> stuff in sysfs taking more memory and not providing any real value

Right. I'll drop this patch for now.

I do agree that this creates unnecessary pressure on some systems. But
on other systems, it is useful to know how all the loaded modules were
actually configured to track down something weird.

Regarding /sys/modules/X/parameters, the only knobs I know of are
CONFIG_SYSFS and the last argument to module_param(). If this is
correct, then it seems tricky to accommodate for both use cases above
without being crude (eg. disable sysfs, edit/sed the sources to change
last argument of module_param() macro, ...). Please correct me if I am
wrong. But if I am not, how about having a new knob allowing to
precisely control the presence/absence of /sys/module (another
CONFIG_*)? That way, the last argument to module_param() can be
interpreted as the permission when /sys/module is supported, and would
be ignored otherwise: module authors could gradually change their
perm=S_IRUGO and family without caring much about memory footprint.
And both use cases above could be supported without having to disable
sysfs altogether or change the sources.

I would be happy to work on this if it makes any sense.

Regards,

Re: [PATCH 1/2] forcedeth: make module parameters readable in /sys/module

[Bill Fink]

On Wed, 18 May 2011, Stephen Hemminger wrote:

> On Wed, 18 May 2011 14:09:59 -0700
> David Decotigny <decot@google.com> wrote:
> 
> > This change allows to publish the values of the module parameters in
> > /sys/module.
> 
> Although this makes more info for developer, it also means more
> stuff in sysfs taking more memory and not providing any real value
> that can't be found by looking at the /etc/modprobe.d for any parameters
> user might have set.

As a user, I find having the module parameter info in
/sys/module/driver/parameters/* extremely useful at times.
In tracking down weird problems I can for example do:

	grep . /sys/module/driver/parameters/*

to get a snapshot of module parameters.  I can then compare
this with other similar systems to see if there are any
differences of note that might be significant (anything
from differences due to kernel versions to just forgetting
some change I made).

I don't see that it's really much extra significant overhead
on the system either.

					-Bill