Re: [PATCH] Convert serial_core oopses to BUG_ON

[Pavel Machek <pavel@ucw.cz>]

On St 01-03-06 17:10:46, Russell King wrote:
> On Tue, Feb 28, 2006 at 03:32:56PM -0800, Andrew Morton wrote:
> > Martin Michlmayr <tbm@cyrius.com> wrote:
> > >
> > > * Andrew Morton <akpm@osdl.org> [2006-02-28 10:17]:
> > > > > It will oops in hard-to-guess, place, anyway.
> > > > Will it?   Where?  Unfixably?
> > > 
> > > http://www.linux-mips.org/archives/linux-mips/2006-02/msg00241.html is
> > > one example we just had on MIPS.  On SGI IP22, using the serial
> > > console, you'd get the following on shutdown:
> > > 
> > > The system is going down for reboot NOW!
> > > INIT: Sending processes the TERM signal
> > > INIT: Sending proces
> > > 
> > > and then nothing at all.  I'd never have suspected the serial driver,
> > > had not users reported that the machine shutdowns properly when using
> > > the framebuffer.
> > > 
> > > For the record, I don't mind whether it's BUG_ON or WARN_ON, but I
> > > just wanted to give this as an example of an "oops in hard-to-guess,
> > > place".
> > 
> > >From my reading of the above thread, putting the proposed workaround into
> > serial core will indeed allow people's machines to keep running while
> > reminding us about the driver bugs.
> 
> I would much rather the buggy drivers were actually fixed - is there a
> reason why the drivers can't actually be fixed (other than lazyness)?
> 
> Once they're fixed, adding a BUG_ON then becomes practical IMHO - it'll
> stop new driver writers being confused.

Okay, but lets add BUG_ONs that actually work. BUG_ON in second hunk
could never trigger, and last hunk did not help in specific case of
bluetooth problem. This should be better: it warns at right places,
but allows system to survive. I'll now try to fix bluetooth problem.

Signed-off-by: Pavel Machek <pavel@suse.cz>

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 95fb493..cc1faa3 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -71,6 +71,11 @@ static void uart_change_pm(struct uart_s
 void uart_write_wakeup(struct uart_port *port)
 {
 	struct uart_info *info = port->info;
+	/*
+	 * This means you called this function _after_ the port was
+	 * closed.  No cookie for you.
+	 */
+	BUG_ON(!info);
 	tasklet_schedule(&info->tlet);
 }
 
@@ -471,14 +476,26 @@ static void uart_flush_chars(struct tty_
 }
 
 static int
-uart_write(struct tty_struct *tty, const unsigned char * buf, int count)
+uart_write(struct tty_struct *tty, const unsigned char *buf, int count)
 {
 	struct uart_state *state = tty->driver_data;
-	struct uart_port *port = state->port;
-	struct circ_buf *circ = &state->info->xmit;
+	struct uart_port *port;
+	struct circ_buf *circ;
 	unsigned long flags;
 	int c, ret = 0;
 
+	/*
+	 * This means you called this function _after_ the port was
+	 * closed.  No cookie for you.
+	 */
+	if (!state || !state->info) {
+		WARN_ON(1);
+		return -EL3HLT;
+	}
+
+	port = state->port;
+	circ = &state->info->xmit;
+
 	if (!circ->buf)
 		return 0;
 
@@ -521,6 +538,15 @@ static void uart_flush_buffer(struct tty
 	struct uart_port *port = state->port;
 	unsigned long flags;
 
+	/*
+	 * This means you called this function _after_ the port was
+	 * closed.  No cookie for you.
+	 */
+	if (!state || !state->info) {
+		WARN_ON(1);
+		return;
+	}
+
 	DPRINTK("uart_flush_buffer(%d) called\n", tty->index);
 
 	spin_lock_irqsave(&port->lock, flags);


-- 
Web maintainer for suspend.sf.net (www.sf.net/projects/suspend) wanted...