Messages in this thread | | | Subject | Re: [PATCH] 8250 UART backup timer | From | Alex Williamson <> | Date | Tue, 04 Jul 2006 21:39:03 -0600 |
| |
Ping. Any other comments on this? Thanks,
Alex
On Tue, 2006-06-27 at 13:04 -0600, Alex Williamson wrote: > The patch below works around a minor bug found in the UART of the > remote management card used in many HP ia64 and parisc servers (aka the > Diva UARTs). The problem is that the UART does not reassert the THRE > interrupt if it has been previously cleared and the IIR THRI bit is > re-enabled. This can produce a very annoying failure mode when used as > a serial console, allowing a boot/reboot to hang indefinitely until an > RX interrupt kicks it into working again (ie. an unattended reboot could > stall). > > To solve this problem, a backup timer is introduced that runs > alongside the standard interrupt driven mechanism. This timer wakes up > periodically, checks for a hang condition and gets characters moving > again. This backup mechanism is only enabled if the UART is detected as > having this problem, so systems without these UARTs will have no > additional overhead. > > This version of the patch incorporates previous comments from Pavel > and removes races in the bug detection code. The test is now done > before the irq linking to prevent races with interrupt handler clearing > the THRE interrupt. Short delays and syncs are also added to ensure the > device is able to update register state before the result is tested. > Comments? Thanks, > > Alex > > Signed-off-by: Alex Williamson <alex.williamson@hp.com> > --- > > diff -r e3554576c29a drivers/serial/8250.c > --- a/drivers/serial/8250.c Sun Jun 18 02:00:07 2006 +0000 > +++ b/drivers/serial/8250.c Tue Jun 27 12:27:03 2006 -0600 > @@ -353,6 +353,23 @@ serial_out(struct uart_8250_port *up, in > } > } > > +static void > +serial_out_sync(struct uart_8250_port *up, int offset, int value) > +{ > + switch (up->port.iotype) { > + case UPIO_MEM: > + case UPIO_MEM32: > +#ifdef CONFIG_SERIAL_8250_AU1X00 > + case UPIO_AU: > +#endif > + serial_out(up, offset, value); > + (void)serial_in(up, UART_LCR); /* safe, no side-effects */ > + break; > + default: > + serial_out(up, offset, value); > + } > +} > + > /* > * We used to support using pause I/O for certain machines. We > * haven't supported this for a while, but just in case it's badly > @@ -1436,6 +1453,13 @@ static void serial_unlink_irq_chain(stru > serial_do_unlink(i, up); > } > > +/* Base timer interval for polling */ > +static inline int > +poll_timeout(int timeout) > +{ > + return timeout > 6 ? (timeout / 2 - 2) : 1; > +} > + > /* > * This function is used to handle ports that do not have an > * interrupt. This doesn't work very well for 16450's, but gives > @@ -1445,16 +1469,52 @@ static void serial8250_timeout(unsigned > static void serial8250_timeout(unsigned long data) > { > struct uart_8250_port *up = (struct uart_8250_port *)data; > - unsigned int timeout; > unsigned int iir; > > iir = serial_in(up, UART_IIR); > if (!(iir & UART_IIR_NO_INT)) > serial8250_handle_port(up, NULL); > > - timeout = up->port.timeout; > - timeout = timeout > 6 ? (timeout / 2 - 2) : 1; > - mod_timer(&up->timer, jiffies + timeout); > + mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout)); > +} > + > +static void serial8250_backup_timeout(unsigned long data) > +{ > + struct uart_8250_port *up = (struct uart_8250_port *)data; > + unsigned int iir, ier = 0; > + > + /* > + * Must disable interrupts or else we risk racing with the interrupt > + * based handler. > + */ > + if (is_real_interrupt(up->port.irq)) { > + ier = serial_in(up, UART_IER); > + serial_out(up, UART_IER, 0); > + } > + > + iir = serial_in(up, UART_IIR); > + > + /* > + * This should be a safe test for anyone who doesn't trust the > + * IIR bits on their UART, but it's specifically designed for > + * the "Diva" UART used on the management processor on many HP > + * ia64 and parisc boxes. > + */ > + if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) && > + (!uart_circ_empty(&up->port.info->xmit) || up->port.x_char) && > + (serial_in(up, UART_LSR) & UART_LSR_THRE)) { > + iir &= ~(UART_IIR_ID | UART_IIR_NO_INT); > + iir |= UART_IIR_THRI; > + } > + > + if (!(iir & UART_IIR_NO_INT)) > + serial8250_handle_port(up, NULL); > + > + if (is_real_interrupt(up->port.irq)) > + serial_out(up, UART_IER, ier); > + > + /* Standard timer interval plus 0.2s to keep the port running */ > + mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout) + HZ/5); > } > > static unsigned int serial8250_tx_empty(struct uart_port *port) > @@ -1523,6 +1583,36 @@ static void serial8250_break_ctl(struct > up->lcr &= ~UART_LCR_SBC; > serial_out(up, UART_LCR, up->lcr); > spin_unlock_irqrestore(&up->port.lock, flags); > +} > + > +#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE) > + > +/* > + * Wait for transmitter & holding register to empty > + */ > +static inline void wait_for_xmitr(struct uart_8250_port *up, int bits) > +{ > + unsigned int status, timeout = 10000; > + > + /* Wait up to 10ms for the character(s) to be sent. */ > + do { > + status = serial_in(up, UART_LSR); > + > + if (status & UART_LSR_BI) > + up->lsr_break_flag = UART_LSR_BI; > + > + if (--timeout == 0) > + break; > + udelay(1); > + } while ((status & bits) != bits); > + > + /* Wait up to 1s for flow control if necessary */ > + if (up->port.flags & UPF_CONS_FLOW) { > + timeout = 1000000; > + while (--timeout && > + ((serial_in(up, UART_MSR) & UART_MSR_CTS) == 0)) > + udelay(1); > + } > } > > static int serial8250_startup(struct uart_port *port) > @@ -1598,18 +1688,50 @@ static int serial8250_startup(struct uar > serial_outp(up, UART_LCR, 0); > } > > + if (is_real_interrupt(up->port.irq)) { > + /* > + * Test for UARTs that do not reassert THRE when the > + * transmitter is idle and the interrupt has already > + * been cleared. Real 16550s should always reassert > + * this interrupt whenever the transmitter is idle and > + * the interrupt is enabled. Delays are necessary to > + * allow register changes to become visible. > + */ > + spin_lock_irqsave(&up->port.lock, flags); > + > + wait_for_xmitr(up, UART_LSR_THRE); > + serial_out_sync(up, UART_IER, UART_IER_THRI); > + udelay(1); /* allow THRE to set */ > + (void)serial_in(up, UART_IIR); > + serial_out(up, UART_IER, 0); > + serial_out_sync(up, UART_IER, UART_IER_THRI); > + udelay(1); /* allow a working UART time to re-assert THRE */ > + iir = serial_in(up, UART_IIR); > + serial_out(up, UART_IER, 0); > + > + spin_unlock_irqrestore(&up->port.lock, flags); > + > + /* > + * If the interrupt is not reasserted, setup a timer to > + * kick the UART on a regular basis. > + */ > + if (iir & UART_IIR_NO_INT) { > + pr_debug("ttyS%d - using backup timer\n", port->line); > + up->timer.function = serial8250_backup_timeout; > + up->timer.data = (unsigned long)up; > + mod_timer(&up->timer, jiffies + > + poll_timeout(up->port.timeout) + HZ/5); > + } > + } > + > /* > * If the "interrupt" for this port doesn't correspond with any > * hardware interrupt, we use a timer-based system. The original > * driver used to do this with IRQ0. > */ > if (!is_real_interrupt(up->port.irq)) { > - unsigned int timeout = up->port.timeout; > - > - timeout = timeout > 6 ? (timeout / 2 - 2) : 1; > - > up->timer.data = (unsigned long)up; > - mod_timer(&up->timer, jiffies + timeout); > + mod_timer(&up->timer, jiffies + poll_timeout(up->port.timeout)); > } else { > retval = serial_link_irq_chain(up); > if (retval) > @@ -1725,9 +1847,9 @@ static void serial8250_shutdown(struct u > */ > (void) serial_in(up, UART_RX); > > - if (!is_real_interrupt(up->port.irq)) > - del_timer_sync(&up->timer); > - else > + del_timer_sync(&up->timer); > + up->timer.function = serial8250_timeout; > + if (is_real_interrupt(up->port.irq)) > serial_unlink_irq_chain(up); > } > > @@ -2187,36 +2309,6 @@ serial8250_register_ports(struct uart_dr > > #ifdef CONFIG_SERIAL_8250_CONSOLE > > -#define BOTH_EMPTY (UART_LSR_TEMT | UART_LSR_THRE) > - > -/* > - * Wait for transmitter & holding register to empty > - */ > -static inline void wait_for_xmitr(struct uart_8250_port *up, int bits) > -{ > - unsigned int status, tmout = 10000; > - > - /* Wait up to 10ms for the character(s) to be sent. */ > - do { > - status = serial_in(up, UART_LSR); > - > - if (status & UART_LSR_BI) > - up->lsr_break_flag = UART_LSR_BI; > - > - if (--tmout == 0) > - break; > - udelay(1); > - } while ((status & bits) != bits); > - > - /* Wait up to 1s for flow control if necessary */ > - if (up->port.flags & UPF_CONS_FLOW) { > - tmout = 1000000; > - while (--tmout && > - ((serial_in(up, UART_MSR) & UART_MSR_CTS) == 0)) > - udelay(1); > - } > -} > - > static void serial8250_console_putchar(struct uart_port *port, int ch) > { > struct uart_8250_port *up = (struct uart_8250_port *)port; > > > - > 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/ > -- Alex Williamson HP Open Source & Linux Org.
- 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/
| |