[PATCH 0/2] 8250_omap: Fix issues with throttling of 8250 port

[PATCH 0/2] 8250_omap: Fix issues with throttling of 8250 port

[Vignesh R]

This series contains couple of fixes related to throttling of 8250 uart
port when tty buffer is under pressure.


Vignesh R (2):
  serial: 8250: Don't service RX FIFO if interrupts are disabled
  serial: 8250: 8250_omap: Fix throttling when DMA is enabled

 drivers/tty/serial/8250/8250_omap.c | 11 ++++++++++-
 drivers/tty/serial/8250/8250_port.c |  3 ++-
 2 files changed, 12 insertions(+), 2 deletions(-)

-- 
2.16.1

[PATCH 1/2] serial: 8250: Don't service RX FIFO if interrupts are disabled

[Vignesh R]

Currently, data in RX FIFO is read based on UART_LSR register state even
if RDI and RLSI interrupts are disabled in UART_IER register.
This is because when IRQ handler is called due to TX FIFO empty event,
RX FIFO is serviced based on UART_LSR register status instead of
UART_IIR status. This defeats the purpose of disabling UART RX
FIFO interrupts during throttling(see, omap_8250_throttle()) as IRQ
handler continues to drain UART RX FIFO resulting in overflow of buffer
at tty layer.
Fix this by making sure that driver drains UART RX FIFO only when
UART_IIR_RDI is set along with UART_LSR_BI or UART_LSR_DR bits.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/tty/serial/8250/8250_port.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 1328c7e70108..ffbb955d1c06 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1854,7 +1854,8 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 
 	status = serial_port_in(port, UART_LSR);
 
