[PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[David Woodhouse]

We weren't flushing the TX FIFO on 8250 uarts when uart_flush_buffer()
was called. This adds a flush_buffer() method to the uart_port
operations and calls it from uart_flush_buffer().

This also adds a method to the line discipline which is called upon line
status changes, and uses the helper function for that to clean up the
uart hardware drivers slightly, removing the explicit wakeup on
delta_msr_wait which is used for TIOCMIWAIT.

I'll be putting together a line discipline which actually needs this
callback shortly.

-- 
dwmw2

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[David Woodhouse]

On Fri, 2004-11-05 at 13:06 +0000, David Woodhouse wrote:
> We weren't flushing the TX FIFO on 8250 uarts when uart_flush_buffer()
> was called. This adds a flush_buffer() method to the uart_port
> operations and calls it from uart_flush_buffer().
> 
> This also adds a method to the line discipline which is called upon line
> status changes, and uses the helper function for that to clean up the
> uart hardware drivers slightly, removing the explicit wakeup on
> delta_msr_wait which is used for TIOCMIWAIT.
> 
> I'll be putting together a line discipline which actually needs this
> callback shortly.

Doh.

===== drivers/char/tty_io.c 1.152 vs edited =====
--- 1.152/drivers/char/tty_io.c	2004-10-25 21:06:49 +01:00
+++ edited/drivers/char/tty_io.c	2004-11-05 12:28:24 +00:00
@@ -720,6 +720,26 @@
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
+ *	tty_status_change -	notify of line status changes
+ *	@tty: terminal
+ *
+ *	Helper for informing the line discipline that the modem
+ *	status lines may have changed.
+ */
+
+void tty_status_changed(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+	if(ld) {
+		if(ld->status_changed)
+			ld->status_changed(tty);
+		tty_ldisc_deref(ld);
+	}
+}
+
+EXPORT_SYMBOL_GPL(tty_status_changed);
+
+/**
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
  *
===== drivers/serial/8250.c 1.89 vs edited =====
--- 1.89/drivers/serial/8250.c	2004-11-02 09:31:48 +00:00
+++ edited/drivers/serial/8250.c	2004-11-05 12:59:51 +00:00
@@ -351,6 +351,24 @@
 	}
 }
 
+static void serial8250_flush_buffer(struct uart_port *port)
+{
+	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	unsigned long flags;
+	unsigned char fcr;
+
+	if (!up->capabilities & UART_CAP_FIFO)
+		return;
+
+	spin_lock_irqsave(&up->port.lock, flags);
+
+	fcr = serial_inp(up, UART_FCR);
+	fcr |= UART_FCR_CLEAR_XMIT;
+	serial_outp(up, UART_FCR, fcr);
+
+	spin_unlock_irqrestore(&up->port.lock, flags);
+}
+	
 /*
  * IER sleep support.  UARTs which have EFRs need the "extended
  * capability" bit enabled.  Note that on XR16C850s, we need to
@@ -1115,7 +1133,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port);
 }
 
 /*
@@ -1916,6 +1934,7 @@
 
 static struct uart_ops serial8250_pops = {
 	.tx_empty	= serial8250_tx_empty,
+	.flush_buffer	= serial8250_flush_buffer,
 	.set_mctrl	= serial8250_set_mctrl,
 	.get_mctrl	= serial8250_get_mctrl,
 	.stop_tx	= serial8250_stop_tx,
===== drivers/serial/amba-pl010.c 1.23 vs edited =====
--- 1.23/drivers/serial/amba-pl010.c	2004-04-14 17:31:16 +01:00
+++ edited/drivers/serial/amba-pl010.c	2004-11-05 12:51:09 +00:00
@@ -277,7 +277,7 @@
 	if (delta & UART01x_FR_CTS)
 		uart_handle_cts_change(&uap->port, status & UART01x_FR_CTS);
 
-	wake_up_interruptible(&uap->port.info->delta_msr_wait);
+	uart_status_changed(&uap->port);
 }
 
 static irqreturn_t pl010_int(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/amba-pl011.c 1.5 vs edited =====
--- 1.5/drivers/serial/amba-pl011.c	2004-10-30 15:09:16 +01:00
+++ edited/drivers/serial/amba-pl011.c	2004-11-05 12:51:31 +00:00
@@ -240,7 +240,7 @@
 	if (delta & UART01x_FR_CTS)
 		uart_handle_cts_change(&uap->port, status & UART01x_FR_CTS);
 
-	wake_up_interruptible(&uap->port.info->delta_msr_wait);
+	uart_status_changed(&uap->port);
 }
 
 static irqreturn_t pl011_int(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/au1x00_uart.c 1.4 vs edited =====
--- 1.4/drivers/serial/au1x00_uart.c	2004-10-30 23:12:12 +01:00
+++ edited/drivers/serial/au1x00_uart.c	2004-11-05 12:51:57 +00:00
@@ -375,7 +375,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port);
 }
 
 /*
===== drivers/serial/icom.c 1.3 vs edited =====
--- 1.3/drivers/serial/icom.c	2004-11-01 12:29:39 +00:00
+++ edited/drivers/serial/icom.c	2004-11-05 12:52:33 +00:00
@@ -686,8 +686,8 @@
 			uart_handle_cts_change(&icom_port->uart_port,
 					       delta_status & ICOM_CTS);
 
-		wake_up_interruptible(&icom_port->uart_port.info->
-				      delta_msr_wait);
+		uart_status_changed(&icom_port->uart_port);
+
 		old_status = status;
 	}
 	spin_unlock(&icom_port->uart_port.lock);
===== drivers/serial/ip22zilog.c 1.1 vs edited =====
--- 1.1/drivers/serial/ip22zilog.c	2004-02-19 03:42:44 +00:00
+++ edited/drivers/serial/ip22zilog.c	2004-11-05 12:53:18 +00:00
@@ -383,7 +383,7 @@
 			uart_handle_cts_change(&up->port,
 					       (status & CTS));
 
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		uart_status_changed(&up->port.info);
 	}
 
 	up->prev_status = status;
===== drivers/serial/pmac_zilog.c 1.16 vs edited =====
--- 1.16/drivers/serial/pmac_zilog.c	2004-11-01 12:29:40 +00:00
+++ edited/drivers/serial/pmac_zilog.c	2004-11-05 12:53:45 +00:00
@@ -361,7 +361,7 @@
 			uart_handle_cts_change(&uap->port,
 					       !(status & CTS));
 
-		wake_up_interruptible(&uap->port.info->delta_msr_wait);
+		uart_status_changed(&uap->port.info);
 	}
 
 	uap->prev_status = status;
===== drivers/serial/pxa.c 1.5 vs edited =====
--- 1.5/drivers/serial/pxa.c	2004-09-28 20:35:56 +01:00
+++ edited/drivers/serial/pxa.c	2004-11-05 12:54:06 +00:00
@@ -251,7 +251,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 /*
===== drivers/serial/sa1100.c 1.26 vs edited =====
--- 1.26/drivers/serial/sa1100.c	2004-04-09 22:55:47 +01:00
+++ edited/drivers/serial/sa1100.c	2004-11-05 12:54:32 +00:00
@@ -120,7 +120,7 @@
 	if (changed & TIOCM_CTS)
 		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
 
-	wake_up_interruptible(&sport->port.info->delta_msr_wait);
+	uart_status_changed(&sport->port);
 }
 
 /*
===== drivers/serial/serial_core.c 1.94 vs edited =====
--- 1.94/drivers/serial/serial_core.c	2004-11-01 12:29:41 +00:00
+++ edited/drivers/serial/serial_core.c	2004-11-05 12:35:05 +00:00
@@ -515,6 +515,8 @@
 
 	spin_lock_irqsave(&port->lock, flags);
 	uart_circ_clear(&state->info->xmit);
+	if (port->ops->flush_buffer)
+		port->ops->flush_buffer(port);
 	spin_unlock_irqrestore(&port->lock, flags);
 	tty_wakeup(tty);
 }
===== drivers/serial/serial_lh7a40x.c 1.1 vs edited =====
--- 1.1/drivers/serial/serial_lh7a40x.c	2004-06-05 11:30:48 +01:00
+++ edited/drivers/serial/serial_lh7a40x.c	2004-11-05 12:54:55 +00:00
@@ -263,7 +263,7 @@
 	if (delta & CTS)
 		uart_handle_cts_change (port, status & CTS);
 
-	wake_up_interruptible (&port->info->delta_msr_wait);
+	uart_status_changed(port);
 }
 
 static irqreturn_t lh7a40xuart_int (int irq, void* dev_id,
===== drivers/serial/sunsab.c 1.35 vs edited =====
--- 1.35/drivers/serial/sunsab.c	2004-08-26 23:38:22 +01:00
+++ edited/drivers/serial/sunsab.c	2004-11-05 12:55:15 +00:00
@@ -305,7 +305,7 @@
 		up->port.icount.dsr++;
 	}
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 static irqreturn_t sunsab_interrupt(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/sunsu.c 1.46 vs edited =====
--- 1.46/drivers/serial/sunsu.c	2004-10-31 21:33:17 +00:00
+++ edited/drivers/serial/sunsu.c	2004-11-05 12:55:29 +00:00
@@ -452,7 +452,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 static irqreturn_t sunsu_serial_interrupt(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/sunzilog.c 1.46 vs edited =====
--- 1.46/drivers/serial/sunzilog.c	2004-09-20 21:46:03 +01:00
+++ edited/drivers/serial/sunzilog.c	2004-11-05 12:55:46 +00:00
@@ -464,7 +464,7 @@
 			uart_handle_cts_change(&up->port,
 					       (status & CTS));
 
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		uart_status_changed(&up->port.info);
 	}
 
 	up->prev_status = status;
===== drivers/serial/uart00.c 1.17 vs edited =====
--- 1.17/drivers/serial/uart00.c	2004-04-29 13:14:52 +01:00
+++ edited/drivers/serial/uart00.c	2004-11-05 12:56:06 +00:00
@@ -249,7 +249,7 @@
 	if (status & UART_MSR_DCTS_MSK)
 		uart_handle_cts_change(port, status & UART_MSR_CTS_MSK);
 
-	wake_up_interruptible(&port->info->delta_msr_wait);
+	uart_status_changed(port);
 }
 
 static irqreturn_t uart00_int(int irq, void *dev_id, struct pt_regs *regs)
===== include/linux/serial_core.h 1.45 vs edited =====
--- 1.45/include/linux/serial_core.h	2004-10-30 21:24:41 +01:00
+++ edited/include/linux/serial_core.h	2004-11-05 13:00:36 +00:00
@@ -111,6 +111,7 @@
  */
 struct uart_ops {
 	unsigned int	(*tx_empty)(struct uart_port *);
+	void		(*flush_buffer)(struct uart_port *);
 	void		(*set_mctrl)(struct uart_port *, unsigned int mctrl);
 	unsigned int	(*get_mctrl)(struct uart_port *);
 	void		(*stop_tx)(struct uart_port *, unsigned int tty_stop);
@@ -448,6 +449,16 @@
 			}
 		}
 	}
