Hi Greg,
These patches are the accumulation of fixes resulting from audits
of the tty and serial cores; mostly cleanups of open() and close().
Most of the changes remove condition testing and handling which
cannot occur because the tty lock is held for the tty driver open()
call.
The exceptions are the last 3 patches which address API design and
locking issues for uart drivers.
At least 4 known bugs remain:
1. Several drivers mishandle the start_tx() method; I hope to finish
an audit of these soon for fixing in 3.16.
2. uart_open() mismanages the port count if an error occurs,
because uart_close() is always called even if open() returns
an error
3. Most of the port->flags usage is SMP-unsafe; atomics are mixed
with spinlock-protected updates.
4. uart_remove_one_port() is unsafe, as there is no provision for
preventing in-use port removal.
Regards,
Peter Hurley (22):
tty: Document locking for tty driver methods
tty: Document locking for tty_port_close{,start,end}()
tty: Document locking for tty_port_open()
tty: Document locking for tty_port_block_til_ready()
tty: Document locking for tty_port_hangup()
tty: Move tty->closing from port lock critical section
tty: ipwireless: Remove tty->closing abort from ipw_open()
serial: Use UPF_* constants with struct uart_port flags
tty: Remove tty_hung_up_p() tests from tty drivers' open()
char: synclink: Remove WARN_ON for bad port count
tty: Call hangup method in modern style
tty: serial: Fix termios/port flags mismatch
serial: blackfin: Fix CTS flow control
tty: Remove tty_wait_until_sent_from_close()
isdn: tty: Use private flag for ASYNC_CLOSING
tty: mxser: Use tty->closing for ASYNC_CLOSING
tty: Remove ASYNC_CLOSING
tty: Move tty hung up check from port->lock critical section
serial: Style fix
serial: Refactor uart_flush_buffer() from uart_close()
serial: core: Remove superfluous ldisc flush from uart_close()
serial: Fix locking for uart driver set_termios() method
Documentation/serial/driver | 6 ++--
drivers/char/pcmcia/synclink_cs.c | 11 -------
drivers/isdn/i4l/isdn_tty.c | 16 +++++-----
drivers/s390/char/con3215.c | 3 +-
drivers/staging/dgrp/dgrp_tty.c | 10 -------
drivers/tty/cyclades.c | 9 ------
drivers/tty/hvc/hvc_console.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
drivers/tty/ipwireless/tty.c | 5 ----
drivers/tty/mxser.c | 6 ++--
drivers/tty/rocket.c | 14 +--------
drivers/tty/serial/68328serial.c | 2 --
drivers/tty/serial/amba-pl011.c | 1 +
drivers/tty/serial/atmel_serial.c | 2 ++
drivers/tty/serial/bfin_sport_uart.c | 11 +++++--
drivers/tty/serial/bfin_uart.c | 13 ++++----
drivers/tty/serial/crisv10.c | 47 ++++-------------------------
drivers/tty/serial/lantiq.c | 2 +-
drivers/tty/serial/mcf.c | 4 +--
drivers/tty/serial/serial-tegra.c | 2 ++
drivers/tty/serial/serial_core.c | 31 ++++++++-----------
drivers/tty/serial/timbuart.c | 2 ++
drivers/tty/synclink.c | 26 ++++------------
drivers/tty/synclink_gt.c | 22 +++-----------
drivers/tty/synclinkmp.c | 23 +++-----------
drivers/tty/tty_io.c | 2 +-
drivers/tty/tty_port.c | 58 ++++++++++++++++++++----------------
include/linux/isdn.h | 1 +
include/linux/tty.h | 18 -----------
include/linux/tty_driver.h | 8 +++--
include/uapi/linux/tty_flags.h | 2 --
net/irda/ircomm/ircomm_tty.c | 39 ++----------------------
32 files changed, 117 insertions(+), 283 deletions(-)
--
2.0.0
The tty core calls the tty driver's open, close and hangup
methods holding the tty lock.
Signed-off-by: Peter Hurley <[email protected]>
---
include/linux/tty_driver.h | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h
index 756a609..e48c608 100644
--- a/include/linux/tty_driver.h
+++ b/include/linux/tty_driver.h
@@ -35,14 +35,14 @@
* This routine is mandatory; if this routine is not filled in,
* the attempted open will fail with ENODEV.
*
- * Required method.
- *
+ * Required method. Called with tty lock held.
+ *
* void (*close)(struct tty_struct * tty, struct file * filp);
*
* This routine is called when a particular tty device is closed.
* Note: called even if the corresponding open() failed.
*
- * Required method.
+ * Required method. Called with tty lock held.
*
* void (*shutdown)(struct tty_struct * tty);
*
@@ -172,6 +172,8 @@
*
* Optional:
*
+ * Called with tty lock held.
+ *
* int (*break_ctl)(struct tty_struct *tty, int state);
*
* This optional routine requests the tty driver to turn on or
--
2.0.0
tty->closing cannot be set on ipw_open() because the ipwireless tty
driver does not call any functions that set tty->closing.
CC: Jiri Kosina <[email protected]>
CC: David Sterba <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/ipwireless/tty.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/tty/ipwireless/tty.c b/drivers/tty/ipwireless/tty.c
index 17ee3bf..345cebb 100644
--- a/drivers/tty/ipwireless/tty.c
+++ b/drivers/tty/ipwireless/tty.c
@@ -93,11 +93,6 @@ static int ipw_open(struct tty_struct *linux_tty, struct file *filp)
return -ENODEV;
mutex_lock(&tty->ipw_tty_mutex);
-
- if (tty->closing) {
- mutex_unlock(&tty->ipw_tty_mutex);
- return -ENODEV;
- }
if (tty->port.count == 0)
tty->tx_bytes_queued = 0;
--
2.0.0
Since at least before 2.6.30, it has not been possible to observe
a hung up file pointer in a tty driver's open() method unless/until
the driver open() releases the tty_lock() (eg., before blocking).
This is because tty_open() adds the file pointer while holding
the tty_lock() _and_ doesn't release the lock until after calling
the tty driver's open() method. [ Before tty_lock(), this was
lock_kernel(). ]
Since __tty_hangup() first waits on the tty_lock() before
enumerating and hanging up the open file pointers, either
__tty_hangup() will wait for the tty_lock() or tty_open() will
not yet have added the file pointer. For example,
CPU 0 | CPU 1
|
tty_open | __tty_hangup
.. | ..
tty_lock | ..
tty_reopen | tty_lock / blocks
.. |
tty_add_file(tty, filp) |
.. |
tty->ops->open(tty, filp) |
tty_port_open |
tty_port_block_til_ready |
.. |
while (1) |
.. |
tty_unlock | / unblocks
schedule | for each filp on tty->tty_files
| f_ops = tty_hung_up_fops;
| ..
| tty_unlock
tty_lock |
.. |
tty_unlock |
Note that since tty_port_block_til_ready() and similar drop
the tty_lock while blocking, when woken, the file pointer
must then be tested for having been hung up.
Also, fix bit-rotted drivers that used extra_count to track the
port->count bump.
CC: Mikael Starvik <[email protected]>
CC: Jesper Nilsson <[email protected]>
CC: Samuel Ortiz <[email protected]>
CC: "David S. Miller" <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/char/pcmcia/synclink_cs.c | 2 +-
drivers/staging/dgrp/dgrp_tty.c | 10 ----------
drivers/tty/cyclades.c | 2 +-
drivers/tty/serial/crisv10.c | 15 +++++----------
drivers/tty/serial/serial_core.c | 8 --------
drivers/tty/synclink.c | 10 +++-------
drivers/tty/synclink_gt.c | 10 +++-------
drivers/tty/synclinkmp.c | 11 +++--------
drivers/tty/tty_port.c | 8 +++-----
net/irda/ircomm/ircomm_tty.c | 6 ++----
10 files changed, 21 insertions(+), 61 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 8320abd..a63970f 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -2510,7 +2510,7 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
__FILE__, __LINE__, tty->driver->name, port->count);
/* If port is closing, signal caller to try again */
- if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING){
+ if (port->flags & ASYNC_CLOSING){
wait_event_interruptible_tty(tty, port->close_wait,
!(port->flags & ASYNC_CLOSING));
retval = ((port->flags & ASYNC_HUP_NOTIFY) ?
diff --git a/drivers/staging/dgrp/dgrp_tty.c b/drivers/staging/dgrp/dgrp_tty.c
index 30d2602..cdd1a69 100644
--- a/drivers/staging/dgrp/dgrp_tty.c
+++ b/drivers/staging/dgrp/dgrp_tty.c
@@ -632,16 +632,6 @@ static int dgrp_tty_open(struct tty_struct *tty, struct file *file)
tty->driver_data = un;
/*
- * If we are in the middle of hanging up,
- * then return an error
- */
- if (tty_hung_up_p(file)) {
- retval = ((un->un_flag & UN_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
- goto done;
- }
-
- /*
* If the port is in the middle of closing, then block
* until it is done, then try again.
*/
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index a57bb5a..fd66f57 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -1579,7 +1579,7 @@ static int cy_open(struct tty_struct *tty, struct file *filp)
/*
* If the port is the middle of closing, bail out now
*/
- if (tty_hung_up_p(filp) || (info->port.flags & ASYNC_CLOSING)) {
+ if (info->port.flags & ASYNC_CLOSING) {
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
return (info->port.flags & ASYNC_HUP_NOTIFY) ? -EAGAIN: -ERESTARTSYS;
diff --git a/drivers/tty/serial/crisv10.c b/drivers/tty/serial/crisv10.c
index d567ac5..58e6f61 100644
--- a/drivers/tty/serial/crisv10.c
+++ b/drivers/tty/serial/crisv10.c
@@ -3831,14 +3831,13 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
DECLARE_WAITQUEUE(wait, current);
unsigned long flags;
int retval;
- int do_clocal = 0, extra_count = 0;
+ int do_clocal = 0;
/*
* If the device is in the middle of being closed, then block
* until it's done, and then try again.
*/
- if (tty_hung_up_p(filp) ||
- (info->port.flags & ASYNC_CLOSING)) {
+ if (info->port.flags & ASYNC_CLOSING) {
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
#ifdef SERIAL_DO_RESTART
@@ -3879,10 +3878,7 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
info->line, info->port.count);
#endif
local_irq_save(flags);
- if (!tty_hung_up_p(filp)) {
- extra_count++;
- info->port.count--;
- }
+ info->port.count--;
local_irq_restore(flags);
info->port.blocked_open++;
while (1) {
@@ -3921,7 +3917,7 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
}
set_current_state(TASK_RUNNING);
remove_wait_queue(&info->port.open_wait, &wait);
- if (extra_count)
+ if (!tty_hung_up_p(filp))
info->port.count++;
info->port.blocked_open--;
#ifdef SERIAL_DEBUG_OPEN
@@ -3976,8 +3972,7 @@ rs_open(struct tty_struct *tty, struct file * filp)
/*
* If the port is in the middle of closing, bail out now
*/
- if (tty_hung_up_p(filp) ||
- (info->port.flags & ASYNC_CLOSING)) {
+ if (info->port.flags & ASYNC_CLOSING) {
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
#ifdef SERIAL_DO_RESTART
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f2febb4..c8879ce 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1572,14 +1572,6 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
tty_port_tty_set(port, tty);
/*
- * If the port is in the middle of closing, bail out now.
- */
- if (tty_hung_up_p(filp)) {
- retval = -EAGAIN;
- goto err_dec_count;
- }
-
- /*
* Start up the serial port.
*/
retval = uart_startup(tty, state, 0);
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index d48e040..b799170 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -3267,7 +3267,6 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp,
DECLARE_WAITQUEUE(wait, current);
int retval;
bool do_clocal = false;
- bool extra_count = false;
unsigned long flags;
int dcd;
struct tty_port *port = &info->port;
@@ -3300,10 +3299,7 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp,
__FILE__,__LINE__, tty->driver->name, port->count );
spin_lock_irqsave(&info->irq_spinlock, flags);
- if (!tty_hung_up_p(filp)) {
- extra_count = true;
- port->count--;
- }
+ port->count--;
spin_unlock_irqrestore(&info->irq_spinlock, flags);
port->blocked_open++;
@@ -3342,7 +3338,7 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp,
remove_wait_queue(&port->open_wait, &wait);
/* FIXME: Racy on hangup during close wait */
- if (extra_count)
+ if (!tty_hung_up_p(filp))
port->count++;
port->blocked_open--;
@@ -3403,7 +3399,7 @@ static int mgsl_open(struct tty_struct *tty, struct file * filp)
__FILE__,__LINE__,tty->driver->name, info->port.count);
/* If port is closing, signal caller to try again */
- if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
+ if (info->port.flags & ASYNC_CLOSING){
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index c359a91..ba1dbcd 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -673,7 +673,7 @@ static int open(struct tty_struct *tty, struct file *filp)
DBGINFO(("%s open, old ref count = %d\n", info->device_name, info->port.count));
/* If port is closing, signal caller to try again */
- if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
+ if (info->port.flags & ASYNC_CLOSING){
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
@@ -3273,7 +3273,6 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
DECLARE_WAITQUEUE(wait, current);
int retval;
bool do_clocal = false;
- bool extra_count = false;
unsigned long flags;
int cd;
struct tty_port *port = &info->port;
@@ -3300,10 +3299,7 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
add_wait_queue(&port->open_wait, &wait);
spin_lock_irqsave(&info->lock, flags);
- if (!tty_hung_up_p(filp)) {
- extra_count = true;
- port->count--;
- }
+ port->count--;
spin_unlock_irqrestore(&info->lock, flags);
port->blocked_open++;
@@ -3338,7 +3334,7 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
set_current_state(TASK_RUNNING);
remove_wait_queue(&port->open_wait, &wait);
- if (extra_count)
+ if (!tty_hung_up_p(filp))
port->count++;
port->blocked_open--;
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index 53ba853..c3f9091 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -753,7 +753,7 @@ static int open(struct tty_struct *tty, struct file *filp)
__FILE__,__LINE__,tty->driver->name, info->port.count);
/* If port is closing, signal caller to try again */
- if (tty_hung_up_p(filp) || info->port.flags & ASYNC_CLOSING){
+ if (info->port.flags & ASYNC_CLOSING){
wait_event_interruptible_tty(tty, info->port.close_wait,
!(info->port.flags & ASYNC_CLOSING));
retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
@@ -3288,7 +3288,6 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
DECLARE_WAITQUEUE(wait, current);
int retval;
bool do_clocal = false;
- bool extra_count = false;
unsigned long flags;
int cd;
struct tty_port *port = &info->port;
@@ -3322,10 +3321,7 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
__FILE__,__LINE__, tty->driver->name, port->count );
spin_lock_irqsave(&info->lock, flags);
- if (!tty_hung_up_p(filp)) {
- extra_count = true;
- port->count--;
- }
+ port->count--;
spin_unlock_irqrestore(&info->lock, flags);
port->blocked_open++;
@@ -3362,8 +3358,7 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
set_current_state(TASK_RUNNING);
remove_wait_queue(&port->open_wait, &wait);
-
- if (extra_count)
+ if (!tty_hung_up_p(filp))
port->count++;
port->blocked_open--;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 9209d63..1b93357 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -365,7 +365,7 @@ int tty_port_block_til_ready(struct tty_port *port,
DEFINE_WAIT(wait);
/* block if port is in the process of being closed */
- if (tty_hung_up_p(filp) || port->flags & ASYNC_CLOSING) {
+ if (port->flags & ASYNC_CLOSING) {
wait_event_interruptible_tty(tty, port->close_wait,
!(port->flags & ASYNC_CLOSING));
if (port->flags & ASYNC_HUP_NOTIFY)
@@ -399,8 +399,7 @@ int tty_port_block_til_ready(struct tty_port *port,
/* The port lock protects the port counts */
spin_lock_irqsave(&port->lock, flags);
- if (!tty_hung_up_p(filp))
- port->count--;
+ port->count--;
port->blocked_open++;
spin_unlock_irqrestore(&port->lock, flags);
@@ -593,8 +592,7 @@ int tty_port_open(struct tty_port *port, struct tty_struct *tty,
struct file *filp)
{
spin_lock_irq(&port->lock);
- if (!tty_hung_up_p(filp))
- ++port->count;
+ ++port->count;
spin_unlock_irq(&port->lock);
tty_port_tty_set(port, tty);
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 2ba8b97..61ceb4c 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -320,8 +320,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
__FILE__, __LINE__, tty->driver->name, port->count);
spin_lock_irqsave(&port->lock, flags);
- if (!tty_hung_up_p(filp))
- port->count--;
+ port->count--;
port->blocked_open++;
spin_unlock_irqrestore(&port->lock, flags);
@@ -458,8 +457,7 @@ static int ircomm_tty_open(struct tty_struct *tty, struct file *filp)
/*
* If the port is the middle of closing, bail out now
*/
- if (tty_hung_up_p(filp) ||
- test_bit(ASYNCB_CLOSING, &self->port.flags)) {
+ if (test_bit(ASYNCB_CLOSING, &self->port.flags)) {
/* Hm, why are we blocking on ASYNC_CLOSING if we
* do return -EAGAIN/-ERESTARTSYS below anyway?
--
2.0.0
Fix ASYNC_* constant usage that should be the corresponding UPF_*
constant.
CC: Grant Likely <[email protected]>
CC: Rob Herring <[email protected]>
CC: [email protected]
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/lantiq.c | 2 +-
drivers/tty/serial/mcf.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 88d01e0..6e29b01 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -709,7 +709,7 @@ lqasc_probe(struct platform_device *pdev)
port = <q_port->port;
port->iotype = SERIAL_IO_MEM;
- port->flags = ASYNC_BOOT_AUTOCONF | UPF_IOREMAP;
+ port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP;
port->ops = &lqasc_pops;
port->fifosize = 16;
port->type = PORT_LTQ_ASC,
diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
index 0edfaf8..1e59faf 100644
--- a/drivers/tty/serial/mcf.c
+++ b/drivers/tty/serial/mcf.c
@@ -538,7 +538,7 @@ int __init early_mcf_setup(struct mcf_platform_uart *platp)
port->iotype = SERIAL_IO_MEM;
port->irq = platp[i].irq;
port->uartclk = MCF_BUSCLK;
- port->flags = ASYNC_BOOT_AUTOCONF;
+ port->flags = UPF_BOOT_AUTOCONF;
port->ops = &mcf_uart_ops;
}
@@ -663,7 +663,7 @@ static int mcf_probe(struct platform_device *pdev)
port->irq = platp[i].irq;
port->uartclk = MCF_BUSCLK;
port->ops = &mcf_uart_ops;
- port->flags = ASYNC_BOOT_AUTOCONF;
+ port->flags = UPF_BOOT_AUTOCONF;
uart_add_one_port(&mcf_driver, port);
}
--
2.0.0
The tty lock is held when the tty driver's hangup() method is called
(from the lone call-site, __tty_hangup()). The call-tree audit [1]
of tty_port_hangup() is a closed graph of the callers of
tty_port_hangup(); ie., all callers originate only from __tty_hangup().
Of these callers, none drop the tty lock prior to calling
tty_port_hangup().
[1]
Call-tree audit of tty_port_hangup()
__tty_hangup()
tty->ops->hangup() --+
|
rs_hangup():arch/ia64/hp/sim/simserial.c
line_hangup():arch/um/drivers/line.c
gdm_tty_hangup():drivers/staging/gdm724x/gdm_tty.c
fwtty_hangup():drivers/staging/fwserial/fwserial.c
acm_tty_hangup():drivers/usb/class/cdc-acm.c
serial_hangup():drivers/usb/serial/usb-serial.c
ipoctal_hangup():drivers/ipack/devices/ipoctal.c
cy_hangup():drivers/tty/cyclades.c
isicom_hangup():drivers/tty/isicom.c
rp_hangup():drivers/tty/rocket.c
dashtty_hangup():drivers/tty/metag_da.c
moxa_hangup():drivers/tty/moxa.c
gsmtty_hangup():drivers/tty/n_gsm.c
goldfish_tty_hangup():drivers/tty/goldfish.c
ehv_bc_tty_hangup():drivers/tty/ehv_bytechan.c
mxser_hangup():drivers/tty/mxser.c
kgdb_nmi_tty_hangup():drivers/tty/serial/kgdb_nmi.c
ifx_spi_hangup():drivers/tty/serial/ifx6x60.c
ntty_hangup():drivers/tty/nozomi.c
capinc_tty_hangup():drivers/isdn/capi/capi.c
mgslpc_hangup():drivers/char/pcmcia/synclink_cs.c
sdio_uart_hangup():drivers/mmc/card/sdio_uart.c
rfcomm_tty_hangup():net/bluetooth/rfcomm/tty.c
|
+- tty_port_hangup()
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_port.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 69e072b..7309594 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -227,6 +227,8 @@ out:
*
* Perform port level tty hangup flag and count changes. Drop the tty
* reference.
+ *
+ * Caller holds tty lock.
*/
void tty_port_hangup(struct tty_port *port)
--
2.0.0
tty->closing informs the line discipline that the hardware will
be shutting down imminently, and to disable further input other
than soft flow control (but to still allow additional output).
However, the tty lock is the necessary lock for preventing
concurrent changes to tty->closing. As shown by the call-tree
audit [1] of functions that modify tty->closing, the tty lock
is already held for those functions.
[1]
Call-tree audit of functions that modify tty->closing
* does not include call tree to tty_port_close(), tty_port_close_start(),
or tty_port_close_end() which is already documented in
'tty: Document locking for tty_port_close{,start,end}' that shows
callers to those 3 functions hold the tty lock
tty_release()
tty->ops->close() --+
|
__tty_hangup() |
tty->ops->close() --+
|
mp_close():drivers/staging/sb105x/sb_pci_mp.c
dngc_tty_close():drivers/staging/dgnc/dgnc_tty.c
dgap_tty_close():drivers/staging/dgap/dgap_tty.c
dgrp_tty_close():drivers/staging/dgrp/dgrp_tty.c
rp_close():drivers/tty/rocket.c
hvsi_close():drivers/tty/hvc/hvsi.c
rs_close():drivers/tty/serial/68328serial.c
rs_close():drivers/tty/serial/crisv10.c
uart_close():drivers/tty/serial/serial_core.c
isdn_tty_close():drivers/isdn/i4l/isdn_tty.c
tty3215_close():drivers/s390/char/con3215.c
tty_open()
tty_ldisc_setup() ----+
|
__tty_hangup() |
tty_ldisc_hangup() ---+
|
tty_set_ldisc() --------+
tty_ldisc_restore() --+
|
+- tty_ldisc_open()
ld->ops->open() --+
|
+- n_tty_open()
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/serial_core.c | 2 +-
drivers/tty/tty_port.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index b68550d..f2febb4 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1356,8 +1356,8 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
tty_ldisc_flush(tty);
tty_port_tty_set(port, NULL);
- spin_lock_irqsave(&port->lock, flags);
tty->closing = 0;
+ spin_lock_irqsave(&port->lock, flags);
if (port->blocked_open) {
spin_unlock_irqrestore(&port->lock, flags);
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 7309594..9209d63 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -497,9 +497,10 @@ int tty_port_close_start(struct tty_port *port,
return 0;
}
set_bit(ASYNCB_CLOSING, &port->flags);
- tty->closing = 1;
spin_unlock_irqrestore(&port->lock, flags);
+ tty->closing = 1;
+
if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
/* Don't block on a stalled port, just pull the chain */
if (tty->flow_stopped)
@@ -522,9 +523,10 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
{
unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
tty->closing = 0;
+ spin_lock_irqsave(&port->lock, flags);
+
if (port->blocked_open) {
spin_unlock_irqrestore(&port->lock, flags);
if (port->close_delay) {
--
2.0.0
The tty lock is held when the tty driver's open() method is called
(from tty_open()). The call-tree audit [1] of tty_port_block_til_ready()
is a closed graph of the callers of tty_port_block_til_ready();
ie., all callers originate only from tty_open().
Of these callers, none drop the tty lock.
Also, document tty_port_block_til_ready() may drop and reacquire
the tty lock when blocking, which means the tty or tty_port may have
changed state.
[1]
Call-tree audit of tty_port_block_til_ready()
* does not include call tree of tty_port_open() which is already
documented in 'tty: Document locking from tty_port_open()'
tty_open()
tty->ops->open() --+
|
cy_open():drivers/tty/cyclades.c
rp_open():drivers/tty/rocket.c
rs_open():drivers/tty/amiserial.c
moxa_open():drivers/tty/moxa.c
gsmtty_open():drivers/tty/n_gsm.c
rs_open():drivers/tty/serial/68328serial.c
uart_open():drivers/tty/serial/serial_core.c
isdn_tty_open():drivers/isdn/i4l/isdn_tty.c
mgslpc_open():drivers/char/pcmcia/synclink_cs.c
|
+- tty_port_block_til_ready()
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_port.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index f469869..69e072b 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -348,6 +348,11 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
* do carrier detect and the dtr_rts method if it supports software
* management of these lines. Note that the dtr/rts raise is done each
* iteration as a hangup may have previously dropped them while we wait.
+ *
+ * Caller holds tty lock.
+ *
+ * NB: May drop and reacquire tty lock when blocking, so tty and tty_port
+ * may have changed state (eg., may have been hung up).
*/
int tty_port_block_til_ready(struct tty_port *port,
--
2.0.0
The tty lock is held when the tty driver's open method is called
(from the lone call-site, tty_open()). The call-tree audit [1] of
tty_port_open() is a closed graph of the callers of tty_port_open();
ie., all callers originate from only tty_open().
Of these callers, none drop the tty lock.
Also, document that tty_port_block_til_ready() may drop and reacquire
the tty lock when blocking, which means the tty or tty_port may have
changed state.
[1]
Call-tree audit of tty_port_open()
tty_open()
tty->ops->open() --+
|
rs_open():arch/ia64/hp/sim/simserial.c
*line_open():arch/um/drivers/line.c
gdm_tty_open():drivers/staging/gdm724x/gdm_tty.c
fwtty_open():drivers/staging/fwserial/fwserial.c
acm_tty_open():drivers/usb/class/cdc-acm.c
serial_open():drivers/usb/serial/usb-serial.c
pti_tty_driver_open():drivers/misc/pti.c
ipoctal_open():drivers/ipack/devices/ipoctal.c
isicom_open():drivers/tty/isicom.c
dashtty_open():drivers/tty/metag_da.c
goldfish_tty_open():drivers/tty/goldfish.c
ehv_bc_tty_open():drivers/tty/ehv_bytechan.c
mxser_open():drivers/tty/mxser.c
kgdb_nmi_tty_open():drivers/tty/serial/kgdb_nmi.c
ifx_spi_open():drivers/tty/serial/ifx6x60.c
smd_tty_open():drivers/tty/serial/msm_smd_tty.c
ntty_open():drivers/tty/nozomi.c
capinc_tty_open():drivers/isdn/capi/capi.c
tpk_open():drivers/char/ttyprintk.c
sdio_uart_open():drivers/mmc/card/sdio_uart.c
rfcomm_tty_open():net/bluetooth/rfcomm/tty.c
|
+- tty_port_open()
* line_open() is the .open method for 2 um drivers
declared in ./arch/um/drivers/stdio_console.c and
in ./arch/um/drivers/ssl.c, and not called directly
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_port.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index bcc15f7..f469869 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -572,6 +572,14 @@ int tty_port_install(struct tty_port *port, struct tty_driver *driver,
}
EXPORT_SYMBOL_GPL(tty_port_install);
+/**
+ * tty_port_open
+ *
+ * Caller holds tty lock.
+ *
+ * NB: may drop and reacquire tty lock (in tty_port_block_til_ready()) so
+ * tty and tty_port may have changed state (eg., may be hung up now)
+ */
int tty_port_open(struct tty_port *port, struct tty_struct *tty,
struct file *filp)
{
--
2.0.0
The tty lock is held when the tty driver's .close method is called
(from the two lone call-sites of tty_release() and __tty_hangup()).
The call-tree audit[1] of tty_port_close(), tty_port_close_start,
and tty_port_close_end() is a closed graph of the callers of these
3 functions; ie., all callers originate from only tty_release()
or __tty_hangup().
Of these callers, none drop the tty lock.
Also, document tty_port_close_start() may drop and reacquire the
tty lock in tty_wait_until_sent_from_close(), which means the tty
or tty_port may have changed state (but not reopened or hung up).
[1]
Call-tree audit of tty_port_close, tty_port_close_start, and tty_port_close_end()
tty_release()
tty->ops->close() --+
|
__tty_hangup() |
tty->ops->close() --+
|
+- rp_close():drivers/tty/rocket.c -------------------+
+- uart_close():drivers/tty/serial/serial_core.c -----+
| +- tty_port_close_start()
|
|
+- close():drivers/tty/synclinkmp.c ------------------+
+- rs_close():drivers/tty/amiserial.c ----------------+
+- gsmtty_close():drivers/tty/n_gsm.c ----------------+
+- mxser_close():drivers/tty/mxser.c -----------------+
+- close():drivers/tty/synclink_gt.c -----------------+
+- mgsl_close():drivers/tty/synclink.c ---------------+
+- isdn_tty_close():drivers/isdn/i4l/isdn_tty.c ------+
+- mgslpc_close():drivers/char/pcmcia/synclink_cs.c --+
+- ircomm_tty_close():net/irda/ircomm/ircomm_tty.c ---+
| |
rs_close():arch/ia64/hp/sim/simserial.c |
*line_close():arch/um/drivers/line.c |
gdm_tty_close():drivers/staging/gdm724x/gdm_tty.c
fwtty_close():drivers/staging/fwserial/fwserial.c
acm_tty_close():drivers/usb/class/cdc-acm.c
serial_close():drivers/usb/serial/usb-serial.c
pti_tty_driver_close():drivers/misc/pti.c
ipoctal_close():drivers/ipack/devices/ipoctal.c
cy_close():drivers/tty/cyclades.c
isicom_close():drivers/tty/isicom.c
dashtty_close():drivers/tty/metag_da.c
moxa_close():drivers/tty/moxa.c
goldfish_tty_close():drivers/tty/goldfish.c
ehv_bc_tty_close():drivers/tty/ehv_bytechan.c
kgdb_nmi_tty_close():drivers/tty/serial/kgdb_nmi.c
ifx_spi_close():drivers/tty/serial/ifx6x60.c
smd_tty_close():drivers/tty/serial/msm_smd_tty.c
ntty_close():drivers/tty/nozomi.c
capinc_tty_close():drivers/isdn/capi/capi.c
tpk_close():drivers/char/ttyprintk.c
sdio_uart_close():drivers/mmc/card/sdio_uart.c |
rfcomm_tty_close():net/bluetooth/rfcomm/tty.c |
| |
+- tty_port_close():drivers/tty/tty_port.c -----------+
|
+- tty_port_close_start()
+- tty_port_close_end()
* line_close() is the .close method for 2 um drivers,
declared in ./arch/um/drivers/stdio_console.c and
in ./arch/um/drivers/ssl.c, and not called directly
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_port.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 3f746c8..bcc15f7 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -458,6 +458,10 @@ static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
schedule_timeout_interruptible(timeout);
}
+/* Caller holds tty lock.
+ * NB: may drop and reacquire tty lock (in tty_wait_until_sent_from_close())
+ * so tty and tty port may have changed state (but not hung up or reopened).
+ */
int tty_port_close_start(struct tty_port *port,
struct tty_struct *tty, struct file *filp)
{
@@ -506,6 +510,7 @@ int tty_port_close_start(struct tty_port *port,
}
EXPORT_SYMBOL(tty_port_close_start);
+/* Caller holds tty lock */
void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
{
unsigned long flags;
@@ -528,6 +533,15 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
}
EXPORT_SYMBOL(tty_port_close_end);
+/**
+ * tty_port_close
+ *
+ * Caller holds tty lock
+ *
+ * NB: may drop and reacquire tty lock (in tty_port_close_start()->
+ * tty_wait_until_sent_from_close()) so tty and tty_port may have changed
+ * state (but not hung up or reopened).
+ */
void tty_port_close(struct tty_port *port, struct tty_struct *tty,
struct file *filp)
{
--
2.0.0
blackfin uart port drivers mistakenly set the struct uart_port
flags bit UPF_BUG_THRE (which only has meaning to the 8250 core)
while trying to set ASYNC_CTS_FLOW.
Uart port drivers can override termios settings based on actual
hardware support in their .set_termios method; the serial core
sets the appropriate port flags based on the overrides.
Overriding only the initial termios settings is accomplished
by only perform those overrides if the old termios parameter is
NULL.
CC: Sonic Zhang <[email protected]>
CC: [email protected]
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/bfin_sport_uart.c | 11 ++++++++---
drivers/tty/serial/bfin_uart.c | 13 ++++++++-----
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/bfin_sport_uart.c b/drivers/tty/serial/bfin_sport_uart.c
index 4f22970..5a1342e 100644
--- a/drivers/tty/serial/bfin_sport_uart.c
+++ b/drivers/tty/serial/bfin_sport_uart.c
@@ -500,6 +500,13 @@ static void sport_set_termios(struct uart_port *port,
pr_debug("%s enter, c_cflag:%08x\n", __func__, termios->c_cflag);
+#ifdef CONFIG_SERIAL_BFIN_SPORT_CTSRTS
+ if (old == NULL && up->cts_pin != -1)
+ termios->c_cflag |= CRTSCTS;
+ else if (up->cts_pin == -1)
+ termios->c_cflag &= ~CRTSCTS;
+#endif
+
switch (termios->c_cflag & CSIZE) {
case CS8:
up->csize = 8;
@@ -813,10 +820,8 @@ static int sport_uart_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (res == NULL)
sport->cts_pin = -1;
- else {
+ else
sport->cts_pin = res->start;
- sport->port.flags |= ASYNC_CTS_FLOW;
- }
res = platform_get_resource(pdev, IORESOURCE_IO, 1);
if (res == NULL)
diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index 869ceba..c641064 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -793,6 +793,13 @@ bfin_serial_set_termios(struct uart_port *port, struct ktermios *termios,
unsigned int ier, lcr = 0;
unsigned long timeout;
+#ifdef CONFIG_SERIAL_BFIN_CTSRTS
+ if (old == NULL && uart->cts_pin != -1)
+ termios->c_cflag |= CRTSCTS;
+ else if (uart->cts_pin == -1)
+ termios->c_cflag &= ~CRTSCTS;
+#endif
+
switch (termios->c_cflag & CSIZE) {
case CS8:
lcr = WLS(8);
@@ -1325,12 +1332,8 @@ static int bfin_serial_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_IO, 0);
if (res == NULL)
uart->cts_pin = -1;
- else {
+ else
uart->cts_pin = res->start;
-#ifdef CONFIG_SERIAL_BFIN_CTSRTS
- uart->port.flags |= ASYNC_CTS_FLOW;
-#endif
- }
res = platform_get_resource(pdev, IORESOURCE_IO, 1);
if (res == NULL)
--
2.0.0
tty_wait_until_sent_from_close() drops the tty lock while waiting
for the tty driver to finish sending previously accepted data (ie.,
data remaining in its write buffer and transmit fifo).
However, dropping the tty lock is a hold-over from when the tty
lock was system-wide; ie., one lock for all ttys.
Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
'tty: localise the lock', dropping the tty lock has not been necessary.
CC: Karsten Keil <[email protected]>
CC: [email protected]
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/isdn/i4l/isdn_tty.c | 2 +-
drivers/tty/hvc/hvc_console.c | 2 +-
drivers/tty/hvc/hvcs.c | 2 +-
drivers/tty/tty_port.c | 11 ++---------
include/linux/tty.h | 18 ------------------
5 files changed, 5 insertions(+), 30 deletions(-)
diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 3c5f249..732f68a 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1587,7 +1587,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
* line status register.
*/
if (port->flags & ASYNC_INITIALIZED) {
- tty_wait_until_sent_from_close(tty, 3000); /* 30 seconds timeout */
+ tty_wait_until_sent(tty, 3000); /* 30 seconds timeout */
/*
* Before we drop DTR, make sure the UART transmitter
* has completely drained; this is especially
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 0ff7fda..2297dc7 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -417,7 +417,7 @@ static void hvc_close(struct tty_struct *tty, struct file * filp)
* there is no buffered data otherwise sleeps on a wait queue
* waking periodically to check chars_in_buffer().
*/
- tty_wait_until_sent_from_close(tty, HVC_CLOSE_WAIT);
+ tty_wait_until_sent(tty, HVC_CLOSE_WAIT);
} else {
if (hp->port.count < 0)
printk(KERN_ERR "hvc_close %X: oops, count is %d\n",
diff --git a/drivers/tty/hvc/hvcs.c b/drivers/tty/hvc/hvcs.c
index 81e939e..236302d 100644
--- a/drivers/tty/hvc/hvcs.c
+++ b/drivers/tty/hvc/hvcs.c
@@ -1230,7 +1230,7 @@ static void hvcs_close(struct tty_struct *tty, struct file *filp)
irq = hvcsd->vdev->irq;
spin_unlock_irqrestore(&hvcsd->lock, flags);
- tty_wait_until_sent_from_close(tty, HVCS_CLOSE_WAIT);
+ tty_wait_until_sent(tty, HVCS_CLOSE_WAIT);
/*
* This line is important because it tells hvcs_open that this
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 1b93357..6b6214b 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -464,10 +464,7 @@ static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
schedule_timeout_interruptible(timeout);
}
-/* Caller holds tty lock.
- * NB: may drop and reacquire tty lock (in tty_wait_until_sent_from_close())
- * so tty and tty port may have changed state (but not hung up or reopened).
- */
+/* Caller holds tty lock. */
int tty_port_close_start(struct tty_port *port,
struct tty_struct *tty, struct file *filp)
{
@@ -505,7 +502,7 @@ int tty_port_close_start(struct tty_port *port,
if (tty->flow_stopped)
tty_driver_flush_buffer(tty);
if (port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
- tty_wait_until_sent_from_close(tty, port->closing_wait);
+ tty_wait_until_sent(tty, port->closing_wait);
if (port->drain_delay)
tty_port_drain_delay(port, tty);
}
@@ -545,10 +542,6 @@ EXPORT_SYMBOL(tty_port_close_end);
* tty_port_close
*
* Caller holds tty lock
- *
- * NB: may drop and reacquire tty lock (in tty_port_close_start()->
- * tty_wait_until_sent_from_close()) so tty and tty_port may have changed
- * state (but not hung up or reopened).
*/
void tty_port_close(struct tty_port *port, struct tty_struct *tty,
struct file *filp)
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1c3316a..f3eb70d 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -644,24 +644,6 @@ extern void __lockfunc tty_unlock_pair(struct tty_struct *tty,
struct tty_struct *tty2);
/*
- * this shall be called only from where BTM is held (like close)
- *
- * We need this to ensure nobody waits for us to finish while we are waiting.
- * Without this we were encountering system stalls.
- *
- * This should be indeed removed with BTM removal later.
- *
- * Locking: BTM required. Nobody is allowed to hold port->mutex.
- */
-static inline void tty_wait_until_sent_from_close(struct tty_struct *tty,
- long timeout)
-{
- tty_unlock(tty); /* tty->ops->close holds the BTM, drop it while waiting */
- tty_wait_until_sent(tty, timeout);
- tty_lock(tty);
-}
-
-/*
* wait_event_interruptible_tty -- wait for a condition with the tty lock held
*
* The condition we are waiting for might take a long time to
--
2.0.0
ASYNC_CLOSING is no longer used in the tty core; use tty->closing
as substitute.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/mxser.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 4c4a236..573bc00 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -2255,10 +2255,8 @@ static irqreturn_t mxser_interrupt(int irq, void *dev_id)
break;
iir &= MOXA_MUST_IIR_MASK;
tty = tty_port_tty_get(&port->port);
- if (!tty ||
- (port->port.flags & ASYNC_CLOSING) ||
- !(port->port.flags &
- ASYNC_INITIALIZED)) {
+ if (!tty || tty->closing ||
+ !(port->port.flags & ASYNC_INITIALIZED)) {
status = inb(port->ioaddr + UART_LSR);
outb(0x27, port->ioaddr + UART_FCR);
inb(port->ioaddr + UART_MSR);
--
2.0.0
Since at least before 2.6.30, tty drivers that do not drop the tty lock
while closing cannot observe ASYNC_CLOSING set while holding the
tty lock; this includes the tty driver's open() and hangup() methods,
since the tty core calls these methods holding the tty lock.
For these drivers, waiting for ASYNC_CLOSING to clear while opening
is not required, since this condition cannot occur. Similarly, even
when the open() method drops and reacquires the tty lock after
blocking, ASYNC_CLOSING cannot be set (again, for drivers that
do not drop the tty lock while closing).
Now that tty port drivers no longer drop the tty lock while closing
(since 'tty: Remove tty_wait_until_sent_from_close()'), the same
conditions apply: waiting for ASYNC_CLOSING to clear while opening
is not required, nor is re-checking ASYNC_CLOSING after dropping and
reacquiring the tty lock while blocking (eg., in *_block_til_ready()).
Since ASYNC_CLOSING is not tested elsewhere, it is safe to remove
the flag.
CC: Martin Schwidefsky <[email protected]>
CC: Heiko Carstens <[email protected]>
CC: [email protected]
CC: Mikael Starvik <[email protected]>
CC: Jesper Nilsson <[email protected]>
CC: [email protected]
CC: Samuel Ortiz <[email protected]>
CC: "David S. Miller" <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/char/pcmcia/synclink_cs.c | 9 ---------
drivers/s390/char/con3215.c | 3 +--
drivers/tty/cyclades.c | 9 ---------
drivers/tty/rocket.c | 14 +-------------
drivers/tty/serial/68328serial.c | 2 --
drivers/tty/serial/crisv10.c | 36 ++----------------------------------
drivers/tty/serial/serial_core.c | 1 -
drivers/tty/synclink.c | 18 ++++--------------
drivers/tty/synclink_gt.c | 14 ++------------
drivers/tty/synclinkmp.c | 14 ++------------
drivers/tty/tty_port.c | 16 ++--------------
include/uapi/linux/tty_flags.h | 2 --
net/irda/ircomm/ircomm_tty.c | 35 +----------------------------------
13 files changed, 15 insertions(+), 158 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 0ea9986..ca18543f 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -2507,15 +2507,6 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
printk("%s(%d):mgslpc_open(%s), old ref count = %d\n",
__FILE__, __LINE__, tty->driver->name, port->count);
- /* If port is closing, signal caller to try again */
- if (port->flags & ASYNC_CLOSING){
- wait_event_interruptible_tty(tty, port->close_wait,
- !(port->flags & ASYNC_CLOSING));
- retval = ((port->flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
- goto cleanup;
- }
-
port->low_latency = (port->flags & ASYNC_LOW_LATENCY) ? 1 : 0;
spin_lock_irqsave(&info->netlock, flags);
diff --git a/drivers/s390/char/con3215.c b/drivers/s390/char/con3215.c
index 5af7f0b..2fba207 100644
--- a/drivers/s390/char/con3215.c
+++ b/drivers/s390/char/con3215.c
@@ -641,7 +641,6 @@ static void raw3215_shutdown(struct raw3215_info *raw)
if ((raw->flags & RAW3215_WORKING) ||
raw->queued_write != NULL ||
raw->queued_read != NULL) {
- raw->port.flags |= ASYNC_CLOSING;
add_wait_queue(&raw->empty_wait, &wait);
set_current_state(TASK_INTERRUPTIBLE);
spin_unlock_irqrestore(get_ccwdev_lock(raw->cdev), flags);
@@ -649,7 +648,7 @@ static void raw3215_shutdown(struct raw3215_info *raw)
spin_lock_irqsave(get_ccwdev_lock(raw->cdev), flags);
remove_wait_queue(&raw->empty_wait, &wait);
set_current_state(TASK_RUNNING);
- raw->port.flags &= ~(ASYNC_INITIALIZED | ASYNC_CLOSING);
+ raw->port.flags &= ~ASYNC_INITIALIZED;
}
spin_unlock_irqrestore(get_ccwdev_lock(raw->cdev), flags);
}
diff --git a/drivers/tty/cyclades.c b/drivers/tty/cyclades.c
index fd66f57..2666421 100644
--- a/drivers/tty/cyclades.c
+++ b/drivers/tty/cyclades.c
@@ -1577,15 +1577,6 @@ static int cy_open(struct tty_struct *tty, struct file *filp)
#endif
/*
- * If the port is the middle of closing, bail out now
- */
- if (info->port.flags & ASYNC_CLOSING) {
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
- return (info->port.flags & ASYNC_HUP_NOTIFY) ? -EAGAIN: -ERESTARTSYS;
- }
-
- /*
* Start up serial port
*/
retval = cy_startup(info, tty);
diff --git a/drivers/tty/rocket.c b/drivers/tty/rocket.c
index 383c4c7..882750d 100644
--- a/drivers/tty/rocket.c
+++ b/drivers/tty/rocket.c
@@ -895,14 +895,6 @@ static int rp_open(struct tty_struct *tty, struct file *filp)
if (!page)
return -ENOMEM;
- if (port->flags & ASYNC_CLOSING) {
- retval = wait_for_completion_interruptible(&info->close_wait);
- free_page(page);
- if (retval)
- return retval;
- return ((port->flags & ASYNC_HUP_NOTIFY) ? -EAGAIN : -ERESTARTSYS);
- }
-
/*
* We must not sleep from here until the port is marked fully in use.
*/
@@ -1051,7 +1043,7 @@ static void rp_close(struct tty_struct *tty, struct file *filp)
}
}
spin_lock_irq(&port->lock);
- info->port.flags &= ~(ASYNC_INITIALIZED | ASYNC_CLOSING | ASYNC_NORMAL_ACTIVE);
+ info->port.flags &= ~(ASYNC_INITIALIZED | ASYNC_NORMAL_ACTIVE);
tty->closing = 0;
spin_unlock_irq(&port->lock);
mutex_unlock(&port->mutex);
@@ -1511,10 +1503,6 @@ static void rp_hangup(struct tty_struct *tty)
#endif
rp_flush_buffer(tty);
spin_lock_irqsave(&info->port.lock, flags);
- if (info->port.flags & ASYNC_CLOSING) {
- spin_unlock_irqrestore(&info->port.lock, flags);
- return;
- }
if (info->port.count)
atomic_dec(&rp_num_ports_open);
clear_bit((info->aiop * 8) + info->chan, (void *) &xmit_flags[info->board]);
diff --git a/drivers/tty/serial/68328serial.c b/drivers/tty/serial/68328serial.c
index 5dc9c4b..b438ea5 100644
--- a/drivers/tty/serial/68328serial.c
+++ b/drivers/tty/serial/68328serial.c
@@ -1029,7 +1029,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
local_irq_restore(flags);
return;
}
- port->flags |= ASYNC_CLOSING;
/*
* Now we wait for the transmit buffer to clear; and we notify
* the line discipline to only process XON/XOFF characters.
@@ -1069,7 +1068,6 @@ static void rs_close(struct tty_struct *tty, struct file * filp)
msleep_interruptible(jiffies_to_msecs(port->close_delay));
wake_up_interruptible(&port->open_wait);
}
- port->flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CLOSING);
wake_up_interruptible(&port->close_wait);
local_irq_restore(flags);
}
diff --git a/drivers/tty/serial/crisv10.c b/drivers/tty/serial/crisv10.c
index 58e6f61..6989d7d 100644
--- a/drivers/tty/serial/crisv10.c
+++ b/drivers/tty/serial/crisv10.c
@@ -3674,7 +3674,6 @@ rs_close(struct tty_struct *tty, struct file * filp)
local_irq_restore(flags);
return;
}
- info->port.flags |= ASYNC_CLOSING;
/*
* Save the termios structure, since this port may have
* separate termios for callout and dialin.
@@ -3719,7 +3718,7 @@ rs_close(struct tty_struct *tty, struct file * filp)
schedule_timeout_interruptible(info->port.close_delay);
wake_up_interruptible(&info->port.open_wait);
}
- info->port.flags &= ~(ASYNC_NORMAL_ACTIVE|ASYNC_CLOSING);
+ info->port.flags &= ~ASYNC_NORMAL_ACTIVE;
wake_up_interruptible(&info->port.close_wait);
local_irq_restore(flags);
@@ -3834,23 +3833,6 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
int do_clocal = 0;
/*
- * If the device is in the middle of being closed, then block
- * until it's done, and then try again.
- */
- if (info->port.flags & ASYNC_CLOSING) {
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
-#ifdef SERIAL_DO_RESTART
- if (info->port.flags & ASYNC_HUP_NOTIFY)
- return -EAGAIN;
- else
- return -ERESTARTSYS;
-#else
- return -EAGAIN;
-#endif
- }
-
- /*
* If non-blocking mode is set, or the port is not enabled,
* then make the check up front and then exit.
*/
@@ -3900,7 +3882,7 @@ block_til_ready(struct tty_struct *tty, struct file * filp,
#endif
break;
}
- if (!(info->port.flags & ASYNC_CLOSING) && do_clocal)
+ if (do_clocal)
/* && (do_clocal || DCD_IS_ASSERTED) */
break;
if (signal_pending(current)) {
@@ -3970,20 +3952,6 @@ rs_open(struct tty_struct *tty, struct file * filp)
info->port.low_latency = !!(info->port.flags & ASYNC_LOW_LATENCY);
/*
- * If the port is in the middle of closing, bail out now
- */
- if (info->port.flags & ASYNC_CLOSING) {
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
-#ifdef SERIAL_DO_RESTART
- return ((info->port.flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
-#else
- return -EAGAIN;
-#endif
- }
-
- /*
* If DMA is enabled try to allocate the irq's.
*/
if (info->port.count == 1) {
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f64cf45..e97653e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1376,7 +1376,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
* Wake up anyone trying to open this port.
*/
clear_bit(ASYNCB_NORMAL_ACTIVE, &port->flags);
- clear_bit(ASYNCB_CLOSING, &port->flags);
spin_unlock_irqrestore(&port->lock, flags);
wake_up_interruptible(&port->open_wait);
wake_up_interruptible(&port->close_wait);
diff --git a/drivers/tty/synclink.c b/drivers/tty/synclink.c
index b799170..86c2a8d 100644
--- a/drivers/tty/synclink.c
+++ b/drivers/tty/synclink.c
@@ -3314,12 +3314,11 @@ static int block_til_ready(struct tty_struct *tty, struct file * filp,
-EAGAIN : -ERESTARTSYS;
break;
}
-
+
dcd = tty_port_carrier_raised(&info->port);
-
- if (!(port->flags & ASYNC_CLOSING) && (do_clocal || dcd))
- break;
-
+ if (do_clocal || dcd)
+ break;
+
if (signal_pending(current)) {
retval = -ERESTARTSYS;
break;
@@ -3398,15 +3397,6 @@ static int mgsl_open(struct tty_struct *tty, struct file * filp)
printk("%s(%d):mgsl_open(%s), old ref count = %d\n",
__FILE__,__LINE__,tty->driver->name, info->port.count);
- /* If port is closing, signal caller to try again */
- if (info->port.flags & ASYNC_CLOSING){
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
- retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
- goto cleanup;
- }
-
info->port.low_latency = (info->port.flags & ASYNC_LOW_LATENCY) ? 1 : 0;
spin_lock_irqsave(&info->netlock, flags);
diff --git a/drivers/tty/synclink_gt.c b/drivers/tty/synclink_gt.c
index ba1dbcd..2a798b9 100644
--- a/drivers/tty/synclink_gt.c
+++ b/drivers/tty/synclink_gt.c
@@ -672,15 +672,6 @@ static int open(struct tty_struct *tty, struct file *filp)
DBGINFO(("%s open, old ref count = %d\n", info->device_name, info->port.count));
- /* If port is closing, signal caller to try again */
- if (info->port.flags & ASYNC_CLOSING){
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
- retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
- goto cleanup;
- }
-
mutex_lock(&info->port.mutex);
info->port.low_latency = (info->port.flags & ASYNC_LOW_LATENCY) ? 1 : 0;
@@ -3316,9 +3307,8 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
}
cd = tty_port_carrier_raised(port);
-
- if (!(port->flags & ASYNC_CLOSING) && (do_clocal || cd ))
- break;
+ if (do_clocal || cd )
+ break;
if (signal_pending(current)) {
retval = -ERESTARTSYS;
diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c
index c3f9091..050f611 100644
--- a/drivers/tty/synclinkmp.c
+++ b/drivers/tty/synclinkmp.c
@@ -752,15 +752,6 @@ static int open(struct tty_struct *tty, struct file *filp)
printk("%s(%d):%s open(), old ref count = %d\n",
__FILE__,__LINE__,tty->driver->name, info->port.count);
- /* If port is closing, signal caller to try again */
- if (info->port.flags & ASYNC_CLOSING){
- wait_event_interruptible_tty(tty, info->port.close_wait,
- !(info->port.flags & ASYNC_CLOSING));
- retval = ((info->port.flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS);
- goto cleanup;
- }
-
info->port.low_latency = (info->port.flags & ASYNC_LOW_LATENCY) ? 1 : 0;
spin_lock_irqsave(&info->netlock, flags);
@@ -3338,9 +3329,8 @@ static int block_til_ready(struct tty_struct *tty, struct file *filp,
}
cd = tty_port_carrier_raised(port);
-
- if (!(port->flags & ASYNC_CLOSING) && (do_clocal || cd))
- break;
+ if (do_clocal || cd)
+ break;
if (signal_pending(current)) {
retval = -ERESTARTSYS;
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 6b6214b..896eebf 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -364,16 +364,6 @@ int tty_port_block_til_ready(struct tty_port *port,
unsigned long flags;
DEFINE_WAIT(wait);
- /* block if port is in the process of being closed */
- if (port->flags & ASYNC_CLOSING) {
- wait_event_interruptible_tty(tty, port->close_wait,
- !(port->flags & ASYNC_CLOSING));
- if (port->flags & ASYNC_HUP_NOTIFY)
- return -EAGAIN;
- else
- return -ERESTARTSYS;
- }
-
/* if non-blocking mode is set we can pass directly to open unless
the port has just hung up or is in another error state */
if (tty->flags & (1 << TTY_IO_ERROR)) {
@@ -424,8 +414,7 @@ int tty_port_block_til_ready(struct tty_port *port,
* Never ask drivers if CLOCAL is set, this causes troubles
* on some hardware.
*/
- if (!(port->flags & ASYNC_CLOSING) &&
- (do_clocal || tty_port_carrier_raised(port)))
+ if (do_clocal || tty_port_carrier_raised(port))
break;
if (signal_pending(current)) {
retval = -ERESTARTSYS;
@@ -492,7 +481,6 @@ int tty_port_close_start(struct tty_port *port,
spin_unlock_irqrestore(&port->lock, flags);
return 0;
}
- set_bit(ASYNCB_CLOSING, &port->flags);
spin_unlock_irqrestore(&port->lock, flags);
tty->closing = 1;
@@ -532,7 +520,7 @@ void tty_port_close_end(struct tty_port *port, struct tty_struct *tty)
spin_lock_irqsave(&port->lock, flags);
wake_up_interruptible(&port->open_wait);
}
- port->flags &= ~(ASYNC_NORMAL_ACTIVE | ASYNC_CLOSING);
+ port->flags &= ~ASYNC_NORMAL_ACTIVE;
wake_up_interruptible(&port->close_wait);
spin_unlock_irqrestore(&port->lock, flags);
}
diff --git a/include/uapi/linux/tty_flags.h b/include/uapi/linux/tty_flags.h
index eefcb48..4d78f3e 100644
--- a/include/uapi/linux/tty_flags.h
+++ b/include/uapi/linux/tty_flags.h
@@ -33,7 +33,6 @@
#define ASYNCB_SUSPENDED 30 /* Serial port is suspended */
#define ASYNCB_NORMAL_ACTIVE 29 /* Normal device is active */
#define ASYNCB_BOOT_AUTOCONF 28 /* Autoconfigure port on bootup */
-#define ASYNCB_CLOSING 27 /* Serial port is closing */
#define ASYNCB_CTS_FLOW 26 /* Do CTS flow control */
#define ASYNCB_CHECK_CD 25 /* i.e., CLOCAL */
#define ASYNCB_SHARE_IRQ 24 /* for multifunction cards, no longer used */
@@ -68,7 +67,6 @@
#define ASYNC_INITIALIZED (1U << ASYNCB_INITIALIZED)
#define ASYNC_NORMAL_ACTIVE (1U << ASYNCB_NORMAL_ACTIVE)
#define ASYNC_BOOT_AUTOCONF (1U << ASYNCB_BOOT_AUTOCONF)
-#define ASYNC_CLOSING (1U << ASYNCB_CLOSING)
#define ASYNC_CTS_FLOW (1U << ASYNCB_CTS_FLOW)
#define ASYNC_CHECK_CD (1U << ASYNCB_CHECK_CD)
#define ASYNC_SHARE_IRQ (1U << ASYNCB_SHARE_IRQ)
diff --git a/net/irda/ircomm/ircomm_tty.c b/net/irda/ircomm/ircomm_tty.c
index 61ceb4c..88a0116 100644
--- a/net/irda/ircomm/ircomm_tty.c
+++ b/net/irda/ircomm/ircomm_tty.c
@@ -342,8 +342,7 @@ static int ircomm_tty_block_til_ready(struct ircomm_tty_cb *self,
* specified, we cannot return before the IrCOMM link is
* ready
*/
- if (!test_bit(ASYNCB_CLOSING, &port->flags) &&
- (do_clocal || tty_port_carrier_raised(port)) &&
+ if ((do_clocal || tty_port_carrier_raised(port)) &&
self->state == IRCOMM_TTY_READY)
{
break;
@@ -454,34 +453,6 @@ static int ircomm_tty_open(struct tty_struct *tty, struct file *filp)
/* Not really used by us, but lets do it anyway */
self->port.low_latency = (self->port.flags & ASYNC_LOW_LATENCY) ? 1 : 0;
- /*
- * If the port is the middle of closing, bail out now
- */
- if (test_bit(ASYNCB_CLOSING, &self->port.flags)) {
-
- /* Hm, why are we blocking on ASYNC_CLOSING if we
- * do return -EAGAIN/-ERESTARTSYS below anyway?
- * IMHO it's either not needed in the first place
- * or for some reason we need to make sure the async
- * closing has been finished - if so, wouldn't we
- * probably better sleep uninterruptible?
- */
-
- if (wait_event_interruptible(self->port.close_wait,
- !test_bit(ASYNCB_CLOSING, &self->port.flags))) {
- IRDA_WARNING("%s - got signal while blocking on ASYNC_CLOSING!\n",
- __func__);
- return -ERESTARTSYS;
- }
-
-#ifdef SERIAL_DO_RESTART
- return (self->port.flags & ASYNC_HUP_NOTIFY) ?
- -EAGAIN : -ERESTARTSYS;
-#else
- return -EAGAIN;
-#endif
- }
-
/* Check if this is a "normal" ircomm device, or an irlpt device */
if (self->line < 0x10) {
self->service_type = IRCOMM_3_WIRE | IRCOMM_9_WIRE;
@@ -1331,10 +1302,6 @@ static void ircomm_tty_line_info(struct ircomm_tty_cb *self, struct seq_file *m)
seq_printf(m, "%cASYNC_LOW_LATENCY", sep);
sep = '|';
}
- if (self->port.flags & ASYNC_CLOSING) {
- seq_printf(m, "%cASYNC_CLOSING", sep);
- sep = '|';
- }
if (self->port.flags & ASYNC_NORMAL_ACTIVE) {
seq_printf(m, "%cASYNC_NORMAL_ACTIVE", sep);
sep = '|';
--
2.0.0
tty_port_close_start() already validates the port counts and issues
a diagnostic if validation fails.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/char/pcmcia/synclink_cs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index a63970f..0ea9986 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -2347,8 +2347,6 @@ static void mgslpc_close(struct tty_struct *tty, struct file * filp)
printk("%s(%d):mgslpc_close(%s) entry, count=%d\n",
__FILE__, __LINE__, info->device_name, port->count);
- WARN_ON(!port->count);
-
if (tty_port_close_start(port, tty, filp) == 0)
goto cleanup;
--
2.0.0
ASYNC_CLOSING is no longer used in the tty core; use private flag
info->closing as substitute.
CC: Karsten Keil <[email protected]>
CC: [email protected]
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/isdn/i4l/isdn_tty.c | 14 +++++++-------
include/linux/isdn.h | 1 +
2 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
index 732f68a..5310932 100644
--- a/drivers/isdn/i4l/isdn_tty.c
+++ b/drivers/isdn/i4l/isdn_tty.c
@@ -1577,8 +1577,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
#endif
return;
}
- port->flags |= ASYNC_CLOSING;
-
+ info->closing = 1;
tty->closing = 1;
/*
* At this point we stop accepting input. To do this, we
@@ -1606,6 +1605,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
tty_ldisc_flush(tty);
port->tty = NULL;
info->ncarrier = 0;
+ info->closing = 0;
tty_port_close_end(port, tty);
#ifdef ISDN_DEBUG_MODEM_OPEN
@@ -1806,6 +1806,7 @@ isdn_tty_modem_init(void)
spin_lock_init(&info->readlock);
sprintf(info->last_cause, "0000");
sprintf(info->last_num, "none");
+ info->closing = 0;
info->last_dir = 0;
info->last_lhup = 1;
info->last_l2 = -1;
@@ -2241,7 +2242,7 @@ isdn_tty_at_cout(char *msg, modem_info *info)
l = strlen(msg);
spin_lock_irqsave(&info->readlock, flags);
- if (port->flags & ASYNC_CLOSING) {
+ if (info->closing) {
spin_unlock_irqrestore(&info->readlock, flags);
return;
}
@@ -2391,13 +2392,12 @@ isdn_tty_modem_result(int code, modem_info *info)
case RESULT_NO_CARRIER:
#ifdef ISDN_DEBUG_MODEM_HUP
printk(KERN_DEBUG "modem_result: NO CARRIER %d %d\n",
- (info->port.flags & ASYNC_CLOSING),
- (!info->port.tty));
+ info->closing, (!info->port.tty));
#endif
m->mdmreg[REG_RINGCNT] = 0;
del_timer(&info->nc_timer);
info->ncarrier = 0;
- if ((info->port.flags & ASYNC_CLOSING) || (!info->port.tty))
+ if (info->closing || (!info->port.tty))
return;
#ifdef CONFIG_ISDN_AUDIO
@@ -2530,7 +2530,7 @@ isdn_tty_modem_result(int code, modem_info *info)
}
}
if (code == RESULT_NO_CARRIER) {
- if ((info->port.flags & ASYNC_CLOSING) || (!info->port.tty))
+ if (info->closing || (!info->port.tty))
return;
if (info->port.flags & ASYNC_CHECK_CD)
diff --git a/include/linux/isdn.h b/include/linux/isdn.h
index 1e9a0f2..fe80475 100644
--- a/include/linux/isdn.h
+++ b/include/linux/isdn.h
@@ -311,6 +311,7 @@ typedef struct atemu {
typedef struct modem_info {
int magic;
struct tty_port port;
+ int closing:1; /* port count has dropped to 0 */
int x_char; /* xon/xoff character */
int mcr; /* Modem control register */
int msr; /* Modem status register */
--
2.0.0
Unwrap if() conditional; no functional change.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/serial_core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index e97653e..f13a769 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1278,8 +1278,7 @@ static void uart_set_termios(struct tty_struct *tty,
/* Handle transition away from B0 status */
else if (!(old_termios->c_cflag & CBAUD) && (cflag & CBAUD)) {
unsigned int mask = TIOCM_DTR;
- if (!(cflag & CRTSCTS) ||
- !test_bit(TTY_THROTTLED, &tty->flags))
+ if (!(cflag & CRTSCTS) || !test_bit(TTY_THROTTLED, &tty->flags))
mask |= TIOCM_RTS;
uart_set_mctrl(uport, mask);
}
--
2.0.0
The port->lock does not protect the filp->f_op field; move
the tty_hung_up_p() test outside the port->lock critical section
in tty_port_close_start().
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_port.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 896eebf..be5deff 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -459,12 +459,10 @@ int tty_port_close_start(struct tty_port *port,
{
unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
- if (tty_hung_up_p(filp)) {
- spin_unlock_irqrestore(&port->lock, flags);
+ if (tty_hung_up_p(filp))
return 0;
- }
+ spin_lock_irqsave(&port->lock, flags);
if (tty->count == 1 && port->count != 1) {
printk(KERN_WARNING
"tty_port_close_start: tty->count = 1 port count = %d.\n",
--
2.0.0
The tty_ldisc_flush() after port hardware shutdown is unnecessary;
the ldisc flush was just performed before the hardware shutdown
in tty_port_close_start() and the ldisc will be released when
uart_close() returns (because the last port close implies the
last tty close).
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/serial_core.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 212ee07..15212d7 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1351,9 +1351,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
mutex_lock(&port->mutex);
uart_shutdown(tty, state);
-
- tty_ldisc_flush(tty);
-
tty_port_tty_set(port, NULL);
tty->closing = 0;
spin_lock_irqsave(&port->lock, flags);
--
2.0.0
The low-level uart driver may modify termios settings to override
settings that are not compatible with the uart, such as CRTSCTS.
Thus, callers of the low-level uart driver's set_termios() method must
hold termios_rwsem write lock to prevent concurrent access to termios,
in case such override occurs.
The termios_rwsem lock requirement does not extend to console setup
(ie., uart_set_options), as console setup cannot race with tty
operations. Nor does this lock requirement extend to functions which
cannot be concurrent with tty ioctls (ie., uart_port_startup() and
uart_resume_port()).
Further, always claim the port mutex to protect hardware
re-reprogramming in the set_termios() uart driver method. Note this
is unnecessary for console initialization in uart_set_options()
which cannot be concurrent with other uart operations.
Signed-off-by: Peter Hurley <[email protected]>
---
Documentation/serial/driver | 6 ++++--
drivers/tty/serial/serial_core.c | 8 +++++++-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Documentation/serial/driver b/Documentation/serial/driver
index c3a7689..b1a22d4 100644
--- a/Documentation/serial/driver
+++ b/Documentation/serial/driver
@@ -59,7 +59,9 @@ The core driver uses the info->tmpbuf_sem lock to prevent multi-threaded
access to the info->tmpbuf bouncebuffer used for port writes.
The port_sem semaphore is used to protect against ports being added/
-removed or reconfigured at inappropriate times.
+removed or reconfigured at inappropriate times. Since v2.6.27, this
+semaphore has been the 'mutex' member of the tty_port struct, and
+commonly referred to as the port mutex (or port->mutex).
uart_ops
@@ -246,7 +248,7 @@ hardware.
Other flags may be used (eg, xon/xoff characters) if your
hardware supports hardware "soft" flow control.
- Locking: none.
+ Locking: caller holds port->mutex
Interrupts: caller dependent.
This call must not sleep
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 15212d7..5ed5a46 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -427,7 +427,7 @@ uart_get_divisor(struct uart_port *port, unsigned int baud)
EXPORT_SYMBOL(uart_get_divisor);
-/* FIXME: Consistent locking policy */
+/* Caller holds port mutex */
static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
struct ktermios *old_termios)
{
@@ -1162,11 +1162,15 @@ uart_ioctl(struct tty_struct *tty, unsigned int cmd,
break;
case TIOCSSERIAL:
+ down_write(&tty->termios_rwsem);
ret = uart_set_info_user(tty, state, uarg);
+ up_write(&tty->termios_rwsem);
break;
case TIOCSERCONFIG:
+ down_write(&tty->termios_rwsem);
ret = uart_do_autoconfig(tty, state);
+ up_write(&tty->termios_rwsem);
break;
case TIOCSERGWILD: /* obsolete */
@@ -1268,7 +1272,9 @@ static void uart_set_termios(struct tty_struct *tty,
return;
}
+ mutex_lock(&state->port.mutex);
uart_change_speed(tty, state, old_termios);
+ mutex_unlock(&state->port.mutex);
/* reload cflag from termios; port driver may have overriden flags */
cflag = tty->termios.c_cflag;
--
2.0.0
In the context of the final tty & port close, flushing the tx
ring buffer after the hardware has already been shutdown and
the ring buffer freed is neither required nor desirable.
uart_flush_buffer() performs 3 operations:
1. Resets tx ring buffer indices, but the tx ring buffer has
already been freed and the indices are reset if the port is
re-opened.
2. Calls uart driver's flush_buffer() method
5 in-tree uart drivers define flush_buffer() methods:
amba-pl011, atmel-serial, imx, serial-tegra, timbuart
These have been refactored into the shutdown() method, if
required.
3. Kicks the ldisc for more writing, but this is undesirable.
The file handle is being released; any waiting writer will
will be kicked out by tty_release() with a warning. Further,
the N_TTY ldisc may generate SIGIO for a file handle which
is no longer valid.
Cc: Nicolas Ferre <[email protected]>
Cc: Russell King <[email protected]>
Cc: Laxman Dewangan <[email protected]>
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/amba-pl011.c | 1 +
drivers/tty/serial/atmel_serial.c | 2 ++
drivers/tty/serial/serial-tegra.c | 2 ++
drivers/tty/serial/serial_core.c | 1 -
drivers/tty/serial/timbuart.c | 2 ++
5 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index dacf0a0..4255eef 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1676,6 +1676,7 @@ static void pl011_shutdown(struct uart_port *port)
plat->exit();
}
+ pl011_dma_flush_buffer(port);
}
static void
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 53eeea1..8aea441 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1664,6 +1664,8 @@ static void atmel_shutdown(struct uart_port *port)
* Free the interrupt
*/
free_irq(port->irq, port);
+
+ atmel_flush_buffer(port);
}
/*
diff --git a/drivers/tty/serial/serial-tegra.c b/drivers/tty/serial/serial-tegra.c
index d5c2a28..2f5d699 100644
--- a/drivers/tty/serial/serial-tegra.c
+++ b/drivers/tty/serial/serial-tegra.c
@@ -1042,6 +1042,8 @@ static void tegra_uart_shutdown(struct uart_port *u)
tegra_uart_dma_channel_free(tup, true);
tegra_uart_dma_channel_free(tup, false);
free_irq(u->irq, tup);
+
+ tegra_uart_flush_buffer(u);
}
static void tegra_uart_enable_ms(struct uart_port *u)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index f13a769..212ee07 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1351,7 +1351,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
mutex_lock(&port->mutex);
uart_shutdown(tty, state);
- uart_flush_buffer(tty);
tty_ldisc_flush(tty);
diff --git a/drivers/tty/serial/timbuart.c b/drivers/tty/serial/timbuart.c
index f87097a..57d8dc0 100644
--- a/drivers/tty/serial/timbuart.c
+++ b/drivers/tty/serial/timbuart.c
@@ -278,6 +278,8 @@ static void timbuart_shutdown(struct uart_port *port)
dev_dbg(port->dev, "%s\n", __func__);
free_irq(port->irq, uart);
iowrite32(0, port->membase + TIMBUART_IER);
+
+ timbuart_flush_buffer(port);
}
static int get_bindex(int baud)
--
2.0.0
Uart port drivers may reconfigure termios settings based on available
hardware support; set/clear ASYNC_CTS_FLOW and ASYNC_CHECK_CD _after_
calling the port driver's .set_termios method.
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/serial/serial_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index c8879ce..f64cf45 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -443,6 +443,7 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
return;
termios = &tty->termios;
+ uport->ops->set_termios(uport, termios, old_termios);
/*
* Set flags based on termios cflag
@@ -456,8 +457,6 @@ static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
clear_bit(ASYNCB_CHECK_CD, &port->flags);
else
set_bit(ASYNCB_CHECK_CD, &port->flags);
-
- uport->ops->set_termios(uport, termios, old_termios);
}
static inline int __uart_put_char(struct uart_port *port,
@@ -1270,6 +1269,8 @@ static void uart_set_termios(struct tty_struct *tty,
}
uart_change_speed(tty, state, old_termios);
+ /* reload cflag from termios; port driver may have overriden flags */
+ cflag = tty->termios.c_cflag;
/* Handle transition to B0 status */
if ((old_termios->c_cflag & CBAUD) && !(cflag & CBAUD))
--
2.0.0
Signed-off-by: Peter Hurley <[email protected]>
---
drivers/tty/tty_io.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3411071..714320b 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -688,7 +688,7 @@ static void __tty_hangup(struct tty_struct *tty, int exit_session)
for (n = 0; n < closecount; n++)
tty->ops->close(tty, cons_filp);
} else if (tty->ops->hangup)
- (tty->ops->hangup)(tty);
+ tty->ops->hangup(tty);
/*
* We don't want to have driver/ldisc interactions beyond
* the ones we did here. The driver layer expects no
--
2.0.0
On Mon, Jun 16, 2014 at 09:17:06AM -0400, Peter Hurley wrote:
> Since at least before 2.6.30, it has not been possible to observe
> a hung up file pointer in a tty driver's open() method unless/until
> the driver open() releases the tty_lock() (eg., before blocking).
>
> This is because tty_open() adds the file pointer while holding
> the tty_lock() _and_ doesn't release the lock until after calling
> the tty driver's open() method. [ Before tty_lock(), this was
> lock_kernel(). ]
>
> Since __tty_hangup() first waits on the tty_lock() before
> enumerating and hanging up the open file pointers, either
> __tty_hangup() will wait for the tty_lock() or tty_open() will
> not yet have added the file pointer. For example,
>
> CPU 0 | CPU 1
> |
> tty_open | __tty_hangup
> .. | ..
> tty_lock | ..
> tty_reopen | tty_lock / blocks
> .. |
> tty_add_file(tty, filp) |
> .. |
> tty->ops->open(tty, filp) |
> tty_port_open |
> tty_port_block_til_ready |
> .. |
> while (1) |
> .. |
> tty_unlock | / unblocks
> schedule | for each filp on tty->tty_files
> | f_ops = tty_hung_up_fops;
> | ..
> | tty_unlock
> tty_lock |
> .. |
> tty_unlock |
>
> Note that since tty_port_block_til_ready() and similar drop
> the tty_lock while blocking, when woken, the file pointer
> must then be tested for having been hung up.
>
> Also, fix bit-rotted drivers that used extra_count to track the
> port->count bump.
>
> CC: Mikael Starvik <[email protected]>
For the CRIS part:
Acked-by: Jesper Nilsson <[email protected]>
> CC: Samuel Ortiz <[email protected]>
> CC: "David S. Miller" <[email protected]>
> Signed-off-by: Peter Hurley <[email protected]>
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Mon, Jun 16, 2014 at 09:17:14AM -0400, Peter Hurley wrote:
> Since at least before 2.6.30, tty drivers that do not drop the tty lock
> while closing cannot observe ASYNC_CLOSING set while holding the
> tty lock; this includes the tty driver's open() and hangup() methods,
> since the tty core calls these methods holding the tty lock.
>
> For these drivers, waiting for ASYNC_CLOSING to clear while opening
> is not required, since this condition cannot occur. Similarly, even
> when the open() method drops and reacquires the tty lock after
> blocking, ASYNC_CLOSING cannot be set (again, for drivers that
> do not drop the tty lock while closing).
>
> Now that tty port drivers no longer drop the tty lock while closing
> (since 'tty: Remove tty_wait_until_sent_from_close()'), the same
> conditions apply: waiting for ASYNC_CLOSING to clear while opening
> is not required, nor is re-checking ASYNC_CLOSING after dropping and
> reacquiring the tty lock while blocking (eg., in *_block_til_ready()).
>
> Since ASYNC_CLOSING is not tested elsewhere, it is safe to remove
> the flag.
>
> CC: Martin Schwidefsky <[email protected]>
> CC: Heiko Carstens <[email protected]>
> CC: [email protected]
> CC: Mikael Starvik <[email protected]>
For the CRIS part:
Acked-by: Jesper Nilsson <[email protected]>
> CC: [email protected]
> CC: Samuel Ortiz <[email protected]>
> CC: "David S. Miller" <[email protected]>
> Signed-off-by: Peter Hurley <[email protected]>
/^JN - Jesper Nilsson
--
Jesper Nilsson -- [email protected]
On Mon, Jun 16, 2014 at 09:17:04AM -0400, Peter Hurley wrote:
> tty->closing cannot be set on ipw_open() because the ipwireless tty
> driver does not call any functions that set tty->closing.
>
> CC: Jiri Kosina <[email protected]>
Acked-by: David Sterba <[email protected]>
From: Of Peter Hurley
> ASYNC_CLOSING is no longer used in the tty core; use private flag
> info->closing as substitute.
...
> @@ -311,6 +311,7 @@ typedef struct atemu {
> typedef struct modem_info {
> int magic;
> struct tty_port port;
> + int closing:1; /* port count has dropped to 0 */
> int x_char; /* xon/xoff character */
> int mcr; /* Modem control register */
> int msr; /* Modem status register */
That should probably be a bool and set to true/false.
You are probably adding a load of padding.
David
Hi David,
On 06/16/2014 11:37 AM, David Laight wrote:
> From: Of Peter Hurley
>> ASYNC_CLOSING is no longer used in the tty core; use private flag
>> info->closing as substitute.
> ...
>> @@ -311,6 +311,7 @@ typedef struct atemu {
>> typedef struct modem_info {
>> int magic;
>> struct tty_port port;
>> + int closing:1; /* port count has dropped to 0 */
>> int x_char; /* xon/xoff character */
>> int mcr; /* Modem control register */
>> int msr; /* Modem status register */
>
> That should probably be a bool and set to true/false.
> You are probably adding a load of padding.
struct modem_info is over 1K, with several existing int-as-bool fields.
An array of 64 struct modem_info are statically allocated with every isdn device.
It doesn't look like memory consumption has been a consideration with the isdn driver.
Regards,
Peter Hurley
On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
> tty_wait_until_sent_from_close() drops the tty lock while waiting
> for the tty driver to finish sending previously accepted data (ie.,
> data remaining in its write buffer and transmit fifo).
>
> However, dropping the tty lock is a hold-over from when the tty
> lock was system-wide; ie., one lock for all ttys.
>
> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
> 'tty: localise the lock', dropping the tty lock has not been necessary.
>
> CC: Karsten Keil <[email protected]>
> CC: [email protected]
> Signed-off-by: Peter Hurley <[email protected]>
I don't understand the second half of the changelog, it doesn't seem
to fit here: there deadlock that we are trying to avoid here happens
when the *same* tty needs the lock to complete the function that
sends the pending data. I don't think we do still do that any more,
but it doesn't seem related to the tty lock being system-wide or not.
Arnd
From: Arnd Bergmann
> On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
> > tty_wait_until_sent_from_close() drops the tty lock while waiting
> > for the tty driver to finish sending previously accepted data (ie.,
> > data remaining in its write buffer and transmit fifo).
> >
> > However, dropping the tty lock is a hold-over from when the tty
> > lock was system-wide; ie., one lock for all ttys.
> >
> > Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
> > 'tty: localise the lock', dropping the tty lock has not been necessary.
> >
> > CC: Karsten Keil <[email protected]>
> > CC: [email protected]
> > Signed-off-by: Peter Hurley <[email protected]>
>
> I don't understand the second half of the changelog, it doesn't seem
> to fit here: there deadlock that we are trying to avoid here happens
> when the *same* tty needs the lock to complete the function that
> sends the pending data. I don't think we do still do that any more,
> but it doesn't seem related to the tty lock being system-wide or not.
While I've not looked at the code in question; my thoughts were that
holding any lock while waiting for output to drain (or anything else
really) probably isn't a good idea.
You might find that something else needs the lock - even if only
some kind of status request.
David
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 06/17/2014 04:00 AM, Arnd Bergmann wrote:
> On Monday 16 June 2014 09:17:11 Peter Hurley wrote:
>> tty_wait_until_sent_from_close() drops the tty lock while waiting
>> for the tty driver to finish sending previously accepted data (ie.,
>> data remaining in its write buffer and transmit fifo).
>>
>> However, dropping the tty lock is a hold-over from when the tty
>> lock was system-wide; ie., one lock for all ttys.
>>
>> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
>> 'tty: localise the lock', dropping the tty lock has not been necessary.
>>
>> CC: Karsten Keil <[email protected]>
>> CC: [email protected]
>> Signed-off-by: Peter Hurley <[email protected]>
>
> I don't understand the second half of the changelog, it doesn't seem
> to fit here: there deadlock that we are trying to avoid here happens
> when the *same* tty needs the lock to complete the function that
> sends the pending data. I don't think we do still do that any more,
> but it doesn't seem related to the tty lock being system-wide or not.
The tty lock is not used in the i/o path; it's purpose is to
mutually exclude state changes in open(), close() and hangup().
The commit that added this [1] comments that _other_ ttys may wait
for this tty to complete, and comments in the code note that this
function should be removed when the system-wide tty mutex was removed
(which happened with the commit noted in the changelog).
Regards,
Peter Hurley
[1]
commit a57a7bf3fc7eff00f07eb9c805774d911a3f2472
Author: Jiri Slaby <[email protected]>
Date: Thu Aug 25 15:12:06 2011 +0200
TTY: define tty_wait_until_sent_from_close
We need this helper to fix system stalls. The issue is that the rest
of the system TTYs wait for us to finish waiting. This wasn't an issue
with BKL. BKL used to unlock implicitly.
This is based on the Arnd suggestion.
Signed-off-by: Jiri Slaby <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
From: Peter Hurley
...
> > I don't understand the second half of the changelog, it doesn't seem
> > to fit here: there deadlock that we are trying to avoid here happens
> > when the *same* tty needs the lock to complete the function that
> > sends the pending data. I don't think we do still do that any more,
> > but it doesn't seem related to the tty lock being system-wide or not.
>
> The tty lock is not used in the i/o path; it's purpose is to
> mutually exclude state changes in open(), close() and hangup().
>
> The commit that added this [1] comments that _other_ ttys may wait
> for this tty to complete, and comments in the code note that this
> function should be removed when the system-wide tty mutex was removed
> (which happened with the commit noted in the changelog).
What happens if another process tries to do a non-blocking open
while you are sleeping in close waiting for output to drain?
Hopefully this returns before that data has drained.
David
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tuesday 17 June 2014 11:03:50 David Laight wrote:
> From: Peter Hurley
> ...
> > > I don't understand the second half of the changelog, it doesn't seem
> > > to fit here: there deadlock that we are trying to avoid here happens
> > > when the *same* tty needs the lock to complete the function that
> > > sends the pending data. I don't think we do still do that any more,
> > > but it doesn't seem related to the tty lock being system-wide or not.
> >
> > The tty lock is not used in the i/o path; it's purpose is to
> > mutually exclude state changes in open(), close() and hangup().
> >
> > The commit that added this [1] comments that _other_ ttys may wait
> > for this tty to complete, and comments in the code note that this
> > function should be removed when the system-wide tty mutex was removed
> > (which happened with the commit noted in the changelog).
>
> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
>
> Hopefully this returns before that data has drained.
Before the patch, I believe tty_reopen() would return -EIO because
the TTY_CLOSING flag is set. After the patch, tty_open() blocks
on tty_lock() before calling tty_reopen(). AFAICT, this is independent
of O_NONBLOCK.
Arnd
On 06/17/2014 07:03 AM, David Laight wrote:
> From: Peter Hurley
> ...
>>> I don't understand the second half of the changelog, it doesn't seem
>>> to fit here: there deadlock that we are trying to avoid here happens
>>> when the *same* tty needs the lock to complete the function that
>>> sends the pending data. I don't think we do still do that any more,
>>> but it doesn't seem related to the tty lock being system-wide or not.
>>
>> The tty lock is not used in the i/o path; it's purpose is to
>> mutually exclude state changes in open(), close() and hangup().
>>
>> The commit that added this [1] comments that _other_ ttys may wait
>> for this tty to complete, and comments in the code note that this
>> function should be removed when the system-wide tty mutex was removed
>> (which happened with the commit noted in the changelog).
>
> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
>
> Hopefully this returns before that data has drained.
Good point.
tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.
Regards,
Peter Hurley
> Before the patch, I believe tty_reopen() would return -EIO because
> the TTY_CLOSING flag is set. After the patch, tty_open() blocks
> on tty_lock() before calling tty_reopen(). AFAICT, this is independent
> of O_NONBLOCK.
That would be a bug then. Returning -EIO is fine (if unfriendly). The
O_NONBLOCK can't block in this case though because the port could take a
long time to give up trying to dribble its bits (up to 30 seconds or so)
Alan
On Mon, 16 Jun 2014 09:17:12 -0400
Peter Hurley <[email protected]> wrote:
> ASYNC_CLOSING is no longer used in the tty core; use private flag
> info->closing as substitute.
>
> CC: Karsten Keil <[email protected]>
> CC: [email protected]
> Signed-off-by: Peter Hurley <[email protected]>
> ---
> drivers/isdn/i4l/isdn_tty.c | 14 +++++++-------
> include/linux/isdn.h | 1 +
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/isdn/i4l/isdn_tty.c b/drivers/isdn/i4l/isdn_tty.c
> index 732f68a..5310932 100644
> --- a/drivers/isdn/i4l/isdn_tty.c
> +++ b/drivers/isdn/i4l/isdn_tty.c
> @@ -1577,8 +1577,7 @@ isdn_tty_close(struct tty_struct *tty, struct file *filp)
> #endif
> return;
> }
> - port->flags |= ASYNC_CLOSING;
> -
> + info->closing = 1;
This is not sane C because
> + int closing:1; /* port count has dropped to 0 */
has the values 0 and -1.
Using a bool would let the compiler figure out what it wanted to do and
do the right thing. It'll probably generate identical code for most
processors but it gives it the freedom to do better.
Alan
On 06/17/2014 07:32 AM, Peter Hurley wrote:
> On 06/17/2014 07:03 AM, David Laight wrote:
>> From: Peter Hurley
>> ...
>>>> I don't understand the second half of the changelog, it doesn't seem
>>>> to fit here: there deadlock that we are trying to avoid here happens
>>>> when the *same* tty needs the lock to complete the function that
>>>> sends the pending data. I don't think we do still do that any more,
>>>> but it doesn't seem related to the tty lock being system-wide or not.
>>>
>>> The tty lock is not used in the i/o path; it's purpose is to
>>> mutually exclude state changes in open(), close() and hangup().
>>>
>>> The commit that added this [1] comments that _other_ ttys may wait
>>> for this tty to complete, and comments in the code note that this
>>> function should be removed when the system-wide tty mutex was removed
>>> (which happened with the commit noted in the changelog).
>>
>> What happens if another process tries to do a non-blocking open
>> while you are sleeping in close waiting for output to drain?
>>
>> Hopefully this returns before that data has drained.
>
> Good point.
>
> tty_open() should be trylocking both mutexes anyway in O_NONBLOCK.
Further, the tty lock should not be nested within the tty_mutex lock
in a reopen, regardless of O_NONBLOCK.
AFAICT, the tty_mutex in the reopen scenario is only protecting the
tty count bump of the linked tty (if the tty is a pty).
I think with some refactoring and returning with a tty reference held
from both tty_open_current_tty() and tty_driver_lookup_tty(), the tty
lock in tty_open() can be attempted without nesting in the tty_mutex.
Regardless, I'll be splitting this series and I'll be sure to cc
you all when I resubmit these changes (after testing).
Regards,
Peter Hurley
On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
> tty_wait_until_sent_from_close() drops the tty lock while waiting
> for the tty driver to finish sending previously accepted data (ie.,
> data remaining in its write buffer and transmit fifo).
>
> However, dropping the tty lock is a hold-over from when the tty
> lock was system-wide; ie., one lock for all ttys.
>
> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
> 'tty: localise the lock', dropping the tty lock has not been necessary.
>
> CC: Karsten Keil <[email protected]>
> CC: [email protected]
> Signed-off-by: Peter Hurley <[email protected]>
> ---
> drivers/isdn/i4l/isdn_tty.c | 2 +-
> drivers/tty/hvc/hvc_console.c | 2 +-
> drivers/tty/hvc/hvcs.c | 2 +-
> drivers/tty/tty_port.c | 11 ++---------
> include/linux/tty.h | 18 ------------------
> 5 files changed, 5 insertions(+), 30 deletions(-)
I've applied the first 13 patches in this series, as it looks like you
were going to split things up from here, right? Can you refresh these
and resend when you have that done?
thanks,
greg k-h
On 07/10/2014 07:09 PM, Greg Kroah-Hartman wrote:
> On Mon, Jun 16, 2014 at 09:17:11AM -0400, Peter Hurley wrote:
>> tty_wait_until_sent_from_close() drops the tty lock while waiting
>> for the tty driver to finish sending previously accepted data (ie.,
>> data remaining in its write buffer and transmit fifo).
>>
>> However, dropping the tty lock is a hold-over from when the tty
>> lock was system-wide; ie., one lock for all ttys.
>>
>> Since commit 89c8d91e31f267703e365593f6bfebb9f6d2ad01,
>> 'tty: localise the lock', dropping the tty lock has not been necessary.
>>
>> CC: Karsten Keil <[email protected]>
>> CC: [email protected]
>> Signed-off-by: Peter Hurley <[email protected]>
>> ---
>> drivers/isdn/i4l/isdn_tty.c | 2 +-
>> drivers/tty/hvc/hvc_console.c | 2 +-
>> drivers/tty/hvc/hvcs.c | 2 +-
>> drivers/tty/tty_port.c | 11 ++---------
>> include/linux/tty.h | 18 ------------------
>> 5 files changed, 5 insertions(+), 30 deletions(-)
>
> I've applied the first 13 patches in this series, as it looks like you
> were going to split things up from here, right?
Yes, thanks for doing that.
> Can you refresh these and resend when you have that done?
Unfortunately, that probably won't be until after the 3.17 merge window,
for 3.18. The tty_open() rework is not trivial and there is an issue
with the ldisc flush removal patch.
I'm hoping to include the tty flow control fixes with that stuff as well.
Regards,
Peter Hurley
On 06/16/2014 09:17 AM, Peter Hurley wrote:
> The tty_ldisc_flush() after port hardware shutdown is unnecessary;
> the ldisc flush was just performed before the hardware shutdown
> in tty_port_close_start() and the ldisc will be released when
> uart_close() returns (because the last port close implies the
> last tty close).
>
> Signed-off-by: Peter Hurley <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 212ee07..15212d7 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -1351,9 +1351,6 @@ static void uart_close(struct tty_struct *tty, struct file *filp)
>
> mutex_lock(&port->mutex);
> uart_shutdown(tty, state);
> -
> - tty_ldisc_flush(tty);
> -
This isn't exactly correct. There is a small window of time between the
ldisc flush being performed in tty_port_close_start() and the
subsequent stop_rx() in uart_close(). This might allow for data to be
received, and a racing tty reopen to receive that stale data. Unlikely, but
possible.
> tty_port_tty_set(port, NULL);
> tty->closing = 0;
> spin_lock_irqsave(&port->lock, flags);
>
On 06/17/2014 07:03 AM, David Laight wrote:
> From: Peter Hurley
> ...
>>> I don't understand the second half of the changelog, it doesn't seem
>>> to fit here: there deadlock that we are trying to avoid here happens
>>> when the *same* tty needs the lock to complete the function that
>>> sends the pending data. I don't think we do still do that any more,
>>> but it doesn't seem related to the tty lock being system-wide or not.
>>
>> The tty lock is not used in the i/o path; it's purpose is to
>> mutually exclude state changes in open(), close() and hangup().
>>
>> The commit that added this [1] comments that _other_ ttys may wait
>> for this tty to complete, and comments in the code note that this
>> function should be removed when the system-wide tty mutex was removed
>> (which happened with the commit noted in the changelog).
I just wanted to revisit this discussion briefly so I can clarify the
situation regarding holding the tty lock while closing, and how that
affects parallel opens.
I've unnested the tty lock from the tty mutex (which I'm still testing)
but will be submitting after the merge window re-opens for 3.19. So this
is more relevant now.
The original patch that led to this thread is here:
https://lkml.org/lkml/2014/6/16/306
> What happens if another process tries to do a non-blocking open
> while you are sleeping in close waiting for output to drain?
>
> Hopefully this returns before that data has drained.
Current mainline blocks on _any_ racing re-open while this lock is
dropped in tty_wait_until_sent_from_close(); blocking while
ASYNC_CLOSING has been in mainline since at least 2.6.29 and that just
merged existing code together. See tty_port_block_til_ready(); note
the test for O_NONBLOCK is after the wait while ASYNC_CLOSING.
IOW, currently a non-blocking open will sleep for the _entire_ duration
of a parallel hardware shutdown, and when it wakes, the error return will
cause a release of its tty, and it will restart with a fresh attempt
to open. Same with a blocking open that is already waiting; when its
woken the hardware shutdown has already completed so ASYNC_INITIALIZED
is cleared, which forces a release and restart too.
The point being that holding the tty lock across the _entire_ close
is equivalent to the current outcome, regardless of O_NONBLOCK.
I'm reluctant to start returning EGAIN for non-blocking tty opens
because no tty driver does that now, and I don't think userspace will
deal well with new return codes from tty opens.
Regards,
Peter Hurley
> The point being that holding the tty lock across the _entire_ close
> is equivalent to the current outcome, regardless of O_NONBLOCK.
>
> I'm reluctant to start returning EGAIN for non-blocking tty opens
> because no tty driver does that now, and I don't think userspace will
> deal well with new return codes from tty opens.
I do not know about the non blocking case mattering. The blocking open
does need to wait, when I broke that case before I broke the console
login drivers (mingetty).
Returning EAGAIN would also only work if poll/select did the right thing.
Currently Linux can't support a System5 style ttymon process because of
this limitation, which means, for example, that systemd can't implement a
single thread to manage all console prompts/setup
Alan