[Skiboot] [PATCH] console: Completely flush output buffer before power down and reboot

[Stewart Smith]

Russell Currey <ruscur at russell.cc> writes:
> Completely flush the output buffer of the console driver before
> power down and reboot.  Implements the flushing function for uart
> consoles, which includes the astbmc and rhesus platforms.
>
> Adds a new function, flush(), to the con_ops struct that allows
> each console driver to specify how their output buffers are flushed.
>
> In the cec_power_down and cec_reboot functions, the flush function
> of the driver is called if it exists.
>
> This fixes an issue where some console output is sometimes lost before
> power down or reboot in uart consoles. If this issue is also prevalent
> in other console types then it can be fixed later by adding a .flush
> to that driver's con_ops.

I guess this is specifically problematic in some panic() situations,
especially when the panic reboot timeout is set to 0 (i.e. that Linux
does *not* reboot after panic) as we then spin in a (for lack of a
better term) death-loop and never call into OPAL to do something like,
say, flush the panic message?

> Signed-off-by: Russell Currey <ruscur at russell.cc>

should this also head towards stable? I think we've had a bug open for
it... or at least there's a possible case to make for including it in
stable....

> --- a/hw/lpc-uart.c
> +++ b/hw/lpc-uart.c
> @@ -129,36 +129,6 @@ static void uart_update_ier(void)
>  	}
>  }
>  
> -/*
> - * Internal console driver (output only)
> - */
> -static size_t uart_con_write(const char *buf, size_t len)
> -{
> -	size_t written = 0;
> -
> -	/* If LPC bus is bad, we just swallow data */
> -	if (!lpc_ok())
> -		return written;
> -
> -	lock(&uart_lock);
> -	while(written < len) {
> -		if (tx_room == 0) {
> -			uart_wait_tx_room();
> -			if (tx_room == 0)
> -				goto bail;
> -		} else {
> -			uart_write(REG_THR, buf[written++]);
> -			tx_room--;
> -		}
> -	}
> - bail:
> -	unlock(&uart_lock);
> -	return written;
> -}
> -
> -static struct con_ops uart_con_driver = {
> -	.write = uart_con_write
> -};
>  

This looks like an in-place move, which I'd prefer to happen as a
separate commit.

In merging this patch, I've reverted this change just to keep the diff
as easy to read as possible. Feel free to submit another patch
rearranging things in hw/lpc-uart.c to make more sense though.

> +static void uart_con_flush_all(void)
> +{
> +	while(out_buf_prod != out_buf_cons)
> +		uart_flush_out();
> +}
> +
> +static struct con_ops uart_con_driver = {
> +	.write = uart_con_write,
> +	.flush = uart_con_flush_all
> +};

This also looks like it was moved around, in applying the patch i've
left the structure where it was (to make the diff clearer). Feel free to
submit another patch that moves it to somewhere better.

> diff --git a/include/console.h b/include/console.h
> index e426adb..0dbbf39 100644
> --- a/include/console.h
> +++ b/include/console.h
> @@ -51,6 +51,7 @@ struct con_ops {
>  	size_t (*write)(const char *buf, size_t len);
>  	size_t (*read)(char *buf, size_t len);
>  	bool (*poll_read)(void);
> +	void (*flush)(void);
>  };
>  
>  extern struct lock con_lock;
> @@ -61,6 +62,8 @@ extern bool flush_console(void);
>  extern bool __flush_console(bool flush_to_drivers);
>  extern void set_console(struct con_ops *driver);
>  
> +extern void flush_driver_output_buffer(void);
> +

I renamed this to flush_console_driver(); as that seemed to say what it
was doing a bit better (and also meant I could easily remove the
comments above the calls in platform.c

>  extern int mambo_read(void);
>  extern void mambo_write(const char *buf, size_t count);
>  extern void enable_mambo_console(void);

merged into master as of 1205ead90747 (with my changes mentioned above).