[PATCH 0/5] earlycon hang under some conditions

[PATCH 0/5] earlycon hang under some conditions

[Jeffy Chen]

I was testing earlycon with 8250 dw serial console. And it hangs in
these cases:
1/ kernel hang when calling early write function after free_initmem:
a) the earlycon not disabled after the init code(due to keep_bootcon or
   not specify a real console to switch to)
b) the early write func is marked as __init, for example 8250_early.

2/ kernel hang when calling early write function after disable unused
clks/pm domain:
a) the earlycon not disabled after the init code
b) the disable unused clks/pm domain kill the requiered clks/pm
   domain, since they are not referenced by the earlycon.

3/ kernel hang when calling early write function after the serial
   console driver runtime suspended:
a) the earlycon not disabled after the init code
b) the serial console driver's runtime suspend kills the requiered
   clks/pm domain, since they are not referenced by the earlycon.

This serial fix 1/ case only.



Jeffy Chen (5):
  serial: arc: Remove __init marking from early write
  serial: omap: Remove __init marking from early write
  serial: xuartps: Remove __init marking from early write
  serial: 8250_ingenic: Remove __init marking from early write
  serial: 8250_early: Remove __init marking from early write

 drivers/tty/serial/8250/8250_early.c   |  8 ++++----
 drivers/tty/serial/8250/8250_ingenic.c |  8 ++++----
 drivers/tty/serial/arc_uart.c          |  4 ++--
 drivers/tty/serial/omap-serial.c       | 13 ++++++-------
 drivers/tty/serial/xilinx_uartps.c     |  2 +-
 5 files changed, 17 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH 3/5] serial: xuartps: Remove __init marking from early write

[Jeffy Chen]

The earlycon would be alive outside the init code in these cases:
1/ we have keep_bootcon in cmdline.
2/ we don't have a real console to switch to.

So remove the __init marking to avoid invalid memory access.

Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com>
---

 drivers/tty/serial/xilinx_uartps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index fde55dc..31a630a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1163,7 +1163,7 @@ static void cdns_uart_console_putchar(struct uart_port *port, int ch)
 	writel(ch, port->membase + CDNS_UART_FIFO);
 }
 
-static void __init cdns_early_write(struct console *con, const char *s,
+static void cdns_early_write(struct console *con, const char *s,
 				    unsigned n)
 {
 	struct earlycon_device *dev = con->data;
-- 
2.1.4

[PATCH 0/5] earlycon hang under some conditions

[Brian Norris]

Hi Jeffy,

On Tue, Jul 18, 2017 at 02:02:53PM +0800, Jeffy Chen wrote:
> I was testing earlycon with 8250 dw serial console. And it hangs in
> these cases:
> 1/ kernel hang when calling early write function after free_initmem:
> a) the earlycon not disabled after the init code(due to keep_bootcon or
>    not specify a real console to switch to)
> b) the early write func is marked as __init, for example 8250_early.

FWIW, I tested 8250_early with "keep_bootcon", and this fixes that:

Tested-by: Brian Norris <briannorris@chromium.org>

But then I get double console, if I have both a "real" and "early"
console.

If I were to *only* have the early console, then I might hit the
problems you mention:

> 2/ kernel hang when calling early write function after disable unused
> clks/pm domain:
> a) the earlycon not disabled after the init code
> b) the disable unused clks/pm domain kill the requiered clks/pm
>    domain, since they are not referenced by the earlycon.
> 
> 3/ kernel hang when calling early write function after the serial
>    console driver runtime suspended:
> a) the earlycon not disabled after the init code
> b) the serial console driver's runtime suspend kills the requiered
>    clks/pm domain, since they are not referenced by the earlycon.
> 
> This serial fix 1/ case only.

Problems 2 and 3 look like something that's not really in scope for
early consoles. There's a reason they are mainly supported for early
boot, and we try to switch off of them. There isn't really a good way to
handle all the clock and PM infrastructure without...switching off the
earlycon and using the real one :)

So, I guess this patchset has value for systems where the clock/PM
requirements are simple enough, and the earlycon can actually be useful
beyond early init.

Brian

> 
> 
> Jeffy Chen (5):
>   serial: arc: Remove __init marking from early write
>   serial: omap: Remove __init marking from early write
>   serial: xuartps: Remove __init marking from early write
>   serial: 8250_ingenic: Remove __init marking from early write
>   serial: 8250_early: Remove __init marking from early write
> 
>  drivers/tty/serial/8250/8250_early.c   |  8 ++++----
>  drivers/tty/serial/8250/8250_ingenic.c |  8 ++++----
>  drivers/tty/serial/arc_uart.c          |  4 ++--
>  drivers/tty/serial/omap-serial.c       | 13 ++++++-------
>  drivers/tty/serial/xilinx_uartps.c     |  2 +-
>  5 files changed, 17 insertions(+), 18 deletions(-)
> 
> -- 
> 2.1.4
> 
> 

[PATCH 0/5] earlycon hang under some conditions

[Doug Anderson]

Hi,

