[PATCH 1/2] serial_core: Unregister console in uart_remove_one_port()

[PATCH 1/2] serial_core: Unregister console in uart_remove_one_port()

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>

If the serial port being removed is used as a console, it must also be
unregistered from the console subsystem using unregister_console().

uart_ops.release_port() will release resources (e.g. iounmap() the serial
port registers), causing a crash on subsequent kernel output if the console
is still registered.

Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
---
 drivers/tty/serial/serial_core.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ece2049bd270..8ece7f14d89d 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2677,6 +2677,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
 		tty_vhangup(port->tty);
 
 	/*
+	 * If the port is used as a console, unregister it
+	 */
+	if (uart_console(uport))
+		unregister_console(uport->cons);
+
+	/*
 	 * Free the port IO and memory resources, if any.
 	 */
 	if (uport->type != PORT_UNKNOWN)
-- 
1.7.9.5

[PATCH 2/2] serial: sh-sci: Add missing call to uart_remove_one_port() in failure path

[Geert Uytterhoeven]

From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>

If cpufreq_register_notifier() fails, we have to remove the port added by
sci_probe_single(), which is not done by sci_cleanup_single().

Else the serial port stays active from the point of view of the serial
subsystem, and it may crash when userspace getty is started, or when the
loadable driver module is unloaded.

This was introduced by commit 6dae14216c85eea13db7b12c469475c5d30e5499
("serial: sh-sci: Fix probe error paths").

Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
---
This depends on "serial_core: Unregister console in uart_remove_one_port()"
when using a serial console.
---
 drivers/tty/serial/sh-sci.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index be33d2b0613b..7958115e6a51 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -2564,6 +2564,7 @@ static int sci_probe(struct platform_device *dev)
 	ret = cpufreq_register_notifier(&sp->freq_transition,
 					CPUFREQ_TRANSITION_NOTIFIER);
 	if (unlikely(ret < 0)) {
+		uart_remove_one_port(&sci_uart_driver, &sp->port);
 		sci_cleanup_single(sp);
 		return ret;
 	}
-- 
1.7.9.5

Re: [PATCH 2/2] serial: sh-sci: Add missing call to uart_remove_one_port() in failure path

[Simon Horman]

On Fri, Feb 28, 2014 at 02:21:33PM +0100, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> 
> If cpufreq_register_notifier() fails, we have to remove the port added by
> sci_probe_single(), which is not done by sci_cleanup_single().
> 
> Else the serial port stays active from the point of view of the serial
> subsystem, and it may crash when userspace getty is started, or when the
> loadable driver module is unloaded.
> 
> This was introduced by commit 6dae14216c85eea13db7b12c469475c5d30e5499
> ("serial: sh-sci: Fix probe error paths").
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>

Acked-by: Simon Horman <horms+renesas@verge.net.au>

This looks reasonable to me.

Greg, could you take this one?


> ---
> This depends on "serial_core: Unregister console in uart_remove_one_port()"
> when using a serial console.
> ---
>  drivers/tty/serial/sh-sci.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index be33d2b0613b..7958115e6a51 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2564,6 +2564,7 @@ static int sci_probe(struct platform_device *dev)
>  	ret = cpufreq_register_notifier(&sp->freq_transition,
>  					CPUFREQ_TRANSITION_NOTIFIER);
>  	if (unlikely(ret < 0)) {
> +		uart_remove_one_port(&sci_uart_driver, &sp->port);
>  		sci_cleanup_single(sp);
>  		return ret;
>  	}
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] serial: sh-sci: Add missing call to uart_remove_one_port() in failure path

[Greg Kroah-Hartman]

On Thu, Mar 06, 2014 at 01:19:13PM +0900, Simon Horman wrote:
> On Fri, Feb 28, 2014 at 02:21:33PM +0100, Geert Uytterhoeven wrote:
> > From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> > 
> > If cpufreq_register_notifier() fails, we have to remove the port added by
> > sci_probe_single(), which is not done by sci_cleanup_single().
> > 
> > Else the serial port stays active from the point of view of the serial
> > subsystem, and it may crash when userspace getty is started, or when the
> > loadable driver module is unloaded.
> > 
> > This was introduced by commit 6dae14216c85eea13db7b12c469475c5d30e5499
> > ("serial: sh-sci: Fix probe error paths").
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> 
> Acked-by: Simon Horman <horms+renesas@verge.net.au>
> 
> This looks reasonable to me.
> 
> Greg, could you take this one?

Will do, thanks.

Re: [PATCH 1/2] serial_core: Unregister console in uart_remove_one_port()

[Peter Hurley]

[ +cc Alan ]

On 02/28/2014 08:21 AM, Geert Uytterhoeven wrote:
> From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>
> If the serial port being removed is used as a console, it must also be
> unregistered from the console subsystem using unregister_console().
>
> uart_ops.release_port() will release resources (e.g. iounmap() the serial
> port registers), causing a crash on subsequent kernel output if the console
> is still registered.

This brings up an interesting point:  where are the serial_core drivers
indicating to the tty core that the port is bound to a console?
IOW, I cannot find a single 'port->console = 1' anywhere in the serial tree.

It would be bad if the uart port disappears while a console is running
on it...

> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
> ---
>   drivers/tty/serial/serial_core.c |    6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index ece2049bd270..8ece7f14d89d 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2677,6 +2677,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>   		tty_vhangup(port->tty);
>
>   	/*
> +	 * If the port is used as a console, unregister it
> +	 */
> +	if (uart_console(uport))
> +		unregister_console(uport->cons);
> +
> +	/*
>   	 * Free the port IO and memory resources, if any.
>   	 */
>   	if (uport->type != PORT_UNKNOWN)
>

Re: [PATCH 1/2] serial_core: Unregister console in uart_remove_one_port()

[Peter Hurley]

On 03/10/2014 09:06 PM, Peter Hurley wrote:
> [ +cc Alan ]
>
> On 02/28/2014 08:21 AM, Geert Uytterhoeven wrote:
>> From: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>>
>> If the serial port being removed is used as a console, it must also be
>> unregistered from the console subsystem using unregister_console().
>>
>> uart_ops.release_port() will release resources (e.g. iounmap() the serial
>> port registers), causing a crash on subsequent kernel output if the console
>> is still registered.
>
> This brings up an interesting point:  where are the serial_core drivers
> indicating to the tty core that the port is bound to a console?
> IOW, I cannot find a single 'port->console = 1' anywhere in the serial tree.

Answering half my own question: I see that serial_core doesn't use
tty_port_shutdown() but rather has it's own uart_shutdown() equivalent, and
thus doesn't use 'port->console'.

Ok.

But I still don't see what prevents a uart_port_shutdown() on a running
serial console.

Regards,
Peter Hurley

> It would be bad if the uart port disappears while a console is running
> on it...
>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
>> ---
>>   drivers/tty/serial/serial_core.c |    6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
>> index ece2049bd270..8ece7f14d89d 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -2677,6 +2677,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>>           tty_vhangup(port->tty);
>>
>>       /*
>> +     * If the port is used as a console, unregister it
>> +     */
>> +    if (uart_console(uport))
>> +        unregister_console(uport->cons);
>> +
>> +    /*
>>        * Free the port IO and memory resources, if any.
>>        */
>>       if (uport->type != PORT_UNKNOWN)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/