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

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

[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

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 */

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

[Gene Heskett]

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

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

[Corey Minyard]

Gene Heskett wrote:
> 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...
>
>   
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

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

[Gene Heskett]

On Friday 04 May 2007, Corey Minyard wrote:
>Gene Heskett wrote:
>> 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...
>
>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

Well, I believe it is.  Or did, see below.

I've been testing the two competing schedulers, and just now got around to 
testing Con's last 0.48 version, which at first feel isn't that bad.

And I also have to bitch because everytime I change schedulers, all my USB 
stuff jumps around and I have to reconfigure everything to use whatever the 
hell random ttyUSB# it gets with this particular scheduler at boot time.

For instance, with Con's scheduler, the FTDI device feeding a cm11a from heyu 
consistently gets /dev/ttyUSB1, and the FTDI device connecting my ups always 
gets /dev/ttyUSB0.

Now, I built this kernel without that patch.  Heyu, once reconfigured for the 
new address, works, but its a very low traffic connection.

upsd OTOH is attempting to talk to the ups, I can see the leds on the FTDI 
trying to establish the usual once per second sort of a gallumph, and failing, 
and upsd is now using 40 to 50% of the cpu.  I can't feel it though, so thats 
to the sd-0.48 patches credit, its working quite well so far.

So now, I'm going to apply that patch in the subject line to this 
2.6.21.1-sd-0.48 kernel and reboot.

Ok, done.  And everything usb serial is running normally except upsd, its 
working, but using 45% of the cpu.  So your patch DOES fix SOME of the serial 
usb problems here, in that upsd can now talk to the ups.

Now, I'm going to revert this same patch that I had applied to 2.6.21.1-CFS-v9 
and see if my problems come back.

Rebooted to that, and I'm bumfuzzled, everything is working, and I didn't have 
to reconfigure everything because the ports were swapped.  Jelly side up?  
Damnedifiknow...  heyu is spending quite a bit of time at the top of the list 
in htop at about 2% cpu, so I tail the log, the motion sensor is going 
banannas, but when I check for the cause, I found the missus is sitting in 
the porch swing yakking on the phone, which is tripping it. :-)

And, just to come full circle, I re-applied that patch to 2.6.21.1-CFS-v9 and 
rebooted yet again, so I'm back to where I started.  The thing I note most is 
that while heyu is still logging the missus on the porch swing, it never even 
gets to the top page of the htop display, without the patch, it was using 
about 2% of the cpu and was often the top item in he display.

Hopefully you can sort something meaningfull out of the above.  I'm too close 
to the forest to see the trees I think.

Now, there has been an even longer thread discussing the throwing away of data 
over usb serial in particular going on here, and I've NDI if this patch would 
correct the problems they are seeing, which in that case appear to be a 
situation where the data is being fed in a constant stream, but only read 
intermittently, resulting in the need to cleanly throw away data, vs this 
situation where in both cases here, any data input is read immediately by one 
or the other of the monitoring clients.

Previously I was having all sorts of problems & miss-comm with this kernel 
until I appled this patch, now I've reversed it and things are still working.

I'll continue to run this CFS kernel, with this patch until the next scheduler 
test patch comes by I guess, or we have a 22-rc1 to beat on.

Con's sd-0.48 feels great, but leaves upsd swinging in the breeze totally 
without this patch, and with it, upsd works but still uses 45% of the cpu, so 
this is the only viable combination of the 2 schedulers and with or without 
this patch.  Whatever it does, with Ingo's scheduler, its a fix, with Con's 
its still a day late & a dollar short.

Thanks for your patience Corey.

-- 
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)
Death is nature's way of saying `Howdy'.

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

[Russell King]

On Sun, May 06, 2007 at 12:58:25PM -0400, Gene Heskett wrote:
> [long message snipped]
>
> Thanks for your patience Corey.

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

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

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

[Corey Minyard]

Russell King wrote:
> On Sun, May 06, 2007 at 12:58:25PM -0400, Gene Heskett wrote:
>   
>> [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

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

[Gene Heskett]

On Wednesday 16 May 2007, Corey Minyard wrote:
>Russell King wrote:
>> On Sun, May 06, 2007 at 12:58:25PM -0400, Gene Heskett wrote:
>>> [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

Gene here.  My impression was that this patch did help in that it appeared to 
clean up what was thought to be less than optimum code in that area.  There 
were a few times when it didn't seem to take quite as many kills and restarts 
of that ill-coded proprietary daemon to make things behave.

OTOH, I get the very strong impression there is another, more serious buglet 
someplace else that does a pretty good job of masking any black and white 
comparisons one might make about this patch.

Older code, as in 4 or 5 minor kernel versions back, appeared to work 
correctly, either on a serial port, or a usb port with a pl2303, if one could 
tolerate the miss-fires it did occasionally.  Now of course the pl2303 seems 
to be broken, both of the ones I have quit working at all with 2.6.21 final.

-- 
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)
* |Rain| prepares for polygon soup
<|Rain|> sweet merciful crap, it works?
* |Rain| faints

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

[Corey Minyard]

Andrew, can we include this in the mm series for testing?

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 <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.22-rc7/drivers/serial/8250.c
===================================================================
--- linux-2.6.22-rc7.orig/drivers/serial/8250.c
+++ linux-2.6.22-rc7/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.
@@ -1238,6 +1247,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) & 0x0f;
 			if ((up->port.type == PORT_RM9000) ?
 				(lsr & UART_LSR_THRE &&
@@ -1290,18 +1300,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
 			 */
@@ -1392,6 +1394,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)
@@ -1591,7 +1595,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
@@ -1610,9 +1615,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;
 	}
