[PATCH 1/1] Xilinx uartlite serial driver: Add lock in isr to avoid SMP race condition

[PATCH 1/1] Xilinx uartlite serial driver: Add lock in isr to avoid SMP race condition

[Kris Modrak]

Trasmitted characters could be lost or written twice due to a missing lock
in the critical section of the isr. The missing lock causes a race condition
on SMP systems between ulite_transmit when called from the isr and
ulite_start_tx as they can execute simultaneously on separate cores. The call
from ulite_start_tx is made from serial_core.c with the lock taken.

Signed-off-by: Kris Modrak <krismodrak@hotmail.com>
---
 drivers/tty/serial/uartlite.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index 9fc22f4..ee151a2 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -196,9 +196,13 @@ static irqreturn_t ulite_isr(int irq, void *dev_id)
 	int busy, n = 0;
 
 	do {
-		int stat = uart_in32(ULITE_STATUS, port);
+		int stat;
+
+		spin_lock(&port->lock);
+		stat = uart_in32(ULITE_STATUS, port);
 		busy  = ulite_receive(port, stat);
 		busy |= ulite_transmit(port, stat);
+		spin_unlock(&port->lock);
 		n++;
 	} while (busy);
 
-- 
1.7.5.4

Re: [PATCH 1/1] Xilinx uartlite serial driver: Add lock in isr to avoid SMP race condition

[Peter Hurley]

Hi Kris,

On 09/16/2014 08:18 PM, Kris Modrak wrote:
> Trasmitted characters could be lost or written twice due to a missing lock
> in the critical section of the isr. The missing lock causes a race condition
> on SMP systems between ulite_transmit when called from the isr and
> ulite_start_tx as they can execute simultaneously on separate cores. The call
> from ulite_start_tx is made from serial_core.c with the lock taken.
> 
> Signed-off-by: Kris Modrak <krismodrak@hotmail.com>
> ---
>  drivers/tty/serial/uartlite.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index 9fc22f4..ee151a2 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -196,9 +196,13 @@ static irqreturn_t ulite_isr(int irq, void *dev_id)
>  	int busy, n = 0;
>  
>  	do {
> -		int stat = uart_in32(ULITE_STATUS, port);
> +		int stat;
> +
> +		spin_lock(&port->lock);
> +		stat = uart_in32(ULITE_STATUS, port);
>  		busy  = ulite_receive(port, stat);
>  		busy |= ulite_transmit(port, stat);
> +		spin_unlock(&port->lock);
>  		n++;
>  	} while (busy);

Might as well take the spinlock for the entire loop.
A nice upgrade here would be to limit the loop to a fixed number of
iterations as well (in a separate patch).

Regards,
Peter Hurley

PS - That ISR has some odd logic; if data was only transmitted but not
     received, it still submits input work even though there is no work
     to perform (ie., tty_flip_buffer_push() should only be called if
     ulite_receive() returned non-zero).