lkml.org 
[lkml]   [2021]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] serial: 8250: add lock for dma rx
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@dji.com>

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

\
 
 \ /
  Last update: 2021-12-09 08:39    [W:0.062 / U:0.252 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site