+}
+
+/**
+ *	uart_status_changed - notify ldisc of a change of modem status lines
+ *	@post: uart_port structure for the open port
+ */
+static inline void uart_status_changed(struct uart_port *port)
+{
+	tty_status_changed(port->info->tty);
+	wake_up_interruptible(&port->info->delta_msr_wait);
 }
 
 /*
===== include/linux/tty.h 1.30 vs edited =====
--- 1.30/include/linux/tty.h	2004-10-21 18:03:22 +01:00
+++ edited/include/linux/tty.h	2004-11-05 12:38:02 +00:00
@@ -374,6 +374,7 @@
 extern void tty_ldisc_put(int);
 
 extern void tty_wakeup(struct tty_struct *tty);
+extern void tty_status_changed(struct tty_struct *tty);
 extern void tty_ldisc_flush(struct tty_struct *tty);
 
 struct semaphore;
===== include/linux/tty_ldisc.h 1.7 vs edited =====
--- 1.7/include/linux/tty_ldisc.h	2004-10-21 18:03:22 +01:00
+++ edited/include/linux/tty_ldisc.h	2004-11-05 12:08:29 +00:00
@@ -102,6 +102,11 @@
  *	cease I/O to the tty driver. Can sleep. The driver should
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
+ *
+ * void (*status_changed)(struct tty_struct *)
+ *
+ *	Called when the modem status lines (CTS, DSR, DCD etc.) are
+ *	changed. May not sleep. 
  */
 
 #include <linux/fs.h>
