[PATCH] powerpc/lpar - defer prefered console setup

[PATCH] powerpc/lpar - defer prefered console setup

[Bastian Blank]

Hi

The powerpc lpar code adds a prefered console at a very early state,
during arch_setup. This runs even before the console setup from the
command line and takes preference.

This patch moves the prefered console setup into an arch_initcall which
runs later and allows the specification of the correct console on the
command line. The udbg console remains as boot console.

There is a different problem that the code does not pick up the correct
console because it uses a part (4) of the lpar device number (30000004)
instead of the correct index 1.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 9235c46..626290d 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -57,6 +57,7 @@ extern void pSeries_find_serial_port(void);
 
 
 int vtermno;	/* virtual terminal# for udbg  */
+static char *console_name;
 
 #define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
 static void udbg_hvsi_putc(char c)
@@ -232,18 +233,24 @@ void __init find_udbg_vterm(void)
 		udbg_putc = udbg_putcLP;
 		udbg_getc = udbg_getcLP;
 		udbg_getc_poll = udbg_getc_pollLP;
-		add_preferred_console("hvc", termno[0] & 0xff, NULL);
+		console_name = "hvc";
 	} else if (of_device_is_compatible(stdout_node, "hvterm-protocol")) {
-		vtermno = termno[0];
 		udbg_putc = udbg_hvsi_putc;
 		udbg_getc = udbg_hvsi_getc;
 		udbg_getc_poll = udbg_hvsi_getc_poll;
-		add_preferred_console("hvsi", termno[0] & 0xff, NULL);
+		console_name = "hvsi";
 	}
 out:
 	of_node_put(stdout_node);
 }
 
+static void __init enable_vterm(void)
+{
+	if (console_name)
+		add_preferred_console(console_name, vtermno, NULL);
+}
+arch_initcall(enable_vterm);
+
 void vpa_init(int cpu)
 {
 	int hwcpu = get_hard_smp_processor_id(cpu);

-- 
Genius doesn't work on an assembly line basis.  You can't simply say,
"Today I will be brilliant."
		-- Kirk, "The Ultimate Computer", stardate 4731.3

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Benjamin Herrenschmidt]

On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> Hi
> 
> The powerpc lpar code adds a prefered console at a very early state,
> during arch_setup. This runs even before the console setup from the
> command line and takes preference.
> 
> This patch moves the prefered console setup into an arch_initcall which
> runs later and allows the specification of the correct console on the
> command line. The udbg console remains as boot console.
> 
> There is a different problem that the code does not pick up the correct
> console because it uses a part (4) of the lpar device number (30000004)
> instead of the correct index 1.
> 
> Signed-off-by: Bastian Blank <waldi@debian.org>

Shouldn't it be a console_initcall() ? 

Now, regarding the "different problem" I think it's even worse than
that, looking at the code there's some non sensical things in here...

add_preferred_console() argument is what gets compared to
console->index, right ? Now if you look at hvc_instantiate(),
it compares each "index" argument passed in to hvc_con_driver.index,
and that index argument passed from hvc_find_vtys() has strictly
nothing to do with the vtermno, it's purely the index of the node
found in order...

So I think the whole stuff is non-sensical and needs fixing. 

Cheers,
Ben.

> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index 9235c46..626290d 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -57,6 +57,7 @@ extern void pSeries_find_serial_port(void);
>  
> 
>  int vtermno;	/* virtual terminal# for udbg  */
> +static char *console_name;
>  
>  #define __ALIGNED__ __attribute__((__aligned__(sizeof(long))))
>  static void udbg_hvsi_putc(char c)
> @@ -232,18 +233,24 @@ void __init find_udbg_vterm(void)
>  		udbg_putc = udbg_putcLP;
>  		udbg_getc = udbg_getcLP;
>  		udbg_getc_poll = udbg_getc_pollLP;
> -		add_preferred_console("hvc", termno[0] & 0xff, NULL);
> +		console_name = "hvc";
>  	} else if (of_device_is_compatible(stdout_node, "hvterm-protocol")) {
> -		vtermno = termno[0];
>  		udbg_putc = udbg_hvsi_putc;
>  		udbg_getc = udbg_hvsi_getc;
>  		udbg_getc_poll = udbg_hvsi_getc_poll;
> -		add_preferred_console("hvsi", termno[0] & 0xff, NULL);
> +		console_name = "hvsi";
>  	}
>  out:
>  	of_node_put(stdout_node);
>  }
>  
> +static void __init enable_vterm(void)
> +{
> +	if (console_name)
> +		add_preferred_console(console_name, vtermno, NULL);
> +}
> +arch_initcall(enable_vterm);
> +
>  void vpa_init(int cpu)
>  {
>  	int hwcpu = get_hard_smp_processor_id(cpu);
> 

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Bastian Blank]

