Re: [PATCH] Serial 8250: Handle saving the clear-on-read bits from the LSR and MSR

[Gene Heskett <gene.heskett@gmail.com>]

On Friday 04 May 2007, Corey Minyard wrote:
>I did think of one other thing: if you clear the MSR delta flags while
>polling the serial console, you need to call the routine to check the
>modem status since the interrupt will not happen.  So here's the
>patch...
>
>Subject: Serial 8250: Handle saving the clear-on-read bits from the LSR and
> MSR
>
I applied this patch because I was curious to see what effect if any it had on 
some serial based acessories I use here, like a cm11 x10 controller with 
heyu, and a belkin ups with its supplied software.

But at first boot, its not 100% operationally safe, but please read on.

1) heyu (the x10 control program) is now stuck in a loop, repeating the last 
command issued, at about 40 second repeat intervals.  AND it is using 90%+ of 
the cpu while it executes each loop, which takes about 7 to 10 seconds each 
time.  heyu is interfacing to the world via /dev/ttyUSB0, through an FTDI 
adapter so I'm not able to make a mental connection here between this patch 
and that effect.

2) HOWEVER!  The belkin upsd daemon, which started being a cpu hog, using 40% 
of the cpu continuously since something in the 2.6.20 to 2.6.21 time frame, 
and this was regardless of whether it was talking through a /dev/ttyUSB# port 
and either a pl2303, or an FTDI adaptor, or direct through /dev/ttyS0, is at 
least for /dev/ttyS0, now operating normally and apparently 100% stable.  So 
I pulled out another FTDI adaptor, and reconfigured the daemon to 
use /dev/ttyUSB1, and that is also operating 100% normally now.

Can someone explain 1) above?  Even odder still, I'd killed and restarted heyu 
several times to no avail before I tried putting upsd on /dev/ttyUSB1, but 
after moving the upsd access from /dev/ttyS0 to /dev/ttyUSB1, then heyu, 
running through /dev/ttyUSB0, is now operating 100% normally.  ???

Does anyone have any idea what kind of bait I should put out to kill this 
nilmerg?  I'm happy, its all working again for a change, but I'll be damned 
if I ain't totally bumfuzzled.  And I wonder if it will survive a reboot...