@@ -138,6 +143,7 @@
 			       char *fp, int count);
 	int	(*receive_room)(struct tty_struct *);
 	void	(*write_wakeup)(struct tty_struct *);
+	void	(*status_changed)(struct tty_struct *);
 
 	struct  module *owner;
 	

-- 
dwmw2

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> We weren't flushing the TX FIFO on 8250 uarts when uart_flush_buffer()
>  was called. This adds a flush_buffer() method to the uart_port
>  operations and calls it from uart_flush_buffer().

Your patch makes my computer stop working, which saddens me.


NMI Watchdog detected LOCKUP on CPU1CPU 1 
Modules linked in:                        
Pid: 1, comm: init Not tainted 2.6.10-rc1-mm4
RIP: 0010:[<ffffffff803b04c0>] <ffffffff803b04c0>{_spin_lock_irqsave+31}
RSP: 0018:ffff81007ff19d68  EFLAGS: 00000013                            
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 0000000000000000 RDI: 0000000000000001
RBP: ffffffff80585da0 R08: ffff81000b663920 R09: 0000000000000000
R10: ffff81017ffa3980 R11: 0000000000000040 R12: 0000000000000246
R13: ffff81017f4de000 R14: ffff81000b65ad28 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffffffff80594c80(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b                           
CR2: 0000000000000000 CR3: 000000017f4e6000 CR4: 00000000000006e0
Process init (pid: 1, threadinfo ffff81007ff18000, task ffff81007ffaf740)
Stack: 0000000000000013 ffffffff80585da0 ffffffff80585da0 ffffffff8028fe34 
       0000000000000246 ffff81000b65ad08 0000000000000246 ffffffff8028d8d3 
       ffffffff80585da0 0000000000000246                                   
Call Trace:<ffffffff8028fe34>{serial8250_flush_buffer+15} <ffffffff8028d8d3>{uart_flush_buffer+75} 
       <ffffffff8028e9bc>{uart_close+332} <ffffffff80272fa9>{release_dev+578}                      
       <ffffffff8017b942>{chrdev_open+438} <ffffffff8015ae42>{poison_obj+56}  
       <ffffffff80174273>{filp_dtor+0} <ffffffff802737a2>{tty_release+17}    
       <ffffffff8017406d>{__fput+100} <ffffffff80171911>{filp_close+107}  
       <ffffffff8017199c>{sys_close+132} <ffffffff8010f346>{system_call+126} 


Maybe the lock which uart_flush_buffer() takes is the same as the lock
which serial8250_flush_buffer() takes?

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[David Woodhouse]

On Tue, 2004-11-09 at 01:22 -0800, Andrew Morton wrote:
> David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > We weren't flushing the TX FIFO on 8250 uarts when uart_flush_buffer()
> >  was called. This adds a flush_buffer() method to the uart_port
> >  operations and calls it from uart_flush_buffer().
> 
> Your patch makes my computer stop working, which saddens me.

> Maybe the lock which uart_flush_buffer() takes is the same as the lock
> which serial8250_flush_buffer() takes?

Er, yes it is, sorry. This version should make you less unhappy:

===== drivers/char/tty_io.c 1.152 vs edited =====
--- 1.152/drivers/char/tty_io.c	Mon Oct 25 21:06:49 2004
+++ edited/drivers/char/tty_io.c	Fri Nov  5 13:03:32 2004
@@ -720,6 +720,26 @@
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
+ *	tty_status_change -	notify of line status changes
+ *	@tty: terminal
+ *
+ *	Helper for informing the line discipline that the modem
+ *	status lines may have changed.
+ */
+
+void tty_status_changed(struct tty_struct *tty)
+{
+	struct tty_ldisc *ld = tty_ldisc_ref(tty);
+	if(ld) {
+		if(ld->status_changed)
+			ld->status_changed(tty);
+		tty_ldisc_deref(ld);
+	}
+}
+
+EXPORT_SYMBOL_GPL(tty_status_changed);
+
+/**
  *	tty_ldisc_flush	-	flush line discipline queue
  *	@tty: tty
  *
===== drivers/serial/8250.c 1.77 vs edited =====
--- 1.77/drivers/serial/8250.c	Mon Oct 25 13:05:26 2004
+++ edited/drivers/serial/8250.c	Tue Nov  9 11:05:15 2004
@@ -356,6 +356,20 @@
 	}
 }
 
+static void serial8250_flush_buffer(struct uart_port *port)
+{
+	struct uart_8250_port *up = (struct uart_8250_port *)port;
+	unsigned long flags;
+	unsigned char fcr;
+
+	if (!up->capabilities & UART_CAP_FIFO)
+		return;
+
+	fcr = serial_inp(up, UART_FCR);
+	fcr |= UART_FCR_CLEAR_XMIT;
+	serial_outp(up, UART_FCR, fcr);
+}
+	
 /*
  * IER sleep support.  UARTs which have EFRs need the "extended
  * capability" bit enabled.  Note that on XR16C850s, we need to
@@ -1120,7 +1134,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port);
 }
 
 /*
@@ -1940,6 +1954,7 @@
 
 static struct uart_ops serial8250_pops = {
 	.tx_empty	= serial8250_tx_empty,
+	.flush_buffer	= serial8250_flush_buffer,
 	.set_mctrl	= serial8250_set_mctrl,
 	.get_mctrl	= serial8250_get_mctrl,
 	.stop_tx	= serial8250_stop_tx,
===== drivers/serial/amba-pl010.c 1.23 vs edited =====
--- 1.23/drivers/serial/amba-pl010.c	Wed Apr 14 17:31:16 2004
+++ edited/drivers/serial/amba-pl010.c	Fri Nov  5 13:03:32 2004
@@ -277,7 +277,7 @@
 	if (delta & UART01x_FR_CTS)
 		uart_handle_cts_change(&uap->port, status & UART01x_FR_CTS);
 
-	wake_up_interruptible(&uap->port.info->delta_msr_wait);
+	uart_status_changed(&uap->port);
 }
 
 static irqreturn_t pl010_int(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/amba-pl011.c 1.4 vs edited =====
--- 1.4/drivers/serial/amba-pl011.c	Tue Oct  5 11:04:12 2004
+++ edited/drivers/serial/amba-pl011.c	Fri Nov  5 13:03:32 2004
@@ -240,7 +240,7 @@
 	if (delta & UART01x_FR_CTS)
 		uart_handle_cts_change(&uap->port, status & UART01x_FR_CTS);
 
-	wake_up_interruptible(&uap->port.info->delta_msr_wait);
+	uart_status_changed(&uap->port);
 }
 
 static irqreturn_t pl011_int(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/au1x00_uart.c 1.3 vs edited =====
--- 1.3/drivers/serial/au1x00_uart.c	Sat Oct 23 00:06:15 2004
+++ edited/drivers/serial/au1x00_uart.c	Fri Nov  5 13:03:32 2004
@@ -375,7 +375,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port);
 }
 
 /*
===== drivers/serial/icom.c 1.2 vs edited =====
--- 1.2/drivers/serial/icom.c	Wed Oct 20 09:37:15 2004
+++ edited/drivers/serial/icom.c	Fri Nov  5 13:03:32 2004
@@ -692,8 +692,8 @@
 			uart_handle_cts_change(&icom_port->uart_port,
 					       delta_status & ICOM_CTS);
 
-		wake_up_interruptible(&icom_port->uart_port.info->
-				      delta_msr_wait);
+		uart_status_changed(&icom_port->uart_port);
+
 		old_status = status;
 	}
 	spin_unlock(&icom_port->uart_port.lock);
===== drivers/serial/ip22zilog.c 1.1 vs edited =====
--- 1.1/drivers/serial/ip22zilog.c	Thu Feb 19 03:42:44 2004
+++ edited/drivers/serial/ip22zilog.c	Fri Nov  5 13:03:32 2004
@@ -383,7 +383,7 @@
 			uart_handle_cts_change(&up->port,
 					       (status & CTS));
 
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		uart_status_changed(&up->port.info);
 	}
 
 	up->prev_status = status;
===== drivers/serial/pmac_zilog.c 1.15 vs edited =====
--- 1.15/drivers/serial/pmac_zilog.c	Wed Oct 20 09:37:15 2004
+++ edited/drivers/serial/pmac_zilog.c	Fri Nov  5 13:03:32 2004
@@ -361,7 +361,7 @@
 			uart_handle_cts_change(&uap->port,
 					       !(status & CTS));
 
-		wake_up_interruptible(&uap->port.info->delta_msr_wait);
+		uart_status_changed(&uap->port.info);
 	}
 
 	uap->prev_status = status;
===== drivers/serial/pxa.c 1.5 vs edited =====
--- 1.5/drivers/serial/pxa.c	Tue Sep 28 20:35:56 2004
+++ edited/drivers/serial/pxa.c	Fri Nov  5 13:03:32 2004
@@ -251,7 +251,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 /*
===== drivers/serial/sa1100.c 1.26 vs edited =====
--- 1.26/drivers/serial/sa1100.c	Fri Apr  9 22:55:47 2004
+++ edited/drivers/serial/sa1100.c	Fri Nov  5 13:03:32 2004
@@ -120,7 +120,7 @@
 	if (changed & TIOCM_CTS)
 		uart_handle_cts_change(&sport->port, status & TIOCM_CTS);
 
-	wake_up_interruptible(&sport->port.info->delta_msr_wait);
+	uart_status_changed(&sport->port);
 }
 
 /*
===== drivers/serial/serial_core.c 1.92 vs edited =====
--- 1.92/drivers/serial/serial_core.c	Thu Oct 21 18:03:22 2004
+++ edited/drivers/serial/serial_core.c	Fri Nov  5 13:03:32 2004
@@ -567,6 +567,8 @@
 
 	spin_lock_irqsave(&port->lock, flags);
 	uart_circ_clear(&state->info->xmit);
+	if (port->ops->flush_buffer)
+		port->ops->flush_buffer(port);
 	spin_unlock_irqrestore(&port->lock, flags);
 	tty_wakeup(tty);
 }
===== drivers/serial/serial_lh7a40x.c 1.1 vs edited =====
--- 1.1/drivers/serial/serial_lh7a40x.c	Sat Jun  5 11:30:48 2004
+++ edited/drivers/serial/serial_lh7a40x.c	Fri Nov  5 13:03:32 2004
@@ -263,7 +263,7 @@
 	if (delta & CTS)
 		uart_handle_cts_change (port, status & CTS);
 
-	wake_up_interruptible (&port->info->delta_msr_wait);
+	uart_status_changed(port);
 }
 
 static irqreturn_t lh7a40xuart_int (int irq, void* dev_id,
===== drivers/serial/sunsab.c 1.35 vs edited =====
--- 1.35/drivers/serial/sunsab.c	Thu Aug 26 23:38:22 2004
+++ edited/drivers/serial/sunsab.c	Fri Nov  5 13:03:32 2004
@@ -305,7 +305,7 @@
 		up->port.icount.dsr++;
 	}
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 static irqreturn_t sunsab_interrupt(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/sunsu.c 1.45 vs edited =====
--- 1.45/drivers/serial/sunsu.c	Mon Sep 20 21:46:03 2004
+++ edited/drivers/serial/sunsu.c	Fri Nov  5 13:03:32 2004
@@ -453,7 +453,7 @@
 	if (status & UART_MSR_DCTS)
 		uart_handle_cts_change(&up->port, status & UART_MSR_CTS);
 
-	wake_up_interruptible(&up->port.info->delta_msr_wait);
+	uart_status_changed(&up->port.info);
 }
 
 static irqreturn_t sunsu_serial_interrupt(int irq, void *dev_id, struct pt_regs *regs)
===== drivers/serial/sunzilog.c 1.46 vs edited =====
--- 1.46/drivers/serial/sunzilog.c	Mon Sep 20 21:46:03 2004
+++ edited/drivers/serial/sunzilog.c	Fri Nov  5 13:03:32 2004
@@ -464,7 +464,7 @@
 			uart_handle_cts_change(&up->port,
 					       (status & CTS));
 
-		wake_up_interruptible(&up->port.info->delta_msr_wait);
+		uart_status_changed(&up->port.info);
 	}
 
 	up->prev_status = status;
===== drivers/serial/uart00.c 1.17 vs edited =====
--- 1.17/drivers/serial/uart00.c	Thu Apr 29 13:14:52 2004
+++ edited/drivers/serial/uart00.c	Fri Nov  5 13:03:32 2004
@@ -249,7 +249,7 @@
 	if (status & UART_MSR_DCTS_MSK)
 		uart_handle_cts_change(port, status & UART_MSR_CTS_MSK);
 
-	wake_up_interruptible(&port->info->delta_msr_wait);
+	uart_status_changed(port);
 }
 
 static irqreturn_t uart00_int(int irq, void *dev_id, struct pt_regs *regs)
===== include/linux/serial_core.h 1.43 vs edited =====
--- 1.43/include/linux/serial_core.h	Tue Sep 14 01:23:24 2004
+++ edited/include/linux/serial_core.h	Fri Nov  5 13:03:32 2004
@@ -108,6 +108,7 @@
  */
 struct uart_ops {
 	unsigned int	(*tx_empty)(struct uart_port *);
+	void		(*flush_buffer)(struct uart_port *);
 	void		(*set_mctrl)(struct uart_port *, unsigned int mctrl);
 	unsigned int	(*get_mctrl)(struct uart_port *);
 	void		(*stop_tx)(struct uart_port *, unsigned int tty_stop);
@@ -448,6 +449,16 @@
 			}
 		}
 	}
