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

[Alistair Popple]

Stewart,

On Mon, 9 Nov 2015 13:10:29 Stewart Smith wrote:
> 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?

Actually it's the opposite. If the reboot timeout is set we reboot but not 
always before flushing the lpc uart buffer and hence we miss the output.

We still have a bug when the reboot timeout is 0. I believe when the kernel 
panics opal_poll_event() stops getting called. Hence the lpc uart buffer gets 
filled but nothing is called to flush it out.

> > 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....

Yeah, probably a good idea.

> > --- 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).
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>