* [PATCH 1/3] serial: 8250: Fix __stop_tx() & DMA Tx restart races
[not found] <20220615090651.15340-1-ilpo.jarvinen@linux.intel.com>
@ 2022-06-15 9:06 ` Ilpo Järvinen
2022-06-15 9:06 ` [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR Ilpo Järvinen
2022-06-15 9:06 ` [PATCH 3/3] serial: 8250_dma: No need for if (dma->tx_err) Ilpo Järvinen
2 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2022-06-15 9:06 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Ilpo Järvinen,
linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR
[not found] <20220615090651.15340-1-ilpo.jarvinen@linux.intel.com>
2022-06-15 9:06 ` [PATCH 1/3] serial: 8250: Fix __stop_tx() & DMA Tx restart races Ilpo Järvinen
@ 2022-06-15 9:06 ` Ilpo Järvinen
2022-06-15 10:07 ` Andy Shevchenko
2022-06-27 12:49 ` Greg KH
2022-06-15 9:06 ` [PATCH 3/3] serial: 8250_dma: No need for if (dma->tx_err) Ilpo Järvinen
2 siblings, 2 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2022-06-15 9:06 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, Andy Shevchenko,
Ilpo Järvinen, linux-kernel
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR
2022-06-15 9:06 ` [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR Ilpo Järvinen
@ 2022-06-15 10:07 ` Andy Shevchenko
2022-06-15 10:29 ` Ilpo Järvinen
2022-06-27 12:49 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2022-06-15 10:07 UTC (permalink / raw)
To: Ilpo Järvinen; +Cc: linux-serial, Greg KH, Jiri Slaby, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR
2022-06-15 10:07 ` Andy Shevchenko
@ 2022-06-15 10:29 ` Ilpo Järvinen
0 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2022-06-15 10:29 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-serial, Greg KH, Jiri Slaby, LKML
[-- 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR
2022-06-15 9:06 ` [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR Ilpo Järvinen
2022-06-15 10:07 ` Andy Shevchenko
@ 2022-06-27 12:49 ` Greg KH
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2022-06-27 12:49 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: linux-serial, Jiri Slaby, Andy Shevchenko, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] serial: 8250_dma: No need for if (dma->tx_err)
[not found] <20220615090651.15340-1-ilpo.jarvinen@linux.intel.com>
2022-06-15 9:06 ` [PATCH 1/3] serial: 8250: Fix __stop_tx() & DMA Tx restart races Ilpo Järvinen
2022-06-15 9:06 ` [PATCH 2/3] serial: 8250_dw: Take port lock while accessing LSR Ilpo Järvinen
@ 2022-06-15 9:06 ` Ilpo Järvinen
2 siblings, 0 replies; 6+ messages in thread
From: Ilpo Järvinen @ 2022-06-15 9:06 UTC (permalink / raw)
To: linux-serial, Greg KH, Jiri Slaby, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 6+ messages in thread