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

From: Anton Vorontsov
Date: Wed Jul 09 2008 - 15:30:01 EST


On Tue, Jul 01, 2008 at 07:02:54PM +0400, Anton Vorontsov wrote:
[...]
> Signed-off-by: Anton Vorontsov <avorontsov@xxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxx
irc://irc.freenode.net/bd2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/