linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] tty: xuartps: Fix lock ups
@ 2015-11-05  7:21 Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 01/10] tty: xuartps: Beautify read-modify writes Soren Brinkmann
                   ` (9 more replies)
  0 siblings, 10 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Hi,

this is v2, hopefully without any build issues. The changes are the same
as before, just the warning reported by kbuild test robot about unused
variables have been fixed. And, I added one more patch with some
cleanup/refactoring.

I recently found my system locking up under some conditions. I dug
through the code a bit and the result is this collections of various
changes. Some of it may not be required and has just been created out of
half-baked theories and re-reading the documentation. The actual failing
scenarios seem to be:
 - RX IRQ conditions are handled while the receiver is disabled, leaving
   the driver in an infinite loop
 - console_put_char seems to be interrupted by uart_shutdown disabling
   the transmitter while/just before printing

I.e. overall I tried to serialize all operations using the port lock to
avoid such interaction.

	Sören

Sören Brinkmann (10):
  tty: xuartps: Beautify read-modify writes
  tty: xuartps: Use spinlock to serialize HW access
  tty: xuartps: Always enable transmitter in start_tx
  tty: xuartps: Clear interrupt status register in shutdown
  tty: xuartps: Improve startup function
  tty: xuartps: Keep lock for whole ISR
  tty: xuartps: Acquire port lock for shutdown
  tty: xuartps: Move RX path into helper function
  tty: xuartps: Only handle RX IRQs when RX is enabled
  tty: xuartps: Cleanup: Reformat if-else

 drivers/tty/serial/xilinx_uartps.c | 247 ++++++++++++++++++++-----------------
 1 file changed, 136 insertions(+), 111 deletions(-)

-- 
2.6.2.3.ga463a5b


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 01/10] tty: xuartps: Beautify read-modify writes
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 02/10] tty: xuartps: Use spinlock to serialize HW access Soren Brinkmann
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Non-functional, formatting changes to ease reading the code.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 009e0dbc12d2..50d4082d2354 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -515,12 +515,14 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
 		return;
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-	/* Set the TX enable bit and clear the TX disable bit to enable the
+	/*
+	 * Set the TX enable bit and clear the TX disable bit to enable the
 	 * transmitter.
 	 */
-	writel((status & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= ~CDNS_UART_CR_TX_DIS;
+	status |= CDNS_UART_CR_TX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
@@ -1123,8 +1125,9 @@ static void cdns_uart_console_write(struct console *co, const char *s,
 	 * clear the TX disable bit to enable the transmitter.
 	 */
 	ctrl = readl(port->membase + CDNS_UART_CR_OFFSET);
-	writel((ctrl & ~CDNS_UART_CR_TX_DIS) | CDNS_UART_CR_TX_EN,
-			port->membase + CDNS_UART_CR_OFFSET);
+	ctrl &= ~CDNS_UART_CR_TX_DIS;
+	ctrl |= CDNS_UART_CR_TX_EN;
+	writel(ctrl, port->membase + CDNS_UART_CR_OFFSET);
 
 	uart_console_write(port, s, count, cdns_uart_console_putchar);
 	cdns_uart_console_wait_tx(port);
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 02/10] tty: xuartps: Use spinlock to serialize HW access
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 01/10] tty: xuartps: Beautify read-modify writes Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 03/10] tty: xuartps: Always enable transmitter in start_tx Soren Brinkmann
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Instead of disabling the IRQ, use the spin lock to serialize accesses to
the HW. This protects the driver from interference of non-IRQ callbacks
with each other and makes the driver more consistent in its
serialization method.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - remove unused variable
---
 drivers/tty/serial/xilinx_uartps.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 50d4082d2354..2c98c357d9a0 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -945,12 +945,10 @@ static void cdns_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
 #ifdef CONFIG_CONSOLE_POLL
 static int cdns_uart_poll_get_char(struct uart_port *port)
 {
-	u32 imr;
 	int c;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Check if FIFO is empty */
 	if (readl(port->membase + CDNS_UART_SR_OFFSET) & CDNS_UART_SR_RXEMPTY)
@@ -959,19 +957,16 @@ static int cdns_uart_poll_get_char(struct uart_port *port)
 		c = (unsigned char) readl(
 					port->membase + CDNS_UART_FIFO_OFFSET);
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return c;
 }
 
 static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 {
-	u32 imr;
+	unsigned long flags;
 
-	/* Disable all interrupts */
-	imr = readl(port->membase + CDNS_UART_IMR_OFFSET);
-	writel(imr, port->membase + CDNS_UART_IDR_OFFSET);
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Wait until FIFO is empty */
 	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
@@ -986,8 +981,7 @@ static void cdns_uart_poll_put_char(struct uart_port *port, unsigned char c)
 				CDNS_UART_SR_TXEMPTY))
 		cpu_relax();
 
-	/* Enable interrupts */
-	writel(imr, port->membase + CDNS_UART_IER_OFFSET);
+	spin_unlock_irqrestore(&port->lock, flags);
 
 	return;
 }
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 03/10] tty: xuartps: Always enable transmitter in start_tx
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 01/10] tty: xuartps: Beautify read-modify writes Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 02/10] tty: xuartps: Use spinlock to serialize HW access Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 04/10] tty: xuartps: Clear interrupt status register in shutdown Soren Brinkmann
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

start_tx must start transmitting characters. Regardless of the state of
the circular buffer, always enable the transmitter hardware.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 2c98c357d9a0..df6778d17949 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -512,9 +512,6 @@ static void cdns_uart_start_tx(struct uart_port *port)
 {
 	unsigned int status, numbytes = port->fifosize;
 
-	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
-		return;
-
 	/*
 	 * Set the TX enable bit and clear the TX disable bit to enable the
 	 * transmitter.
@@ -524,6 +521,9 @@ static void cdns_uart_start_tx(struct uart_port *port)
 	status |= CDNS_UART_CR_TX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
+	if (uart_circ_empty(&port->state->xmit) || uart_tx_stopped(port))
+		return;
+
 	while (numbytes-- && ((readl(port->membase + CDNS_UART_SR_OFFSET) &
 				CDNS_UART_SR_TXFULL)) != CDNS_UART_SR_TXFULL) {
 		/* Break if no more data available in the UART buffer */
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 04/10] tty: xuartps: Clear interrupt status register in shutdown
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (2 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 03/10] tty: xuartps: Always enable transmitter in start_tx Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 05/10] tty: xuartps: Improve startup function Soren Brinkmann
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

When shutting down the UART, clear the interrupt status register.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index df6778d17949..b7fc30639292 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -825,6 +825,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
 	writel(status, port->membase + CDNS_UART_IDR_OFFSET);
+	writel(0xffffffff, port->membase + CDNS_UART_ISR_OFFSET);
 
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 05/10] tty: xuartps: Improve startup function
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (3 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 04/10] tty: xuartps: Clear interrupt status register in shutdown Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 06/10] tty: xuartps: Keep lock for whole ISR Soren Brinkmann
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

The startup function is supposed to initialize the UART for receiving.
Hence, don't enable the TX part. Also, protect HW accesses with the port
lock.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index b7fc30639292..167e0f4bcf7a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -755,6 +755,7 @@ static void cdns_uart_set_termios(struct uart_port *port,
  */
 static int cdns_uart_startup(struct uart_port *port)
 {
+	unsigned long flags;
 	unsigned int retval = 0, status = 0;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
@@ -762,6 +763,8 @@ static int cdns_uart_startup(struct uart_port *port)
 	if (retval)
 		return retval;
 
+	spin_lock_irqsave(&port->lock, flags);
+
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
@@ -772,15 +775,14 @@ static int cdns_uart_startup(struct uart_port *port)
 	writel(CDNS_UART_CR_TXRST | CDNS_UART_CR_RXRST,
 			port->membase + CDNS_UART_CR_OFFSET);
 
-	status = readl(port->membase + CDNS_UART_CR_OFFSET);
-
-	/* Clear the RX disable and TX disable bits and then set the TX enable
-	 * bit and RX enable bit to enable the transmitter and receiver.
+	/*
+	 * Clear the RX disable bit and then set the RX enable bit to enable
+	 * the receiver.
 	 */
-	writel((status & ~(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS))
-			| (CDNS_UART_CR_TX_EN | CDNS_UART_CR_RX_EN |
-			CDNS_UART_CR_STOPBRK),
-			port->membase + CDNS_UART_CR_OFFSET);
+	status = readl(port->membase + CDNS_UART_CR_OFFSET);
+	status &= CDNS_UART_CR_RX_DIS;
+	status |= CDNS_UART_CR_RX_EN;
+	writel(status, port->membase + CDNS_UART_CR_OFFSET);
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -811,6 +813,8 @@ static int cdns_uart_startup(struct uart_port *port)
 		CDNS_UART_IXR_RXTRIG | CDNS_UART_IXR_TOUT,
 		port->membase + CDNS_UART_IER_OFFSET);
 
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	return retval;
 }
 
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 06/10] tty: xuartps: Keep lock for whole ISR
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (4 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 05/10] tty: xuartps: Improve startup function Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 07/10] tty: xuartps: Acquire port lock for shutdown Soren Brinkmann
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

The RX path in the interrupt handler released a lock unnecessarily.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 167e0f4bcf7a..5efd1b9d220f 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -265,9 +265,7 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
 					data, status);
 		}
-		spin_unlock(&port->lock);
 		tty_flip_buffer_push(&port->state->port);
-		spin_lock(&port->lock);
 	}
 
 	/* Dispatch an appropriate handler */
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 07/10] tty: xuartps: Acquire port lock for shutdown
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (5 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 06/10] tty: xuartps: Keep lock for whole ISR Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 08/10] tty: xuartps: Move RX path into helper function Soren Brinkmann
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Shutting down the UART port can happen while console operations are in
progress. Holding the port lock serializes these operations and avoids
the UART HW to be disabled in the middle of console prints.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 5efd1b9d220f..7fd226d65496 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -823,6 +823,9 @@ static int cdns_uart_startup(struct uart_port *port)
 static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
 
 	/* Disable interrupts */
 	status = readl(port->membase + CDNS_UART_IMR_OFFSET);
@@ -832,6 +835,9 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+
 	free_irq(port->irq, port);
 }
 
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 08/10] tty: xuartps: Move RX path into helper function
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (6 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 07/10] tty: xuartps: Acquire port lock for shutdown Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 10/10] tty: xuartps: Cleanup: Reformat if-else Soren Brinkmann
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Move RX-related IRQ handling into a helper function.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 50 +++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 7fd226d65496..aa1176e6661a 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -172,28 +172,8 @@ struct cdns_uart {
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
-/**
- * cdns_uart_isr - Interrupt handler
- * @irq: Irq number
- * @dev_id: Id of the port
- *
- * Return: IRQHANDLED
- */
-static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 {
-	struct uart_port *port = (struct uart_port *)dev_id;
-	unsigned long flags;
-	unsigned int isrstatus, numbytes;
-	unsigned int data;
-	char status = TTY_NORMAL;
-
-	spin_lock_irqsave(&port->lock, flags);
-
-	/* Read the interrupt status register to determine which
-	 * interrupt(s) is/are active.
-	 */
-	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
-
 	/*
 	 * There is no hardware break detection, so we interpret framing
 	 * error with all-zeros data as a break sequence. Most of the time,
@@ -223,6 +203,9 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 		/* Receive Timeout Interrupt */
 		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
 					CDNS_UART_SR_RXEMPTY)) {
+			u32 data;
+			char status = TTY_NORMAL;
+
 			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
 
 			/* Non-NULL byte after BREAK is garbage (99%) */
@@ -263,10 +246,33 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 			}
 
 			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					data, status);
+					 data, status);
 		}
 		tty_flip_buffer_push(&port->state->port);
 	}
+}
+
+/**
+ * cdns_uart_isr - Interrupt handler
+ * @irq: Irq number
+ * @dev_id: Id of the port
+ *
+ * Return: IRQHANDLED
+ */
+static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
+{
+	struct uart_port *port = (struct uart_port *)dev_id;
+	unsigned long flags;
+	unsigned int isrstatus, numbytes;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/* Read the interrupt status register to determine which
+	 * interrupt(s) is/are active.
+	 */
+	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
+
+	cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (7 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 08/10] tty: xuartps: Move RX path into helper function Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  2015-11-05  7:21 ` [PATCH v2 10/10] tty: xuartps: Cleanup: Reformat if-else Soren Brinkmann
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Ignore RX-related interrupts if RX is not enabled.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 drivers/tty/serial/xilinx_uartps.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index aa1176e6661a..413229661f5d 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -161,6 +161,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
  * @pclk:		APB clock
  * @baud:		Current baud rate
  * @clk_rate_change_nb:	Notifier block for clock changes
