[PATCH] serial: core: fix console problems on uart_close

[PATCH] serial: core: fix console problems on uart_close

[Rob Herring]

Commit 761ed4a94582 ('tty: serial_core: convert uart_close to use
tty_port_close') started setting the ttyport console flag for serial
drivers. This is causing crashes, hangs, or garbage output on several
platforms because the serial shutdown is skipped and IRQs are left
enabled.

Partially revert commit 761ed4a94582 and drop reporting UART tty_ports
as a console leaving the console handling to the serial_core as it was
before.

Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close")
Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
Reported-by: Mugunthan V N <mugunthanvnm@ti.com>
Cc: Peter Hurley <peter@hurleysoftware.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
Geert's fix seems to be inadequate for some systems as Mike reported[1]. 
This fix works in Mike's case.

Geert, Mugunthan, Can you all test this is enough to fix your issues.

Rob

[1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1254753.html


 drivers/tty/serial/serial_core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 6e4f63627479..664c99aeeca5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2746,8 +2746,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
 	uport->cons = drv->cons;
 	uport->minor = drv->tty_driver->minor_start + uport->line;
 
-	port->console = uart_console(uport);
-
 	/*
 	 * If this port is a console, then the spinlock is already
 	 * initialised.
-- 
2.10.1

Re: [PATCH] serial: core: fix console problems on uart_close

[Geert Uytterhoeven]

Hi Rob,

On Mon, Oct 24, 2016 at 10:56 PM, Rob Herring <robh@kernel.org> wrote:
> Commit 761ed4a94582 ('tty: serial_core: convert uart_close to use
> tty_port_close') started setting the ttyport console flag for serial
> drivers. This is causing crashes, hangs, or garbage output on several
> platforms because the serial shutdown is skipped and IRQs are left
> enabled.

No, for me it hangs because uart_tty_port_shutdown() is _not_ skipped
when the UART is used as a serial console. Writing to the serial port while
it's shut down hangs.

> Partially revert commit 761ed4a94582 and drop reporting UART tty_ports
> as a console leaving the console handling to the serial_core as it was
> before.
>
> Fixes: 761ed4a94582ab29 ("tty: serial_core: convert uart_close to use tty_port_close")
> Reported-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reported-by: Mike Galbraith <umgwanakikbuti@gmail.com>
> Reported-by: Mugunthan V N <mugunthanvnm@ti.com>
> Cc: Peter Hurley <peter@hurleysoftware.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
> Geert's fix seems to be inadequate for some systems as Mike reported[1].
> This fix works in Mike's case.
>
> Geert, Mugunthan, Can you all test this is enough to fix your issues.

This doesn't work for me, as port->console is still false. It should be true
when used as a serial console, to make sure tty_port_shutdown() skips
an active serial console.

Mike: I see you are using a PC, while I'm using an ARM board (with DT).
Are you using a serial console? If yes, what's the value of port->console
before and after the call to uart_console() that Rob's patch below removes?

Thanks!

> [1] https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1254753.html
>
>  drivers/tty/serial/serial_core.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 6e4f63627479..664c99aeeca5 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2746,8 +2746,6 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>         uport->cons = drv->cons;
>         uport->minor = drv->tty_driver->minor_start + uport->line;
>
> -       port->console = uart_console(uport);
> -
>         /*
>          * If this port is a console, then the spinlock is already
>          * initialised.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] serial: core: fix console problems on uart_close

[Mike Galbraith]

On Tue, 2016-10-25 at 12:40 +0200, Geert Uytterhoeven wrote:

> Mike: I see you are using a PC, while I'm using an ARM board (with DT).
> Are you using a serial console? If yes, what's the value of port->console
> before and after the call to uart_console() that Rob's patch below removes?

Well, it's a bit larger than the average PC, it's a 64 core HP DL980,
and my console is its virtual serial port.

[   15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled
[   15.228212] MIKE port:ffff880276400000 port->console:0
[   15.240889] MIKE port:ffff880276400000 port->console:0
[   15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
[   15.282538] MIKE port:ffff8802764001a8 port->console:0
[   15.293195] MIKE port:ffff8802764001a8 port->console:1
[   15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
(30 more calls, all 0 pre 0 post)

That's with the below from Jiri Slaby applied to quell the reboot time
explosion.

---
 drivers/tty/serial/serial_core.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -106,12 +106,17 @@ static inline struct uart_port *uart_por
 void uart_write_wakeup(struct uart_port *port)
 {
 	struct uart_state *state = port->state;
+	struct tty_struct *ttyp;
+
 	/*
 	 * This means you called this function _after_ the port was
 	 * closed.  No cookie for you.
 	 */
 	BUG_ON(!state);
-	tty_wakeup(state->port.tty);
+	ttyp = tty_port_tty_get(&state->port);
+	if (ttyp)
+		tty_wakeup(ttyp);
+	tty_kref_put(ttyp);
 }
 
 static void uart_stop(struct tty_struct *tty)

Re: [PATCH] serial: core: fix console problems on uart_close

[Rob Herring]

On Tue, Oct 25, 2016 at 7:55 AM, Mike Galbraith
<umgwanakikbuti@gmail.com> wrote:
> On Tue, 2016-10-25 at 12:40 +0200, Geert Uytterhoeven wrote:
>
>> Mike: I see you are using a PC, while I'm using an ARM board (with DT).
>> Are you using a serial console? If yes, what's the value of port->console
>> before and after the call to uart_console() that Rob's patch below removes?
>
> Well, it's a bit larger than the average PC, it's a 64 core HP DL980,
> and my console is its virtual serial port.
>
> [   15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled
> [   15.228212] MIKE port:ffff880276400000 port->console:0
> [   15.240889] MIKE port:ffff880276400000 port->console:0
> [   15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
> [   15.282538] MIKE port:ffff8802764001a8 port->console:0
> [   15.293195] MIKE port:ffff8802764001a8 port->console:1
> [   15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
> (30 more calls, all 0 pre 0 post)
>
> That's with the below from Jiri Slaby applied to quell the reboot time
> explosion.

This alone is enough to fix things for you?

Rob

Re: [PATCH] serial: core: fix console problems on uart_close

[Mike Galbraith]

On Tue, 2016-10-25 at 08:25 -0500, Rob Herring wrote:
> On Tue, Oct 25, 2016 at 7:55 AM, Mike Galbraith
> <umgwanakikbuti@gmail.com> wrote:
> > On Tue, 2016-10-25 at 12:40 +0200, Geert Uytterhoeven wrote:
> > 
> > > Mike: I see you are using a PC, while I'm using an ARM board (with DT).
> > > Are you using a serial console? If yes, what's the value of port->console
> > > before and after the call to uart_console() that Rob's patch below removes?
> > 
> > Well, it's a bit larger than the average PC, it's a 64 core HP DL980,
> > and my console is its virtual serial port.
> > 
> > [   15.217487] Serial: 8250/16550 driver, 32 ports, IRQ sharing disabled
> > [   15.228212] MIKE port:ffff880276400000 port->console:0
> > [   15.240889] MIKE port:ffff880276400000 port->console:0
> > [   15.269606] 00:03: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a 16550A
> > [   15.282538] MIKE port:ffff8802764001a8 port->console:0
> > [   15.293195] MIKE port:ffff8802764001a8 port->console:1
> > [   15.322806] serial8250: ttyS1 at I/O 0x2f8 (irq = 3, base_baud = 115200) is a 16550A
> > (30 more calls, all 0 pre 0 post)
> > 
> > That's with the below from Jiri Slaby applied to quell the reboot time
> > explosion.
> 
> This alone is enough to fix things for you?

Yup.

	-Mike