[PATCH 0/4] tty_port use for isicom

[PATCH 0/4] tty_port use for isicom

[Alan Cox]

Pending patch proposal plus the isicom fix on the end. Could do with a good
review Jiri.

Signed-off-by: Alan Cox <alan@linux.intel.com>

---

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 ?"

[PATCH 1/4] tty_port: add "tty_port_open" helper

[Alan Cox]

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 <alan@linux.intel.com>
---

 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;

[PATCH 2/4] isicom: switch to the new tty_port_open helper

[Alan Cox]

Trivial conversion in this case so might as well do it while testing the
port_open design is right

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 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,

[PATCH 3/4] isicom: sort out the board init logic

[Alan Cox]

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 <alan@linux.intel.com>
---

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

[PATCH 4/4] isicom: fix deadlock on shutdown

[Alan Cox]

Alexander Strakh <strakh@ispras.ru> 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 <alan@linux.intel.com>
---

 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)

Re: [PATCH 2/4] isicom: switch to the new tty_port_open helper

[Jiri Slaby]

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.

Re: [PATCH 2/4] isicom: switch to the new tty_port_open helper

[Alan Cox]

On Wed, 07 Oct 2009 22:11:01 +0200
Jiri Slaby <jirislaby@gmail.com> 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.

Re: [PATCH 2/4] isicom: switch to the new tty_port_open helper

[Alan Cox]

On Wed, 07 Oct 2009 22:11:01 +0200
Jiri Slaby <jirislaby@gmail.com> 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 ?

Re: [PATCH 2/4] isicom: switch to the new tty_port_open helper

[Jiri Slaby]

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.