+ * @flags:		Driver flags
  */
 struct cdns_uart {
 	struct uart_port	*port;
@@ -168,7 +169,11 @@ struct cdns_uart {
 	struct clk		*pclk;
 	unsigned int		baud;
 	struct notifier_block	clk_rate_change_nb;
+	u32			flags;
 };
+
+#define CDNS_FLAG_RX_ENABLED	BIT(0)
+
 #define to_cdns_uart(_nb) container_of(_nb, struct cdns_uart, \
 		clk_rate_change_nb);
 
@@ -262,6 +267,7 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 {
 	struct uart_port *port = (struct uart_port *)dev_id;
+	struct cdns_uart *cdns_uart = port->private_data;
 	unsigned long flags;
 	unsigned int isrstatus, numbytes;
 
@@ -272,7 +278,8 @@ static irqreturn_t cdns_uart_isr(int irq, void *dev_id)
 	 */
 	isrstatus = readl(port->membase + CDNS_UART_ISR_OFFSET);
 
-	cdns_uart_handle_rx(port, isrstatus);
+	if (cdns_uart->flags & CDNS_FLAG_RX_ENABLED)
+		cdns_uart_handle_rx(port, isrstatus);
 
 	/* Dispatch an appropriate handler */
 	if ((isrstatus & CDNS_UART_IXR_TXEMPTY) == CDNS_UART_IXR_TXEMPTY) {
@@ -576,11 +583,13 @@ static void cdns_uart_stop_tx(struct uart_port *port)
 static void cdns_uart_stop_rx(struct uart_port *port)
 {
 	unsigned int regval;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	regval = readl(port->membase + CDNS_UART_CR_OFFSET);
 	regval |= CDNS_UART_CR_RX_DIS;
 	/* Disable the receiver */
 	writel(regval, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 }
 
 /**
@@ -761,6 +770,7 @@ static int cdns_uart_startup(struct uart_port *port)
 {
 	unsigned long flags;
 	unsigned int retval = 0, status = 0;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	retval = request_irq(port->irq, cdns_uart_isr, 0, CDNS_UART_NAME,
 								(void *)port);
@@ -787,6 +797,7 @@ static int cdns_uart_startup(struct uart_port *port)
 	status &= CDNS_UART_CR_RX_DIS;
 	status |= CDNS_UART_CR_RX_EN;
 	writel(status, port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags |= CDNS_FLAG_RX_ENABLED;
 
 	/* Set the Mode Register with normal mode,8 data bits,1 stop bit,
 	 * no parity.
@@ -830,6 +841,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 {
 	int status;
 	unsigned long flags;
+	struct cdns_uart *cdns_uart = port->private_data;
 
 	spin_lock_irqsave(&port->lock, flags);
 
@@ -841,6 +853,7 @@ static void cdns_uart_shutdown(struct uart_port *port)
 	/* Disable the TX and RX */
 	writel(CDNS_UART_CR_TX_DIS | CDNS_UART_CR_RX_DIS,
 			port->membase + CDNS_UART_CR_OFFSET);
+	cdns_uart->flags &= ~CDNS_FLAG_RX_ENABLED;
 
 	spin_unlock_irqrestore(&port->lock, flags);
 
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 10/10] tty: xuartps: Cleanup: Reformat if-else
  2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
                   ` (8 preceding siblings ...)
  2015-11-05  7:21 ` [PATCH v2 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled Soren Brinkmann
@ 2015-11-05  7:21 ` Soren Brinkmann
  9 siblings, 0 replies; 11+ messages in thread
From: Soren Brinkmann @ 2015-11-05  7:21 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Michal Simek, linux-serial, linux-arm-kernel, linux-kernel,
	Soren Brinkmann

Convert an if-else into the more common early return on error, reducing
the indent level of the happy path.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
v2:
 - added this patch
---
 drivers/tty/serial/xilinx_uartps.c | 124 ++++++++++++++++++-------------------
 1 file changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index 413229661f5d..aed8a31f3399 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -203,58 +203,55 @@ static void cdns_uart_handle_rx(struct uart_port *port, unsigned int isrstatus)
 	isrstatus &= port->read_status_mask;
 	isrstatus &= ~port->ignore_status_mask;
 
-	if ((isrstatus & CDNS_UART_IXR_TOUT) ||
-		(isrstatus & CDNS_UART_IXR_RXTRIG)) {
-		/* Receive Timeout Interrupt */
-		while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
-					CDNS_UART_SR_RXEMPTY)) {
-			u32 data;
-			char status = TTY_NORMAL;
-
-			data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
-
-			/* Non-NULL byte after BREAK is garbage (99%) */
-			if (data && (port->read_status_mask &
-						CDNS_UART_IXR_BRK)) {
-				port->read_status_mask &= ~CDNS_UART_IXR_BRK;
-				port->icount.brk++;
-				if (uart_handle_break(port))
-					continue;
-			}
+	if (!(isrstatus & (CDNS_UART_IXR_TOUT | CDNS_UART_IXR_RXTRIG)))
+		return;
+
+	while (!(readl(port->membase + CDNS_UART_SR_OFFSET) &
+				CDNS_UART_SR_RXEMPTY)) {
+		u32 data;
+		char status = TTY_NORMAL;
+
+		data = readl(port->membase + CDNS_UART_FIFO_OFFSET);
+
+		/* Non-NULL byte after BREAK is garbage (99%) */
+		if (data && (port->read_status_mask & CDNS_UART_IXR_BRK)) {
+			port->read_status_mask &= ~CDNS_UART_IXR_BRK;
+			port->icount.brk++;
+			if (uart_handle_break(port))
+				continue;
+		}
 
 #ifdef SUPPORT_SYSRQ
-			/*
-			 * uart_handle_sysrq_char() doesn't work if
-			 * spinlocked, for some reason
-			 */
-			 if (port->sysrq) {
-				spin_unlock(&port->lock);
-				if (uart_handle_sysrq_char(port,
-							(unsigned char)data)) {
-					spin_lock(&port->lock);
-					continue;
-				}
+		/*
+		 * uart_handle_sysrq_char() doesn't work if
+		 * spinlocked, for some reason
+		 */
+		 if (port->sysrq) {
+			spin_unlock(&port->lock);
+			if (uart_handle_sysrq_char(port, data)) {
 				spin_lock(&port->lock);
+				continue;
 			}
+			spin_lock(&port->lock);
+		}
 #endif
 
-			port->icount.rx++;
-
-			if (isrstatus & CDNS_UART_IXR_PARITY) {
-				port->icount.parity++;
-				status = TTY_PARITY;
-			} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
-				port->icount.frame++;
-				status = TTY_FRAME;
-			} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
-				port->icount.overrun++;
-			}
+		port->icount.rx++;
 
-			uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
-					 data, status);
+		if (isrstatus & CDNS_UART_IXR_PARITY) {
+			port->icount.parity++;
+			status = TTY_PARITY;
+		} else if (isrstatus & CDNS_UART_IXR_FRAMING) {
+			port->icount.frame++;
+			status = TTY_FRAME;
+		} else if (isrstatus & CDNS_UART_IXR_OVERRUN) {
+			port->icount.overrun++;
 		}
-		tty_flip_buffer_push(&port->state->port);
+
+		uart_insert_char(port, isrstatus, CDNS_UART_IXR_OVERRUN,
+				 data, status);
 	}
+	tty_flip_buffer_push(&port->state->port);
 }
 
 /**
@@ -1431,27 +1428,30 @@ static int cdns_uart_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Cannot get uart_port structure\n");
 		rc = -ENODEV;
 		goto err_out_notif_unreg;
-	} else {
-		/* Register the port.
-		 * This function also registers this device with the tty layer
-		 * and triggers invocation of the config_port() entry point.
-		 */
-		port->mapbase = res->start;
-		port->irq = irq;
-		port->dev = &pdev->dev;
-		port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
-		port->private_data = cdns_uart_data;
-		cdns_uart_data->port = port;
-		platform_set_drvdata(pdev, port);
-		rc = uart_add_one_port(&cdns_uart_uart_driver, port);
-		if (rc) {
-			dev_err(&pdev->dev,
-				"uart_add_one_port() failed; err=%i\n", rc);
-			goto err_out_notif_unreg;
-		}
-		return 0;
 	}
 
+	/*
+	 * Register the port.
+	 * This function also registers this device with the tty layer
+	 * and triggers invocation of the config_port() entry point.
+	 */
+	port->mapbase = res->start;
+	port->irq = irq;
+	port->dev = &pdev->dev;
+	port->uartclk = clk_get_rate(cdns_uart_data->uartclk);
+	port->private_data = cdns_uart_data;
+	cdns_uart_data->port = port;
+	platform_set_drvdata(pdev, port);
+
+	rc = uart_add_one_port(&cdns_uart_uart_driver, port);
+	if (rc) {
+		dev_err(&pdev->dev,
+			"uart_add_one_port() failed; err=%i\n", rc);
+		goto err_out_notif_unreg;
+	}
+
+	return 0;
+
 err_out_notif_unreg:
 #ifdef CONFIG_COMMON_CLK
 	clk_notifier_unregister(cdns_uart_data->uartclk,
-- 
2.6.2.3.ga463a5b


^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-11-05  7:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-05  7:21 [PATCH v2 00/10] tty: xuartps: Fix lock ups Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 01/10] tty: xuartps: Beautify read-modify writes Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 02/10] tty: xuartps: Use spinlock to serialize HW access Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 03/10] tty: xuartps: Always enable transmitter in start_tx Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 04/10] tty: xuartps: Clear interrupt status register in shutdown Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 05/10] tty: xuartps: Improve startup function Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 06/10] tty: xuartps: Keep lock for whole ISR Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 07/10] tty: xuartps: Acquire port lock for shutdown Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 08/10] tty: xuartps: Move RX path into helper function Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 09/10] tty: xuartps: Only handle RX IRQs when RX is enabled Soren Brinkmann
2015-11-05  7:21 ` [PATCH v2 10/10] tty: xuartps: Cleanup: Reformat if-else Soren Brinkmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).