PATCH (RFC): Rework the 8250 console fix

PATCH (RFC): Rework the 8250 console fix

[Alan Cox]

Russell got various bits of mail showing the 8250 console fix broke some
setups with the assumptions it made. Based on this suggestion that we
actually need to just do the locking I've prepared an alternative
patch. 

The patch does one thing a little oddly. If an oops is in progress it
only attempts to take the port lock and continues regardless. In that
situation the oops will hit the console but things may be a bit odd
thereafter. Getting the oops out seems to be the important thing to do
in that situation.

There are two questions that I think make this an RFC not a final patch

1.	Should this be pushed up into serial/serial_core.c for all chips.

2.	Is the use of local_irq_save/spin_trylock spin_unlock_irqrestore
going to break any platforms that do weird stuff or do we need a
spin_trylock_irqsave ?


Alan

(no signed off by as it isn't yet appropriate to merge.. and if I don't
sign it off nobody can jump the gun 8))

--- drivers/serial/8250.c~	2006-05-02 14:28:05.430397240 +0100
+++ drivers/serial/8250.c	2006-05-02 14:28:05.430397240 +0100
@@ -2201,7 +2201,18 @@
 {
 	struct uart_8250_port *up = &serial8250_ports[co->index];
 	unsigned int ier;
+	unsigned long flags;
+	int locked = 1;
 
+	if (unlikely(oops_in_progress)) {
+		/* We want our private lock to be ignored during an oops. This
+		   might cause a serial console stall afterwards but the oops data
+		   is the critical information to get out */
+		local_irq_save(flags);
+		locked = spin_trylock(&up->port.lock);
+	} else
+		spin_lock_irqsave(&up->port.lock, flags);
+		
 	touch_nmi_watchdog();
 
 	/*
@@ -2221,8 +2232,12 @@
 	 *	and restore the IER
 	 */
 	wait_for_xmitr(up, BOTH_EMPTY);
-	up->ier |= UART_IER_THRI;
-	serial_out(up, UART_IER, ier | UART_IER_THRI);
+	serial_out(up, UART_IER, ier);
+	
+	if (locked)
+		spin_unlock_irqrestore(&up->port.lock, flags);
+	else
+		local_irq_restore(flags);
 }
 
 static int serial8250_console_setup(struct console *co, char *options)

Re: PATCH (RFC): Rework the 8250 console fix

[David Woodhouse]

On Tue, 2006-05-02 at 15:09 +0100, Alan Cox wrote:
> There are two questions that I think make this an RFC not a final
> patch
> 
> 1.      Should this be pushed up into serial/serial_core.c for all chips. 

Yes, I think it probably should. It's icky enough that we want one copy
of it only.

-- 
dwmw2

Re: PATCH (RFC): Rework the 8250 console fix

[Russell King]

On Tue, May 02, 2006 at 03:05:02PM +0100, David Woodhouse wrote:
> On Tue, 2006-05-02 at 15:09 +0100, Alan Cox wrote:
> > There are two questions that I think make this an RFC not a final
> > patch
> > 
> > 1.      Should this be pushed up into serial/serial_core.c for all chips. 
> 
> Yes, I think it probably should. It's icky enough that we want one copy
> of it only.

That'd mean we have to add extra methods:

void uart_console_write(struct uart_port *port, const char *s,
                        unsigned int count,
                        void (*putchar)(struct uart_port *, int),
			void *(*pre_write)(struct uart_port *),
			void (*post_write)(struct uart_port *, void *));

where the pre-write places data into some kind of driver specific
structure which is allocated by some weird method, and post_write
restores this information.

Why are these driver specific?  Review all the users of uart_console_write
and you'll find that there's many different requirements for what is
done both before and after the uart_console_write method, most of
which should arguably fall under such a lock.

And I think that calling kmalloc() from within an oops (to allocate the
memory to pass the current state between pre and post write methods)
would be very bad news.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: PATCH (RFC): Rework the 8250 console fix

[Russell King]

On Tue, May 02, 2006 at 03:09:43PM +0100, Alan Cox wrote:
> Russell got various bits of mail showing the 8250 console fix broke some
> setups with the assumptions it made. Based on this suggestion that we
> actually need to just do the locking I've prepared an alternative
> patch. 

I had merged your suggestion this morning, though I've just redone it
to fix a bug.

> There are two questions that I think make this an RFC not a final patch
> 
> 1.	Should this be pushed up into serial/serial_core.c for all chips.

No - uart_console_write doesn't know enough about the hardware to be
able to take the spinlock itself.

> 2.	Is the use of local_irq_save/spin_trylock spin_unlock_irqrestore
> going to break any platforms that do weird stuff or do we need a
> spin_trylock_irqsave ?

We do have a spin_trylock_irqsave(), so we don't need to worry about this.

b41d859ea4471c8784c500b97cdf82c7529c19ee
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -2235,10 +2235,17 @@ static void
 serial8250_console_write(struct console *co, const char *s, unsigned int count)
 {
 	struct uart_8250_port *up = &serial8250_ports[co->index];
+	unsigned long flags;
 	unsigned int ier;
+	int locked = 1;
 
 	touch_nmi_watchdog();
 
+	if (oops_in_progress) {
+		locked = spin_trylock_irqsave(&up->port.lock, flags);
+	} else
+		spin_lock_irqsave(&up->port.lock, flags);
+
 	/*
 	 *	First save the IER then disable the interrupts
 	 */
@@ -2257,6 +2265,9 @@ serial8250_console_write(struct console 
 	 */
 	wait_for_xmitr(up, BOTH_EMPTY);
 	serial_out(up, UART_IER, ier);
+
+	if (locked)
+		spin_unlock_irqrestore(&up->port.lock, flags);
 }
 
 static int serial8250_console_setup(struct console *co, char *options)


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core