+}
+
+/**
+ *	uart_status_changed - notify ldisc of a change of modem status lines
+ *	@post: uart_port structure for the open port
+ */
+static inline void uart_status_changed(struct uart_port *port)
+{
+	tty_status_changed(port->info->tty);
+	wake_up_interruptible(&port->info->delta_msr_wait);
 }
 
 /*
===== include/linux/tty.h 1.30 vs edited =====
--- 1.30/include/linux/tty.h	Thu Oct 21 18:03:22 2004
+++ edited/include/linux/tty.h	Fri Nov  5 13:03:32 2004
@@ -374,6 +374,7 @@
 extern void tty_ldisc_put(int);
 
 extern void tty_wakeup(struct tty_struct *tty);
+extern void tty_status_changed(struct tty_struct *tty);
 extern void tty_ldisc_flush(struct tty_struct *tty);
 
 struct semaphore;
===== include/linux/tty_ldisc.h 1.7 vs edited =====
--- 1.7/include/linux/tty_ldisc.h	Thu Oct 21 18:03:22 2004
+++ edited/include/linux/tty_ldisc.h	Fri Nov  5 13:03:32 2004
@@ -102,6 +102,11 @@
  *	cease I/O to the tty driver. Can sleep. The driver should
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
+ *
+ * void (*status_changed)(struct tty_struct *)
+ *
+ *	Called when the modem status lines (CTS, DSR, DCD etc.) are
+ *	changed. May not sleep. 
  */
 
 #include <linux/fs.h>
