答复: [PATCH] serial: 8250: add lock for dma rx

[wigin zeng]

We encountered this issue when UART transfer very intensive.
DMA irq-thread processed on CPU0 and serial irq-thread executing on CPU1, 
In DMA irq-thread will invoke "tty_insert_filp_string" function to add the rx_buf into tty_buffer.
In serial irq-thread also has chance to access tty_insert_flip_char(in serial8250_rx_chars ) to access tty_buffer.
there is race condition, sometimes will cause panic.
We add the spin_lock to sync the tty_buffer operation, and the issue gone after applied the patch.

BRs
Weijun

-----邮件原件-----
发件人: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] 
发送时间: 2021年12月9日 15:39
收件人: wigin zeng <wigin.zeng@xxxxxxx>
抄送: jirislaby@xxxxxxxxxx; linux-serial@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
主题: Re: [PATCH] serial: 8250: add lock for dma rx

【EXTERNAL EMAIL】 DO NOT CLICK any links or attachments unless you can make sure both the sender and the content are trustworthy.


【外部邮件提醒】以下邮件来源于公司外部，请勿点击链接或附件，除非您确认邮件发件人和内容可信。



On Thu, Dec 09, 2021 at 03:33:39PM +0800, wigin.zeng wrote:
> Need to add lock to protect the tty buffer in dma rx handler and 
> serial interrupt handler, there is chance that serial handler and dma 
> handler executing in same time in multi cores and RT enabled scenario.

Are you sure?  Why has this not been a problem before now?  What changed?

> Signed-off-by: wigin.zeng <wigin.zeng@xxxxxxx>

I do not think you have a "." in the name you use to sign documents, right?  Please use your real name here.


> ---
>  drivers/tty/serial/8250/8250_dma.c  | 2 ++  
> drivers/tty/serial/8250/8250_port.c | 3 +++
>  include/linux/serial_core.h         | 1 +
>  3 files changed, 6 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dma.c 
> b/drivers/tty/serial/8250/8250_dma.c
> index 890fa7ddaa7f..592b9906e276 100644
> --- a/drivers/tty/serial/8250/8250_dma.c
> +++ b/drivers/tty/serial/8250/8250_dma.c
> @@ -48,6 +48,7 @@ static void __dma_rx_complete(void *param)
>         struct dma_tx_state     state;
>         int                     count;
>
> +       spin_lock(&p->port.rx_lock);
>         dma->rx_running = 0;
>         dmaengine_tx_status(dma->rxchan, dma->rx_cookie, &state);
>
> @@ -55,6 +56,7 @@ static void __dma_rx_complete(void *param)
>
>         tty_insert_flip_string(tty_port, dma->rx_buf, count);
>         p->port.icount.rx += count;
> +       spin_unlock(&p->port.rx_lock);
>
>         tty_flip_buffer_push(tty_port);  } diff --git 
> a/drivers/tty/serial/8250/8250_port.c 
> b/drivers/tty/serial/8250/8250_port.c
> index 5775cbff8f6e..4d8662df8d61 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1780,6 +1780,7 @@ unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
>         struct uart_port *port = &up->port;
>         int max_count = 256;
>
> +       spin_lock(&port->rx_lock);
>         do {
>                 serial8250_read_char(up, lsr);
>                 if (--max_count == 0)
> @@ -1787,6 +1788,7 @@ unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
>                 lsr = serial_in(up, UART_LSR);
>         } while (lsr & (UART_LSR_DR | UART_LSR_BI));
>
> +       spin_unlock(&port->rx_lock);
>         tty_flip_buffer_push(&port->state->port);
>         return lsr;
>  }
> @@ -3267,6 +3269,7 @@ void serial8250_init_port(struct uart_8250_port *up)
>         struct uart_port *port = &up->port;
>
>         spin_lock_init(&port->lock);
> +       spin_lock_init(&port->rx_lock);
>         port->ops = &serial8250_pops;
>         port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE);
>
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h 
> index c58cc142d23f..77980b6f0c27 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -105,6 +105,7 @@ typedef unsigned int __bitwise upstat_t;
>
>  struct uart_port {
>         spinlock_t              lock;                   /* port lock */
> +       spinlock_t              rx_lock;                /* port rx lock */

Why can you not just use 'lock' here instead if this is really an issue?

And doesn't this slow things down?

thanks,

greg k-h
This email and any attachments thereto may contain private, confidential, and privileged material for the sole use of the intended recipient. Any review, copying, or distribution of this email (or any attachments thereto) by others is strictly prohibited. If you are not the intended recipient, please contact the sender immediately and permanently delete the original and any copies of this email and any attachments thereto.

此电子邮件及附件所包含内容具有机密性，且仅限于接收人使用。未经允许，禁止第三人阅读、复制或传播该电子邮件中的任何信息。如果您不属于以上电子邮件的目标接收者，请您立即通知发送人并删除原电子邮件及其相关的附件。