Pending patch proposal plus the isicom fix on the end. Could do with a good
review Jiri.
Signed-off-by: Alan Cox <[email protected]>
---
Alan Cox (4):
isicom: fix deadlock on shutdown
isicom: sort out the board init logic
isicom: switch to the new tty_port_open helper
tty_port: add "tty_port_open" helper
--
Two fish in a tank: One says to the other "Can you drive this ?"
For the moment this just moves the USB logic over and fixes the 'what if
we open and hangup at the same time' race noticed by Oliver Neukum.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/tty_port.c | 36 ++++++++++++++++++++++++++++-
drivers/usb/serial/usb-serial.c | 49 ++++++++++++++++-----------------------
include/linux/tty.h | 10 +++++++-
3 files changed, 64 insertions(+), 31 deletions(-)
diff --git a/drivers/char/tty_port.c b/drivers/char/tty_port.c
index a4bbb28..2512262 100644
--- a/drivers/char/tty_port.c
+++ b/drivers/char/tty_port.c
@@ -99,10 +99,11 @@ EXPORT_SYMBOL(tty_port_tty_set);
static void tty_port_shutdown(struct tty_port *port)
{
+ mutex_lock(&port->mutex);
if (port->ops->shutdown &&
test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags))
port->ops->shutdown(port);
-
+ mutex_unlock(&port->mutex);
}
/**
@@ -375,3 +376,36 @@ void tty_port_close(struct tty_port *port, struct tty_struct *tty,
tty_port_tty_set(port, NULL);
}
EXPORT_SYMBOL(tty_port_close);
+
+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;
+ spin_unlock_irq(&port->lock);
+ tty_port_tty_set(port, tty);
+
+ /*
+ * Do the device-specific open only if the hardware isn't
+ * already initialized. Serialize open and shutdown using the
+ * port mutex.
+ */
+
+ mutex_lock(&port->mutex);
+
+ if (!test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+ if (port->ops->activate) {
+ int retval = port->ops->activate(port, tty);
+ if (retval) {
+ mutex_unlock(&port->mutex);
+ return retval;
+ }
+ }
+ set_bit(ASYNCB_INITIALIZED, &port->flags);
+ }
+ mutex_unlock(&port->mutex);
+ return tty_port_block_til_ready(port, tty, filp);
+}
+
+EXPORT_SYMBOL(tty_port_open);
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index aa6b2ae..95c34da 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -246,41 +246,31 @@ static int serial_install(struct tty_driver *driver, struct tty_struct *tty)
return retval;
}
-static int serial_open(struct tty_struct *tty, struct file *filp)
+static int serial_activate(struct tty_port *tport, struct tty_struct *tty)
{
- struct usb_serial_port *port = tty->driver_data;
+ struct usb_serial_port *port =
+ container_of(tport, struct usb_serial_port, port);
struct usb_serial *serial = port->serial;
int retval;
- dbg("%s - port %d", __func__, port->number);
-
- spin_lock_irq(&port->port.lock);
- if (!tty_hung_up_p(filp))
- ++port->port.count;
- spin_unlock_irq(&port->port.lock);
- tty_port_tty_set(&port->port, tty);
+ if (mutex_lock_interruptible(&port->mutex))
+ return -ERESTARTSYS;
+ mutex_lock(&serial->disc_mutex);
+ if (serial->disconnected)
+ retval = -ENODEV;
+ else
+ retval = port->serial->type->open(tty, port);
+ mutex_unlock(&serial->disc_mutex);
+ mutex_unlock(&port->mutex);
+ return retval;
+}
- /* Do the device-specific open only if the hardware isn't
- * already initialized.
- */
- if (!test_bit(ASYNCB_INITIALIZED, &port->port.flags)) {
- if (mutex_lock_interruptible(&port->mutex))
- return -ERESTARTSYS;
- mutex_lock(&serial->disc_mutex);
- if (serial->disconnected)
- retval = -ENODEV;
- else
- retval = port->serial->type->open(tty, port);
- mutex_unlock(&serial->disc_mutex);
- mutex_unlock(&port->mutex);
- if (retval)
- return retval;
- set_bit(ASYNCB_INITIALIZED, &port->port.flags);
- }
+static int serial_open(struct tty_struct *tty, struct file *filp)
+{
+ struct usb_serial_port *port = tty->driver_data;
- /* Now do the correct tty layer semantics */
- retval = tty_port_block_til_ready(&port->port, tty, filp);
- return retval;
+ dbg("%s - port %d", __func__, port->number);
+ return tty_port_open(&port->port, tty, filp);
}
/**
@@ -724,6 +714,7 @@ static void serial_dtr_rts(struct tty_port *port, int on)
static const struct tty_port_operations serial_port_ops = {
.carrier_raised = serial_carrier_raised,
.dtr_rts = serial_dtr_rts,
+ .activate = serial_activate,
};
int usb_serial_probe(struct usb_interface *interface,
diff --git a/include/linux/tty.h b/include/linux/tty.h
index ed24493..262c5da 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -191,9 +191,15 @@ struct tty_port_operations {
/* Control the DTR line */
void (*dtr_rts)(struct tty_port *port, int raise);
/* Called when the last close completes or a hangup finishes
- IFF the port was initialized. Do not use to free resources */
+ IFF the port was initialized. Do not use to free resources. Called
+ under the port mutex to serialize against activate/shutdowns */
void (*shutdown)(struct tty_port *port);
void (*drop)(struct tty_port *port);
+ /* Called under the port mutex from tty_port_open, serialized using
+ the port mutex */
+ /* FIXME: long term getting the tty argument *out* of this would be
+ good for consoles */
+ int (*activate)(struct tty_port *port, struct tty_struct *tty);
};
struct tty_port {
@@ -468,6 +474,8 @@ extern int tty_port_close_start(struct tty_port *port,
extern void tty_port_close_end(struct tty_port *port, struct tty_struct *tty);
extern void tty_port_close(struct tty_port *port,
struct tty_struct *tty, struct file *filp);
+extern int tty_port_open(struct tty_port *port,
+ struct tty_struct *tty, struct file *filp);
extern inline int tty_port_users(struct tty_port *port)
{
return port->count + port->blocked_open;
Trivial conversion in this case so might as well do it while testing the
port_open design is right
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/isicom.c | 88 ++++++++++++-------------------------------------
1 files changed, 21 insertions(+), 67 deletions(-)
diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index 426bfdd..e7be3ec 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -804,24 +804,21 @@ static inline void isicom_setup_board(struct isi_board *bp)
bp->status |= BOARD_ACTIVE;
for (channel = 0; channel < bp->port_count; channel++, port++)
drop_dtr_rts(port);
+ bp->count++;
spin_unlock_irqrestore(&bp->card_lock, flags);
}
-static int isicom_setup_port(struct tty_struct *tty)
+static int isicom_activate(struct tty_port *tport, struct tty_struct *tty)
{
- struct isi_port *port = tty->driver_data;
+ struct isi_port *port = container_of(tport, struct isi_port, port);
struct isi_board *card = port->card;
unsigned long flags;
- if (port->port.flags & ASYNC_INITIALIZED)
- return 0;
- if (tty_port_alloc_xmit_buf(&port->port) < 0)
+ if (tty_port_alloc_xmit_buf(tport) < 0)
return -ENOMEM;
spin_lock_irqsave(&card->card_lock, flags);
- clear_bit(TTY_IO_ERROR, &tty->flags);
- if (port->port.count == 1)
- card->count++;
+ isicom_setup_board(card);
port->xmit_cnt = port->xmit_head = port->xmit_tail = 0;
@@ -832,9 +829,7 @@ static int isicom_setup_port(struct tty_struct *tty)
outw(((ISICOM_KILLTX | ISICOM_KILLRX) << 8) | 0x06, card->base);
InterruptTheCard(card->base);
}
-
isicom_config_port(tty);
- port->port.flags |= ASYNC_INITIALIZED;
spin_unlock_irqrestore(&card->card_lock, flags);
return 0;
@@ -871,31 +866,20 @@ static struct tty_port *isicom_find_port(struct tty_struct *tty)
return &port->port;
}
-
+
static int isicom_open(struct tty_struct *tty, struct file *filp)
{
struct isi_port *port;
struct isi_board *card;
struct tty_port *tport;
- int error = 0;
tport = isicom_find_port(tty);
if (tport == NULL)
return -ENODEV;
port = container_of(tport, struct isi_port, port);
card = &isi_card[BOARD(tty->index)];
- isicom_setup_board(card);
- /* FIXME: locking on port.count etc */
- port->port.count++;
- tty->driver_data = port;
- tty_port_tty_set(&port->port, tty);
- /* FIXME: Locking on Initialized flag */
- if (!test_bit(ASYNCB_INITIALIZED, &tport->flags))
- error = isicom_setup_port(tty);
- if (error == 0)
- error = tty_port_block_til_ready(&port->port, tty, filp);
- return error;
+ return tty_port_open(tport, tty, filp);
}
/* close et all */
@@ -914,40 +898,21 @@ static void isicom_shutdown_port(struct isi_port *port)
tty = tty_port_tty_get(&port->port);
- if (!(port->port.flags & ASYNC_INITIALIZED)) {
- tty_kref_put(tty);
- return;
- }
-
tty_port_free_xmit_buf(&port->port);
- port->port.flags &= ~ASYNC_INITIALIZED;
- /* 3rd October 2000 : Vinayak P Risbud */
- tty_port_tty_set(&port->port, NULL);
-
- /*Fix done by Anil .S on 30-04-2001
- remote login through isi port has dtr toggle problem
- due to which the carrier drops before the password prompt
- appears on the remote end. Now we drop the dtr only if the
- HUPCL(Hangup on close) flag is set for the tty*/
-
- if (C_HUPCL(tty))
- /* drop dtr on this port */
- drop_dtr(port);
-
- /* any other port uninits */
- if (tty)
- set_bit(TTY_IO_ERROR, &tty->flags);
-
if (--card->count < 0) {
pr_dbg("isicom_shutdown_port: bad board(0x%lx) count %d.\n",
card->base, card->count);
card->count = 0;
}
- /* last port was closed, shutdown that boad too */
- if (C_HUPCL(tty)) {
- if (!card->count)
- isicom_shutdown_board(card);
+ /* last port was closed, shutdown that board too */
+ if (tty && C_HUPCL(tty)) {
+ /* FIXME: this logic is bogus - it's the old logic that was
+ bogus before but it still wants fixing */
+ if (!card->count) {
+ if (card->status & BOARD_ACTIVE)
+ card->status &= ~BOARD_ACTIVE;
+ }
}
tty_kref_put(tty);
}
@@ -968,7 +933,7 @@ static void isicom_flush_buffer(struct tty_struct *tty)
tty_wakeup(tty);
}
-static void isicom_close_port(struct tty_port *port)
+static void isicom_shutdown(struct tty_port *port)
{
struct isi_port *ip = container_of(port, struct isi_port, port);
struct isi_board *card = ip->card;
@@ -977,10 +942,8 @@ static void isicom_close_port(struct tty_port *port)
/* indicate to the card that no more data can be received
on this port */
spin_lock_irqsave(&card->card_lock, flags);
- if (port->flags & ASYNC_INITIALIZED) {
- card->port_status &= ~(1 << ip->channel);
- outw(card->port_status, card->base + 0x02);
- }
+ card->port_status &= ~(1 << ip->channel);
+ outw(card->port_status, card->base + 0x02);
isicom_shutdown_port(ip);
spin_unlock_irqrestore(&card->card_lock, flags);
}
@@ -991,12 +954,7 @@ static void isicom_close(struct tty_struct *tty, struct file *filp)
struct tty_port *port = &ip->port;
if (isicom_paranoia_check(ip, tty->name, "isicom_close"))
return;
-
- if (tty_port_close_start(port, tty, filp) == 0)
- return;
- isicom_close_port(port);
- isicom_flush_buffer(tty);
- tty_port_close_end(port, tty);
+ tty_port_close(port, tty, filp);
}
/* write et all */
@@ -1326,15 +1284,9 @@ static void isicom_start(struct tty_struct *tty)
static void isicom_hangup(struct tty_struct *tty)
{
struct isi_port *port = tty->driver_data;
- unsigned long flags;
if (isicom_paranoia_check(port, tty->name, "isicom_hangup"))
return;
-
- spin_lock_irqsave(&port->card->card_lock, flags);
- isicom_shutdown_port(port);
- spin_unlock_irqrestore(&port->card->card_lock, flags);
-
tty_port_hangup(&port->port);
}
@@ -1367,6 +1319,8 @@ static const struct tty_operations isicom_ops = {
static const struct tty_port_operations isicom_port_ops = {
.carrier_raised = isicom_carrier_raised,
.dtr_rts = isicom_dtr_rts,
+ .activate = isicom_activate,
+ .shutdown = isicom_shutdown,
};
static int __devinit reset_card(struct pci_dev *pdev,
Split this into two flags - INIT meaning the board is set up and ACTIVE
meaning the board has ports open. Remove the broken HUPCL casing and push
the counts somewhere sensible.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/isicom.c | 41 +++++++++++------------------------------
include/linux/isicom.h | 1 +
2 files changed, 12 insertions(+), 30 deletions(-)
diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index e7be3ec..1e91c30 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -793,21 +793,19 @@ static inline void isicom_setup_board(struct isi_board *bp)
{
int channel;
struct isi_port *port;
- unsigned long flags;
- spin_lock_irqsave(&bp->card_lock, flags);
- if (bp->status & BOARD_ACTIVE) {
- spin_unlock_irqrestore(&bp->card_lock, flags);
- return;
- }
- port = bp->ports;
- bp->status |= BOARD_ACTIVE;
- for (channel = 0; channel < bp->port_count; channel++, port++)
- drop_dtr_rts(port);
bp->count++;
- spin_unlock_irqrestore(&bp->card_lock, flags);
+ if (!(bp->status & BOARD_INIT)) {
+ port = bp->ports;
+ for (channel = 0; channel < bp->port_count; channel++, port++)
+ drop_dtr_rts(port);
+ }
+ bp->status |= BOARD_ACTIVE | BOARD_INIT;
}
+/* Activate and thus setup board are protected from races against shutdown
+ by the tty_port mutex */
+
static int isicom_activate(struct tty_port *tport, struct tty_struct *tty)
{
struct isi_port *port = container_of(tport, struct isi_port, port);
@@ -884,19 +882,10 @@ static int isicom_open(struct tty_struct *tty, struct file *filp)
/* close et all */
-static inline void isicom_shutdown_board(struct isi_board *bp)
-{
- if (bp->status & BOARD_ACTIVE)
- bp->status &= ~BOARD_ACTIVE;
-}
-
/* card->lock HAS to be held */
static void isicom_shutdown_port(struct isi_port *port)
{
struct isi_board *card = port->card;
- struct tty_struct *tty;
-
- tty = tty_port_tty_get(&port->port);
tty_port_free_xmit_buf(&port->port);
if (--card->count < 0) {
@@ -904,17 +893,9 @@ static void isicom_shutdown_port(struct isi_port *port)
card->base, card->count);
card->count = 0;
}
-
/* last port was closed, shutdown that board too */
- if (tty && C_HUPCL(tty)) {
- /* FIXME: this logic is bogus - it's the old logic that was
- bogus before but it still wants fixing */
- if (!card->count) {
- if (card->status & BOARD_ACTIVE)
- card->status &= ~BOARD_ACTIVE;
- }
- }
- tty_kref_put(tty);
+ if (!card->count)
+ card->status &= BOARD_ACTIVE;
}
static void isicom_flush_buffer(struct tty_struct *tty)
diff --git a/include/linux/isicom.h b/include/linux/isicom.h
index bbd4219..b92e056 100644
--- a/include/linux/isicom.h
+++ b/include/linux/isicom.h
@@ -67,6 +67,7 @@
#define FIRMWARE_LOADED 0x0001
#define BOARD_ACTIVE 0x0002
+#define BOARD_INIT 0x0004
/* isi_port status bitmap */
Alexander Strakh <[email protected]> reported
KERNEL_VERSION: 2.6.31
DESCRIBE:
Driver drivers/char/isicom.c might sleep in atomic context, because it
calls
tty_port_xmit_buf under spin_lock.
./drivers/char/isicom.c:
1307 static void isicom_hangup(struct tty_struct *tty)
1308 {
...
1315 spin_lock_irqsave(&port->card->card_lock, flags);
1316 isicom_shutdown_port(port);
...
Path to might_sleep macro from isicom_hangup:
1. isicom_hangup calls spin_lock_irqsave (drivers/char/isicom.c:1315) and
then
calls isicom_shutdown_port.
2. isiscom_shutdown_port calls tty_port_free_xmit_buf at
drivers/char/isicom.c:906
3. tty_port_free_xmit_buf calls mutex_lock at drivers/char/tty_port:48
Found by Linux Driver Verification Project.
Signed-off-by: Alan Cox <[email protected]>
---
drivers/char/isicom.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/char/isicom.c b/drivers/char/isicom.c
index 1e91c30..300d5bd 100644
--- a/drivers/char/isicom.c
+++ b/drivers/char/isicom.c
@@ -887,7 +887,6 @@ static void isicom_shutdown_port(struct isi_port *port)
{
struct isi_board *card = port->card;
- tty_port_free_xmit_buf(&port->port);
if (--card->count < 0) {
pr_dbg("isicom_shutdown_port: bad board(0x%lx) count %d.\n",
card->base, card->count);
@@ -927,6 +926,7 @@ static void isicom_shutdown(struct tty_port *port)
outw(card->port_status, card->base + 0x02);
isicom_shutdown_port(ip);
spin_unlock_irqrestore(&card->card_lock, flags);
+ tty_port_free_xmit_buf(port);
}
static void isicom_close(struct tty_struct *tty, struct file *filp)
On 10/07/2009 04:46 PM, Alan Cox wrote:
> - if (port->port.flags & ASYNC_INITIALIZED)
> - return 0;
> - if (tty_port_alloc_xmit_buf(&port->port) < 0)
> + if (tty_port_alloc_xmit_buf(tport) < 0)
I think this won't work well with 1/4. ->activate is called with
port->port.mutex (the name in this context), but tty_port_alloc_xmit_buf
wants it again.
On Wed, 07 Oct 2009 22:11:01 +0200
Jiri Slaby <[email protected]> wrote:
> On 10/07/2009 04:46 PM, Alan Cox wrote:
> > - if (port->port.flags & ASYNC_INITIALIZED)
> > - return 0;
> > - if (tty_port_alloc_xmit_buf(&port->port) < 0)
> > + if (tty_port_alloc_xmit_buf(tport) < 0)
>
> I think this won't work well with 1/4. ->activate is called with
> port->port.mutex (the name in this context), but tty_port_alloc_xmit_buf
> wants it again.
Indeed - so the port buffer allocation stuff wants revisiting. I'll do
that tomorrow.
On Wed, 07 Oct 2009 22:11:01 +0200
Jiri Slaby <[email protected]> wrote:
> On 10/07/2009 04:46 PM, Alan Cox wrote:
> > - if (port->port.flags & ASYNC_INITIALIZED)
> > - return 0;
> > - if (tty_port_alloc_xmit_buf(&port->port) < 0)
> > + if (tty_port_alloc_xmit_buf(tport) < 0)
>
> I think this won't work well with 1/4. ->activate is called with
> port->port.mutex (the name in this context), but tty_port_alloc_xmit_buf
> wants it again.
Ok fixed that the simple way the buffer allocator now has its own private
mutex. Any other problems strike you ?
On 10/08/2009 03:13 PM, Alan Cox wrote:
> Ok fixed that the simple way the buffer allocator now has its own private
> mutex. Any other problems strike you ?
I don't see any. It looks good. Thanks.