On Tue, Jul 29, 2008 at 01:06:18PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> > Hi
> > 
> > The powerpc lpar code adds a prefered console at a very early state,
> > during arch_setup. This runs even before the console setup from the
> > command line and takes preference.
> > 
> > This patch moves the prefered console setup into an arch_initcall which
> > runs later and allows the specification of the correct console on the
> > command line. The udbg console remains as boot console.
> > 
> Shouldn't it be a console_initcall() ? 

No. console_initcall is for the initial console setup and runs way long
before the command line setup. It needs to run after that.

> > There is a different problem that the code does not pick up the correct
> > console because it uses a part (4) of the lpar device number (30000004)
> > instead of the correct index 1.
> Now, regarding the "different problem" I think it's even worse than
> that, looking at the code there's some non sensical things in here...
> 
> add_preferred_console() argument is what gets compared to
> console->index, right ? Now if you look at hvc_instantiate(),
> it compares each "index" argument passed in to hvc_con_driver.index,
> and that index argument passed from hvc_find_vtys() has strictly
> nothing to do with the vtermno, it's purely the index of the node
> found in order...
> 
> So I think the whole stuff is non-sensical and needs fixing. 

Yep. Would it be a solution to check this in hvc_vio and hvsi and do the
calls there? They now that the device is available and the correct
index. But even then it have to run after the command line. (Or the
console infrastructure fixed to support more then one device of a type.)

Bastian

-- 
Peace was the way.
		-- Kirk, "The City on the Edge of Forever", stardate unknown

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Benjamin Herrenschmidt]

On Tue, 2008-07-29 at 09:36 +0200, Bastian Blank wrote:
> On Tue, Jul 29, 2008 at 01:06:18PM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> > > Hi
> > > 
> > > The powerpc lpar code adds a prefered console at a very early state,
> > > during arch_setup. This runs even before the console setup from the
> > > command line and takes preference.
> > > 
> > > This patch moves the prefered console setup into an arch_initcall which
> > > runs later and allows the specification of the correct console on the
> > > command line. The udbg console remains as boot console.
> > > 
> > Shouldn't it be a console_initcall() ? 
> 
> No. console_initcall is for the initial console setup and runs way long
> before the command line setup. It needs to run after that.

Hrm... we do most of the console discovery from console_initcall for
legacy UARTs. see the code in legacy_serial.c, we just avoid doing
the add_preferred_console() thingy if there's a console= on the command
line :-)

> Yep. Would it be a solution to check this in hvc_vio and hvsi and do the
> calls there? They now that the device is available and the correct
> index. But even then it have to run after the command line. (Or the
> console infrastructure fixed to support more then one device of a type.)

Not sure. The console selection process is just plain weird and I never
really took the time to fully figure it out.

Ben.

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Bastian Blank]

On Tue, Jul 29, 2008 at 05:43:24PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2008-07-29 at 09:36 +0200, Bastian Blank wrote:
> > On Tue, Jul 29, 2008 at 01:06:18PM +1000, Benjamin Herrenschmidt wrote:
> > > On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> > > > Hi
> > > > 
> > > > The powerpc lpar code adds a prefered console at a very early state,
> > > > during arch_setup. This runs even before the console setup from the
> > > > command line and takes preference.
> > > > 
> > > > This patch moves the prefered console setup into an arch_initcall which
> > > > runs later and allows the specification of the correct console on the
> > > > command line. The udbg console remains as boot console.
> > > > 
> > > Shouldn't it be a console_initcall() ? 
> > 
> > No. console_initcall is for the initial console setup and runs way long
> > before the command line setup. It needs to run after that.
> 
> Hrm... we do most of the console discovery from console_initcall for
> legacy UARTs. see the code in legacy_serial.c, we just avoid doing
> the add_preferred_console() thingy if there's a console= on the command
> line :-)

The code did that, but the check was removed because it trips on
netconsole=. See 5faae2e5d1f53df9dce482032c8486bc3a1feffc.

Bastian

-- 
The heart is not a logical organ.
		-- Dr. Janet Wallace, "The Deadly Years", stardate 3479.4

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Benjamin Herrenschmidt]