@@ -1631,13 +1640,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)
@@ -1708,8 +1718,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;
@@ -1718,8 +1727,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();
 		}
@@ -1889,6 +1902,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.
@@ -1906,14 +1931,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;
 }
 
@@ -2484,6 +2501,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.22-rc7/include/linux/serial_reg.h
===================================================================
--- linux-2.6.22-rc7.orig/include/linux/serial_reg.h
+++ linux-2.6.22-rc7/include/linux/serial_reg.h
@@ -118,6 +118,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 */

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

[Alan Cox]

> > Testing for some time in -mm in case of side effects and then pushing it
> > to an early -rc of a new kernel
> >   
> I just pulled 2.6.22-rc6-mm1 and I looked in all the previous 2.6.22 mm 
> patches and I don't see it.  Am I missing something?

Getting it submitted into -mm as a starting point.

Alan

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

[Corey Minyard]

Alan Cox wrote:
> On Fri, 06 Jul 2007 14:42:44 -0500
> Corey Minyard <minyard@acm.org> wrote:
>
>   
>> I haven't heard anything on this patch.  This is moved up to 2.6.22-rc7.
>> Is there anything that needs to change to get it included?
>>     
>
> Testing for some time in -mm in case of side effects and then pushing it
> to an early -rc of a new kernel
>   
I just pulled 2.6.22-rc6-mm1 and I looked in all the previous 2.6.22 mm 
patches and I don't see it.  Am I missing something?

-corey

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

[Alan Cox]

On Fri, 06 Jul 2007 14:42:44 -0500
Corey Minyard <minyard@acm.org> wrote:

> I haven't heard anything on this patch.  This is moved up to 2.6.22-rc7.
> Is there anything that needs to change to get it included?

Testing for some time in -mm in case of side effects and then pushing it
to an early -rc of a new kernel

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

[Corey Minyard]

I haven't heard anything on this patch.  This is moved up to 2.6.22-rc7.
Is there anything that needs to change to get it included?

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 <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.22-rc7/drivers/serial/8250.c
===================================================================
--- linux-2.6.22-rc7.orig/drivers/serial/8250.c
+++ linux-2.6.22-rc7/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.
@@ -1238,6 +1247,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) & 0x0f;
 			if ((up->port.type == PORT_RM9000) ?
 				(lsr & UART_LSR_THRE &&
@@ -1290,18 +1300,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
 			 */
@@ -1392,6 +1394,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)
@@ -1591,7 +1595,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
@@ -1610,9 +1615,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;
 	}
@@ -1631,13 +1640,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)
@@ -1708,8 +1718,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;
@@ -1718,8 +1727,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();
 		}
@@ -1889,6 +1902,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.
@@ -1906,14 +1931,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;
 }
 
@@ -2484,6 +2501,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.22-rc7/include/linux/serial_reg.h
===================================================================
--- linux-2.6.22-rc7.orig/include/linux/serial_reg.h
+++ linux-2.6.22-rc7/include/linux/serial_reg.h
@@ -118,6 +118,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 */

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

[Russell King]

On Wed, May 02, 2007 at 09:29:00AM -0500, Corey Minyard wrote:
> I didn't hear back, so I went ahead and did a patch.

You haven't heard back because I've not had a chance (and still
haven't) to look at the various data sheets I've accumulated.
I'll be catching up with anything not requiring a simple
email reply tomorrow.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

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

[Corey Minyard]

I didn't hear back, so I went ahead and did a patch.  This patch
collects the bits from the LSR (and the MSR, which had the same
problem in one place) for later use.

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 <minyard@acm.org>
Cc: Russell King <rmk+lkml@arm.linux.org.uk>

 drivers/serial/8250.c      |   72 ++++++++++++++++++++++++++-------------------
 include/linux/serial_reg.h |    1 
 2 files changed, 44 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,7 @@ 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;
 
 	/*
 	 * Must disable interrupts or else we risk racing with the interrupt
@@ -1515,9 +1519,11 @@ static void serial8250_backup_timeout(un
 	 * the "Diva" UART used on the management processor on many HP
 	 * ia64 and parisc boxes.
 	 */
+	lsr = serial_in(up, UART_LSR);
+	up->lsr_saved_flags |= lsr & LSR_SAVE_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 +1542,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 +1620,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 +1629,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 +1804,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 +1833,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;
 }
 
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 */