lkml.org 
[lkml]   [2009]   [Feb]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] IRQ handling race and spurious IIR read in serial/8250.c
Added cc:

Ian Jackson wrote:
> Anders Kaseorg writes ("Re: Serial console hangs with Linux 2.6.20 HVM guest"):
>
>> Yes, I took Linux v2.6.20 on amd64, ran `make defconfig`, then ran `make
>> menuconfig` and turned off CONFIG_HOTPLUG_CPU (Processor type and features
>> ò Support for hot-pluggable CPUs).
>>
>
> Thanks. I think I have tracked down the bug. In
> drivers/serial/8250.c in Linux there are two bugs:
> 1. UART_BUG_TXEN can be spuriously set, due to an IRQ race
> 2. The workaround then applied by the kernel is itself buggy
>
> Anders: can you try two tests for me ? Firstly, in
> serial8250_startup, delete the section which sets UART_BUG_TXEN (see
> 2nd patch below); I think this will fix the symptoms for you.
> Secondly, in serial8250_start_tx delete the read from the IIR and the
> relevant branch of the text (see 3rd patch below); I think this will
> also in itself fix your symptoms. I haven't compiled either patch (so
> you may find that eg I missed deleting some variables).
>
>
> The bugs in detail (this discussion applies to 2.6.20 and also to
> 2.6.28.4):
>
> 1. The hunk of serial8250_startup I quote below attempts to discover
> whether writing the IER re-asserts the THRI (transmit ready)
> interrupt. However the spinlock that it has taken out,
> port->lock, is not the one that the IRQ service routine takes
> before reading the IIR (i->lock). As a result, on an SMP system
> the generated interrupt races with the straight-line code in
> serial8250_startup.
>
> If serial8250_startup loses the race (perhaps because the system
> is a VM and its VCPU got preempted), UART_BUG_TXEN is spuriously
> added to bugs. This is quite unlikely in a normal system but in
> certain Xen configurations, particularly ones where there is CPU
> pressure, we may lose the race every time.
>
> It is not exactly clear to me how this ought to be resolved. One
> possibility is that the UART_BUG_TXEN problem might be worked
> around perfectly well by the new and very similar workaround
> UART_BUG_THRE[1] in 2.6.21ish in which case it could just be
> removed.
>
> 2. UART_BUG_TXEN's workaround appears to be intended to be harmless.
> However what it actually does is to read the IIR, thus clearing
> any actual interrupt (including incidentally non-THRI), and then
> only perform the intended servicing if the interrupt was _not_
> asserted. That is, it breaks on any serial port with the bug.
>
> As far as I can see there is not much use in UART_BUG_TXEN reading
> IIR at all, so a suitable change if we want to keep UART_BUG_TXEN
> might be the first patch I enclose below (again, not compiled
> or tested).
>
> If UART_BUG_TXEN is retained something along these lines should be
> done at the very least.
>
> Ian.
>
> [1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=40b36daad0ac704e6d5c1b75789f371ef5b053c1
> in which case UART
>
>
> Proposed initial band-aid fix (against 2.6.28.4):
>
>
> Do not read IIR in serial8250_start_tx when UART_BUG_TXEN
>
> Reading the IIR clears some oustanding interrupts so it is not safe.
> Instead, simply transmit immediately if the buffer is empty without
> regard to IIR.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> --- ../linux-2.6.28.4/drivers/serial/8250.c~ 2009-02-06 21:47:45.000000000 +0000
> +++ ../linux-2.6.28.4/drivers/serial/8250.c 2009-02-11 15:55:24.000000000 +0000
> @@ -1257,14 +1257,12 @@
> serial_out(up, UART_IER, up->ier);
>
> if (up->bugs & UART_BUG_TXEN) {
> - unsigned char lsr, iir;
> + unsigned char lsr;
> lsr = serial_in(up, UART_LSR);
> up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> - iir = serial_in(up, UART_IIR) & 0x0f;
> if ((up->port.type == PORT_RM9000) ?
> - (lsr & UART_LSR_THRE &&
> - (iir == UART_IIR_NO_INT || iir == UART_IIR_THRI)) :
> - (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT))
> + (lsr & UART_LSR_THRE) :
> + (lsr & UART_LSR_TEMT))
> transmit_chars(up);
> }
> }
>
>
> Anders - first patch to try (against 2.6.20):
>
> --- drivers/serial/8250.c~ 2007-02-04 18:44:54.000000000 +0000
> +++ drivers/serial/8250.c 2009-02-11 15:39:43.000000000 +0000
> @@ -1645,25 +1645,6 @@
>
> serial8250_set_mctrl(&up->port, up->port.mctrl);
>
> - /*
> - * Do a quick test to see if we receive an
> - * interrupt when we enable the TX irq.
> - */
> - serial_outp(up, UART_IER, UART_IER_THRI);
> - lsr = serial_in(up, UART_LSR);
> - iir = serial_in(up, UART_IIR);
> - serial_outp(up, UART_IER, 0);
> -
> - if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT) {
> - if (!(up->bugs & UART_BUG_TXEN)) {
> - up->bugs |= UART_BUG_TXEN;
> - pr_debug("ttyS%d - enabling bad tx status workarounds\n",
> - port->line);
> - }
> - } else {
> - up->bugs &= ~UART_BUG_TXEN;
> - }
> -
> spin_unlock_irqrestore(&up->port.lock, flags);
>
> /*
>
>
> Anders - second patch to try (against 2.6.20):
> Fix should be suitable for distribution IMO.
>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> --- drivers/serial/8250.c~ 2007-02-04 18:44:54.000000000 +0000
> +++ drivers/serial/8250.c 2009-02-11 15:41:51.000000000 +0000
> @@ -1136,10 +1136,9 @@
> serial_out(up, UART_IER, up->ier);
>
> if (up->bugs & UART_BUG_TXEN) {
> - unsigned char lsr, iir;
> + unsigned char lsr;
> lsr = serial_in(up, UART_LSR);
> - iir = serial_in(up, UART_IIR);
> - if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
> + if (lsr & UART_LSR_TEMT)
> transmit_chars(up);
> }
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-02-19 20:27    [W:0.232 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site