[PATCH 1/3] serial: 8250: Fix __stop_tx() & DMA Tx restart races

[PATCH 1/3] serial: 8250: Fix __stop_tx() & DMA Tx restart races

[Ilpo Järvinen]

Commit e8ffbb71f783 ("serial: 8250: use THRE & __stop_tx also with
DMA") changed __dma_tx_complete() to enable THRI that is cleared in
__stop_tx() once THRE is asserted as UART runs out bits to transmit. It
is possible, however, that more data arrives in between in which case
serial8250_tx_dma() resumes Tx. THRI is not supposed to be on during
DMA Tx because DMA is based on completion handler, therefore THRI must
be cleared unconditionally in serial8250_tx_dma().

When Tx is about to start, another race window exists with
serial8250_handle_irq() leading to a call into __stop_tx() while the
Tx has already been resumed:

__tx_complete():
  -> spin_lock(port->lock)
  -> dma->tx_running = 0
  -> serial8250_set_THRI()
  -> spin_unlock(port->lock)

uart_start():
				serial8250_handle_irq():
  -> spin_lock(port->lock)
  -> serial8250_tx_dma():
    -> dma->tx_running = 1
  -> spin_unlock(port->lock)
				  -> spin_lock(port->lock)
				  -> __stop_tx()

Close this race by checking !dma->tx_running before calling into
__stop_tx().

Fixes: e8ffbb71f783 ("serial: 8250: use THRE & __stop_tx also with DMA")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c  | 6 +++---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index 7133fceed35e..a8dba4a0a8fb 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -106,10 +106,10 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 				   UART_XMIT_SIZE, DMA_TO_DEVICE);
 
 	dma_async_issue_pending(dma->txchan);
-	if (dma->tx_err) {
+	serial8250_clear_THRI(p);
+	if (dma->tx_err)
 		dma->tx_err = 0;
-		serial8250_clear_THRI(p);
-	}
+
 	return 0;
 err:
 	dma->tx_err = 1;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 953b0fadfd4c..684a1f1fd686 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1936,7 +1936,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 	if ((status & UART_LSR_THRE) && (up->ier & UART_IER_THRI)) {
 		if (!up->dma || up->dma->tx_err)
 			serial8250_tx_chars(up);
-		else
+		else if (!up->dma->tx_running)
 			__stop_tx(up);
 	}
 
-- 
2.30.2

[PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR

[Ilpo Järvinen]

Accessing LSR requires port lock because it mutates lsr_saved_flags
in serial_lsr_in().

Fixes: 197eb5c416ff ("serial: 8250_dw: Use serial_lsr_in() in dw8250_handle_irq()")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 4cc69bb612ab..f67fad3233e9 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -266,7 +266,10 @@ static int dw8250_handle_irq(struct uart_port *p)
 
 	/* Manually stop the Rx DMA transfer when acting as flow controller */
 	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
+		spin_lock_irqsave(&p->lock, flags);
 		status = serial_lsr_in(up);
+		spin_unlock_irqrestore(&p->lock, flags);
+
 		if (status & (UART_LSR_DR | UART_LSR_BI)) {
 			dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
 			dw8250_writel_ext(p, DW_UART_DMASA, 1);
-- 
2.30.2

[PATCH 3/3] serial: 8250_dma: No need for if (dma->tx_err)

[Ilpo Järvinen]

The code can just clear dma->tx_err unconditionally.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index a8dba4a0a8fb..d99020fd3427 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -107,8 +107,7 @@ int serial8250_tx_dma(struct uart_8250_port *p)
 
 	dma_async_issue_pending(dma->txchan);
 	serial8250_clear_THRI(p);
-	if (dma->tx_err)
-		dma->tx_err = 0;
+	dma->tx_err = 0;
 
 	return 0;
 err:
-- 
2.30.2

Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR

[Andy Shevchenko]

On Wed, Jun 15, 2022 at 12:06:50PM +0300, Ilpo Järvinen wrote:
> Accessing LSR requires port lock because it mutates lsr_saved_flags
> in serial_lsr_in().

I got this as patch 2/3, where are the 1/3 and 3/3?

...

> @@ -266,7 +266,10 @@ static int dw8250_handle_irq(struct uart_port *p)
>  
>  	/* Manually stop the Rx DMA transfer when acting as flow controller */
>  	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
> +		spin_lock_irqsave(&p->lock, flags);
>  		status = serial_lsr_in(up);
> +		spin_unlock_irqrestore(&p->lock, flags);

This reminds me the question, why do we need to save flags here? Aren't we in
IRQ context already? (Perhaps another patch might be issued.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1211 bytes --]

On Wed, 15 Jun 2022, Andy Shevchenko wrote:

> On Wed, Jun 15, 2022 at 12:06:50PM +0300, Ilpo Järvinen wrote:
> > Accessing LSR requires port lock because it mutates lsr_saved_flags
> > in serial_lsr_in().
> 
> I got this as patch 2/3, where are the 1/3 and 3/3?

That's probably because get_maintainer.pl didn't pick you up for the other 
patches and I didn't explicitly send them to you. Here they are:

https://lore.kernel.org/linux-serial/20220615090651.15340-1-ilpo.jarvinen@linux.intel.com/T/#t

> > @@ -266,7 +266,10 @@ static int dw8250_handle_irq(struct uart_port *p)
> >  
> >  	/* Manually stop the Rx DMA transfer when acting as flow controller */
> >  	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
> > +		spin_lock_irqsave(&p->lock, flags);
> >  		status = serial_lsr_in(up);
> > +		spin_unlock_irqrestore(&p->lock, flags);
> 
> This reminds me the question, why do we need to save flags here? Aren't we in
> IRQ context already? (Perhaps another patch might be issued.)

Currently serial8250_handle_irq() reads from LSR again. I guess it would 
be possible to read LSR only once and pass it as a parameter. It would 
require some rework though.

-- 
 i.

Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR

[Greg KH]

On Wed, Jun 15, 2022 at 12:06:50PM +0300, Ilpo Järvinen wrote:
> Accessing LSR requires port lock because it mutates lsr_saved_flags
> in serial_lsr_in().
> 
> Fixes: 197eb5c416ff ("serial: 8250_dw: Use serial_lsr_in() in dw8250_handle_irq()")
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 3 +++
>  1 file changed, 3 insertions(+)

Does not apply against my tty-linus branch, you mixed patches up in this
series which is confusing for anyone wanting to apply them.

I've applied the first, but the next two you should resend.

thanks,

greg k-h