[PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Deepak Saxena]

Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC 
XO laptop. Basically what happens is that upon returning from resume, 
serial8250_resume_port() will reconfigure the port for high speed
mode and all console output will be garbled, making debug of the 
resume path painful. This patch modifies serial8250_resume_port() to 
not touch the port in the case where it is the console port and console 
suspend is disabled.

Signed-off-by: Deepak Saxena <dsaxena@laptop.org>

---
The OLPC tree has been carrying a workaround for about two years but
this patch is not the version we've been using. That one can be
found at http://dev.laptop.org/~dsaxena/patches/console_suspend_old.patch.
I prefer the approach of handling this in the 8250 driver itself.

diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index b4b3981..d4b6373 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2887,7 +2887,8 @@ void serial8250_resume_port(int line)
 {
 	struct uart_8250_port *up = &serial8250_ports[line];
 
-	if (up->capabilities & UART_NATSEMI) {
+	if ((up->capabilities & UART_NATSEMI) && 
+	     (!uart_console(&up->port) && console_suspend_enabled)) {
 		unsigned char tmp;
 
 		/* Ensure it's still in high speed mode */
diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index b0bb29d..05fd868 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -54,12 +54,6 @@ static struct lock_class_key port_lock_key;
 
 #define uart_users(state)	((state)->count + (state)->info.port.blocked_open)
 
-#ifdef CONFIG_SERIAL_CORE_CONSOLE
-#define uart_console(port)	((port)->cons && (port)->cons->index == (port)->line)
-#else
-#define uart_console(port)	(0)
-#endif
-
 static void uart_change_speed(struct uart_state *state,
 					struct ktermios *old_termios);
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 57a97e5..9fba77a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -427,6 +427,12 @@ struct tty_driver *uart_console_device(struct console *co, int *index);
 void uart_console_write(struct uart_port *port, const char *s,
 			unsigned int count,
 			void (*putchar)(struct uart_port *, int));
+#ifdef CONFIG_SERIAL_CORE_CONSOLE
+#define uart_console(port)	((port)->cons && (port)->cons->index == (port)->line)
+#else
+#define uart_console(port)	(0)
+#endif
+
 
 /*
  * Port/driver registration/removal



-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Linus Torvalds]

On Tue, 12 May 2009, Deepak Saxena wrote:
> 
> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC 
> XO laptop. Basically what happens is that upon returning from resume, 
> serial8250_resume_port() will reconfigure the port for high speed
> mode and all console output will be garbled, making debug of the 
> resume path painful. This patch modifies serial8250_resume_port() to 
> not touch the port in the case where it is the console port and console 
> suspend is disabled.
> 
> Signed-off-by: Deepak Saxena <dsaxena@laptop.org>
> 
> ---
> The OLPC tree has been carrying a workaround for about two years but
> this patch is not the version we've been using. That one can be
> found at http://dev.laptop.org/~dsaxena/patches/console_suspend_old.patch.
> I prefer the approach of handling this in the 8250 driver itself.

Hmm. I already applied this, but then after looking closer, I undid that. 
Why? It looks buggy:

> -	if (up->capabilities & UART_NATSEMI) {
> +	if ((up->capabilities & UART_NATSEMI) && 
> +	     (!uart_console(&up->port) && console_suspend_enabled)) {
>  		unsigned char tmp;

Isn't that second test wrong? Should it not be

	if ((up->capabilities & UART_NATSEMI) &&
		(console_suspend_enabled || !uart_console(&up->port)) {

instead?

In fact, I'd suggest making a helper function called 
"do_suspend_uart(up)", something like

	/*
	 * Suspend the uart port unless it's a console. 
	 *
	 * But suspend even consoles if "console_suspend_enabled"
	 * is set.
	 */
	static inline int do_suspend_uart(struct uart_port *port)
	{
		return console_suspend_enabled || !uart_console(port);
	}

and then make all these things (including the _existing_ cases in 
uart_suspend_port() use that helper function, rather than writing it out.

		Linus

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Michał Mirosław]

On Tue, May 12, 2009 at 09:00:16PM +0000, Deepak Saxena wrote:
> Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC 
> XO laptop. Basically what happens is that upon returning from resume, 
> serial8250_resume_port() will reconfigure the port for high speed
> mode and all console output will be garbled, making debug of the 
> resume path painful. This patch modifies serial8250_resume_port() to 
> not touch the port in the case where it is the console port and console 
> suspend is disabled.
[cut]
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -2887,7 +2887,8 @@ void serial8250_resume_port(int line)
>  {
>  	struct uart_8250_port *up = &serial8250_ports[line];
>  
> -	if (up->capabilities & UART_NATSEMI) {
> +	if ((up->capabilities & UART_NATSEMI) && 
> +	     (!uart_console(&up->port) && console_suspend_enabled)) {

Shouldn't this be || instead of && - as in: if it is NOT a console port
OR console suspend is enabled?

Best Regards,
Michał Mirosław

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Alan Cox]

> Hmm. I already applied this, but then after looking closer, I undid that. 
> Why? It looks buggy:

I'm a bit suprised that as tty and serial maintainer this is the first
time I see the patch.

> Isn't that second test wrong? Should it not be
> 
> 	if ((up->capabilities & UART_NATSEMI) &&
> 		(console_suspend_enabled || !uart_console(&up->port)) {
> 
> instead?

The patch seems totally bogus anyway. If the console was in a high speed
mode it should be resumed in a high speed mode. What are the actual
details here.

Surely if my console is at 230Kbits/sec then resuming it at a totally
different speed is going to break things for people even if it happens to
help XO debug ?

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Deepak Saxena]

On May 13 2009, at 01:18, Alan Cox was caught saying:
> > Hmm. I already applied this, but then after looking closer, I undid that. 
> > Why? It looks buggy:
> 
> I'm a bit suprised that as tty and serial maintainer this is the first
> time I see the patch.

I looked in MAINTAINERS and 8250 is listed as unmaintained. :)

> > Isn't that second test wrong? Should it not be
> > 
> > 	if ((up->capabilities & UART_NATSEMI) &&
> > 		(console_suspend_enabled || !uart_console(&up->port)) {
> > 
> > instead?
> 
> The patch seems totally bogus anyway. If the console was in a high speed
> mode it should be resumed in a high speed mode. What are the actual
> details here.
> 
> Surely if my console is at 230Kbits/sec then resuming it at a totally
> different speed is going to break things for people even if it happens to
> help XO debug ?

The console is not in high speed mode when we suspend. The original 
commit (b5b82df6) just assumes that if it is a NATSEMI device, we 
should set it to high speed mode at resume w/o checking if that 
was the mode we were in when we suspended. 

~Deepak


-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Deepak Saxena]

On May 12 2009, at 15:00, Linus Torvalds was caught saying:
> 
> Hmm. I already applied this, but then after looking closer, I undid that. 
> Why? It looks buggy:
> 
> > -	if (up->capabilities & UART_NATSEMI) {
> > +	if ((up->capabilities & UART_NATSEMI) && 
> > +	     (!uart_console(&up->port) && console_suspend_enabled)) {
> >  		unsigned char tmp;
> 
> Isn't that second test wrong? Should it not be
> 
> 	if ((up->capabilities & UART_NATSEMI) &&
> 		(console_suspend_enabled || !uart_console(&up->port)) {
> 
> instead?

Yep, thinko on my end.

> 	/*
> 	 * Suspend the uart port unless it's a console. 
> 	 *
> 	 * But suspend even consoles if "console_suspend_enabled"
> 	 * is set.
> 	 */
> 	static inline int do_suspend_uart(struct uart_port *port)
> 	{
> 		return console_suspend_enabled || !uart_console(port);
> 	}
> 
> and then make all these things (including the _existing_ cases in 
> uart_suspend_port() use that helper function, rather than writing it out.

Sounds good. Alan?

~Deepak

-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Joe Perches]

On Wed, 2009-05-13 at 01:18 +0100, Alan Cox wrote:
> > Hmm. I already applied this, but then after looking closer, I undid that. 
> > Why? It looks buggy:
> 
> I'm a bit suprised that as tty and serial maintainer this is the first
> time I see the patch.

Perhaps the MAINTAINERS entry for TTY should include
file patterns?

Would this be reasonable?

TTY LAYER
P:      Alan Cox
M:      alan@lxorguk.ukuu.org.uk
L:      linux-kernel@vger.kernel.org
L:      linux-serial@vger.kernel.org
S:      Maintained
F:      drivers/char/tty_*
F:      drivers/serial/serial_core.c
F:      linux/serial_core.h
F:      linux/serial.h
F:      linux/tty.h

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Alan Cox]

> > Surely if my console is at 230Kbits/sec then resuming it at a totally
> > different speed is going to break things for people even if it happens to
> > help XO debug ?
> 
> The console is not in high speed mode when we suspend. The original 
> commit (b5b82df6) just assumes that if it is a NATSEMI device, we 
> should set it to high speed mode at resume w/o checking if that 
> was the mode we were in when we suspended.

Then hard coding is no improvement either. It needs to restore the mode
that was present at suspend.

I'll update the 8250 Maintainers entry I suppose, I seem to have been
landed it as part of tty 8)

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Linus Torvalds]

On Wed, 13 May 2009, Alan Cox wrote:
> 
> Then hard coding is no improvement either. It needs to restore the mode
> that was present at suspend.

Alan, you're missing the fact that this is a special-case for "this is the 
console, and we're not suspending consoles AT ALL".

IOW, it was never suspended either, and it's a case that is known to be 
fundamentally buggy (we're suspending all the PCI bridges, but not a 
serial device that may be behind them!), but often work in _practice_ 
(because people don't use this thing for random serial devices, but for 
things like integrated serial lines that don't lose power).

So we _could_ just save the mode, but that isn't really what this patch is 
all about. The patch in question is about a total hack to avoid touching a 
piece of hardware that we simply don't consider normal.

			Linus

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Alan Cox]

> So we _could_ just save the mode, but that isn't really what this patch is 
> all about. The patch in question is about a total hack to avoid touching a 
> piece of hardware that we simply don't consider normal.

The driver knows and assumes the chip is stuck back into the right
configuration. Things like ttyS0 shared as console and tty won't work
right if we simply pray the configuration comes back sane. It may well do
on OLPC but on a standard generic PC we can't bet on it. If the user has
a high console speed set this patch will do the wrong thing.

No suspend console meaning "I'd like to debug suspend/resume" is only
useful if we actually put the bits back into a sane state coming out of
suspend to ram (and in some cases suspend to disk)

Deepak's original patch from 2008 is actually much better. He puts the
chip back in the right hardware configuration for the speed. He doesn't
randomly regress other platforms and configurations and he does it by
calling the standard methods in the driver to do the job.

http://dev.laptop.org/~dsaxena/patches/console_suspend_old.patch

It also has another advantage - it fixes the behaviour for almost
everything in drivers/serial not just a specific uart. Technically he
should take tty->termios_mutex before state->mutex in that code path as
the upstream code has changed but in every other respect its the proper
solution.

So can we apply Deepak's original correct 2008 patch instead please ?

Alan

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Deepak Saxena]

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

On May 13 2009, at 16:35, Alan Cox was caught saying:
> Deepak's original patch from 2008 is actually much better. He puts the
> chip back in the right hardware configuration for the speed. He doesn't
> randomly regress other platforms and configurations and he does it by
> calling the standard methods in the driver to do the job.
> 
> http://dev.laptop.org/~dsaxena/patches/console_suspend_old.patch
> 
> It also has another advantage - it fixes the behaviour for almost
> everything in drivers/serial not just a specific uart. Technically he
> should take tty->termios_mutex before state->mutex in that code path as
> the upstream code has changed but in every other respect its the proper
> solution.

I thought the fact that we were touching a core driver to fix what
seemed like an XO issue was too intrusive.

> So can we apply Deepak's original correct 2008 patch instead please ?

I've updated it with proper header and attached.

~Deepak


[-- Attachment #2: restore_console_settings_on_suspend.patch --]
[-- Type: text/x-diff, Size: 2149 bytes --]

Set proper console speed on resume if console suspend is disabled
    
Commit b5b82df6, from May 2007, breaks no_console_suspend on the OLPC
XO laptop. Basically what happens is that upon returning from resume,
serial8250_resume_port() will reconfigure the port for high speed
mode and all console output will be garbled, making debug of the
resume path painful. This patch modifies uart_resume_port() to
reset the port to the state it was in before we suspended.
    
Original patch by Marcelo Tosatti
    
Signed-off-by: Deepak Saxena <dsaxena@laptop.org>

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index b0bb29d..3b71038 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -2052,11 +2052,26 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *port)
 	struct uart_state *state = drv->state + port->line;
 	struct device *tty_dev;
 	struct uart_match match = {port, drv};
+	struct ktermios termios;
 
 	mutex_lock(&state->mutex);
 
+	/*
+	 * First try to use the console cflag setting.
+	 */
+	memset(&termios, 0, sizeof(struct ktermios));
+	termios.c_cflag = port->cons->cflag;
+
+	/*
+	 * If that's unset, use the tty termios setting.
+	 */
+	if (termios.c_cflag == 0)
+		termios = *state->info.port.tty->termios;
+
+
 	if (!console_suspend_enabled && uart_console(port)) {
 		/* no need to resume serial console, it wasn't suspended */
+		port->ops->set_termios(port, &termios, NULL);
 		mutex_unlock(&state->mutex);
 		return 0;
 	}
@@ -2073,20 +2088,6 @@ int uart_resume_port(struct uart_driver *drv, struct uart_port *port)
 	 * Re-enable the console device after suspending.
 	 */
 	if (uart_console(port)) {
-		struct ktermios termios;
-
-		/*
-		 * First try to use the console cflag setting.
-		 */
-		memset(&termios, 0, sizeof(struct ktermios));
-		termios.c_cflag = port->cons->cflag;
-
-		/*
-		 * If that's unset, use the tty termios setting.
-		 */
-		if (state->info.port.tty && termios.c_cflag == 0)
-			termios = *state->info.port.tty->termios;
-
 		uart_change_pm(state, 0);
 		port->ops->set_termios(port, &termios, NULL);
 		console_start(port->cons);

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Alan Cox]

> > So can we apply Deepak's original correct 2008 patch instead please ?
> 
> I've updated it with proper header and attached.

Thanks - much better approach.

Alan

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Deepak Saxena]

On May 13 2009, at 19:46, Alan Cox was caught saying:
> > > So can we apply Deepak's original correct 2008 patch instead please ?
> > 
> > I've updated it with proper header and attached.
> 
> Thanks - much better approach.

Alan, is this queued up for .31?

Thanks,
~Deepak

-- 
In the end, they will not say, "those were dark times,"  they will ask
"why were their poets silent?" - Bertold Brecht

Re: [PATCH] 8250: Don't restore NS16550 mode when console suspend is disabled

[Alan Cox]

On Mon, 18 May 2009 18:24:54 +0000
Deepak Saxena <dsaxena@plexity.net> wrote:

> On May 13 2009, at 19:46, Alan Cox was caught saying:
> > > > So can we apply Deepak's original correct 2008 patch instead please ?
> > > 
> > > I've updated it with proper header and attached.
> > 
> > Thanks - much better approach.
> 
> Alan, is this queued up for .31?

I thought Linus was dealing with it directly ? If not I'll queue it.

Alan