On Tue, 2008-07-29 at 10:07 +0200, Bastian Blank wrote:
> > Hrm... we do most of the console discovery from console_initcall for
> > legacy UARTs. see the code in legacy_serial.c, we just avoid doing
> > the add_preferred_console() thingy if there's a console= on the
> command
> > line :-)
> 
> The code did that, but the check was removed because it trips on
> netconsole=. See 5faae2e5d1f53df9dce482032c8486bc3a1feffc.

Argh. That's a mess. Well, legacy_serial still does that :-)

Ben.

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 990 bytes --]

On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> Hi

Hi Bastian,

> The powerpc lpar code adds a prefered console at a very early state,
> during arch_setup. This runs even before the console setup from the
> command line and takes preference.

It runs before the command line parsing, and so /does not/ take
preference. I thought.

/**
 * add_preferred_console - add a device to the list of preferred consoles.
 ...
 * The last preferred console added will be used for kernel messages
 * and stdin/out/err for init. 

The last console will be added by the console= parsing, and so that will
be used. The console we add in the pseries setup is only used if nothing
is specified on the command line.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Bastian Blank]

On Wed, Jul 30, 2008 at 12:34:51PM +1000, Michael Ellerman wrote:
> On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
>  * add_preferred_console - add a device to the list of preferred consoles.
>  ...
>  * The last preferred console added will be used for kernel messages
>  * and stdin/out/err for init. 
> 
> The last console will be added by the console= parsing, and so that will
> be used. The console we add in the pseries setup is only used if nothing
> is specified on the command line.

Okay, then there is a completely different problem. In the case of
"console=hvc0 console=hvc1" it uses the _first_ add stdin/out bla and
ignores the second one completely.

Bastian

-- 
Four thousand throats may be cut in one night by a running man.
		-- Klingon Soldier, "Day of the Dove", stardate unknown

Re: [PATCH] powerpc/lpar - defer prefered console setup

[Bastian Blank]

On Wed, Jul 30, 2008 at 08:23:13AM +0200, Bastian Blank wrote:
> On Wed, Jul 30, 2008 at 12:34:51PM +1000, Michael Ellerman wrote:
> > On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> >  * add_preferred_console - add a device to the list of preferred consoles.
> >  ...
> >  * The last preferred console added will be used for kernel messages
> >  * and stdin/out/err for init. 
> > 
> > The last console will be added by the console= parsing, and so that will
> > be used. The console we add in the pseries setup is only used if nothing
> > is specified on the command line.
> 
> Okay, then there is a completely different problem. In the case of
> "console=hvc0 console=hvc1" it uses the _first_ add stdin/out bla and
> ignores the second one completely.

Okay, so hvc_console is the culprit. It don't register a preferred
console if it knows it is not the first in the list.

Bastian

-- 
Intuition, however illogical, is recognized as a command prerogative.
		-- Kirk, "Obsession", stardate 3620.7

[PATCH] hvc - register all available consoles (was: Re: [PATCH] powerpc/lpar - defer prefered console setup)

[Bastian Blank]

On Wed, Jul 30, 2008 at 08:29:19AM +0200, Bastian Blank wrote:
> Okay, so hvc_console is the culprit. It don't register a preferred
> console if it knows it is not the first in the list.

The patch registers all available hvc consoles. It adds one "struct
console" for all possible hvc consoles.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index 44160d5..143a4b2 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -137,15 +137,36 @@ static struct hvc_struct *hvc_get_by_index(int index)
 }
 
 
+static void hvc_console_print(struct console *co, const char *b,
+			      unsigned count);
+static struct tty_driver *hvc_console_device(struct console *c, int *index);
+static int __init hvc_console_setup(struct console *co, char *options);
+
 /*
  * Initial console vtermnos for console API usage prior to full console
  * initialization.  Any vty adapter outside this range will not have usable
  * console interfaces but can still be used as a tty device.  This has to be
  * static because kmalloc will not work during early console init.
  */
-static struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
-static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
-	{[0 ... MAX_NR_HVC_CONSOLES - 1] = -1};
+struct hvc_console
+{
+	uint32_t vtermno;
+	struct hv_ops *ops;
+	struct console console;
+};
+static struct hvc_console consoles[MAX_NR_HVC_CONSOLES] = {
+	[0 ... MAX_NR_HVC_CONSOLES - 1] = {
+		.vtermno = -1,
+		.console = {
+			.name		= "hvc",
+			.write		= hvc_console_print,
+			.device		= hvc_console_device,
+			.setup		= hvc_console_setup,
+			.flags		= CON_PRINTBUFFER,
+			.index		= -1,
+		},
+	}
+};
 
 /*
  * Console APIs, NOT TTY.  These APIs are available immediately when
@@ -164,7 +185,7 @@ static void hvc_console_print(struct console *co, const char *b,
 		return;
 
 	/* This console adapter was removed so it is not usable. */
