Skip to content

Commit 638a6f4

Browse files
Evan Greengregkh
authored andcommitted
tty: serial: msm_geni_serial: Fix TX infinite loop
The GENI serial driver handled transmit by leaving stuff in the common circular buffer until it had completely caught up to the head, then clearing it out all at once. This is a suboptimal way to do transmit, as it leaves data in the circular buffer that could be freed. Moreover, the logic implementing it is wrong, and it is easy to get into a situation where the UART infinitely writes out the same buffer. I could reproduce infinite serial output of the same buffer by running dmesg, then hitting Ctrl-C. I believe what happened is xmit_size was something large, marching towards a larger value. Then the generic OS code flushed out the buffer and replaced it with two characters. Now the xmit_size is a large value marching towards a small value, which it wasn't expecting. The driver subtracts xmit_size (very large) from uart_circ_chars_pending (2), underflows, and repeats ad nauseum. The locking isn't wrong here, as the locks are held whenever the buffer is manipulated, it's just that the driver wasn't expecting the buffer to be flushed out from underneath it in between transmits. This change reworks transmit to grab what it can from the circular buffer, and then update ->tail, both fixing the underflow and freeing up space for a smoother circular experience. Signed-off-by: Evan Green <[email protected]> Signed-off-by: Greg Kroah-Hartman <[email protected]>
1 parent d76c743 commit 638a6f4

File tree

1 file changed

+7
-10
lines changed

1 file changed

+7
-10
lines changed

drivers/tty/serial/qcom_geni_serial.c

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,6 @@ struct qcom_geni_serial_port {
9898
enum geni_se_xfer_mode xfer_mode;
9999
bool setup;
100100
int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
101-
unsigned int xmit_size;
102101
unsigned int baud;
103102
unsigned int tx_bytes_pw;
104103
unsigned int rx_bytes_pw;
@@ -462,7 +461,6 @@ static void qcom_geni_serial_stop_tx(struct uart_port *uport)
462461
writel_relaxed(0, uport->membase +
463462
SE_GENI_TX_WATERMARK_REG);
464463
}
465-
port->xmit_size = 0;
466464
writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
467465
status = readl_relaxed(uport->membase + SE_GENI_STATUS);
468466
/* Possible stop tx is called multiple times. */
@@ -592,16 +590,13 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
592590
chunk = uart_circ_chars_pending(xmit);
593591
status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
594592
/* Both FIFO and framework buffer are drained */
595-
if (chunk == port->xmit_size && !status) {
596-
port->xmit_size = 0;
597-
uart_circ_clear(xmit);
593+
if (!chunk && !status) {
598594
qcom_geni_serial_stop_tx(uport);
599595
goto out_write_wakeup;
600596
}
601-
chunk -= port->xmit_size;
602597

603598
avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
604-
tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
599+
tail = xmit->tail;
605600
chunk = min3((size_t)chunk, (size_t)(UART_XMIT_SIZE - tail), avail);
606601
if (!chunk)
607602
goto out_write_wakeup;
@@ -622,14 +617,16 @@ static void qcom_geni_serial_handle_tx(struct uart_port *uport)
622617
iowrite32_rep(uport->membase + SE_GENI_TX_FIFOn, buf, 1);
623618

624619
i += tx_bytes;
625-
tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
620+
tail += tx_bytes;
626621
uport->icount.tx += tx_bytes;
627622
remaining -= tx_bytes;
628623
}
624+
625+
xmit->tail = tail & (UART_XMIT_SIZE - 1);
629626
qcom_geni_serial_poll_tx_done(uport);
630-
port->xmit_size += chunk;
631627
out_write_wakeup:
632-
uart_write_wakeup(uport);
628+
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
629+
uart_write_wakeup(uport);
633630
}
634631

635632
static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)

0 commit comments

Comments
 (0)