[RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Steven Rostedt]

When I enable early_printk on a kernel, I cut and paste the console=
input and add to earlyprintk parameter. But I notice recently that
ktest has not been detecting triple faults. The way it detects it, is
by seeing the kernel banner "Linux version .." with a different kernel
version pop up. Then I noticed that early printk was no longer working
on my console, which was why ktest was not seeing it.

I bisected it down and it was added to 4.0 with this commit:

commit ea9e9d802902 ("Specify PCI based UART for earlyprintk")

because it converted the simple_strtoul() that converts the baud number
into a kstrtoul(). The problem with this is, I had as my baud rate,
115200n8 (acceptable for console=ttyS0), but because of the "n8", the
kstrtoul() doesn't parse the baud rate and returns an error, which sets
the baud rate to the default 9600. This explains the garbage on my
screen.

Now, earlyprintk= kernel parameter does not say it accepts that format.
Thus, one answer would simply be me changing my kernel parameters to
remove the "n8" since it isn't parsed anyway. But I wonder if other
people run into this, and it seems strange that the two consoles for
serial accepts different input.

I could also extend this to have earlyprintk do something with that
"n8" or whatever it has and have it match the console parsing (which,
BTW, still uses simple_strtoul(), as I guess it has to).

This patch just makes my old kernel parameter parsing work like it use
to. Although, if someone were to use a hex number starting with "0x",
this patch would break it. That could be fixed too (hence the RFC).

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index a62536a1be88..83d375148565 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -17,6 +17,7 @@
 #include <asm/intel-mid.h>
 #include <asm/pgtable.h>
 #include <linux/usb/ehci_def.h>
+#include <linux/ctype.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
 #include <asm/pci_x86.h>
@@ -189,6 +190,15 @@ static __init void early_serial_init(char *s)
 	}
 
 	if (*s) {
+		char *p;
+		/*
+		 * In case the input is like console with text after the baud
+		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
+		 */
+		for (p = s; *p && isdigit(*p); p++)
+			;
+		*p = 0;
+
 		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
 			baud = DEFAULT_BAUD;
 	}

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> When I enable early_printk on a kernel, I cut and paste the console=
> input and add to earlyprintk parameter. But I notice recently that
> ktest has not been detecting triple faults. The way it detects it, is
> by seeing the kernel banner "Linux version .." with a different kernel
> version pop up. Then I noticed that early printk was no longer working
> on my console, which was why ktest was not seeing it.
> 
> I bisected it down and it was added to 4.0 with this commit:
> 
> commit ea9e9d802902 ("Specify PCI based UART for earlyprintk")

Ugh, this commit changed x86 code but was not Cc:-ed to any x86 maintainer, and 
the title was pretty misleading as well...

( That patch should have been split into at least two parts: the generic 
  earlyprintk changes affecting all modes, and the 'pciserial' enablement. Anyway, 
  that's water down the bridge. )

> because it converted the simple_strtoul() that converts the baud number into a 
> kstrtoul(). The problem with this is, I had as my baud rate, 115200n8 
> (acceptable for console=ttyS0), but because of the "n8", the kstrtoul() doesn't 
> parse the baud rate and returns an error, which sets the baud rate to the 
> default 9600. This explains the garbage on my screen.

ugh. I bet it also breaks the earlyprintk=ttyS0..,keep format?

> Now, earlyprintk= kernel parameter does not say it accepts that format. Thus, 
> one answer would simply be me changing my kernel parameters to remove the "n8" 
> since it isn't parsed anyway. But I wonder if other people run into this, and it 
> seems strange that the two consoles for serial accepts different input.

So why not revert to the known-working simple_strtoul()? I don't see this as an 
improvement:

> +		/*
> +		 * In case the input is like console with text after the baud
> +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
> +		 */
> +		for (p = s; *p && isdigit(*p); p++)
> +			;
> +		*p = 0;
> +
>  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
>  			baud = DEFAULT_BAUD;


Over the old:

		baud = simple_strtoul(s, &e, 0);

Thanks,

	Ingo

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Steven Rostedt]

On Sat, 4 Jul 2015 13:03:59 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> So why not revert to the known-working simple_strtoul()? I don't see this as an 
> improvement:
> 
> > +		/*
> > +		 * In case the input is like console with text after the baud
> > +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
> > +		 */
> > +		for (p = s; *p && isdigit(*p); p++)
> > +			;
> > +		*p = 0;
> > +
> >  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
> >  			baud = DEFAULT_BAUD;
> 
> 
> Over the old:
> 
> 		baud = simple_strtoul(s, &e, 0);
> 

That was what I actually did first, but then saw this:

 * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
 * Used as a replacement for the obsolete simple_strtoull. Return code must
 * be checked.

in lib/kstrtox.c and thought that it seems that we are trying to phase
out that function. Personally, I prefer keeping it for instances like
this.

