[PATCH 0/5] tty_port_open

[PATCH 0/5] tty_port_open

[Alan Cox]

Add a tty_port_open method and then propogate the effects through the USB
drivers, which nicely fixes a couple of races and cleans up the code.
---

Alan Cox (5):
      opticon: Fix resume logic
      usb_serial: Kill port mutex
      usb_serial: Use the shutdown() operation
      tty_port: coding style cleaning pass
      tty_port: add "tty_port_open" helper


 drivers/char/tty_port.c         |   50 ++++++++++++++++++++---
 drivers/usb/serial/opticon.c    |    7 ++-
 drivers/usb/serial/usb-serial.c |   83 +++++++++++++--------------------------
 include/linux/tty.h             |   10 ++++-
 include/linux/usb/serial.h      |    3 -
 5 files changed, 83 insertions(+), 70 deletions(-)

-- 
The Zenburger: One with everything

[PATCH 1/5] 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/5] tty_port: coding style cleaning pass

[Alan Cox]

Mind the hoover wire...

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

 drivers/char/tty_port.c |   28 ++++++++++++++--------------
 1 files changed, 14 insertions(+), 14 deletions(-)


diff --git a/drivers/char/tty_port.c b/drivers/char/tty_port.c
index 2512262..c16ba2d 100644
--- a/drivers/char/tty_port.c
+++ b/drivers/char/tty_port.c
@@ -199,7 +199,7 @@ EXPORT_SYMBOL(tty_port_lower_dtr_rts);
  *	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.
  */
