Serial 8250: Handle saving the clear-on-read bits from the LSR and MSR

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

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 <[email protected]>

Cc Russell King <[email protected]>

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
===================================================================

drivers/serial/8250.c +56 -29

@@ -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
===================================================================

include/linux/serial_reg.h +1 -1

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 9 lines · Corey Minyard

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...

Quoted 37 lines · Gene Heskett

Quoted 7 lines · Corey Minyard

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. ???

Quoted 4 lines

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...

I really doubt that this patch had anything to do with these issues, especially
the ones over USB. However, if you can reliably remove the patch, test, and reapply the patch, and test, and the issues remain the same, then it's perhaps
something significant, though I would be at a loss to know what it was.

-corey

[long message snipped]

Thanks for your patience Corey.

Quoted 4 lines · Gene Heskett

[long message snipped]

Thanks for your patience Corey.

So, in one sentence or preferably one word, did Corey's patch cause a regression?

From what Gene said, I think the final outcome is that this patch didn't

Quoted 9 lines · Russell King

Quoted 3 lines · Gene Heskett

[long message snipped]

Thanks for your patience Corey.

So, in one sentence or preferably one word, did Corey's patch cause a regression?

From what Gene said, I think the final outcome is that this patch didn't

seem to make any difference. It looks to me that the problems were elsewhere.

So what's the state of this patch?

Thanks,

-corey