[PATCH] netpoll: Introduce netpoll_carrier_timeout kernel option

[PATCH] netpoll: Introduce netpoll_carrier_timeout kernel option

[Anton Vorontsov]

Some PHYs require longer timeouts for carrier detection, and
auto-negotiation process may take indefinite amount of time.

It may be inconvenient to force longer timeouts for sane PHYs,
so let's introduce a kernel command line option.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 Documentation/kernel-parameters.txt |    5 +++++
 net/core/netpoll.c                  |   11 ++++++++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index d77fbd8..9eac5bd 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1531,6 +1531,11 @@ and is between 256 and 4096 characters. It is defined in the file
 			symbolic names: lapic and ioapic
 			Example: nmi_watchdog=2 or nmi_watchdog=panic,lapic
 
+	netpoll_carrier_timeout=
+			[NET] Specifies amount of time (in seconds) that
+			netpoll should wait for a carrier. By default netpoll
+			waits 4 seconds.
+
 	no387		[BUGS=X86-32] Tells the kernel to use the 387 maths
 			emulation library even if a 387 maths coprocessor
 			is present.
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9675f31..4371da2 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -50,6 +50,15 @@ static atomic_t trapped;
 static void zap_completion_queue(void);
 static void arp_reply(struct sk_buff *skb);
 
+static unsigned int carrier_timeout = 4;
+
+static int __init carrier_timeout_setup(char *str)
+{
+	carrier_timeout = simple_strtoul(str, NULL, 0);
+	return 1;
+}
+__setup("netpoll_carrier_timeout=", carrier_timeout_setup);
+
 static void queue_process(struct work_struct *work)
 {
 	struct netpoll_info *npinfo =
@@ -732,7 +741,7 @@ int netpoll_setup(struct netpoll *np)
 		}
 
 		atleast = jiffies + HZ/10;