@@ -138,6 +143,7 @@
 			       char *fp, int count);
 	int	(*receive_room)(struct tty_struct *);
 	void	(*write_wakeup)(struct tty_struct *);
+	void	(*status_changed)(struct tty_struct *);
 
 	struct  module *owner;
 	

-- 
dwmw2

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Alan Cox]

On Maw, 2004-11-09 at 11:07, David Woodhouse wrote:
> + *	tty_status_change -	notify of line status changes
> + *	@tty: terminal
> + *
> + *	Helper for informing the line discipline that the modem
> + *	status lines may have changed.
> + */
> +
> +void tty_status_changed(struct tty_struct *tty)
> +{
> +	struct tty_ldisc *ld = tty_ldisc_ref(tty);
> +	if(ld) {
> +		if(ld->status_changed)
> +			ld->status_changed(tty);
> +		tty_ldisc_deref(ld);
> +	}
> +}
> +

This is the wrong way to do it. I've been trying this and discarded it.
The problem is that data arrival is asynchronous to the event which
means you've not got a clue how to combine the status change and the
data stream. This in itself makes the whole feature useless.

Modem changes have to go inline with the data just like break and
parity.

> + *
> + * void (*status_changed)(struct tty_struct *)
> + *
> + *	Called when the modem status lines (CTS, DSR, DCD etc.) are
> + *	changed. May not sleep. 
>   */

