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