So by all means, put back the simple_strtoul(); I would like that too.

-- Steve

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Steven Rostedt]

On Sat, 4 Jul 2015 13:03:59 +0200
Ingo Molnar <mingo@kernel.org> wrote:


> > +		/*
> > +		 * In case the input is like console with text after the baud
> > +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
> > +		 */
> > +		for (p = s; *p && isdigit(*p); p++)
> > +			;
> > +		*p = 0;
> > +
> >  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
> >  			baud = DEFAULT_BAUD;
> 
>

This was actually one of those cases where I wanted to show that
keeping the old function around is better than the alternative ;-)

If people say we need to phase out simple_strtoull(), then I wanted to
show what kinds of hacks we will have if that happens.

I was hoping that someone would point out that simple_strtoull() is a
better solution. :)

-- Steve

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 4 Jul 2015 13:03:59 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> 
> > > +		/*
> > > +		 * In case the input is like console with text after the baud
> > > +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
> > > +		 */
> > > +		for (p = s; *p && isdigit(*p); p++)
> > > +			;
> > > +		*p = 0;
> > > +
> > >  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
> > >  			baud = DEFAULT_BAUD;
> > 
> >
> 
> This was actually one of those cases where I wanted to show that keeping the old 
> function around is better than the alternative ;-)
> 
> If people say we need to phase out simple_strtoull(), then I wanted to show what 
> kinds of hacks we will have if that happens.
> 
> I was hoping that someone would point out that simple_strtoull() is a better 
> solution. :)

LOL :)

	Ingo

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Peter Hurley]

On 07/04/2015 09:20 AM, Steven Rostedt wrote:
> On Sat, 4 Jul 2015 13:03:59 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> 
>>> +		/*
>>> +		 * In case the input is like console with text after the baud
>>> +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
>>> +		 */
>>> +		for (p = s; *p && isdigit(*p); p++)
>>> +			;
>>> +		*p = 0;
>>> +
>>>  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
>>>  			baud = DEFAULT_BAUD;
>>
>>
> 
> This was actually one of those cases where I wanted to show that
> keeping the old function around is better than the alternative ;-)
> 
> If people say we need to phase out simple_strtoull(), then I wanted to
> show what kinds of hacks we will have if that happens.
> 
> I was hoping that someone would point out that simple_strtoull() is a
> better solution. :)

And made worse by the fact that checkpatch flags simple_strtoul* as
obsolete, so people keep submitting junk like above [1] in an effort to
escape the checkpatch warning.

Which I pointed out to Joe back in Feb. (https://lkml.org/lkml/2015/2/25/217)

Regards,
Peter Hurley

[1] or this recent submission

On 05/26/2015 01:12 PM, Peter Hurley wrote:
> On 05/22/2015 12:06 PM, Bin Gao wrote:
>
>> +{
>> +	char str[4]; /* max 3 chars, plus a NULL terminator */
>> +	char *p = options;
>> +	int i = 0;
>> +
>> +	while (*p) {
>> +		if (i >= 4)
>> +			return -EINVAL;
>> +
>> +		if (*p == delimiter) {
>> +			str[i++] = 0;
>> +			if (endp)
>> +				*endp = p + 1;
>> +			return kstrtou8(str, 10, val); /* decimal, no hex */
>> +		}
>> +
>> +		str[i++] = *p++;
>> +	}
> 
> Is all this to avoid using simple_strtoul()?
> If yes, I'd rather you use simple_strtoul() like the rest of the console
> code and ignore the (misguided) advice that simple_strtoul() is obsolete.

Re: [RFC][PATCH] x86: Allow early_printk to use console style param like 115200n8

[Ingo Molnar]

* Peter Hurley <peter@hurleysoftware.com> wrote:

> On 07/04/2015 09:20 AM, Steven Rostedt wrote:
> > On Sat, 4 Jul 2015 13:03:59 +0200
> > Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > 
> >>> +		/*
> >>> +		 * In case the input is like console with text after the baud
> >>> +		 * rate. e.g. 115200n8. kstrtoul() will error on such input.
> >>> +		 */
> >>> +		for (p = s; *p && isdigit(*p); p++)
> >>> +			;
> >>> +		*p = 0;
> >>> +
> >>>  		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
> >>>  			baud = DEFAULT_BAUD;
> >>
> >>
> > 
> > This was actually one of those cases where I wanted to show that keeping the 
> > old function around is better than the alternative ;-)
> > 
> > If people say we need to phase out simple_strtoull(), then I wanted to show 
> > what kinds of hacks we will have if that happens.
> > 
> > I was hoping that someone would point out that simple_strtoull() is a better 
> > solution. :)
> 
> And made worse by the fact that checkpatch flags simple_strtoul* as obsolete, so 
> people keep submitting junk like above [1] in an effort to escape the checkpatch 
> warning.
> 
> Which I pointed out to Joe back in Feb. (https://lkml.org/lkml/2015/2/25/217)

So this kind of checkpatch-driven crap really needs to stop.

Thanks,

	Ingo

[PATCH] x86: Allow early_printk to use console style param like 115200n8

[Steven Rostedt]

When I enable early_printk on a kernel, I cut and paste the console=
input and add to earlyprintk parameter. But I notice recently that
ktest has not been detecting triple faults. The way it detects it, is
by seeing the kernel banner "Linux version .." with a different kernel
version pop up. Then I noticed that early printk was no longer working
on my console, which was why ktest was not seeing it.

I bisected it down and it was added to 4.0 with this commit:

commit ea9e9d802902 ("Specify PCI based UART for earlyprintk")

because it converted the simple_strtoul() that converts the baud number
into a kstrtoul(). The problem with this is, I had as my baud rate,
115200n8 (acceptable for console=ttyS0), but because of the "n8", the
kstrtoul() doesn't parse the baud rate and returns an error, which sets
the baud rate to the default 9600. This explains the garbage on my
screen.

Now, earlyprintk= kernel parameter does not say it accepts that format.
Thus, one answer would simply be me changing my kernel parameters to
remove the "n8" since it isn't parsed anyway. But I wonder if other
people run into this, and it seems strange that the two consoles for
serial accepts different input.

I could also extend this to have earlyprintk do something with that
"n8" or whatever it has and have it match the console parsing (which,
BTW, still uses simple_strtoul(), as I guess it has to).

This patch just makes my old kernel parameter parsing work like it use
to.

Although, simple_strtoull() is considered obsolete, it is the only
standard string parsing function that parses a number that is attached
to text. Ironically, commit ea9e9d802902 also added several calls to
simple_strtoul()!

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index a62536a1be88..033a2ca06ea8 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -189,7 +189,7 @@ static __init void early_serial_init(char *s)
 	}
 
 	if (*s) {
-		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
+		if ((baud = simple_strtoull(s, &e, 0)) == 0 || s == e)
 			baud = DEFAULT_BAUD;
 	}
 

