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

PATCH In review David Woodhouse 5 participants 15 messages Nov 5, 2004 – Nov 13, 2004

Subsystems (3)

PowerPC TTY layer and serial drivers Char and misc drivers

lore

Share & tools

lore

David Woodhouse

author

2004-11-05 13:16 UTC #0 lore raw

PATCH 1/1 Correctly flush 8250 buffers, notify ldisc of line status changes.

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.

Signature · 1 line

dwmw2

David Woodhouse

author

2004-11-05 13:18 UTC #1 lore raw

Quoted 11 lines · 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.

Doh.

===== drivers/char/tty_io.c 1.152 vs edited =====

drivers/char/tty_io.c +20 -0

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

drivers/serial/8250.c +20 -1

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

drivers/serial/amba-pl010.c +1 -1

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

drivers/serial/amba-pl011.c +1 -1

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

drivers/serial/au1x00_uart.c +1 -1

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

drivers/serial/icom.c +2 -2

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

drivers/serial/ip22zilog.c +1 -1

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

drivers/serial/pmac_zilog.c +1 -1

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

drivers/serial/pxa.c +1 -1

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

drivers/serial/sa1100.c +1 -1

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

drivers/serial/serial_core.c +2 -0

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

drivers/serial/serial_lh7a40x.c +1 -1

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

drivers/serial/sunsab.c +1 -1

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

drivers/serial/sunsu.c +1 -1

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

drivers/serial/sunzilog.c +1 -1

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

drivers/serial/uart00.c +1 -1

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

include/linux/serial_core.h +11 -0

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

include/linux/tty.h +1 -0

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

include/linux/tty_ldisc.h +6 -0

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

Signature · 1 line

dwmw2

Andrew Morton

2004-11-09 09:23 UTC #2 lore raw

In reply to David Woodhouse

Quoted 4 lines · 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().

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?

David Woodhouse

author

2004-11-09 11:26 UTC #3 lore raw

Quoted 10 lines · Andrew Morton

Quoted 4 lines · 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().
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 =====

drivers/char/tty_io.c +20 -0

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

drivers/serial/8250.c +16 -1

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

drivers/serial/amba-pl010.c +1 -1

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

drivers/serial/amba-pl011.c +1 -1

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

drivers/serial/au1x00_uart.c +1 -1

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

drivers/serial/icom.c +2 -2

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

drivers/serial/ip22zilog.c +1 -1

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

drivers/serial/pmac_zilog.c +1 -1

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

drivers/serial/pxa.c +1 -1

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

drivers/serial/sa1100.c +1 -1

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

drivers/serial/serial_core.c +2 -0

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

drivers/serial/serial_lh7a40x.c +1 -1

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

drivers/serial/sunsab.c +1 -1

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

drivers/serial/sunsu.c +1 -1

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

drivers/serial/sunzilog.c +1 -1

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

drivers/serial/uart00.c +1 -1

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

include/linux/serial_core.h +11 -0

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

include/linux/tty.h +1 -0

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

include/linux/tty_ldisc.h +6 -0

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

Signature · 1 line

dwmw2

Alan Cox

2004-11-09 12:19 UTC #4 lore raw

Quoted 7 lines · David Woodhouse

+ * tty_status_change - notify of line status changes
+ * @tty: terminal
+ *
+ * Helper for informing the line discipline that the modem
+ * status lines may have changed.
+ */
drivers/char/tty_io.c +1 -0
+

drivers/char/tty_io.c line 739

Quoted 10 lines

drivers/char/tty_io.c +10 -0

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

Quoted 6 lines

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

Russell King

2004-11-09 13:28 UTC #5 lore raw

Quoted 15 lines · Alan Cox

Quoted 14 lines · David Woodhouse
+ * tty_status_change - notify of line status changes
+ * @tty: terminal
+ *
+ * Helper for informing the line discipline that the modem
+ * status lines may have changed.
+ */
drivers/char/tty_io.c +8 -0
+
+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);

Quoted 8 lines

drivers/char/tty_io.c line 739
Quoted 3 lines
drivers/char/tty_io.c +3 -0
+	}
+}
+
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.

Signature · 4 lines

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

Alan Cox

2004-11-09 14:21 UTC #6 lore raw

Quoted 5 lines

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

David Woodhouse

author

2004-11-09 14:39 UTC #7 lore raw

Quoted 3 lines · Alan Cox

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.

Quoted 5 lines

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

Quoted 1 line

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?

Signature · 1 line

dwmw2

Alan Cox

2004-11-09 14:52 UTC #8 lore raw

Quoted 6 lines · David Woodhouse

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.

Quoted 6 lines

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

Quoted 5 lines

Quoted 1 line
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.

Russell King

2004-11-09 14:47 UTC #9 lore raw

In reply to Alan Cox

Quoted 2 lines · Alan Cox

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.

Signature · 4 lines

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

Alan Cox

2004-11-09 15:53 UTC #10 lore raw

Quoted 5 lines · Russell King

Quoted 2 lines · Alan Cox
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.

Quoted 6 lines

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.

serial/8250.c renamed from linux.vanilla-2.6.10rc1/drivers/serial/8250.c +14 ~~-16~~

@@ -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);

Russell King

2004-11-13 18:10 UTC #11 lore raw

Quoted 3 lines · Alan Cox

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.

Signature · 4 lines

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

Alan Cox

2004-11-13 21:55 UTC #12 lore raw

Quoted 7 lines · Russell King

Quoted 3 lines · Alan Cox
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

Nicolas Pitre

2004-11-13 22:41 UTC #13 lore raw

Quoted 6 lines · Alan Cox

Quoted 5 lines · Russell King
Quoted 3 lines · Alan Cox
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.

Quoted 6 lines

Quoted 2 lines
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

David Woodhouse

author

2004-11-09 14:39 UTC #14 lore raw

In reply to Alan Cox

Quoted 7 lines · Alan Cox

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.

Signature · 1 line

dwmw2