You also forgot to update the Documentation directory.

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Russell King]

On Tue, Nov 09, 2004 at 11:15:30AM +0000, Alan Cox wrote:
> On Maw, 2004-11-09 at 11:07, David Woodhouse wrote:
> > + *	tty_status_change -	notify of line status changes
> > + *	@tty: terminal
> > + *
> > + *	Helper for informing the line discipline that the modem
> > + *	status lines may have changed.
> > + */
> > +
> > +void tty_status_changed(struct tty_struct *tty)
> > +{
> > +	struct tty_ldisc *ld = tty_ldisc_ref(tty);
> > +	if(ld) {
> > +		if(ld->status_changed)
> > +			ld->status_changed(tty);
> > +		tty_ldisc_deref(ld);
> > +	}
> > +}
> > +
> 
> This is the wrong way to do it. I've been trying this and discarded it.
> The problem is that data arrival is asynchronous to the event which
> means you've not got a clue how to combine the status change and the
> data stream. This in itself makes the whole feature useless.

The status change and character receive are asynchronous with respect
to each other anyway.  Consider the case where the serial port says
"we have characters waiting" - we receive a FIFO full of characters.
It then says that the modem status has changed.

You don't actually know when the modem status changed.  It may have
changed before any of these characters were received because character
receiption is is highest priority.

Even if you poll the MSR register after you receive every FIFO character,
you still don't know whether the modem status changed before or after
that character was received.

Therefore, anything which relies on knowing precisely when the modem
status changed in the character stream is unfortunately broken by the
limitations of 8250-based hardware.

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

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Alan Cox]

>
> The status change and character receive are asynchronous with respect
> to each other anyway.  Consider the case where the serial port says
> "we have characters waiting" - we receive a FIFO full of characters.
> It then says that the modem status has changed.

Only to a small degree, and the fifo is configurable in software. RS485
multi
drop users do generally run with FIFO and the message boundaries mean
that the
event accuracy is good enough.

What the callback does is provide a notification *before* the data
arrives, and easily 2mS out which calls a line discipline function that
cannot sleep and cannot viably interact with the data stream.

In addition you've not defined the semantics of that function for
calling back into the tty driver. You tell me "carrier dropped", yet in
that routine you don't define what occurs if I want to respond by
lowering my carrier too.
Right now it'll deadlock if I do anything in that function of any
relevance.

So its broken, totally and utterly. Its the kind of undefined,
unserialized crap that I'm trying to get _OUT_ of the serial layer.

Now when you add it to the flip buffer as another event like errors the
line discipline knows the approximate time it occurred, has defined
semantics for ordering and queuing and can respond by flipping its modem
signals. You can
actually *use* it to implement things like half-duplex and multidrop.

Andrew - please reject the patch.

Alan

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[David Woodhouse]

On Tue, 2004-11-09 at 13:17 +0000, Alan Cox wrote:
> What the callback does is provide a notification *before* the data
> arrives, and easily 2mS out which calls a line discipline function that
> cannot sleep and cannot viably interact with the data stream.