[tip:x86/urgent] x86/earlyprintk: Allow early_printk() to use console style parameters like '115200n8'

[tip-bot for Steven Rostedt]

Commit-ID:  827a82ff399523a954253dfea401af748640f0f4
Gitweb:     http://git.kernel.org/tip/827a82ff399523a954253dfea401af748640f0f4
Author:     Steven Rostedt <rostedt@goodmis.org>
AuthorDate: Mon, 6 Jul 2015 10:14:34 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 6 Jul 2015 17:33:47 +0200

x86/earlyprintk: Allow early_printk() to use console style parameters like '115200n8'

When I enable early_printk on a kernel, I cut and paste the
console= input and add to earlyprintk parameter. But I notice
recently that ktest has not been detecting triple faults. The
way it detects it, is by seeing the kernel banner "Linux version
.." with a different kernel version pop up. Then I noticed that
early printk was no longer working on my console, which was why
ktest was not seeing it.

I bisected it down and it was added to 4.0 with this commit:

  ea9e9d802902 ("Specify PCI based UART for earlyprintk")

because it converted the simple_strtoul() that converts the baud
number into a kstrtoul(). The problem with this is, I had as my
baud rate, 115200n8 (acceptable for console=ttyS0), but because
of the "n8", the kstrtoul() doesn't parse the baud rate and
returns an error, which sets the baud rate to the default 9600.
This explains the garbage on my screen.

Now, earlyprintk= kernel parameter does not say it accepts that
format. Thus, one answer would simply be me changing my kernel
parameters to remove the "n8" since it isn't parsed anyway. But
I wonder if other people run into this, and it seems strange
that the two consoles for serial accepts different input.

I could also extend this to have earlyprintk do something with
that "n8" or whatever it has and have it match the console
parsing (which, BTW, still uses simple_strtoul(), as I guess it
has to).

This patch just makes my old kernel parameter parsing work like
it use to.

Although, simple_strtoull() is considered obsolete, it is the
only standard string parsing function that parses a number that
is attached to text. Ironically, commit ea9e9d802902 also added
several calls to simple_strtoul()!

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Cohen <david.a.cohen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stuart R. Anderson <stuart.r.anderson@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20150706101434.5f6a351b@gandalf.local.home
[ Cleaned it up a bit. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/early_printk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/early_printk.c b/arch/x86/kernel/early_printk.c
index 89427d8..eec40f5 100644
--- a/arch/x86/kernel/early_printk.c
+++ b/arch/x86/kernel/early_printk.c
@@ -175,7 +175,9 @@ static __init void early_serial_init(char *s)
 	}
 
 	if (*s) {
-		if (kstrtoul(s, 0, &baud) < 0 || baud == 0)
+		baud = simple_strtoull(s, &e, 0);
+
+		if (baud == 0 || s == e)
 			baud = DEFAULT_BAUD;
 	}