- 
+
 int tty_port_block_til_ready(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
@@ -248,7 +248,8 @@ int tty_port_block_til_ready(struct tty_port *port,
 			tty_port_raise_dtr_rts(port);
 
 		prepare_to_wait(&port->open_wait, &wait, TASK_INTERRUPTIBLE);
-		/* Check for a hangup or uninitialised port. Return accordingly */
+		/* Check for a hangup or uninitialised port.
+							Return accordingly */
 		if (tty_hung_up_p(filp) || !(port->flags & ASYNC_INITIALIZED)) {
 			if (port->flags & ASYNC_HUP_NOTIFY)
 				retval = -EAGAIN;
@@ -280,11 +281,11 @@ int tty_port_block_til_ready(struct tty_port *port,
 		port->flags |= ASYNC_NORMAL_ACTIVE;
 	spin_unlock_irqrestore(&port->lock, flags);
 	return retval;
-	
 }
 EXPORT_SYMBOL(tty_port_block_til_ready);
 
-int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct file *filp)
+int tty_port_close_start(struct tty_port *port,
+				struct tty_struct *tty, struct file *filp)
 {
 	unsigned long flags;
 
@@ -294,7 +295,7 @@ int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct f
 		return 0;
 	}
 
-	if( tty->count == 1 && port->count != 1) {
+	if (tty->count == 1 && port->count != 1) {
 		printk(KERN_WARNING
 		    "tty_port_close_start: tty->count = 1 port count = %d.\n",
 								port->count);
@@ -326,8 +327,8 @@ int tty_port_close_start(struct tty_port *port, struct tty_struct *tty, struct f
 		long timeout;
 
 		if (bps > 1200)
-			timeout = max_t(long, (HZ * 10 * port->drain_delay) / bps,
-								HZ / 10);
+			timeout = max_t(long,
+				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
 		else
 			timeout = 2 * HZ;
 		schedule_timeout_interruptible(timeout);
@@ -378,7 +379,7 @@ void tty_port_close(struct tty_port *port, struct tty_struct *tty,
 EXPORT_SYMBOL(tty_port_close);
 
 int tty_port_open(struct tty_port *port, struct tty_struct *tty,
-                                                        struct file *filp)
+							struct file *filp)
 {
 	spin_lock_irq(&port->lock);
 	if (!tty_hung_up_p(filp))
@@ -398,14 +399,13 @@ int tty_port_open(struct tty_port *port, struct tty_struct *tty,
 		if (port->ops->activate) {
 			int retval = port->ops->activate(port, tty);
 			if (retval) {
-        		        mutex_unlock(&port->mutex);
-        			return 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);

[PATCH 3/5] usb_serial: Use the shutdown() operation

[Alan Cox]

As Alan Stern pointed out - now we have tty_port_open the shutdown method
and locking allow us to whack the other bits into the full helper methods
and provide a shutdown op which the tty port code will synchronize with 
setup for us.

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

 drivers/usb/serial/usb-serial.c |   39 +++++++++++----------------------------
 1 files changed, 11 insertions(+), 28 deletions(-)


diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 95c34da..e189338 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -253,15 +253,12 @@ static int serial_activate(struct tty_port *tport, struct tty_struct *tty)
 	struct usb_serial *serial = port->serial;
 	int retval;
 
-	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;
 }
 
@@ -275,57 +272,40 @@ static int serial_open(struct tty_struct *tty, struct file *filp)
 
 /**
  * serial_down - shut down hardware
- * @port: port to shut down
+ * @tport: tty port to shut down
  *
  * Shut down a USB serial port unless it is the console.  We never
- * shut down the console hardware as it will always be in use.
+ * shut down the console hardware as it will always be in use. Serialized
+ * against activate by the tport mutex and kept to matching open/close pairs
+ * of calls by the ASYNCB_INITIALIZED flag.
  */
-static void serial_down(struct usb_serial_port *port)
+static void serial_down(struct tty_port *tport)
 {
+	struct usb_serial_port *port =
+		container_of(tport, struct usb_serial_port, port);
 	struct usb_serial_driver *drv = port->serial->type;
-
 	/*
 	 * The console is magical.  Do not hang up the console hardware
 	 * or there will be tears.
 	 */
 	if (port->console)
 		return;
-
-	/* Don't call the close method if the hardware hasn't been
-	 * initialized.
-	 */
-	if (!test_and_clear_bit(ASYNCB_INITIALIZED, &port->port.flags))
-		return;
-
-	mutex_lock(&port->mutex);
 	if (drv->close)
 		drv->close(port);
-	mutex_unlock(&port->mutex);
 }
 
 static void serial_hangup(struct tty_struct *tty)
 {
 	struct usb_serial_port *port = tty->driver_data;
-
 	dbg("%s - port %d", __func__, port->number);
-
-	serial_down(port);
 	tty_port_hangup(&port->port);
 }
 
 static void serial_close(struct tty_struct *tty, struct file *filp)
 {
 	struct usb_serial_port *port = tty->driver_data;
-
 	dbg("%s - port %d", __func__, port->number);
-
-	if (tty_hung_up_p(filp))
-		return;
-	if (tty_port_close_start(&port->port, tty, filp) == 0)
-		return;
-	serial_down(port);
-	tty_port_close_end(&port->port, tty);
-	tty_port_tty_set(&port->port, NULL);
+	tty_port_close(&port->port, tty, filp);
 }
 
 /**
@@ -715,6 +695,7 @@ static const struct tty_port_operations serial_port_ops = {
 	.carrier_raised = serial_carrier_raised,
 	.dtr_rts = serial_dtr_rts,
 	.activate = serial_activate,
+	.shutdown = serial_down,
 };
 
 int usb_serial_probe(struct usb_interface *interface,
@@ -913,6 +894,8 @@ int usb_serial_probe(struct usb_interface *interface,
 		port->port.ops = &serial_port_ops;
 		port->serial = serial;
 		spin_lock_init(&port->lock);
+		/* Keep this for private driver use for the moment but
+		   should probably go away */
 		mutex_init(&port->mutex);
 		INIT_WORK(&port->work, usb_serial_port_work);
 		serial->port[i] = port;

[PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

The tty port has a port mutex used for all the port related locking so we
don't need the one in the USB serial layer any more.

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

 drivers/usb/serial/opticon.c    |    4 ++--
 drivers/usb/serial/usb-serial.c |    1 -
 include/linux/usb/serial.h      |    3 ---
 3 files changed, 2 insertions(+), 6 deletions(-)


diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index 1085a57..c3a9ce2 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -498,12 +498,12 @@ static int opticon_resume(struct usb_interface *intf)
 	struct usb_serial_port *port = serial->port[0];
 	int result;
 
-	mutex_lock(&port->mutex);
+	mutex_lock(&port->port.mutex);
 	if (port->port.count)
 		result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);
 	else
 		result = 0;
-	mutex_unlock(&port->mutex);
+	mutex_unlock(&port->port.mutex);
 	return result;
 }
 
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index e189338..421ef1f 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -896,7 +896,6 @@ int usb_serial_probe(struct usb_interface *interface,
 		spin_lock_init(&port->lock);
 		/* Keep this for private driver use for the moment but
 		   should probably go away */
-		mutex_init(&port->mutex);
 		INIT_WORK(&port->work, usb_serial_port_work);
 		serial->port[i] = port;
 		port->dev.parent = &interface->dev;
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index c17eb64..ad24c8c 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -39,8 +39,6 @@ enum port_dev_state {
  * @serial: pointer back to the struct usb_serial owner of this port.
  * @port: pointer to the corresponding tty_port for this port.
  * @lock: spinlock to grab when updating portions of this structure.
- * @mutex: mutex used to synchronize serial_open() and serial_close()
- *	access for this port.
  * @number: the number of the port (the minor number).
  * @interrupt_in_buffer: pointer to the interrupt in buffer for this port.
  * @interrupt_in_urb: pointer to the interrupt in struct urb for this port.
@@ -77,7 +75,6 @@ struct usb_serial_port {
 	struct usb_serial	*serial;
 	struct tty_port		port;
 	spinlock_t		lock;
-	struct mutex            mutex;
 	unsigned char		number;
 
 	unsigned char		*interrupt_in_buffer;

[PATCH 5/5] opticon: Fix resume logic

[Alan Cox]

Opticon now takes the right mutex to check the port status but the status
check is done wrongly for the modern serial code, so fix it.

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

 drivers/usb/serial/opticon.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)


diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index c3a9ce2..4ff9e7a 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -499,7 +499,8 @@ static int opticon_resume(struct usb_interface *intf)
 	int result;
 
 	mutex_lock(&port->port.mutex);
-	if (port->port.count)
+	/* This is protected by the port mutex against close/open */
+	if (test_bit(ASYNCB_INITIALIZED, &port->port.flags))
 		result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);
 	else
 		result = 0;

Re: [PATCH 5/5] opticon: Fix resume logic

[Oliver Neukum]

Am Dienstag, 6. Oktober 2009 17:06:57 schrieb Alan Cox:
> Opticon now takes the right mutex to check the port status but the status
> check is done wrongly for the modern serial code, so fix it.

As Alan Stern noticed, it seems like we have an ab-ba deadlock here
between open and resume regarding pm_mutex and port->mutex.

	Regards
		Oliver

Re: [PATCH 5/5] opticon: Fix resume logic

[Alan Cox]

On Tue, 6 Oct 2009 23:12:17 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> Am Dienstag, 6. Oktober 2009 17:06:57 schrieb Alan Cox:
> > Opticon now takes the right mutex to check the port status but the status
> > check is done wrongly for the modern serial code, so fix it.
> 
> As Alan Stern noticed, it seems like we have an ab-ba deadlock here
> between open and resume regarding pm_mutex and port->mutex.

Oh well I guess someone with hardware will have to fix that.

Do we actually need a separate pm_mutex anyway ?

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1028 bytes --]

Am Dienstag, 6. Oktober 2009 17:06:46 schrieb Alan Cox:> +++ b/drivers/usb/serial/opticon.c> @@ -498,12 +498,12 @@ static int opticon_resume(struct usb_interface *intf)>         struct usb_serial_port *port = serial->port[0];>         int result;>  > -       mutex_lock(&port->mutex);> +       mutex_lock(&port->port.mutex);>         if (port->port.count)>                 result = usb_submit_urb(priv->bulk_read_urb, GFP_NOIO);>         else>                 result = 0;> -       mutex_unlock(&port->mutex);> +       mutex_unlock(&port->port.mutex);>         return result;>  }
We will need some generic way to autoresume from open.Resume will need to lock against open() and need to be calledfrom within open(). Any ideas for an unugly interface?
	Regards		Oliver
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 5/5] opticon: Fix resume logic

[Johan Hovold]

On Tue, Oct 06, 2009 at 11:23:31PM +0100, Alan Cox wrote:
> On Tue, 6 Oct 2009 23:12:17 +0200
> Oliver Neukum <oliver@neukum.org> wrote:
> 
> > Am Dienstag, 6. Oktober 2009 17:06:57 schrieb Alan Cox:
> > > Opticon now takes the right mutex to check the port status but the status
> > > check is done wrongly for the modern serial code, so fix it.
> > 
> > As Alan Stern noticed, it seems like we have an ab-ba deadlock here
> > between open and resume regarding pm_mutex and port->mutex.
> 
> Oh well I guess someone with hardware will have to fix that.
> 
> Do we actually need a separate pm_mutex anyway ?

The pm_mutex is actually not aquired during open (and Alan Stern just
confirmed that), so there is no dead-lock with port->mutex.

/Johan

Re: [PATCH 5/5] opticon: Fix resume logic

[Alan Stern]

On Tue, 6 Oct 2009, Alan Cox wrote:

> On Tue, 6 Oct 2009 23:12:17 +0200
> Oliver Neukum <oliver@neukum.org> wrote:
> 
> > Am Dienstag, 6. Oktober 2009 17:06:57 schrieb Alan Cox:
> > > Opticon now takes the right mutex to check the port status but the status
> > > check is done wrongly for the modern serial code, so fix it.
> > 
> > As Alan Stern noticed, it seems like we have an ab-ba deadlock here
> > between open and resume regarding pm_mutex and port->mutex.

Johan pointed out that I was mistaken in saying the pm_mutex is
acquired during open.  It actually is acquired in serial_install() and
serial_cleanup(), which are called without the port mutex.  So I guess
we're okay after all.

In the general case, that is.  Specific drivers may still run into
trouble.  For example, option_open() and sierra_open() do acquire the 
pm_mutex.

> Oh well I guess someone with hardware will have to fix that.
> 
> Do we actually need a separate pm_mutex anyway ?

In principle we don't, and Rafael Wysocki's new runtime PM framework
doesn't use one.  However the current USB runtime PM framework does, so
until we switch over (hopefully in time for 2.6.33) it's important to
watch out for this sort of conflict.

Alan Stern

Re: [PATCH 5/5] opticon: Fix resume logic

[Oliver Neukum]

Am Mittwoch, 7. Oktober 2009 17:56:02 schrieb Johan Hovold:
> On Tue, Oct 06, 2009 at 11:23:31PM +0100, Alan Cox wrote:
> > On Tue, 6 Oct 2009 23:12:17 +0200
> >
> > Oliver Neukum <oliver@neukum.org> wrote:
> > > Am Dienstag, 6. Oktober 2009 17:06:57 schrieb Alan Cox:
> > > > Opticon now takes the right mutex to check the port status but the
> > > > status check is done wrongly for the modern serial code, so fix it.
> > >
> > > As Alan Stern noticed, it seems like we have an ab-ba deadlock here
> > > between open and resume regarding pm_mutex and port->mutex.
> >
> > Oh well I guess someone with hardware will have to fix that.
> >
> > Do we actually need a separate pm_mutex anyway ?
>
> The pm_mutex is actually not aquired during open (and Alan Stern just
> confirmed that), so there is no dead-lock with port->mutex.

This is currently true, but will no longer be true if autosuspend is
implemented for this driver.

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Wed, 7 Oct 2009, Oliver Neukum wrote:

> We will need some generic way to autoresume from open.
> Resume will need to lock against open() and need to be called
> from within open(). Any ideas for an unugly interface?

It's not quite that bad.  Resume doesn't need to lock against open. 
If open is called while resume is running then when it tries to do its
own resume, it will either block (waiting for the pm_mutex) or return
immediately (if it sees the device is already resumed).

A more interesting question is how to synchronize both open/close and
suspend/resume against throttle/unthrottle.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Mittwoch, 7. Oktober 2009 18:03:08 schrieb Alan Stern:
> On Wed, 7 Oct 2009, Oliver Neukum wrote:
> > We will need some generic way to autoresume from open.
> > Resume will need to lock against open() and need to be called
> > from within open(). Any ideas for an unugly interface?
>
> It's not quite that bad.  Resume doesn't need to lock against open.
> If open is called while resume is running then when it tries to do its
> own resume, it will either block (waiting for the pm_mutex) or return
> immediately (if it sees the device is already resumed).

But resume() needs to know whether the read URBs need to be
submitted or not.

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Wed, 7 Oct 2009, Oliver Neukum wrote:

> Am Mittwoch, 7. Oktober 2009 18:03:08 schrieb Alan Stern:
> > On Wed, 7 Oct 2009, Oliver Neukum wrote:
> > > We will need some generic way to autoresume from open.
> > > Resume will need to lock against open() and need to be called
> > > from within open(). Any ideas for an unugly interface?
> >
> > It's not quite that bad.  Resume doesn't need to lock against open.
> > If open is called while resume is running then when it tries to do its
> > own resume, it will either block (waiting for the pm_mutex) or return
> > immediately (if it sees the device is already resumed).
> 
> But resume() needs to know whether the read URBs need to be
> submitted or not.

Given that there are several pathways for turning on or turning off the 
read URBs, the best answer is to use a flag.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

On Wed, 7 Oct 2009 12:03:08 -0400 (EDT)
Alan Stern <stern@rowland.harvard.edu> wrote:

> On Wed, 7 Oct 2009, Oliver Neukum wrote:
> 
> > We will need some generic way to autoresume from open.
> > Resume will need to lock against open() and need to be called
> > from within open(). Any ideas for an unugly interface?
> 
> It's not quite that bad.  Resume doesn't need to lock against open. 
> If open is called while resume is running then when it tries to do its
> own resume, it will either block (waiting for the pm_mutex) or return
> immediately (if it sees the device is already resumed).

It would probably be cleaner if they could lock against each other

> A more interesting question is how to synchronize both open/close and
> suspend/resume against throttle/unthrottle.

throttle and unthrottle can sleep and obviously have to as they do a fair
bit of work sometimes (xon/xoff, mode line waggling etc)

The current ordering here is quite ugly because we open the ldisc before
the tty which means the ldisc sometimes calls unthrottle before the tty
is opened which is not nice. On the close side we have the same thing via
tty_ldisc_release.

We can take the port->mutex lock in the throttle/unthrottle methods as
far as I can see - there are no obvious problem cases. We do call
->throttle and ->unthrottle from the ldisc open but this occurs outside
of any call to the tty driver open/close method so I don't see any
deadlock.

It adds an ordering of termios lock before port mutex when taking both
but that's not a problem and really implicit in the structure of the code
anyway.

Alan

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Wed, 7 Oct 2009, Alan Cox wrote:

> On Wed, 7 Oct 2009 12:03:08 -0400 (EDT)
> Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > On Wed, 7 Oct 2009, Oliver Neukum wrote:
> > 
> > > We will need some generic way to autoresume from open.
> > > Resume will need to lock against open() and need to be called
> > > from within open(). Any ideas for an unugly interface?
> > 
> > It's not quite that bad.  Resume doesn't need to lock against open. 
> > If open is called while resume is running then when it tries to do its
> > own resume, it will either block (waiting for the pm_mutex) or return
> > immediately (if it sees the device is already resumed).
> 
> It would probably be cleaner if they could lock against each other

What you mean isn't clear.  After all, open sometimes has to call
resume.  So how could resume lock against open?

> > A more interesting question is how to synchronize both open/close and
> > suspend/resume against throttle/unthrottle.
> 
> throttle and unthrottle can sleep and obviously have to as they do a fair
> bit of work sometimes (xon/xoff, mode line waggling etc)
> 
> The current ordering here is quite ugly because we open the ldisc before
> the tty which means the ldisc sometimes calls unthrottle before the tty
> is opened which is not nice. On the close side we have the same thing via
> tty_ldisc_release.
> 
> We can take the port->mutex lock in the throttle/unthrottle methods as
> far as I can see - there are no obvious problem cases. We do call
> ->throttle and ->unthrottle from the ldisc open but this occurs outside
> of any call to the tty driver open/close method so I don't see any
> deadlock.
> 
> It adds an ordering of termios lock before port mutex when taking both
> but that's not a problem and really implicit in the structure of the code
> anyway.

Does this imply that unthrottle should try to autoresume?  There does 
appear to be a potential race between unthrottle and autosuspend.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

> > It would probably be cleaner if they could lock against each other
> 
> What you mean isn't clear.  After all, open sometimes has to call
> resume.  So how could resume lock against open?

Probably it needs a counting lock as the code is currently structured -
which is a bit ugly. What paths do we end up going through the device
open method into resume in the same thread ?

> Does this imply that unthrottle should try to autoresume?  There does 
> appear to be a potential race between unthrottle and autosuspend.

The more I look at it the more it implies to me that the ldiscs doing
this should instead be taught some better manners instead. The real nasty
is that a driver might not have initialised the locking it needs do that
exclusion until open occurs. I think n_tty is probably the only offender
and if so I'd rather fix that and make it a rule that you don't do that,
trying to fix it other ways is going to be more horrible I imagine.

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Wed, 7 Oct 2009, Alan Cox wrote:

> > > It would probably be cleaner if they could lock against each other
> > 
> > What you mean isn't clear.  After all, open sometimes has to call
> > resume.  So how could resume lock against open?
> 
> Probably it needs a counting lock as the code is currently structured -
> which is a bit ugly. What paths do we end up going through the device
> open method into resume in the same thread ?

Currently there are no such paths.  I keep forgetting that the resume
is done in serial_install() rather than serial_open().  Eventually the
tty_port reorganization will probably force the resume to move into the
activate method.

However in the option and sierra drivers there is a perverse path from
close to resume: Both their close methods call
usb_autopm_get_interface().  This could be removed without much
trouble; perhaps we should do so.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Mittwoch, 7. Oktober 2009 20:52:21 schrieb Alan Stern:
> However in the option and sierra drivers there is a perverse path from
> close to resume: Both their close methods call
> usb_autopm_get_interface().  This could be removed without much
> trouble; perhaps we should do so.

I am afraid this won't do in the long run. Some drivers will want to
shut down devices by communicating with them in close().

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

On Wed, 7 Oct 2009 22:56:20 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> Am Mittwoch, 7. Oktober 2009 20:52:21 schrieb Alan Stern:
> > However in the option and sierra drivers there is a perverse path from
> > close to resume: Both their close methods call
> > usb_autopm_get_interface().  This could be removed without much
> > trouble; perhaps we should do so.
> 
> I am afraid this won't do in the long run. Some drivers will want to
> shut down devices by communicating with them in close().

drivers/serial will need a power management hook to use
tty_port_{open/close} so perhaps that can be covered for both. In the
serial case it needs to kick stuff out of PCI D3 mostly and could
probably be fudged but if USB needs it perhaps it should be explicit.

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Wed, 7 Oct 2009, Alan Cox wrote:

> On Wed, 7 Oct 2009 22:56:20 +0200
> Oliver Neukum <oliver@neukum.org> wrote:
> 
> > Am Mittwoch, 7. Oktober 2009 20:52:21 schrieb Alan Stern:
> > > However in the option and sierra drivers there is a perverse path from
> > > close to resume: Both their close methods call
> > > usb_autopm_get_interface().  This could be removed without much
> > > trouble; perhaps we should do so.
> > 
> > I am afraid this won't do in the long run. Some drivers will want to
> > shut down devices by communicating with them in close().
> 
> drivers/serial will need a power management hook to use
> tty_port_{open/close} so perhaps that can be covered for both. In the
> serial case it needs to kick stuff out of PCI D3 mostly and could
> probably be fudged but if USB needs it perhaps it should be explicit.

I'm losing track of the original point of this thread.  IIRC, the 
problem is how the resume method should know whether or not to submit 
the receive URB(s).  It can't afford to acquire the port mutex because 
it might be called by open or close, at which time the mutex is already 
held.

Other schemes could work, but to me it seems simplest to rely on a flag
protected by a spinlock.  The flag would mean "URBs are supposed to be
queued unless we are suspended".  It would be set by open and 
unthrottle, and cleared by close and throttle.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Mittwoch, 7. Oktober 2009 19:23:59 schrieb Alan Stern:
> > We can take the port->mutex lock in the throttle/unthrottle methods as
> > far as I can see - there are no obvious problem cases. We do call
> > ->throttle and ->unthrottle from the ldisc open but this occurs outside
> > of any call to the tty driver open/close method so I don't see any
> > deadlock.
> >
> > It adds an ordering of termios lock before port mutex when taking both
> > but that's not a problem and really implicit in the structure of the code
> > anyway.
>
> Does this imply that unthrottle should try to autoresume?  There does
> appear to be a potential race between unthrottle and autosuspend.

The race exists. But I don't think unthrottle should autoresume.
It would be better to just set a flag and defer this to resume.
If the device supports remote wakeup there'll be no need to autoresume,
if not, throttle/unthrottle are too rare to justify the complexity.

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
> I'm losing track of the original point of this thread.  IIRC, the
> problem is how the resume method should know whether or not to submit
> the receive URB(s).  It can't afford to acquire the port mutex because
> it might be called by open or close, at which time the mutex is already
> held.
>
> Other schemes could work, but to me it seems simplest to rely on a flag
> protected by a spinlock.  The flag would mean "URBs are supposed to be
> queued unless we are suspended".  It would be set by open and
> unthrottle, and cleared by close and throttle.

1. Why a spinlock?
2. Can we get by with only one flag?

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Thu, 8 Oct 2009, Oliver Neukum wrote:

> Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
> > I'm losing track of the original point of this thread.  IIRC, the
> > problem is how the resume method should know whether or not to submit
> > the receive URB(s).  It can't afford to acquire the port mutex because
> > it might be called by open or close, at which time the mutex is already
> > held.
> >
> > Other schemes could work, but to me it seems simplest to rely on a flag
> > protected by a spinlock.  The flag would mean "URBs are supposed to be
> > queued unless we are suspended".  It would be set by open and
> > unthrottle, and cleared by close and throttle.
> 
> 1. Why a spinlock?

Because the amount of work involved seems too small for a mutex.  But 
you could use a mutex if you wanted, since everything occurs in process 
context.

> 2. Can we get by with only one flag?

If all you want to do is answer a single question ("Should URBs be 
submitted") then a single flag should be all you need.  Why, do you 
think more information will be necessary?  You can always add more.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Donnerstag, 8. Oktober 2009 16:58:39 schrieb Alan Stern:
> On Thu, 8 Oct 2009, Oliver Neukum wrote:
> > Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:

> > > Other schemes could work, but to me it seems simplest to rely on a flag
> > > protected by a spinlock.  The flag would mean "URBs are supposed to be
> > > queued unless we are suspended".  It would be set by open and
> > > unthrottle, and cleared by close and throttle.
> >
> > 1. Why a spinlock?
>
> Because the amount of work involved seems too small for a mutex.  But
> you could use a mutex if you wanted, since everything occurs in process
> context.

We have to submit URBs under that lock.

> > 2. Can we get by with only one flag?
>
> If all you want to do is answer a single question ("Should URBs be
> submitted") then a single flag should be all you need.  Why, do you
> think more information will be necessary?  You can always add more.

We have at least three reasons URBs should not be submitted.
- closure
- throttling
- suspension
Resume() should not submit if either closure or throttling are active,
neither should unthrottle() resubmit if closure or suspension are active.

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Thu, 8 Oct 2009, Oliver Neukum wrote:

> Am Donnerstag, 8. Oktober 2009 16:58:39 schrieb Alan Stern:
> > On Thu, 8 Oct 2009, Oliver Neukum wrote:
> > > Am Mittwoch, 7. Oktober 2009 23:34:12 schrieb Alan Stern:
> 
> > > > Other schemes could work, but to me it seems simplest to rely on a flag
> > > > protected by a spinlock.  The flag would mean "URBs are supposed to be
> > > > queued unless we are suspended".  It would be set by open and
> > > > unthrottle, and cleared by close and throttle.
> > >
> > > 1. Why a spinlock?
> >
> > Because the amount of work involved seems too small for a mutex.  But
> > you could use a mutex if you wanted, since everything occurs in process
> > context.
> 
> We have to submit URBs under that lock.

Yes.  What's your point?

> > > 2. Can we get by with only one flag?
> >
> > If all you want to do is answer a single question ("Should URBs be
> > submitted") then a single flag should be all you need.  Why, do you
> > think more information will be necessary?  You can always add more.
> 
> We have at least three reasons URBs should not be submitted.
> - closure
> - throttling
> - suspension
> Resume() should not submit if either closure or throttling are active,
> neither should unthrottle() resubmit if closure or suspension are active.

True.  Nor should open() submit if throttling is active.  Feel free to 
use three separate flags.  :-)

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Thu, 8 Oct 2009, Alan Stern wrote:

> > > > 2. Can we get by with only one flag?
> > >
> > > If all you want to do is answer a single question ("Should URBs be
> > > submitted") then a single flag should be all you need.  Why, do you
> > > think more information will be necessary?  You can always add more.
> > 
> > We have at least three reasons URBs should not be submitted.
> > - closure
> > - throttling
> > - suspension
> > Resume() should not submit if either closure or throttling are active,
> > neither should unthrottle() resubmit if closure or suspension are active.
> 
> True.  Nor should open() submit if throttling is active.  Feel free to 
> use three separate flags.  :-)

On further thought, unthrottle should autoresume if the device is 
open and autosuspended (but it shouldn't do anything if the device is 
suspended).  After all, the reason for the autosuspend may have been 
the lack of activity caused by the throttling.

In practice this isn't likely to come up.  It would be surprising if 
throttling lasted long enough to cause an autosuspend or if the core 
decided to throttle while the device was autosuspended and hence idle.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Oliver Neukum]

Am Donnerstag, 8. Oktober 2009 22:06:10 schrieb Alan Stern:

>
> On further thought, unthrottle should autoresume if the device is
> open and autosuspended (but it shouldn't do anything if the device is
> suspended).  After all, the reason for the autosuspend may have been
> the lack of activity caused by the throttling.
>
> In practice this isn't likely to come up.  It would be surprising if
> throttling lasted long enough to cause an autosuspend or if the core
> decided to throttle while the device was autosuspended and hence idle.

So you say that throttle() should do an autopm_put?

	Regards
		Oliver

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

On Thu, 8 Oct 2009 22:40:36 +0200
Oliver Neukum <oliver@neukum.org> wrote:

> Am Donnerstag, 8. Oktober 2009 22:06:10 schrieb Alan Stern:
> 
> >
> > On further thought, unthrottle should autoresume if the device is
> > open and autosuspended (but it shouldn't do anything if the device is
> > suspended).  After all, the reason for the autosuspend may have been
> > the lack of activity caused by the throttling.
> >
> > In practice this isn't likely to come up.  It would be surprising if
> > throttling lasted long enough to cause an autosuspend or if the core
> > decided to throttle while the device was autosuspended and hence idle.
> 
> So you say that throttle() should do an autopm_put?

You need to be very very sure you cannot lose a byte of data or have the
modem lines disrupted in any way if you do that.

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Stern]

On Thu, 8 Oct 2009, Oliver Neukum wrote:

> Am Donnerstag, 8. Oktober 2009 22:06:10 schrieb Alan Stern:
> 
> >
> > On further thought, unthrottle should autoresume if the device is
> > open and autosuspended (but it shouldn't do anything if the device is
> > suspended).  After all, the reason for the autosuspend may have been
> > the lack of activity caused by the throttling.
> >
> > In practice this isn't likely to come up.  It would be surprising if
> > throttling lasted long enough to cause an autosuspend or if the core
> > decided to throttle while the device was autosuspended and hence idle.
> 
> So you say that throttle() should do an autopm_put?

The way you've coded the sierra and option drivers, it's not necessary.  
Those drivers do an autopm_get_async during submission and an
autopm_put_async after the completion of every output URB (and they
update the last_busy time in the completion of every input URB).  When
the driver is throttled no URBs will be submitted, so the usage count
will remain at 0 with no effort on the part of throttle().

For other drivers that use the simpler "autoresume on tty install,
autosuspend on tty cleanup" approach provided by usb-serial.c, the
throttle routines obviously don't need to worry about runtime PM.

Alan Stern

Re: [PATCH 4/5] usb_serial: Kill port mutex

[Alan Cox]

> update the last_busy time in the completion of every input URB).  When
> the driver is throttled no URBs will be submitted, so the usage count
> will remain at 0 with no effort on the part of throttle().

You can still get modem changes (set_mctrl), break etc with the port
throttled, or indeed output.