-	if (vtermnos[index] < 0)
+	if (consoles[index].vtermno < 0)
 		return;
 
 	while (count > 0 || i > 0) {
@@ -178,7 +199,7 @@ static void hvc_console_print(struct console *co, const char *b,
 				--count;
 			}
 		} else {
-			r = cons_ops[index]->put_chars(vtermnos[index], c, i);
+			r = consoles[index].ops->put_chars(consoles[index].vtermno, c, i);
 			if (r < 0) {
 				/* throw away chars on error */
 				i = 0;
@@ -193,7 +214,7 @@ static void hvc_console_print(struct console *co, const char *b,
 
 static struct tty_driver *hvc_console_device(struct console *c, int *index)
 {
-	if (vtermnos[c->index] == -1)
+	if (consoles[c->index].vtermno == -1)
 		return NULL;
 
 	*index = c->index;
@@ -205,43 +226,12 @@ static int __init hvc_console_setup(struct console *co, char *options)
 	if (co->index < 0 || co->index >= MAX_NR_HVC_CONSOLES)
 		return -ENODEV;
 
-	if (vtermnos[co->index] == -1)
+	if (consoles[co->index].vtermno == -1)
 		return -ENODEV;
 
 	return 0;
 }
 
-static struct console hvc_con_driver = {
-	.name		= "hvc",
-	.write		= hvc_console_print,
-	.device		= hvc_console_device,
-	.setup		= hvc_console_setup,
-	.flags		= CON_PRINTBUFFER,
-	.index		= -1,
-};
-
-/*
- * Early console initialization.  Precedes driver initialization.
- *
- * (1) we are first, and the user specified another driver
- * -- index will remain -1
- * (2) we are first and the user specified no driver
- * -- index will be set to 0, then we will fail setup.
- * (3)  we are first and the user specified our driver
- * -- index will be set to user specified driver, and we will fail
- * (4) we are after driver, and this initcall will register us
- * -- if the user didn't specify a driver then the console will match
- *
- * Note that for cases 2 and 3, we will match later when the io driver
- * calls hvc_instantiate() and call register again.
- */
-static int __init hvc_console_init(void)
-{
-	register_console(&hvc_con_driver);
-	return 0;
-}
-console_initcall(hvc_console_init);
-
 /* callback when the kboject ref count reaches zero. */
 static void destroy_hvc_struct(struct kref *kref)
 {
@@ -267,12 +257,13 @@ static void destroy_hvc_struct(struct kref *kref)
  */
 int hvc_instantiate(uint32_t vtermno, int index, struct hv_ops *ops)
 {
+	struct hvc_console *hc;
 	struct hvc_struct *hp;
 
 	if (index < 0 || index >= MAX_NR_HVC_CONSOLES)
 		return -1;
 
-	if (vtermnos[index] != -1)
+	if (consoles[index].vtermno != -1)
 		return -1;
 
 	/* make sure no no tty has been registered in this index */
@@ -282,19 +273,17 @@ int hvc_instantiate(uint32_t vtermno, int index, struct hv_ops *ops)
 		return -1;
 	}
 
-	vtermnos[index] = vtermno;
-	cons_ops[index] = ops;
+	hc = &consoles[index];
+
+	hc->vtermno = vtermno;
+	hc->ops = ops;
+	hc->console.index = index;
 
 	/* reserve all indices up to and including this index */
 	if (last_hvc < index)
 		last_hvc = index;
 
-	/* if this index is what the user requested, then register
-	 * now (setup won't fail at this point).  It's ok to just
-	 * call register again if previously .setup failed.
-	 */
-	if (index == hvc_con_driver.index)
-		register_console(&hvc_con_driver);
+	register_console(&hc->console);
 
 	return 0;
 }
@@ -637,7 +626,7 @@ static int hvc_poll(struct hvc_struct *hp)
 		}
 		for (i = 0; i < n; ++i) {
 #ifdef CONFIG_MAGIC_SYSRQ
-			if (hp->index == hvc_con_driver.index) {
+			if (consoles[hp->index].console.flags & CON_CONSDEV) {
 				/* Handle the SysRq Hack */
 				/* XXX should support a sequence */
 				if (buf[i] == '\x0f') {	/* ^O */
@@ -775,8 +764,8 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int irq,
 	 * see if this vterm id matches one registered for console.
 	 */
 	for (i=0; i < MAX_NR_HVC_CONSOLES; i++)
-		if (vtermnos[i] == hp->vtermno &&
-		    cons_ops[i] == hp->ops)
+		if (consoles[i].vtermno == hp->vtermno &&
+		    consoles[i].ops == hp->ops)
 			break;
 
 	/* no matching slot, just use a counter */
@@ -800,7 +789,7 @@ int __devexit hvc_remove(struct hvc_struct *hp)
 	tty = hp->tty;
 
 	if (hp->index < MAX_NR_HVC_CONSOLES)
-		vtermnos[hp->index] = -1;
+		consoles[hp->index].vtermno = -1;
 
 	/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
 
@@ -881,13 +870,16 @@ out:
  */
 static void __exit hvc_exit(void)
 {
+	int i;
+
 	if (hvc_driver) {
 		kthread_stop(hvc_task);
 
 		tty_unregister_driver(hvc_driver);
 		/* return tty_struct instances allocated in hvc_init(). */
 		put_tty_driver(hvc_driver);
-		unregister_console(&hvc_con_driver);
+		for (i = 0; i < MAX_NR_HVC_CONSOLES; i++)
+			unregister_console(&consoles->console);
 	}
 }
 module_exit(hvc_exit);

-- 
There is an order of things in this universe.
		-- Apollo, "Who Mourns for Adonais?" stardate 3468.1

Re: [PATCH] hvc - register all available consoles (was: Re: [PATCH] powerpc/lpar - defer prefered console setup)

[Milton Miller]

On Wed Jul 30 at 17:34:38 EST in 2008, Bastian Blank wrote:
> On Wed, Jul 30, 2008 at 08:29:19AM +0200, Bastian Blank wrote:
>> Okay, so hvc_console is the culprit. It don't register a preferred
>> console if it knows it is not the first in the list.
>
> The patch registers all available hvc consoles. It adds one "struct
> console" for all possible hvc consoles.

[ a 6 page patch adding forward declartions, arrays of console
structs, moving lots of code and creating N struct console in the
hvc_driver shell]

After previously having written:
> On Wed, Jul 30, 2008 at 08:23:13AM +0200, Bastian Blank wrote:
>> On Wed, Jul 30, 2008 at 12:34:51PM +1000, Michael Ellerman wrote:
>>> On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
>>>  * add_preferred_console - add a device to the list of preferred consoles.
>>>  ...
>>>  * The last preferred console added will be used for kernel messages
>>>  * and stdin/out/err for init. 
>>> 
>>> The last console will be added by the console= parsing, and so that will
>>> be used. The console we add in the pseries setup is only used if nothing
>>> is specified on the command line.
>> 
>> Okay, then there is a completely different problem. In the case of
>> "console=hvc0 console=hvc1" it uses the _first_ add stdin/out bla and
>> ignores the second one completely.
> 
> Okay, so hvc_console is the culprit. It don't register a preferred
> console if it knows it is not the first in the list.
> 
> Bastian


[ back to the patch ]
> -/*
> - * Early console initialization.  Precedes driver initialization.
> - *
> - * (1) we are first, and the user specified another driver
> - * -- index will remain -1
> - * (2) we are first and the user specified no driver
> - * -- index will be set to 0, then we will fail setup.
> - * (3)  we are first and the user specified our driver
> - * -- index will be set to user specified driver, and we will fail
> - * (4) we are after driver, and this initcall will register us
> - * -- if the user didn't specify a driver then the console will match
> - *
> - * Note that for cases 2 and 3, we will match later when the io driver
> - * calls hvc_instantiate() and call register again.
> - */
> -static int __init hvc_console_init(void)
> -{
> -	register_console(&hvc_con_driver);
> -	return 0;
> 


Please explain how the reasoning you removed breaks down.

What is your usage case?


More importantly , how is this different than, say, drivers/serial/8250.c,
which also registers just one struct console?

would not console=ttyS0 console=ttyS1 have the same problem?


Please instrument the calls to register_console and add_preferred_console
and give a detailed description of the problem you are encountering.

Perhaps the real fix should be scan the command line for console= at
console_init time?   How does that compare to __setup that is called now?



>  		for (i = 0; i < n; ++i) {
>  #ifdef CONFIG_MAGIC_SYSRQ
> -			if (hp->index == hvc_con_driver.index) {
> +			if (consoles[hp->index].console.flags & CON_CONSDEV) {
>  				/* Handle the SysRq Hack */
>  				/* XXX should support a sequence */
>  				if (buf[i] == '\x0f') {	/* ^O */
> @@ -775,8 +764,8 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int irq,
>  	 * see if this vterm id matches one registered for console.
>  	 */
>  	for (i=0; i < MAX_NR_HVC_CONSOLES; i++)
> -		if (vtermnos[i] == hp->vtermno &&
> -		    cons_ops[i] == hp->ops)
> +		if (consoles[i].vtermno == hp->vtermno &&
> +		    consoles[i].ops == hp->ops)
>  			break;
>  


NACK
you broke this assertion:
>  /*
>   * Initial console vtermnos for console API usage prior to full console
>   * initialization.  Any vty adapter outside this range will not have usable
>   * console interfaces but can still be used as a tty device.  This has to be
>   * static because kmalloc will not work during early console init.
>   */


The idea is you might want serial port to 250 other partitions, but only
need to have your choice of console be on the first 8 or so.

milton

Re: [PATCH] hvc - register all available consoles (was: Re: [PATCH] powerpc/lpar - defer prefered console setup)

[Bastian Blank]

On Wed, Jul 30, 2008 at 04:13:47AM -0500, Milton Miller wrote:
> On Wed Jul 30 at 17:34:38 EST in 2008, Bastian Blank wrote:
> > On Wed, Jul 30, 2008 at 08:29:19AM +0200, Bastian Blank wrote:
> >> Okay, so hvc_console is the culprit. It don't register a preferred
> >> console if it knows it is not the first in the list.
> >
> > The patch registers all available hvc consoles. It adds one "struct
> > console" for all possible hvc consoles.
> 
> [ a 6 page patch adding forward declartions, arrays of console
> structs, moving lots of code and creating N struct console in the
> hvc_driver shell]
> 
> After previously having written:
> > On Wed, Jul 30, 2008 at 08:23:13AM +0200, Bastian Blank wrote:
> >> On Wed, Jul 30, 2008 at 12:34:51PM +1000, Michael Ellerman wrote:
> >>> On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
> >>>  * add_preferred_console - add a device to the list of preferred consoles.
> >>>  ...
> >>>  * The last preferred console added will be used for kernel messages
> >>>  * and stdin/out/err for init. 
> >>> 
> >>> The last console will be added by the console= parsing, and so that will
> >>> be used. The console we add in the pseries setup is only used if nothing
> >>> is specified on the command line.
> >> 
> >> Okay, then there is a completely different problem. In the case of
> >> "console=hvc0 console=hvc1" it uses the _first_ add stdin/out bla and
> >> ignores the second one completely.
> > 
> > Okay, so hvc_console is the culprit. It don't register a preferred
> > console if it knows it is not the first in the list.
> > 
> > Bastian
> 
> 
> [ back to the patch ]
> > -/*
> > - * Early console initialization.  Precedes driver initialization.
> > - *
> > - * (1) we are first, and the user specified another driver
> > - * -- index will remain -1
> > - * (2) we are first and the user specified no driver
> > - * -- index will be set to 0, then we will fail setup.
> > - * (3)  we are first and the user specified our driver
> > - * -- index will be set to user specified driver, and we will fail
> > - * (4) we are after driver, and this initcall will register us
> > - * -- if the user didn't specify a driver then the console will match
> > - *
> > - * Note that for cases 2 and 3, we will match later when the io driver
> > - * calls hvc_instantiate() and call register again.
> > - */
> > -static int __init hvc_console_init(void)
> > -{
> > -	register_console(&hvc_con_driver);
> > -	return 0;
> > 
> 
> 
> Please explain how the reasoning you removed breaks down.
> What is your usage case?

I have several hvc consoles on a Power hypervisor.

> More importantly , how is this different than, say, drivers/serial/8250.c,
> which also registers just one struct console?
> would not console=ttyS0 console=ttyS1 have the same problem?

Yes, it have the same problem. Only one of the two (I think the first)
will get enabled as console.

> Please instrument the calls to register_console and add_preferred_console
> and give a detailed description of the problem you are encountering.

| add_preferred_console("hvc", 4, NULL)

This call was added recently by the Power LPAR code.

| add_preferred_console("hvc", 1, NULL)

This comes from the command line ("console=hvc1").

| hvc_config_driver.index == -1
| register_console(&hvc_con_driver)
| hvc_config_driver.index == 4

This call is used to detect the id of the to be enabled hvc device. See
the comment of hvc_console_init.  register_console sets it to the
_first_ id it finds, in this case 4.  There is no other call to
register_console because there is no hvc console with id 4 registered
and hvc_instantiate checks this.

The list of consoles looks like:
- hvc, 4
- hvc, 1

The boot console (udbg0) is destructed later without a real console
remaining.

> Perhaps the real fix should be scan the command line for console= at
> console_init time?   How does that compare to __setup that is called now?

This was removed because it break different things. See
5faae2e5d1f53df9dce482032c8486bc3a1feffc.

> >  		for (i = 0; i < n; ++i) {
> >  #ifdef CONFIG_MAGIC_SYSRQ
> > -			if (hp->index == hvc_con_driver.index) {
> > +			if (consoles[hp->index].console.flags & CON_CONSDEV) {
> >  				/* Handle the SysRq Hack */
> >  				/* XXX should support a sequence */
> >  				if (buf[i] == '\x0f') {	/* ^O */
> > @@ -775,8 +764,8 @@ struct hvc_struct __devinit *hvc_alloc(uint32_t vtermno, int irq,
> >  	 * see if this vterm id matches one registered for console.
> >  	 */
> >  	for (i=0; i < MAX_NR_HVC_CONSOLES; i++)
> > -		if (vtermnos[i] == hp->vtermno &&
> > -		    cons_ops[i] == hp->ops)
> > +		if (consoles[i].vtermno == hp->vtermno &&
> > +		    consoles[i].ops == hp->ops)
> >  			break;
> >  
> 
> 
> NACK
> you broke this assertion:
> >  /*
> >   * Initial console vtermnos for console API usage prior to full console
> >   * initialization.  Any vty adapter outside this range will not have usable
> >   * console interfaces but can still be used as a tty device.  This has to be
> >   * static because kmalloc will not work during early console init.
> >   */

Well. It speaks about "range", but in fact it was exactly one.

> The idea is you might want serial port to 250 other partitions, but only
> need to have your choice of console be on the first 8 or so.

hvc is limited to 16 devices.

Bastian

-- 
No one wants war.
		-- Kirk, "Errand of Mercy", stardate 3201.7

Re: [PATCH] hvc - register all available consoles (was: Re: [PATCH] powerpc/lpar - defer prefered console setup)

[Milton Miller]

Please CC me, I'm not subscribed to the list.

On Wed Jul 30 at 20:07:01 EST in 2008, Bastian Blank wrote:
> On Wed, Jul 30, 2008 at 04:13:47AM -0500, Milton Miller wrote:
>> On Wed Jul 30 at 17:34:38 EST in 2008, Bastian Blank wrote:
>>> On Wed, Jul 30, 2008 at 08:29:19AM +0200, Bastian Blank wrote:
>>>> Okay, so hvc_console is the culprit. It don't register a preferred
>>>> console if it knows it is not the first in the list.
>>>
>>> The patch registers all available hvc consoles. It adds one "struct
>>> console" for all possible hvc consoles.
>>
>> [ a 6 page patch adding forward declartions, arrays of console
>> structs, moving lots of code and creating N struct console in the
>> hvc_driver shell]
>>
>> After previously having written:
>>> On Wed, Jul 30, 2008 at 08:23:13AM +0200, Bastian Blank wrote:
>>>> On Wed, Jul 30, 2008 at 12:34:51PM +1000, Michael Ellerman wrote:
>>>>> On Mon, 2008-07-28 at 20:56 +0200, Bastian Blank wrote:
>>>>>  * add_preferred_console - add a device to the list of preferred 
>>>>> consoles.
>>>>>  ...
>>>>>  * The last preferred console added will be used for kernel 
>>>>> messages
>>>>>  * and stdin/out/err for init.
>>>>>
>>>>> The last console will be added by the console= parsing, and so 
>>>>> that will
>>>>> be used. The console we add in the pseries setup is only used if 
>>>>> nothing
>>>>> is specified on the command line.
>>>>
>>>> Okay, then there is a completely different problem. In the case of
>>>> "console=hvc0 console=hvc1" it uses the _first_ add stdin/out bla 
>>>> and
>>>> ignores the second one completely.
>>>
>>> Okay, so hvc_console is the culprit. It don't register a preferred
>>> console if it knows it is not the first in the list.
>>>
>>> Bastian
>>
>>
>> [ back to the patch ]
>>> -/*
>>> - * Early console initialization.  Precedes driver initialization.
>>> - *
>>> - * (1) we are first, and the user specified another driver
>>> - * -- index will remain -1
>>> - * (2) we are first and the user specified no driver
>>> - * -- index will be set to 0, then we will fail setup.
>>> - * (3)  we are first and the user specified our driver
>>> - * -- index will be set to user specified driver, and we will fail
>>> - * (4) we are after driver, and this initcall will register us
>>> - * -- if the user didn't specify a driver then the console will 
>>> match
>>> - *
>>> - * Note that for cases 2 and 3, we will match later when the io 
>>> driver
>>> - * calls hvc_instantiate() and call register again.
>>> - */
>>> -static int __init hvc_console_init(void)
>>> -{
>>> -   register_console(&hvc_con_driver);
>>> -   return 0;
>>>
>>
>>
>> Please explain how the reasoning you removed breaks down.
>> What is your usage case?
>
> I have several hvc consoles on a Power hypervisor.

A pretty short description, lacking detail.

>
>> More importantly , how is this different than, say, 
>> drivers/serial/8250.c,
>> which also registers just one struct console?
>> would not console=ttyS0 console=ttyS1 have the same problem?
>
> Yes, it have the same problem. Only one of the two (I think the first)
> will get enabled as console.

So lets try to fix the generic code and not one driver.

>> Please instrument the calls to register_console and 
>> add_preferred_console
>> and give a detailed description of the problem you are encountering.
>
> | add_preferred_console("hvc", 4, NULL)
>
> This call was added recently by the Power LPAR code.

Not really, it was added in 2.6.16 in 2005 
(463ce0e103f419f51b1769111e73fe8bb305d0ec), but we recently stopped 
avoiding the call in your use case.  But the same issue would exist if 
the boot loader issued a console= then appended a user supplied 
console=, so lets try to fix the whole problem.

> | add_preferred_console("hvc", 1, NULL)
>
> This comes from the command line ("console=hvc1").

So this meets the assertion that the latter preferred console came from 
the command line, and the command line is supposed to get the last 
console registered.

> | hvc_config_driver.index == -1
> | register_console(&hvc_con_driver)
> | hvc_config_driver.index == 4

So you did not indicate which call site of register_console in the hvc 
driver was called.

When I broke it out the hvc_driver from its clients, there were two 
call paths to register the console, as mentioned in the conmment I 
quoted.  Which path is the problem?


> This call is used to detect the id of the to be enabled hvc device. See
> the comment of hvc_console_init.  register_console sets it to the
> _first_ id it finds, in this case 4.  There is no other call to
> register_console because there is no hvc console with id 4 registered
> and hvc_instantiate checks this.
>
> The list of consoles looks like:
> - hvc, 4
> - hvc, 1
>

So you claim on the call to register_console, both 
add_preferred_console calls had occurred, but the code set changed 
console->index from -1 to that of the first call instead of the last 
match for the driver?   That sounds like the real bug.

> The boot console (udbg0) is destructed later without a real console
> remaining.
>
>> Perhaps the real fix should be scan the command line for console= at
>> console_init time?   How does that compare to __setup that is called 
>> now?

No, I was referring to console_init (in drivers/char/tty_io.c or so 
called from init/main.c).   I was thinking perhaps that the __setup 
function had not been run so we saw only the setup_arch call and not 
the command_line call to add_preferred_console had occurred, but that 
appears not to be the case.

> This was removed because it break different things. See
> 5faae2e5d1f53df9dce482032c8486bc3a1feffc.

No that is a different check.   That was a scanning for console= by the 
setup_arch code.  And it appears kernel/printk.c now has an explicit 
exported variable for drivers to check that console= had been found on 
the command line.  Although it would still not trigger in this case 
because we call before the command line is processed, as you noted, the 
design should take the last one as Michael noted and therefore it 
should work.

>>
>> NACK
>> you broke this assertion:
>>>  /*
>>>   * Initial console vtermnos for console API usage prior to full 
>>> console
>>>   * initialization.  Any vty adapter outside this range will not 
>>> have usable
>>>   * console interfaces but can still be used as a tty device.  This 
>>> has to be
>>>   * static because kmalloc will not work during early console init.
>>>   */
>
> Well. It speaks about "range", but in fact it was exactly one.

No you are counting the wrong thing.  The design is supposed to support 
any single port to be the console (we have one struct console), but 
only one that registers in a fixed slot (hvc_instantiate) in the range 
(0, MAX_HVC_CONSOLES), can be selected for a console driver.

>> The idea is you might want serial port to 250 other partitions, but 
>> only
>> need to have your choice of console be on the first 8 or so.
>
> hvc is limited to 16 devices.

You are limited to one of the first 16 devices/ports being selectable 
as your console, but the number of tty devices is a totally separate 
option, and is limited by the number of tty structs we allocate (which 
should be dynamic) and the range of the minor that we register.

Don't confuse the current defaults (which might insanely be 8 ttys but 
16 possible fixed slots to choose your console from) from the designed 
flexibility.

milton