>Reading the LSR clears the break, parity, frame error, and overrun
>bits in the 8250 chip, but these are not being saved in all places
>that read the LSR.  Same goes for the MSR delta bits.  Save the LSR
>bits off whenever the lsr is read so they can be handled later in the
>receive routine.  Save the MSR bits to be handled in the modem status
>routine.
>
>Also, clear the stored bits and clear the interrupt registers before
>enabling interrupts, to avoid handling old values of the stored bits
>in the interrupt routines.
>
>Signed-off-by: Corey Minyard <minyard@acm.org>
>Cc: Russell King <rmk+lkml@arm.linux.org.uk>
>
> drivers/serial/8250.c      |   85
> +++++++++++++++++++++++++++++---------------- include/linux/serial_reg.h | 
>   1
> 2 files changed, 57 insertions(+), 29 deletions(-)
>
>Index: linux-2.6.21/drivers/serial/8250.c
>===================================================================
>--- linux-2.6.21.orig/drivers/serial/8250.c
>+++ linux-2.6.21/drivers/serial/8250.c
>@@ -129,7 +129,16 @@ struct uart_8250_port {
> 	unsigned char		mcr;
> 	unsigned char		mcr_mask;	/* mask of user bits */
> 	unsigned char		mcr_force;	/* mask of forced bits */
>-	unsigned char		lsr_break_flag;
>+
>+	/*
>+	 * Some bits in registers are cleared on a read, so they must
>+	 * be saved whenever the register is read but the bits will not
>+	 * be immediately processed.
>+	 */
>+#define LSR_SAVE_FLAGS UART_LSR_BRK_ERROR_BITS
>+	unsigned char		lsr_saved_flags;
>+#define MSR_SAVE_FLAGS UART_MSR_ANY_DELTA
>+	unsigned char		msr_saved_flags;
>
> 	/*
> 	 * We provide a per-port pm hook.
>@@ -1159,6 +1168,7 @@ static void serial8250_start_tx(struct u
> 		if (up->bugs & UART_BUG_TXEN) {
> 			unsigned char lsr, iir;
> 			lsr = serial_in(up, UART_LSR);
>+			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> 			iir = serial_in(up, UART_IIR);
> 			if (lsr & UART_LSR_TEMT && iir & UART_IIR_NO_INT)
> 				transmit_chars(up);
>@@ -1208,18 +1218,10 @@ receive_chars(struct uart_8250_port *up,
> 		flag = TTY_NORMAL;
> 		up->port.icount.rx++;
>
>-#ifdef CONFIG_SERIAL_8250_CONSOLE
>-		/*
>-		 * Recover the break flag from console xmit
>-		 */
>-		if (up->port.line == up->port.cons->index) {
>-			lsr |= up->lsr_break_flag;
>-			up->lsr_break_flag = 0;
>-		}
>-#endif
>+		lsr |= up->lsr_saved_flags;
>+		up->lsr_saved_flags = 0;
>
>-		if (unlikely(lsr & (UART_LSR_BI | UART_LSR_PE |
>-				    UART_LSR_FE | UART_LSR_OE))) {
>+		if (unlikely(lsr & UART_LSR_BRK_ERROR_BITS)) {
> 			/*
> 			 * For statistics only
> 			 */
>@@ -1310,6 +1312,8 @@ static unsigned int check_modem_status(s
> {
> 	unsigned int status = serial_in(up, UART_MSR);
>
>+	status |= up->msr_saved_flags;
>+	up->msr_saved_flags = 0;
> 	if (status & UART_MSR_ANY_DELTA && up->ier & UART_IER_MSI &&
> 	    up->port.info != NULL) {
> 		if (status & UART_MSR_TERI)
>@@ -1496,7 +1500,8 @@ static void serial8250_timeout(unsigned
> static void serial8250_backup_timeout(unsigned long data)
> {
> 	struct uart_8250_port *up = (struct uart_8250_port *)data;
>-	unsigned int iir, ier = 0;
>+	unsigned int iir, ier = 0, lsr;
>+	unsigned long flags;
>
> 	/*
> 	 * Must disable interrupts or else we risk racing with the interrupt
>@@ -1515,9 +1520,13 @@ static void serial8250_backup_timeout(un
> 	 * the "Diva" UART used on the management processor on many HP
> 	 * ia64 and parisc boxes.
> 	 */
>+	spin_lock_irqsave(&up->port.lock, flags);
>+	lsr = serial_in(up, UART_LSR);
>+	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
>+	spin_unlock_irqrestore(&up->port.lock, flags);
> 	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)) {
>+	    (lsr & UART_LSR_THRE)) {
> 		iir &= ~(UART_IIR_ID | UART_IIR_NO_INT);
> 		iir |= UART_IIR_THRI;
> 	}
>@@ -1536,13 +1545,14 @@ static unsigned int serial8250_tx_empty(
> {
> 	struct uart_8250_port *up = (struct uart_8250_port *)port;
> 	unsigned long flags;
>-	unsigned int ret;
>+	unsigned int lsr;
>
> 	spin_lock_irqsave(&up->port.lock, flags);
>-	ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>+	lsr = serial_in(up, UART_LSR);
>+	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> 	spin_unlock_irqrestore(&up->port.lock, flags);
>
>-	return ret;
>+	return lsr & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> }
>
> static unsigned int serial8250_get_mctrl(struct uart_port *port)
>@@ -1613,8 +1623,7 @@ static inline void wait_for_xmitr(struct
> 	do {
> 		status = serial_in(up, UART_LSR);
>
>-		if (status & UART_LSR_BI)
>-			up->lsr_break_flag = UART_LSR_BI;
>+		up->lsr_saved_flags |= status & LSR_SAVE_FLAGS;
>
> 		if (--tmout == 0)
> 			break;
>@@ -1623,8 +1632,12 @@ static inline void wait_for_xmitr(struct
>
> 	/* Wait up to 1s for flow control if necessary */
> 	if (up->port.flags & UPF_CONS_FLOW) {
>-		tmout = 1000000;
>-		while (!(serial_in(up, UART_MSR) & UART_MSR_CTS) && --tmout) {
>+		unsigned int tmout;
>+		for (tmout = 1000000; tmout; tmout--) {
>+			unsigned int msr = serial_in(up, UART_MSR);
>+			up->msr_saved_flags |= msr & MSR_SAVE_FLAGS;
>+			if (msr & UART_MSR_CTS)
>+				break;
> 			udelay(1);
> 			touch_nmi_watchdog();
> 		}
>@@ -1794,6 +1807,18 @@ static int serial8250_startup(struct uar
> 	spin_unlock_irqrestore(&up->port.lock, flags);
>
> 	/*
>+	 * Clear the interrupt registers again for luck, and clear the
>+	 * saved flags to avoid getting false values from polling
>+	 * routines or the previous session.
>+	 */
>+	(void) serial_inp(up, UART_LSR);
>+	(void) serial_inp(up, UART_RX);
>+	(void) serial_inp(up, UART_IIR);
>+	(void) serial_inp(up, UART_MSR);
>+	up->lsr_saved_flags = 0;
>+	up->msr_saved_flags = 0;
>+
>+	/*
> 	 * Finally, enable interrupts.  Note: Modem status interrupts
> 	 * are set via set_termios(), which will be occurring imminently
> 	 * anyway, so we don't enable them here.
>@@ -1811,14 +1836,6 @@ static int serial8250_startup(struct uar
> 		(void) inb_p(icp);
> 	}
>
>-	/*
>-	 * And clear the interrupt registers again for luck.
>-	 */
>-	(void) serial_inp(up, UART_LSR);
>-	(void) serial_inp(up, UART_RX);
>-	(void) serial_inp(up, UART_IIR);
>-	(void) serial_inp(up, UART_MSR);
>-
> 	return 0;
> }
>
>@@ -2387,6 +2404,16 @@ serial8250_console_write(struct console
> 	wait_for_xmitr(up, BOTH_EMPTY);
> 	serial_out(up, UART_IER, ier);
>
>+	/*
>+	 *	The receive handling will happen properly because the
>+	 *	receive ready bit will still be set; it is not cleared
>+	 *	on read.  However, modem control will not, we must
>+	 *	call it if we have saved something in the saved flags
>+	 *	while processing with interrupts off.
>+	 */
>+	if (up->msr_saved_flags)
>+		check_modem_status(up);
>+
> 	if (locked)
> 		spin_unlock(&up->port.lock);
> 	local_irq_restore(flags);
>Index: linux-2.6.21/include/linux/serial_reg.h
>===================================================================
>--- linux-2.6.21.orig/include/linux/serial_reg.h
>+++ linux-2.6.21/include/linux/serial_reg.h
>@@ -116,6 +116,7 @@
> #define UART_LSR_PE		0x04 /* Parity error indicator */
> #define UART_LSR_OE		0x02 /* Overrun error indicator */
> #define UART_LSR_DR		0x01 /* Receiver data ready */
>+#define UART_LSR_BRK_ERROR_BITS	0x1E /* BI, FE, PE, OE bits */
>
> #define UART_MSR	6	/* In:  Modem Status Register */
> #define UART_MSR_DCD		0x80 /* Data Carrier Detect */
>-
>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/



-- 
Cheers, Gene
"There are four boxes to be used in defense of liberty:
 soap, ballot, jury, and ammo. Please use in that order."
-Ed Howdershelt (Author)
#define NULL 0           /* silly thing is, we don't even use this */
             -- Larry Wall in perl.c from the perl source code