-	if (status & (UART_LSR_DR | UART_LSR_BI)) {
+	if (status & (UART_LSR_DR | UART_LSR_BI) &&
+	    iir & UART_IIR_RDI) {
 		if (!up->dma || handle_rx_dma(up, iir))
 			status = serial8250_rx_chars(up, status);
 	}
-- 
2.16.1

Re: [PATCH 1/2] serial: 8250: Don't service RX FIFO if interrupts are disabled

[Andy Shevchenko]

On Thu, Feb 8, 2018 at 2:55 PM, Vignesh R <vigneshr@ti.com> wrote:
> Currently, data in RX FIFO is read based on UART_LSR register state even
> if RDI and RLSI interrupts are disabled in UART_IER register.
> This is because when IRQ handler is called due to TX FIFO empty event,
> RX FIFO is serviced based on UART_LSR register status instead of
> UART_IIR status. This defeats the purpose of disabling UART RX
> FIFO interrupts during throttling(see, omap_8250_throttle()) as IRQ
> handler continues to drain UART RX FIFO resulting in overflow of buffer
> at tty layer.
> Fix this by making sure that driver drains UART RX FIFO only when
> UART_IIR_RDI is set along with UART_LSR_BI or UART_LSR_DR bits.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>

> -       if (status & (UART_LSR_DR | UART_LSR_BI)) {
> +       if (status & (UART_LSR_DR | UART_LSR_BI) &&
> +           iir & UART_IIR_RDI) {

>                 if (!up->dma || handle_rx_dma(up, iir))

handle_rx_dma() checks for IRQ status as well.

But for now it seems we are on safe side since checks are done versus
IRQ status with bit 2 set, meaning that iir & RDI will be true.

>                         status = serial8250_rx_chars(up, status);
>         }

Anyway, thanks for the patch, though I need some time to test it on
non-OMAP hardware with DMA enabled.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] serial: 8250: Don't service RX FIFO if interrupts are disabled

[Vignesh R]

On 08-Feb-18 8:46 PM, Andy Shevchenko wrote:
> On Thu, Feb 8, 2018 at 2:55 PM, Vignesh R <vigneshr@ti.com> wrote:
>> Currently, data in RX FIFO is read based on UART_LSR register state even
>> if RDI and RLSI interrupts are disabled in UART_IER register.
>> This is because when IRQ handler is called due to TX FIFO empty event,
>> RX FIFO is serviced based on UART_LSR register status instead of
>> UART_IIR status. This defeats the purpose of disabling UART RX
>> FIFO interrupts during throttling(see, omap_8250_throttle()) as IRQ
>> handler continues to drain UART RX FIFO resulting in overflow of buffer
>> at tty layer.
>> Fix this by making sure that driver drains UART RX FIFO only when
>> UART_IIR_RDI is set along with UART_LSR_BI or UART_LSR_DR bits.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
> 
>> -       if (status & (UART_LSR_DR | UART_LSR_BI)) {
>> +       if (status & (UART_LSR_DR | UART_LSR_BI) &&
>> +           iir & UART_IIR_RDI) {
> 
>>                 if (!up->dma || handle_rx_dma(up, iir))
> 
> handle_rx_dma() checks for IRQ status as well.
> 
> But for now it seems we are on safe side since checks are done versus
> IRQ status with bit 2 set, meaning that iir & RDI will be true.
> 
>>                         status = serial8250_rx_chars(up, status);
>>         }
> 
> Anyway, thanks for the patch, though I need some time to test it on
> non-OMAP hardware with DMA enabled.

This patch is needed even when DMA is not enabled.
It would be great if you could test this. But, I don't see any other
8250 drivers apart from 8250_omap.c implementing
.throttle()/.unthrottle() callbacks.

Regards
Vignesh

[PATCH 2/2] serial: 8250: 8250_omap: Fix throttling when DMA is enabled

[Vignesh R]

omap_8250_throttle() is called when tty RX buffer is about to overflow
and can no longer keep up with the rate at which UART is receiving data.
So, the expectation of this callback, is that UART stops RX and asserts
HW flow control to signal the sender to stop sending more data.
omap_8250_throttle() disables RX FIFO interrupts thus FIFO is no longer
serviced, leading to assertion of flow control once RX FIFO is full.
But, this does not work when DMA is enabled as driver keeps queuing new
RX DMA request in completion handler without brothering about throttling
request made by the higher layer.
This patch introduces a flag that can be used to determine whether or
not to queue next RX DMA request based on throttling request.

Without this patch, tty buffer overflows are reported at higher
baudrates.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/tty/serial/8250/8250_omap.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
index 57f6eba47f44..624b501fd253 100644
--- a/drivers/tty/serial/8250/8250_omap.c
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -114,6 +114,7 @@ struct omap8250_priv {
 	struct uart_8250_dma omap8250_dma;
 	spinlock_t rx_dma_lock;
 	bool rx_dma_broken;
+	bool throttled;
 };
 
 #ifdef CONFIG_SERIAL_8250_DMA
@@ -692,6 +693,7 @@ static void omap_8250_shutdown(struct uart_port *port)
 
 static void omap_8250_throttle(struct uart_port *port)
 {
+	struct omap8250_priv *priv = port->private_data;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned long flags;
 
@@ -700,6 +702,7 @@ static void omap_8250_throttle(struct uart_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
 	serial_out(up, UART_IER, up->ier);
+	priv->throttled = true;
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	pm_runtime_mark_last_busy(port->dev);
@@ -738,12 +741,16 @@ static int omap_8250_rs485_config(struct uart_port *port,
 
 static void omap_8250_unthrottle(struct uart_port *port)
 {
+	struct omap8250_priv *priv = port->private_data;
 	struct uart_8250_port *up = up_to_u8250p(port);
 	unsigned long flags;
 
 	pm_runtime_get_sync(port->dev);
 
 	spin_lock_irqsave(&port->lock, flags);
+	priv->throttled = false;
+	if (up->dma)
+		up->dma->rx_dma(up);
 	up->ier |= UART_IER_RLSI | UART_IER_RDI;
 	serial_out(up, UART_IER, up->ier);
 	spin_unlock_irqrestore(&port->lock, flags);
@@ -788,6 +795,7 @@ static void __dma_rx_do_complete(struct uart_8250_port *p)
 static void __dma_rx_complete(void *param)
 {
 	struct uart_8250_port *p = param;
+	struct omap8250_priv *priv = p->port.private_data;
 	struct uart_8250_dma *dma = p->dma;
 	struct dma_tx_state     state;
 	unsigned long flags;
@@ -805,7 +813,8 @@ static void __dma_rx_complete(void *param)
 		return;
 	}
 	__dma_rx_do_complete(p);
-	omap_8250_rx_dma(p);
+	if (!priv->throttled)
+		omap_8250_rx_dma(p);
 
 	spin_unlock_irqrestore(&p->port.lock, flags);
 }
-- 
2.16.1