Re: [PATCH v2] serial: 8250: fix shared interrupts issues with SMP and RT kernels

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Tue, Jul 01, 2008 at 07:02:54PM +0400, Anton Vorontsov wrote:
[...]
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> On Tue, Jul 01, 2008 at 02:43:53PM +0100, Alan Cox wrote:
[...]
> > It looks correct to me except that you cannot use spin_lock/disable_irq
> > in that way safely. You must always disable_irq before taking the lock,
> > or prove it is safe and use disable_irq_nosync
> > 
> > The reason:
> > 		CPU#0 spin_lock_... [taken]
> > 		CPU#1 IRQ
> > 		CPU#1 spin_lock [waits]
> > 		CPU#0 disable_irq (deadlock)
> 
> This deadlock possibility is interesting by itself, thanks for
> mentioning it.
> 
> But this can't happen here. IRQ will not grab the up->port.lock,
> because port isn't registered in the 8250 IRQ handling chain (yet).

Alan, are there any issues with the proof or the patch itself?

Thanks,

> As for _nosync, probably this is good idea indeed, and should be safe
> AFAICS.
> 
>  drivers/serial/8250.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 76ccef7..cad0c2d 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1831,7 +1831,9 @@ static int serial8250_startup(struct uart_port *port)
>  		 * the interrupt is enabled.  Delays are necessary to
>  		 * allow register changes to become visible.
>  		 */
> -		spin_lock_irqsave(&up->port.lock, flags);
> +		spin_lock(&up->port.lock);
> +		if (up->port.flags & UPF_SHARE_IRQ)
> +			disable_irq_nosync(up->port.irq);
>  
>  		wait_for_xmitr(up, UART_LSR_THRE);
>  		serial_out_sync(up, UART_IER, UART_IER_THRI);
> @@ -1843,7 +1845,9 @@ static int serial8250_startup(struct uart_port *port)
>  		iir = serial_in(up, UART_IIR);
>  		serial_out(up, UART_IER, 0);
>  
> -		spin_unlock_irqrestore(&up->port.lock, flags);
> +		if (up->port.flags & UPF_SHARE_IRQ)
> +			enable_irq(up->port.irq);
> +		spin_unlock(&up->port.lock);
>  
>  		/*
>  		 * If the interrupt is not reasserted, setup a timer to
> -- 
> 1.5.5.4
> 

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2