* [PATCH 1/3] serial: amba-pl011: use port lock to guard control register access
2013-12-10 10:18 [PATCH 0/3] serial: amba-pl011: fix race conditions during initialisation Jon Medhurst
@ 2013-12-10 10:18 ` Jon Medhurst
2013-12-10 10:18 ` [PATCH 2/3] serial: amba-pl011: factor out code for writing LCR_H register Jon Medhurst
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Jon Medhurst @ 2013-12-10 10:18 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] serial: amba-pl011: factor out code for writing LCR_H register
2013-12-10 10:18 [PATCH 0/3] serial: amba-pl011: fix race conditions during initialisation Jon Medhurst
2013-12-10 10:18 ` [PATCH 1/3] serial: amba-pl011: use port lock to guard control register access Jon Medhurst
@ 2013-12-10 10:18 ` Jon Medhurst
2013-12-10 10:19 ` [PATCH 3/3] serial: amba-pl011: preseserve hardware settings during initialisation Jon Medhurst
2013-12-10 18:09 ` [PATCH 0/3] serial: amba-pl011: fix race conditions " Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Jon Medhurst @ 2013-12-10 10:18 UTC (permalink / raw)
To: linux-arm-kernel
The code to cope with a split tx/rx LCR_H register is non-trivial
so put it into it's own function to avoid duplication.
Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 39 +++++++++++++++++----------------------
1 file changed, 17 insertions(+), 22 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index a0dfb86..8ec1729 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1513,6 +1513,21 @@ static int pl011_hwinit(struct uart_port *port)
return retval;
}
+static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
+{
+ writew(lcr_h, uap->port.membase + uap->lcrh_rx);
+ if (uap->lcrh_rx != uap->lcrh_tx) {
+ 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(lcr_h, uap->port.membase + uap->lcrh_tx);
+ }
+}
+
static int pl011_startup(struct uart_port *port)
{
struct uart_amba_port *uap = (struct uart_amba_port *)port;
@@ -1543,17 +1558,7 @@ static int pl011_startup(struct uart_port *port)
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);
- }
+ pl011_write_lcr_h(uap, 0);
writew(0, uap->port.membase + UART01x_DR);
while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
barrier();
@@ -1803,17 +1808,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
* UART011_FBRD & UART011_IBRD.
* ----------^----------^----------^----------^-----
*/
- writew(lcr_h, port->membase + uap->lcrh_rx);
- if (uap->lcrh_rx != uap->lcrh_tx) {
- 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(lcr_h, port->membase + uap->lcrh_tx);
- }
+ pl011_write_lcr_h(uap, lcr_h);
writew(old_cr, port->membase + UART011_CR);
spin_unlock_irqrestore(&port->lock, flags);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] serial: amba-pl011: preseserve hardware settings during initialisation
2013-12-10 10:18 [PATCH 0/3] serial: amba-pl011: fix race conditions during initialisation Jon Medhurst
2013-12-10 10:18 ` [PATCH 1/3] serial: amba-pl011: use port lock to guard control register access Jon Medhurst
2013-12-10 10:18 ` [PATCH 2/3] serial: amba-pl011: factor out code for writing LCR_H register Jon Medhurst
@ 2013-12-10 10:19 ` Jon Medhurst
2013-12-10 18:09 ` [PATCH 0/3] serial: amba-pl011: fix race conditions " Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Jon Medhurst @ 2013-12-10 10:19 UTC (permalink / raw)
To: linux-arm-kernel
During initialisation, a UART may already be in use for a console, so
take care to preserve things like baud rate and data format to avoid
corrupting console output.
Signed-off-by: Jon Medhurst <tixy@linaro.org>
---
drivers/tty/serial/amba-pl011.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8ec1729..7012ef3 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1531,7 +1531,7 @@ static void pl011_write_lcr_h(struct uart_amba_port *uap, unsigned int lcr_h)
static int pl011_startup(struct uart_port *port)
{
struct uart_amba_port *uap = (struct uart_amba_port *)port;
- unsigned int cr;
+ unsigned int cr, lcr_h, fbrd, ibrd;
int retval;
retval = pl011_hwinit(port);
@@ -1550,10 +1550,16 @@ static int pl011_startup(struct uart_port *port)
writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS);
/*
- * Provoke TX FIFO interrupt into asserting.
+ * Provoke TX FIFO interrupt into asserting. Taking care to preserve
+ * baud rate and data format specified by FBRD, IBRD and LCRH as the
+ * UART may already be in use as a console.
*/
spin_lock_irq(&uap->port.lock);
+ fbrd = readw(uap->port.membase + UART011_FBRD);
+ ibrd = readw(uap->port.membase + UART011_IBRD);
+ lcr_h = readw(uap->port.membase + uap->lcrh_rx);
+
cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE;
writew(cr, uap->port.membase + UART011_CR);
writew(0, uap->port.membase + UART011_FBRD);
@@ -1563,6 +1569,10 @@ static int pl011_startup(struct uart_port *port)
while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
barrier();
+ writew(fbrd, uap->port.membase + UART011_FBRD);
+ writew(ibrd, uap->port.membase + UART011_IBRD);
+ pl011_write_lcr_h(uap, lcr_h);
+
/* restore RTS and DTR */
cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 0/3] serial: amba-pl011: fix race conditions during initialisation
2013-12-10 10:18 [PATCH 0/3] serial: amba-pl011: fix race conditions during initialisation Jon Medhurst
` (2 preceding siblings ...)
2013-12-10 10:19 ` [PATCH 3/3] serial: amba-pl011: preseserve hardware settings during initialisation Jon Medhurst
@ 2013-12-10 18:09 ` Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2013-12-10 18:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Dec 10, 2013 at 10:18:57AM +0000, Jon Medhurst wrote:
> When a UART that is being used for a kernel console is also opened
> by user-side, e.g. for use as a terminal, then there are race conditions
> which can lead to terminal input being disabled and console output being
> corrupted. This small patch series fixes these.
>
> Jon Medhurst (3):
> serial: amba-pl011: use port lock to guard control register access
> serial: amba-pl011: factor out code for writing LCR_H register
> serial: amba-pl011: preseserve hardware settings during initialisation
Given that this seems relatively easy to trigger when you know how and
renders the device non-functional perhaps this should be considered for
stable too?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131210/60ba34f5/attachment.sig>
^ permalink raw reply [flat|nested] 5+ messages in thread