[PATCH] serial: pl011: use port lock to guard control register access

[PATCH] serial: pl011: use port lock to guard control register access

[Jon Medhurst (Tixy)]

When the pl011 is being used for a console, pl011_console_write forces
the control register (CR) to enable the UART for transmission and then
restores this to the original value afterwards. It does this while
holding the port lock.

Unfortunately, when the uart is started or shutdown - say in response to
userland using the serial device for a terminal - then this updates the
control register without any locking.

This means we can have

  pl011_console_write   Save CR
  pl011_startup         Initialise CR, e.g. enable receive
  pl011_console_write   Restore old CR with receive not enabled

this result is a serial port which doesn't respond to any input.

A similar race in reverse could happen when the device is shutdown.

We can fix these problems by taking the port lock when updating CR.

Signed-off-by: Jon Medhurst <tixy@linaro.org>
---

I can't help but think I've missed something fundamental here as it
seems wrong to be in the situation at all where were initialising
hardware if it is already being used?

Also, even with this fix I still see spurious/corrupt output on the
serial port at the point the port is initialised, so there is obviously
some other issues in this area.

 drivers/tty/serial/amba-pl011.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 7203864..a0dfb86 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1537,6 +1537,8 @@ static int pl011_startup(struct uart_port *port)
 	/*
 	 * Provoke TX FIFO interrupt into asserting.
 	 */
+	spin_lock_irq(&uap->port.lock);
+
 	cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE;
 	writew(cr, uap->port.membase + UART011_CR);
 	writew(0, uap->port.membase + UART011_FBRD);
@@ -1561,6 +1563,8 @@ static int pl011_startup(struct uart_port *port)
 	cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
 
+	spin_unlock_irq(&uap->port.lock);
+
 	/*
 	 * initialise the old status of the modem signals
 	 */
@@ -1629,11 +1633,13 @@ static void pl011_shutdown(struct uart_port *port)
 	 * it during startup().
 	 */
 	uap->autorts = false;
+	spin_lock_irq(&uap->port.lock);
 	cr = readw(uap->port.membase + UART011_CR);
 	uap->old_cr = cr;
 	cr &= UART011_CR_RTS | UART011_CR_DTR;
 	cr |= UART01x_CR_UARTEN | UART011_CR_TXE;
 	writew(cr, uap->port.membase + UART011_CR);
+	spin_unlock_irq(&uap->port.lock);
 
 	/*
 	 * disable break condition and fifos
-- 
1.7.10.4

[PATCH] serial: pl011: use port lock to guard control register access

[Nicolas Pitre]

On Wed, 4 Dec 2013, Jon Medhurst (Tixy) wrote:

> I can't help but think I've missed something fundamental here as it
> seems wrong to be in the situation at all where were initialising
> hardware if it is already being used?
> 
> Also, even with this fix I still see spurious/corrupt output on the
> serial port at the point the port is initialised, so there is obviously
> some other issues in this area.

I don't know if this is still the case nowdays, but that might be due to 
the fact that the tty layer initializes the port speed to 9600 bauds 
when opened.  So there might be a period when the port goes from 115200 
bauds (from the serial console setup) to 9600 bauds, to finally go back 
to 115200 bauds configured by the getty process.  If the serial console 
is in the middle of outputting something when user space opens the port 
then you might see garbled output for a brief period.

[ On some boards the typical speed is 38400 instead of 115200 but the 
  same concern applies. ]

I think that if a serial console has already initialized a port then the 
tty's initial parameters could simply default to those from the already 
active serial console sharing the same port.


Nicolas

[PATCH] serial: pl011: use port lock to guard control register access

[Jon Medhurst (Tixy)]

On Wed, 2013-12-04 at 11:17 -0500, Nicolas Pitre wrote:
> On Wed, 4 Dec 2013, Jon Medhurst (Tixy) wrote:
> 
> > I can't help but think I've missed something fundamental here as it
> > seems wrong to be in the situation at all where were initialising
> > hardware if it is already being used?
> > 
> > Also, even with this fix I still see spurious/corrupt output on the
> > serial port at the point the port is initialised, so there is obviously
> > some other issues in this area.
> 
> I don't know if this is still the case nowdays, but that might be due to 
> the fact that the tty layer initializes the port speed to 9600 bauds 
> when opened.  So there might be a period when the port goes from 115200 
> bauds (from the serial console setup) to 9600 bauds, to finally go back 
> to 115200 bauds configured by the getty process.  If the serial console 
> is in the middle of outputting something when user space opens the port 
> then you might see garbled output for a brief period.

I've just got back to looking at this and the baud rate and data format
don't get gratuitously changed by the serial framework, just by
pl011_startup which sets the port to max baud rate and shortest bit size
in order to send one character via loopback mode (relevant code copied
below). These values get restored fairly quickly afterwards by a call to
pl011_set_termios, but there is a window where they are wrong, leading
to corruption if console outputs text during this window.

If I save and restore register values in this startup code then that
seems to fix things.

I guess an alternative would be to leave the registers unchanged in this
initialisation code if the port is being used for a console. But I'm
unsure how to reliably detect that is the case or if using the fact that
the port has already been setup once means that it's safe to assume it
stays set up, e.g. state isn't lost though power management actions.

	/*
	 * Provoke TX FIFO interrupt into asserting.
	 */
	cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE;
	writew(cr, uap->port.membase + UART011_CR);
	writew(0, uap->port.membase + UART011_FBRD);
	writew(1, uap->port.membase + UART011_IBRD);
	writew(0, uap->port.membase + uap->lcrh_rx);
	if (uap->lcrh_tx != uap->lcrh_rx) {
		int i;
		/*
		 * Wait 10 PCLKs before writing LCRH_TX register,
		 * to get this delay write read only register 10 times
		 */
		for (i = 0; i < 10; ++i)
			writew(0xff, uap->port.membase + UART011_MIS);
		writew(0, uap->port.membase + uap->lcrh_tx);
	}
	writew(0, uap->port.membase + UART01x_DR);
	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
		barrier();

-- 
Tixy