Actually I needed it to respond to CTS going away, and it provides a
notification *before* the data are *sent*. Which lets me know that the
first bytes of my packet were dropped by the automatic contention
detection circuitry and I need to flush the rest of the packet from the
FIFO rather than letting the hardware driver wait for CTS to come back
then then send a corrupt half-packet.

> Now when you add it to the flip buffer as another event like errors the
> line discipline knows the approximate time it occurred, has defined
> semantics for ordering and queuing and can respond by flipping its modem
> signals. You can actually *use* it to implement things like half-duplex 
> and multidrop.

That solves a different problem, and isn't quite as useful to me. I want
to be able to respond to CTS going low as quickly as possible, by
flushing the rest of the characters from the outgoing queue. I was happy
enough with using a tasklet to actually call the flush method, to avoid
the deadlock you pointed out without changing the locking of all the
hardware drivers). 

> Andrew - please reject the patch.

I'll submit the bit which makes the flush_buffer method work on its own.
Alan, would you care to offer a viable alternative which solves the
problem I'm interested in?

-- 
dwmw2

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Alan Cox]

On Maw, 2004-11-09 at 14:39, David Woodhouse wrote:
> Actually I needed it to respond to CTS going away, and it provides a
> notification *before* the data are *sent*. Which lets me know that the
> first bytes of my packet were dropped by the automatic contention
> detection circuitry and I need to flush the rest of the packet from the
> FIFO rather than letting the hardware driver wait for CTS to come back
> then then send a corrupt half-packet.

But you can't flush the fifo from that callback, you don't have any
locking on it. What are you locking semantics ? Define them, versus
open, versus close, versus other I/O.

> That solves a different problem, and isn't quite as useful to me. I want
> to be able to respond to CTS going low as quickly as possible, by
> flushing the rest of the characters from the outgoing queue. I was happy
> enough with using a tasklet to actually call the flush method, to avoid
> the deadlock you pointed out without changing the locking of all the
> hardware drivers). 

So you want to replace a tasklet that responds to the event with a
tasklet which is called by the event ? Tell me how they differ when low
latency is set on the tty - I don't see any difference in performance

> > Andrew - please reject the patch.
> 
> I'll submit the bit which makes the flush_buffer method work on its own.
> Alan, would you care to offer a viable alternative which solves the
> problem I'm interested in?

If you can't define the locking semantics, its not viable anyway. I see
two things we can do usefully here. The first is to teach
tty_flip_buffer_push more about urgency - since the new tty code I'm
running/crashing/debugging has a real packet queue not just flip buffers
we can queue a tiny message to the queue front and we can probably begin
to cheat a bit more on low_latency v immediate.

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Russell King]

On Tue, Nov 09, 2004 at 01:17:22PM +0000, Alan Cox wrote:
> So its broken, totally and utterly. Its the kind of undefined,
> unserialized crap that I'm trying to get _OUT_ of the serial layer.

I think you mean the tty layer here - the tty layer is where most of
the serialisation problems remain, and the locking in the serial
layer is mostly there to work around the tty layers deficiencies.

I would have liked to have rewritten the tty layer along the lines
that Ted was suggesting, but that would've been far too much work
for me to do alone.

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

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Alan Cox]

On Maw, 2004-11-09 at 14:47, Russell King wrote:
> On Tue, Nov 09, 2004 at 01:17:22PM +0000, Alan Cox wrote:
> > So its broken, totally and utterly. Its the kind of undefined,
> > unserialized crap that I'm trying to get _OUT_ of the serial layer.
> 
> I think you mean the tty layer here - the tty layer is where most of

Out of the serial layer too. While the serial layer does stuff like call
line discipline functions from the wrong context and sick hacks like
calling functions out of tty layer workqueues its also involved.

Right now calls into the ldisc side are all sane (there are two corner
cases left that need the other side fixing). The calls into serial
driver side is still pending repair and also needs the flip buffers
fixing first.

> the serialisation problems remain, and the locking in the serial
> layer is mostly there to work around the tty layers deficiencies.
> 
> I would have liked to have rewritten the tty layer along the lines
> that Ted was suggesting, but that would've been far too much work
> for me to do alone.

I'm working on it. It would be helpful if the drivers/serial code would
use helpers and not dig in places it shouldnt so that the transition can
be cleaner.

Something like this for 8250.c - it fixes the sick hacks calling into
tty
workqueues, the deadlock on overrun and cleans up flip buffer whacking
to use the helper. The helpers handle the overrun case internally so
that cleans up some code, they also happen to be API's I think I can
preserve when updating while direct buffer whacking is probably a nono.

In general tty driver hacking folks should expect

- direct flip buffer poking to break
- flip buffer length checking to break (DMA cards and virtualised
machines want
  variable sized buffers)

so if they can move towards tty_insert_flip_char() they'll make life
easier down the line. I'll try and push a few more "fake" helpers as I
try and get the dynamic flip buffers into shape.

--- ../linux.vanilla-2.6.10rc1/drivers/serial/8250.c	2004-11-05 15:42:15.000000000 +0000
+++ drivers/serial/8250.c	2004-11-09 15:42:49.102511656 +0000
@@ -32,7 +32,7 @@
 #include <linux/serialP.h>
 #include <linux/delay.h>
 #include <linux/device.h>
-
+#include <linux/tty_flip.h>
 #include <asm/io.h>
 #include <asm/irq.h>
 
