serial: core: fix console problems on uart_close

Message ID 20161024205649.32430-1-robh@kernel.org
State New
Headers show

Commit Message

Rob Herring Oct. 24, 2016, 8:56 p.m.
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(-)

-- 
2.10.1

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Geert Uytterhoeven Oct. 25, 2016, 10:40 a.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring Oct. 25, 2016, 1:25 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mike Galbraith Oct. 25, 2016, 1:29 p.m. | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

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.