-		atmost = jiffies + 4*HZ;
+		atmost = jiffies + carrier_timeout * HZ;
 		while (!netif_carrier_ok(ndev)) {
 			if (time_after(jiffies, atmost)) {
 				printk(KERN_NOTICE
-- 
1.6.3.3

Re: [PATCH] netpoll: Introduce netpoll_carrier_timeout kernel option

[Stephen Hemminger]

On Wed, 8 Jul 2009 05:00:30 +0400
Anton Vorontsov <avorontsov@ru.mvista.com> wrote:

> Some PHYs require longer timeouts for carrier detection, and
> auto-negotiation process may take indefinite amount of time.
> 
> It may be inconvenient to force longer timeouts for sane PHYs,
> so let's introduce a kernel command line option.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
>  Documentation/kernel-parameters.txt |    5 +++++
>  net/core/netpoll.c                  |   11 ++++++++++-
>  2 files changed, 15 insertions(+), 1 deletions(-)

A sysctl (or module option) is a less awkward interface for something
like this. Kernel command line parameters are ugly step children
loved only by embedded developers.

[PATCH v2] netpoll: Introduce netpoll_carrier_timeout kernel option

[Anton Vorontsov]

Some PHYs require longer timeouts for carrier detection, and
auto-negotiation process may take indefinite amount of time.

It may be inconvenient to force longer timeouts for sane PHYs,
so let's introduce a kernel command line option.

Since we're using module_param(), the option also can be
changed in runtime.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Tue, Jul 07, 2009 at 06:03:54PM -0700, Stephen Hemminger wrote:
> On Wed, 8 Jul 2009 05:00:30 +0400
> Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> 
> > Some PHYs require longer timeouts for carrier detection, and
> > auto-negotiation process may take indefinite amount of time.
> > 
> > It may be inconvenient to force longer timeouts for sane PHYs,
> > so let's introduce a kernel command line option.
> > 
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > ---
> >  Documentation/kernel-parameters.txt |    5 +++++
> >  net/core/netpoll.c                  |   11 ++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletions(-)
> 
> A sysctl (or module option) is a less awkward interface for something
> like this.

Right you are, and it's less code. Wasn't sure if it makes sense
to use module_param() for non-modular code, but afterall it makes
sense indeed, since with it we can change the timeout in runtime.

> Kernel command line parameters are ugly step children
> loved only by embedded developers.

So true! ;-)

How about this patch?

Thanks,

 Documentation/kernel-parameters.txt |    5 +++++
 net/core/netpoll.c                  |    6 +++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index d77fbd8..9347f4a 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1531,6 +1531,11 @@ and is between 256 and 4096 characters. It is defined in the file
 			symbolic names: lapic and ioapic
 			Example: nmi_watchdog=2 or nmi_watchdog=panic,lapic
 
+	netpoll.carrier_timeout=
+			[NET] Specifies amount of time (in seconds) that
+			netpoll should wait for a carrier. By default netpoll
+			waits 4 seconds.
+
 	no387		[BUGS=X86-32] Tells the kernel to use the 387 maths
 			emulation library even if a 387 maths coprocessor
 			is present.
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9675f31..3afe381 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2002  Red Hat, Inc.
  */
 
+#include <linux/moduleparam.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
 #include <linux/string.h>
@@ -50,6 +51,9 @@ static atomic_t trapped;
 static void zap_completion_queue(void);
 static void arp_reply(struct sk_buff *skb);
 
+static unsigned int carrier_timeout = 4;
+module_param(carrier_timeout, uint, 0644);
+
 static void queue_process(struct work_struct *work)
 {
 	struct netpoll_info *npinfo =
@@ -732,7 +736,7 @@ int netpoll_setup(struct netpoll *np)
 		}
 
 		atleast = jiffies + HZ/10;
-		atmost = jiffies + 4*HZ;
+		atmost = jiffies + carrier_timeout * HZ;
 		while (!netif_carrier_ok(ndev)) {
 			if (time_after(jiffies, atmost)) {
 				printk(KERN_NOTICE
-- 
1.6.3.3

Re: [PATCH v2] netpoll: Introduce netpoll_carrier_timeout kernel option

[Neil Horman]

On Wed, Jul 08, 2009 at 05:30:11AM +0400, Anton Vorontsov wrote:
> Some PHYs require longer timeouts for carrier detection, and
> auto-negotiation process may take indefinite amount of time.
> 
> It may be inconvenient to force longer timeouts for sane PHYs,
> so let's introduce a kernel command line option.
> 
> Since we're using module_param(), the option also can be
> changed in runtime.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> On Tue, Jul 07, 2009 at 06:03:54PM -0700, Stephen Hemminger wrote:
> > On Wed, 8 Jul 2009 05:00:30 +0400
> > Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > 
> > > Some PHYs require longer timeouts for carrier detection, and
> > > auto-negotiation process may take indefinite amount of time.
> > > 
> > > It may be inconvenient to force longer timeouts for sane PHYs,
> > > so let's introduce a kernel command line option.
> > > 
> > > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> > > ---
> > >  Documentation/kernel-parameters.txt |    5 +++++
> > >  net/core/netpoll.c                  |   11 ++++++++++-
> > >  2 files changed, 15 insertions(+), 1 deletions(-)
> > 
> > A sysctl (or module option) is a less awkward interface for something
> > like this.
> 
> Right you are, and it's less code. Wasn't sure if it makes sense
> to use module_param() for non-modular code, but afterall it makes
> sense indeed, since with it we can change the timeout in runtime.
> 
> > Kernel command line parameters are ugly step children
> > loved only by embedded developers.
> 
> So true! ;-)
> 
> How about this patch?
> 
> Thanks,
> 
>  Documentation/kernel-parameters.txt |    5 +++++
>  net/core/netpoll.c                  |    6 +++++-
>  2 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index d77fbd8..9347f4a 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1531,6 +1531,11 @@ and is between 256 and 4096 characters. It is defined in the file
>  			symbolic names: lapic and ioapic
>  			Example: nmi_watchdog=2 or nmi_watchdog=panic,lapic
>  
> +	netpoll.carrier_timeout=
> +			[NET] Specifies amount of time (in seconds) that
> +			netpoll should wait for a carrier. By default netpoll
> +			waits 4 seconds.
> +
I'm not sure the documentation still belongs in kernel-parameters.txt if you
make this a module options, but thats just a nit.



>  
> +#include <linux/moduleparam.h>
>  #include <linux/netdevice.h>
>  #include <linux/etherdevice.h>
>  #include <linux/string.h>
> @@ -50,6 +51,9 @@ static atomic_t trapped;
>  static void zap_completion_queue(void);
>  static void arp_reply(struct sk_buff *skb);
>  
> +static unsigned int carrier_timeout = 4;
> +module_param(carrier_timeout, uint, 0644);
> +
>  static void queue_process(struct work_struct *work)
>  {
>  	struct netpoll_info *npinfo =
> @@ -732,7 +736,7 @@ int netpoll_setup(struct netpoll *np)
>  		}
>  
>  		atleast = jiffies + HZ/10;
> -		atmost = jiffies + 4*HZ;
> +		atmost = jiffies + carrier_timeout * HZ;
>  		while (!netif_carrier_ok(ndev)) {
>  			if (time_after(jiffies, atmost)) {
>  				printk(KERN_NOTICE
> -- 
> 1.6.3.3
> 
I don't mind this functionality at all, but I'm looking at the code, and I have
a hard time understanding why we bring up an interface here at all.  I get that
we might want early netpoll access for netconsole or something like that, but
looking at the console code I don't see where we buffer anything other than the
standard dmesg log.  I don't see much reason why we can't just let normal early
interface initalization from an initramfs bring up an interface like it normally
does.

Neil

> --
> 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
> 

Re: [PATCH v2] netpoll: Introduce netpoll_carrier_timeout kernel option

[Anton Vorontsov]

On Wed, Jul 08, 2009 at 06:59:02AM -0400, Neil Horman wrote:
[...]
> > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> > index d77fbd8..9347f4a 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -1531,6 +1531,11 @@ and is between 256 and 4096 characters. It is defined in the file
> >  			symbolic names: lapic and ioapic
> >  			Example: nmi_watchdog=2 or nmi_watchdog=panic,lapic
> >  
> > +	netpoll.carrier_timeout=
> > +			[NET] Specifies amount of time (in seconds) that
> > +			netpoll should wait for a carrier. By default netpoll
> > +			waits 4 seconds.
> > +
> I'm not sure the documentation still belongs in kernel-parameters.txt if you
> make this a module options, but thats just a nit.

There are other module options described in the
kernel-parameters.txt. Plus it's really a kernel parameter. ;-)

> >  
> > +#include <linux/moduleparam.h>
> >  #include <linux/netdevice.h>
> >  #include <linux/etherdevice.h>
> >  #include <linux/string.h>
> > @@ -50,6 +51,9 @@ static atomic_t trapped;
> >  static void zap_completion_queue(void);
> >  static void arp_reply(struct sk_buff *skb);
> >  
> > +static unsigned int carrier_timeout = 4;
> > +module_param(carrier_timeout, uint, 0644);
> > +
> >  static void queue_process(struct work_struct *work)
> >  {
> >  	struct netpoll_info *npinfo =
> > @@ -732,7 +736,7 @@ int netpoll_setup(struct netpoll *np)
> >  		}
> >  
> >  		atleast = jiffies + HZ/10;
> > -		atmost = jiffies + 4*HZ;
> > +		atmost = jiffies + carrier_timeout * HZ;
> >  		while (!netif_carrier_ok(ndev)) {
> >  			if (time_after(jiffies, atmost)) {
> >  				printk(KERN_NOTICE
> > -- 
> > 1.6.3.3
> > 
> I don't mind this functionality at all, but I'm looking at the code, and I have
> a hard time understanding why we bring up an interface here at all.  I get that
> we might want early netpoll access for netconsole or something like that, but
> looking at the console code I don't see where we buffer anything other than the
> standard dmesg log.  I don't see much reason why we can't just let normal early
> interface initalization from an initramfs bring up an interface like it normally
> does.

The earlier you bring the interface up, the earlier you'll able
to catch kernel oopses. netconsole is quite useful when
you don't have serial ports. The same applies for KGDBoE --
you might want to start debugging the kernel as soon as possible.

Sure, netconsole starts at module_init(), but looking at my dmesg,
netconsole initialization starts closer to the top of the log, so
it'll catch > 60% of oopses, i.e. all oopses in subsystems
that are below net/ in drivers/Makefile.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH v2] netpoll: Introduce netpoll_carrier_timeout kernel option

[David Miller]

From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Wed, 8 Jul 2009 05:30:11 +0400

> Some PHYs require longer timeouts for carrier detection, and
> auto-negotiation process may take indefinite amount of time.
> 
> It may be inconvenient to force longer timeouts for sane PHYs,
> so let's introduce a kernel command line option.
> 
> Since we're using module_param(), the option also can be
> changed in runtime.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Seems reasonable, applied to net-next-2.6, thanks.