On Tue, Jul 18, 2017 at 10:51 AM, Brian Norris <briannorris@chromium.org> wrote:
> Hi Jeffy,
>
> On Tue, Jul 18, 2017 at 02:02:53PM +0800, Jeffy Chen wrote:
>> I was testing earlycon with 8250 dw serial console. And it hangs in
>> these cases:
>> 1/ kernel hang when calling early write function after free_initmem:
>> a) the earlycon not disabled after the init code(due to keep_bootcon or
>>    not specify a real console to switch to)
>> b) the early write func is marked as __init, for example 8250_early.
>
> FWIW, I tested 8250_early with "keep_bootcon", and this fixes that:
>
> Tested-by: Brian Norris <briannorris@chromium.org>
>
> But then I get double console, if I have both a "real" and "early"
> console.
>
> If I were to *only* have the early console, then I might hit the
> problems you mention:
>
>> 2/ kernel hang when calling early write function after disable unused
>> clks/pm domain:
>> a) the earlycon not disabled after the init code
>> b) the disable unused clks/pm domain kill the requiered clks/pm
>>    domain, since they are not referenced by the earlycon.
>>
>> 3/ kernel hang when calling early write function after the serial
>>    console driver runtime suspended:
>> a) the earlycon not disabled after the init code
>> b) the serial console driver's runtime suspend kills the requiered
>>    clks/pm domain, since they are not referenced by the earlycon.
>>
>> This serial fix 1/ case only.
>
> Problems 2 and 3 look like something that's not really in scope for
> early consoles. There's a reason they are mainly supported for early
> boot, and we try to switch off of them. There isn't really a good way to
> handle all the clock and PM infrastructure without...switching off the
> earlycon and using the real one :)
>
> So, I guess this patchset has value for systems where the clock/PM
> requirements are simple enough, and the earlycon can actually be useful
> beyond early init.

Presumably someone using this would just use the "clk_ignore_unused"
and "pd_ignore_unused" kernel parameters to help them...  In this case
that seems fine to me.  The "early" boot console is early because it
can't rely on all of the generic OS mechanisms to do things super
cleanly, so requiring the user to know that they should add the extra
command line arguments seems sane.

-Doug

[PATCH 0/5] earlycon hang under some conditions

[Doug Anderson]

Hi,

On Mon, Jul 17, 2017 at 11:02 PM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> I was testing earlycon with 8250 dw serial console. And it hangs in
> these cases:
> 1/ kernel hang when calling early write function after free_initmem:
> a) the earlycon not disabled after the init code(due to keep_bootcon or
>    not specify a real console to switch to)
> b) the early write func is marked as __init, for example 8250_early.
>
> 2/ kernel hang when calling early write function after disable unused
> clks/pm domain:
> a) the earlycon not disabled after the init code
> b) the disable unused clks/pm domain kill the requiered clks/pm
>    domain, since they are not referenced by the earlycon.
>
> 3/ kernel hang when calling early write function after the serial
>    console driver runtime suspended:
> a) the earlycon not disabled after the init code
> b) the serial console driver's runtime suspend kills the requiered
>    clks/pm domain, since they are not referenced by the earlycon.
>
> This serial fix 1/ case only.
>
>
>
> Jeffy Chen (5):
>   serial: arc: Remove __init marking from early write
>   serial: omap: Remove __init marking from early write
>   serial: xuartps: Remove __init marking from early write
>   serial: 8250_ingenic: Remove __init marking from early write
>   serial: 8250_early: Remove __init marking from early write
>
>  drivers/tty/serial/8250/8250_early.c   |  8 ++++----
>  drivers/tty/serial/8250/8250_ingenic.c |  8 ++++----
>  drivers/tty/serial/arc_uart.c          |  4 ++--
>  drivers/tty/serial/omap-serial.c       | 13 ++++++-------
>  drivers/tty/serial/xilinx_uartps.c     |  2 +-
>  5 files changed, 17 insertions(+), 18 deletions(-)

I looked through all 5 patches and they seem sane to me.  I didn't go
through and test them since Brian already tested the patches for the
one UART driver I'd have access to anyway.

As per my previous email, I don't see any need to solve all the
world'd problems with this patch series, plus the keep_bootcon seems
to be an experts / debugging type option and it seems sane if you need
to take care in using it.

So officially for the series (FWIW since I'm just an interested 3rd party):

Reviewed-by: Douglas Anderson <dianders@chromium.org>


-Doug

[PATCH 0/5] earlycon hang under some conditions

[Andy Shevchenko]

On Tue, Jul 18, 2017 at 9:02 AM, Jeffy Chen <jeffy.chen@rock-chips.com> wrote:
> I was testing earlycon with 8250 dw serial console. And it hangs in
> these cases:
> 1/ kernel hang when calling early write function after free_initmem:
> a) the earlycon not disabled after the init code(due to keep_bootcon or
>    not specify a real console to switch to)
> b) the early write func is marked as __init, for example 8250_early.

> This serial fix 1/ case only.

Here is another opinion.
I can tell that conditions to get into the trouble are quite unusual or rare.

Any rationale behind why one needs such a combination of parameters?

P.S. For doulble output ask Sergey and Petr (they are quite concerned
about it). Actually strange I didn't see their names in Cc list for
console related stuff,

-- 
With Best Regards,
Andy Shevchenko