@@ -978,16 +978,19 @@
 	struct tty_struct *tty = up->port.info->tty;
 	unsigned char ch, lsr = *status;
 	int max_count = 256;
+	char flag;
 
 	do {
+		/* The following is not allowed by the tty layer and
+		   unsafe. It should be fixed ASAP */
 		if (unlikely(tty->flip.count >= TTY_FLIPBUF_SIZE)) {
-			tty->flip.work.func((void *)tty);
-			if (tty->flip.count >= TTY_FLIPBUF_SIZE)
-				return; // if TTY_DONT_FLIP is set
+			if(tty->low_latency)
+				tty_flip_buffer_push(tty);
+			/* If this failed then we will throw away the
+			   bytes but must do so to clear interrupts */
 		}
 		ch = serial_inp(up, UART_RX);
-		*tty->flip.char_buf_ptr = ch;
-		*tty->flip.flag_buf_ptr = TTY_NORMAL;
+		flag = TTY_NORMAL;
 		up->port.icount.rx++;
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
@@ -1030,18 +1033,16 @@
 
 			if (lsr & UART_LSR_BI) {
 				DEBUG_INTR("handling break....");
-				*tty->flip.flag_buf_ptr = TTY_BREAK;
+				flag = TTY_BREAK;
 			} else if (lsr & UART_LSR_PE)
-				*tty->flip.flag_buf_ptr = TTY_PARITY;
+				flag = TTY_PARITY;
 			else if (lsr & UART_LSR_FE)
-				*tty->flip.flag_buf_ptr = TTY_FRAME;
+				flag = TTY_FRAME;
 		}
 		if (uart_handle_sysrq_char(&up->port, ch, regs))
 			goto ignore_char;
 		if ((lsr & up->port.ignore_status_mask) == 0) {
-			tty->flip.flag_buf_ptr++;
-			tty->flip.char_buf_ptr++;
-			tty->flip.count++;
+			tty_insert_flip_char(tty, ch, flag);
 		}
 		if ((lsr & UART_LSR_OE) &&
 		    tty->flip.count < TTY_FLIPBUF_SIZE) {
@@ -1050,10 +1051,7 @@
 			 * immediately, and doesn't affect the current
 			 * character.
 			 */
-			*tty->flip.flag_buf_ptr = TTY_OVERRUN;
-			tty->flip.flag_buf_ptr++;
-			tty->flip.char_buf_ptr++;
-			tty->flip.count++;
+			tty_insert_flip_char(tty, 0, TTY_OVERRUN);
 		}
 	ignore_char:
 		lsr = serial_inp(up, UART_LSR);

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Russell King]

On Tue, Nov 09, 2004 at 02:47:51PM +0000, Alan Cox wrote:
> I'm working on it. It would be helpful if the drivers/serial code would
> use helpers and not dig in places it shouldnt so that the transition can
> be cleaner.

Ok - I've just applied your patch to 8250 and expanded it to cover the
other ARM serial drivers.  Should be merged with Linus' tree tomorrow.

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

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Alan Cox]

On Sad, 2004-11-13 at 18:10, Russell King wrote:
> On Tue, Nov 09, 2004 at 02:47:51PM +0000, Alan Cox wrote:
> > I'm working on it. It would be helpful if the drivers/serial code would
> > use helpers and not dig in places it shouldnt so that the transition can
> > be cleaner.
> 
> Ok - I've just applied your patch to 8250 and expanded it to cover the
> other ARM serial drivers.  Should be merged with Linus' tree tomorrow.

Thanks. I'll add a couple more helpers soon for the drivers/char code
that
copies or DMAs blocks of characters

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[Nicolas Pitre]

On Sat, 13 Nov 2004, Alan Cox wrote:

> On Sad, 2004-11-13 at 18:10, Russell King wrote:
> > On Tue, Nov 09, 2004 at 02:47:51PM +0000, Alan Cox wrote:
> > > I'm working on it. It would be helpful if the drivers/serial code would
> > > use helpers and not dig in places it shouldnt so that the transition can
> > > be cleaner.
> > 
> > Ok - I've just applied your patch to 8250 and expanded it to cover the
> > other ARM serial drivers.  Should be merged with Linus' tree tomorrow.
> 
> Thanks. I'll add a couple more helpers soon for the drivers/char code
> that
> copies or DMAs blocks of characters

Great!  I have serial hardware here that can do DMA but found the 
current tty layer not especially appropriate for DMA usage.


Nicolas

Re: [PATCH] Correctly flush 8250 buffers, notify ldisc of line status changes.

[David Woodhouse]

On Tue, 2004-11-09 at 11:15 +0000, Alan Cox wrote:
> This is the wrong way to do it. I've been trying this and discarded it.
> The problem is that data arrival is asynchronous to the event which
> means you've not got a clue how to combine the status change and the
> data stream. This in itself makes the whole feature useless.
> 
> Modem changes have to go inline with the data just like break and
> parity.

That's not the problem I'm trying to solve. I'm interested in the
transmission path. I have a shared bus, much like CAN, with automatic
contention detection -- if I transmit a 0 while someone else transmits a
1, the adapter automatically aborts its transmission and asserts CTS for
a while. I need to respond to CTS by immediately stopping my own
transmission -- otherwise when the adapter goes back into the normal
state it'll start transmitting again half way through my original
packet.

-- 
dwmw2