Re: USB Ooops PL2303 when unplug while use (linux v3.7.3)

Re: USB Ooops PL2303 when unplug while use (linux v3.7.3)

[Johan Hovold]

On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> I file a report for you, please have a look when you have time.

Thanks for the bug report, Chris.

You have come across what looks like a known issue, which since it's
discovery last summer has been made worse by an unrelated change.

A similar oops was reported and its cause identified in this thread:

	http://marc.info/?l=linux-usb&m=133837337927749&w=2

It turns out that the tty-layer may call the driver's dtr_rts even after
the device has been disconnected and the tty device unregistered. Since
last summer another change has made the problem worse by setting the
port data to NULL which results in even more drivers hitting the
problem.

While waiting for input from the tty-guru Alan Cox, and as the immediate
cause of that oops was remedied (by moving the offending interface
access in the driver in question), the problem was unfortunately
forgotten (or rather down-prioritised) until now.

I can still reliably reproduce a similar oops with pl2303 in the setup
which I used last summer, where I first open the port and then open and
close it repeatedly (while keeping the first reference open). When
disconnecting the device, the first reference is properly hung up, while
the second open fails (due to the disconnect) but still the tty layer
continues with an orderly close (which includes the call to dtr_rts)
despite the second open having failed, and to make things worse, even
when the tty device has been unregistered. I'll elaborate on this in a
follow up mail.

How about your oops (included below for reference) -- can you reproduce
it reliably, or was it a one-time thing? Do you only have minicom open
the port, or is there another process already holding the port open?

I'm responding to this mail with an updated version of a patch I
submitted last summer which works around the tty-bug and fixes the oops
that I can trigger. Care to give it a try?

Greg, I believe we need to get this fix into stable either way. It fixes
a perfectly reproducible null-deref in several usb-serial drivers. Even
if the original cause is in tty, which keeps calling dtr_rts even after
tty_unregister_device returns, I see no reason not to refactor the
disconnect handling for dtr_rts to usb-serial core (while fixing the
new null-derefs). This way it can also more easily be removed once the
tty bug has been fixed (it could even be more than one).

Thanks,
Johan

> [171977.391986] pl2303 ttyUSB0: pl2303 converter now disconnected from ttyUSB0
> [171977.392001] pl2303 2-1.2:1.0: device disconnected
> [171977.489723] BUG: unable to handle kernel NULL pointer dereference at           (null)
> [171977.489788] IP: [<ffffffff8102210d>] __ticket_spin_lock+0x5/0x1b
> [171977.489837] PGD 944ef067 PUD 944ee067 PMD 0 
> [171977.489874] Oops: 0002 [#1] SMP 
> [171977.489901] Modules linked in: usblp pl2303 usbserial tun ip6table_filter ip6_tables iptable_filter ip_tables ebtable_nat ebtables x_tables ppdev lp bnep rfcomm bluetooth cpufreq_userspace cpufreq_conservative cpufreq_stats cpufreq_powersave xfrm4_tunnel tunnel4 ipcomp xfrm_ipcomp esp4 ah4 binfmt_misc deflate zlib_deflate ctr twofish_generic twofish_avx_x86_64 twofish_x86_64_3way twofish_x86_64 twofish_common serpent_sse2_x86_64 glue_helper lrw serpent_generic xts gf128mul blowfish_x86_64 blowfish_common cast5_avx_x86_64 ablk_helper cryptd cast5_generic des_generic cbc ecb sha512_generic sha256_generic sha1_ssse3 sha1_generic hmac af_key fuse bridge stp llc loop snd_hda_codec_realtek i915 drm_kms_helper snd_hda_intel snd_hda_codec drm snd_hwdep snd_pcm snd_seq snd_timer acpi_cpufreq 
 i2c_algo_bit snd_seq_device kvm_intel snd parport_pc soundcore psmouse video mperf processor thermal_sys button hid_generic wmi parport pcspkr snd_page_alloc serio_raw kvm i2c_i801 i2c_core joydev evdev tpm_tis tpm tpm_bios hid_logitech_dj ext4 mbcache jbd2 crc16 dm_mod usb_storage nbd usbhid hid sd_mod crc_t10dif ahci libahci ehci_hcd libata scsi_mod usbcore usb_common r8169 mii
> [171977.490824] CPU 0 
> [171977.490842] Pid: 9298, comm: minicom Not tainted 3.7.3 #3 LENOVO 1652A7B/To be filled by O.E.M.
> [171977.490901] RIP: 0010:[<ffffffff8102210d>]  [<ffffffff8102210d>] __ticket_spin_lock+0x5/0x1b
> [171977.490960] RSP: 0018:ffff88004a427aa0  EFLAGS: 00010082
> [171977.490996] RAX: 0000000000010000 RBX: 0000000000000282 RCX: 0000000000000000
> [171977.491043] RDX: ffff88012d7b1000 RSI: 0000000000000282 RDI: 0000000000000000
> [171977.491090] RBP: ffff88012d7b1000 R08: 0000000000000000 R09: 0000000000000282
> [171977.491136] R10: 0000000000000282 R11: ffffffff813150a6 R12: 0000000000000000
> [171977.491185] R13: 0000000000000282 R14: ffff88003d745d00 R15: ffff88003d745d00
> [171977.491232] FS:  00007fc23503d700(0000) GS:ffff88013fa00000(0000) knlGS:0000000000000000
> [171977.491287] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [171977.491325] CR2: 0000000000000000 CR3: 000000009493c000 CR4: 00000000000427e0
> [171977.491371] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [171977.491418] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> [171977.491465] Process minicom (pid: 9298, threadinfo ffff88004a426000, task ffff880139a84600)
> [171977.491518] Stack:
> [171977.491533]  ffffffff81022176 ffffffff813150a6 0000000000000282 0000000000000000
> [171977.491590]  ffffffffa0518454 ffff88012d7b1008 ffff88012d7b6800 ffff88012d7b1018
> [171977.491647]  ffffffff811f4881 ffff88012d7b1008 ffff88012d7b6800 0000000000000000
> [171977.491703] Call Trace:
> [171977.491724]  [<ffffffff81022176>] ? default_spin_lock_flags+0x5/0xb
> [171977.491768]  [<ffffffff813150a6>] ? _raw_spin_lock_irqsave+0x1e/0x26
> [171977.491815]  [<ffffffffa0518454>] ? pl2303_dtr_rts+0x21/0x53 [pl2303]
> [171977.491861]  [<ffffffff811f4881>] ? tty_port_close_start+0x16a/0x178
> [171977.491905]  [<ffffffff811f4cc4>] ? tty_port_close+0x11/0x43
> [171977.491944]  [<ffffffff811ecf17>] ? tty_release+0x16d/0x4b9
> [171977.491984]  [<ffffffff810ec16f>] ? __pollwait+0xd0/0xd0
> [171977.492021]  [<ffffffff81313b6e>] ? mutex_lock+0xd/0x2b
> [171977.492059]  [<ffffffffa050b265>] ? serial_activate+0x52/0x7c [usbserial]
> [171977.492104]  [<ffffffff810ec16f>] ? __pollwait+0xd0/0xd0
> [171977.492141]  [<ffffffff811eef55>] ? tty_open+0x33a/0x46a
> [171977.492178]  [<ffffffff810e185f>] ? chrdev_open+0x12e/0x149
> [171977.492216]  [<ffffffff810e1731>] ? cdev_put+0x1a/0x1a
> [171977.492253]  [<ffffffff810dd2d1>] ? do_dentry_open+0x171/0x217
> [171977.492293]  [<ffffffff810dd44a>] ? finish_open+0x2c/0x35
> [171977.492331]  [<ffffffff810e8955>] ? do_last+0x897/0xa38
> [171977.492367]  [<ffffffff810e6c33>] ? __inode_permission+0x62/0xa3
> [171977.492408]  [<ffffffff8104a552>] ? lg_local_lock+0x11/0x14
> [171977.492446]  [<ffffffff810e908c>] ? path_openat+0xc0/0x33b
> [171977.492487]  [<ffffffff810077ad>] ? read_tsc+0x5/0x16
> [171977.492522]  [<ffffffff810e93fa>] ? do_filp_open+0x2c/0x72
> [171977.492561]  [<ffffffff810cf7ef>] ? kmem_cache_alloc+0x26/0xa1
> [171977.492600]  [<ffffffff81315085>] ? _raw_spin_lock+0x5/0x8
> [171977.492638]  [<ffffffff810f3c79>] ? __alloc_fd+0xf9/0x10c
> [171977.492677]  [<ffffffff810dd026>] ? do_sys_open+0x60/0xe7
> [171977.492716]  [<ffffffff8131a629>] ? system_call_fastpath+0x16/0x1b
> [171977.492757] Code: 02 81 4c 89 c9 44 89 c6 48 89 c7 41 5a e9 9d fb ff ff 0f b7 f6 40 0f b6 ff 48 89 c2 41 59 e9 fc fc ff ff 90 90 90 b8 00 00 01 00 <f0> 0f c1 07 89 c2 c1 e8 10 66 39 c2 74 07 f3 90 66 8b 17 eb f4 
> [171977.493018] RIP  [<ffffffff8102210d>] __ticket_spin_lock+0x5/0x1b
> [171977.493062]  RSP <ffff88004a427aa0>
> [171977.493086] CR2: 0000000000000000
> [171977.517440] ---[ end trace 6ffd69640c166d14 ]---

[PATCH] USB: serial: fix null-pointer dereferences on disconnect

[Johan Hovold]

Make sure serial-driver dtr_rts is called with disc_mutex held after
checking the disconnected flag.

Due to a bug in the tty layer, dtr_rts may get called after a device has
been disconnected and the tty-device unregistered. Some drivers have had
individual checks for disconnect to make sure the disconnected interface
was not accessed, but this should really be handled in usb-serial core
(at least until the long-standing tty-bug has been fixed).

Note that the problem has been made more acute with commit 0998d0631001
("device-core: Ensure drvdata = NULL when no driver is bound") as the
port data is now also NULL when dtr_rts is called resulting in further
oopses.

Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Cc: stable@vger.kernel.org
---
 drivers/usb/serial/ftdi_sio.c   | 20 +++++++++-----------
 drivers/usb/serial/mct_u232.c   | 22 +++++++++-------------
 drivers/usb/serial/quatech2.c   | 18 ++++++++----------
 drivers/usb/serial/sierra.c     |  8 +-------
 drivers/usb/serial/ssu100.c     | 19 ++++++++-----------
 drivers/usb/serial/usb-serial.c | 14 ++++++++++++--
 drivers/usb/serial/usb_wwan.c   |  8 +++-----
 7 files changed, 50 insertions(+), 59 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 90ceef1..d07fccf 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1886,24 +1886,22 @@ static void ftdi_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on && usb_control_msg(port->serial->dev,
+	/* Disable flow control */
+	if (!on) {
+		if (usb_control_msg(port->serial->dev,
 			    usb_sndctrlpipe(port->serial->dev, 0),
 			    FTDI_SIO_SET_FLOW_CTRL_REQUEST,
 			    FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
 			    0, priv->interface, NULL, 0,
 			    WDR_TIMEOUT) < 0) {
-			    dev_err(&port->dev, "error from flowcontrol urb\n");
+			dev_err(&port->dev, "error from flowcontrol urb\n");
 		}
-		/* drop RTS and DTR */
-		if (on)
-			set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
-		else
-			clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
+	else
+		clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
 }
 
 /*
diff --git a/drivers/usb/serial/mct_u232.c b/drivers/usb/serial/mct_u232.c
index b691175..d9c8651 100644
--- a/drivers/usb/serial/mct_u232.c
+++ b/drivers/usb/serial/mct_u232.c
@@ -499,19 +499,15 @@ static void mct_u232_dtr_rts(struct usb_serial_port *port, int on)
 	unsigned int control_state;
 	struct mct_u232_private *priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* drop DTR and RTS */
-		spin_lock_irq(&priv->lock);
-		if (on)
-			priv->control_state |= TIOCM_DTR | TIOCM_RTS;
-		else
-			priv->control_state &= ~(TIOCM_DTR | TIOCM_RTS);
-		control_state = priv->control_state;
-		spin_unlock_irq(&priv->lock);
-		mct_u232_set_modem_ctrl(port, control_state);
-	}
-	mutex_unlock(&port->serial->disc_mutex);
+	spin_lock_irq(&priv->lock);
+	if (on)
+		priv->control_state |= TIOCM_DTR | TIOCM_RTS;
+	else
+		priv->control_state &= ~(TIOCM_DTR | TIOCM_RTS);
+	control_state = priv->control_state;
+	spin_unlock_irq(&priv->lock);
+
+	mct_u232_set_modem_ctrl(port, control_state);
 }
 
 static void mct_u232_close(struct usb_serial_port *port)
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index d152be9..a8d5110 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -945,19 +945,17 @@ static void qt2_dtr_rts(struct usb_serial_port *port, int on)
 	struct usb_device *dev = port->serial->dev;
 	struct qt2_port_private *port_priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on && qt2_setregister(dev, port_priv->device_port,
+	/* Disable flow control */
+	if (!on) {
+		if (qt2_setregister(dev, port_priv->device_port,
 					   UART_MCR, 0) < 0)
 			dev_warn(&port->dev, "error from flowcontrol urb\n");
-		/* drop RTS and DTR */
-		if (on)
-			update_mctrl(port_priv, TIOCM_DTR | TIOCM_RTS, 0);
-		else
-			update_mctrl(port_priv, 0, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		update_mctrl(port_priv, TIOCM_DTR | TIOCM_RTS, 0);
+	else
+		update_mctrl(port_priv, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static void qt2_update_msr(struct usb_serial_port *port, unsigned char *ch)
diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index af06f2f..d4426c0 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -861,19 +861,13 @@ static int sierra_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void sierra_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct usb_serial *serial = port->serial;
 	struct sierra_port_private *portdata;
 
 	portdata = usb_get_serial_port_data(port);
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
 
-	if (serial->dev) {
-		mutex_lock(&serial->disc_mutex);
-		if (!serial->disconnected)
-			sierra_send_setup(port);
-		mutex_unlock(&serial->disc_mutex);
-	}
+	sierra_send_setup(port);
 }
 
 static int sierra_startup(struct usb_serial *serial)
diff --git a/drivers/usb/serial/ssu100.c b/drivers/usb/serial/ssu100.c
index 4543ea3..d938396 100644
--- a/drivers/usb/serial/ssu100.c
+++ b/drivers/usb/serial/ssu100.c
@@ -506,19 +506,16 @@ static void ssu100_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct usb_device *dev = port->serial->dev;
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on &&
-		    ssu100_setregister(dev, 0, UART_MCR, 0) < 0)
+	/* Disable flow control */
+	if (!on) {
+		if (ssu100_setregister(dev, 0, UART_MCR, 0) < 0)
 			dev_err(&port->dev, "error from flowcontrol urb\n");
-		/* drop RTS and DTR */
-		if (on)
-			set_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
-		else
-			clear_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		set_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
+	else
+		clear_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
 }
 
 static void ssu100_update_msr(struct usb_serial_port *port, u8 msr)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 64bda13..15af799 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -688,10 +688,20 @@ static int serial_carrier_raised(struct tty_port *port)
 static void serial_dtr_rts(struct tty_port *port, int on)
 {
 	struct usb_serial_port *p = container_of(port, struct usb_serial_port, port);
-	struct usb_serial_driver *drv = p->serial->type;
+	struct usb_serial *serial = p->serial;
+	struct usb_serial_driver *drv = serial->type;
 
-	if (drv->dtr_rts)
+	if (!drv->dtr_rts)
+		return;
+	/*
+	 * Work-around bug in the tty-layer which can result in dtr_rts
+	 * being called after a disconnect (and tty_unregister_device
+	 * has returned). Remove once bug has been squashed.
+	 */
+	mutex_lock(&serial->disc_mutex);
+	if (!serial->disconnected)
 		drv->dtr_rts(p, on);
+	mutex_unlock(&serial->disc_mutex);
 }
 
 static const struct tty_port_operations serial_port_ops = {
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 01c94aa..1355a6c 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -38,7 +38,6 @@
 
 void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct usb_serial *serial = port->serial;
 	struct usb_wwan_port_private *portdata;
 	struct usb_wwan_intf_private *intfdata;
 
@@ -48,12 +47,11 @@ void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
 		return;
 
 	portdata = usb_get_serial_port_data(port);
-	mutex_lock(&serial->disc_mutex);
+	/* FIXME: locking */
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
-	if (serial->dev)
-		intfdata->send_setup(port);
-	mutex_unlock(&serial->disc_mutex);
+
+	intfdata->send_setup(port);
 }
 EXPORT_SYMBOL(usb_wwan_dtr_rts);
 
-- 
1.8.1.1

Re: [PATCH] USB: serial: fix null-pointer dereferences on disconnect

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

On Wed, Feb 13, 2013 at 03:28:51PM +0100, Johan Hovold wrote:
> Make sure serial-driver dtr_rts is called with disc_mutex held after
> checking the disconnected flag.
> 
> Due to a bug in the tty layer, dtr_rts may get called after a device has

why don't you fix the bug in the tty layer instead ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] USB: serial: fix null-pointer dereferences on disconnect

[Johan Hovold]

On Wed, Feb 13, 2013 at 04:34:36PM +0200, Felipe Balbi wrote:
> On Wed, Feb 13, 2013 at 03:28:51PM +0100, Johan Hovold wrote:
> > Make sure serial-driver dtr_rts is called with disc_mutex held after
> > checking the disconnected flag.
> > 
> > Due to a bug in the tty layer, dtr_rts may get called after a device has
> 
> why don't you fix the bug in the tty layer instead ?

I'm workin on it. ;)

As I mentioned earlier I'll send a follow up mail with some more details
on the tty issue and some RFC patches which would actually fix the
immediate issue as well.

There is nothing wrong with checking the disconnected flag either way.
In fact, it could be considered an optimisation as it will be set once
the physical device is gone which is some time before the hangup is
complete and the tty device unregistered. We already use the flag to
prevent further opens after a disconnect.

/Johan

Re: [PATCH] USB: serial: fix null-pointer dereferences on disconnect

[Greg KH]

On Wed, Feb 13, 2013 at 03:28:51PM +0100, Johan Hovold wrote:
> Make sure serial-driver dtr_rts is called with disc_mutex held after
> checking the disconnected flag.
> 
> Due to a bug in the tty layer, dtr_rts may get called after a device has
> been disconnected and the tty-device unregistered. Some drivers have had
> individual checks for disconnect to make sure the disconnected interface
> was not accessed, but this should really be handled in usb-serial core
> (at least until the long-standing tty-bug has been fixed).
> 
> Note that the problem has been made more acute with commit 0998d0631001
> ("device-core: Ensure drvdata = NULL when no driver is bound") as the
> port data is now also NULL when dtr_rts is called resulting in further
> oopses.
> 
> Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
> Cc: stable@vger.kernel.org
> ---

Any reason you didn't sign-off on this patch?

I can't really apply it without that :(

[PATCH resend] USB: serial: fix null-pointer dereferences on disconnect

[Johan Hovold]

Make sure serial-driver dtr_rts is called with disc_mutex held after
checking the disconnected flag.

Due to a bug in the tty layer, dtr_rts may get called after a device has
been disconnected and the tty-device unregistered. Some drivers have had
individual checks for disconnect to make sure the disconnected interface
was not accessed, but this should really be handled in usb-serial core
(at least until the long-standing tty-bug has been fixed).

Note that the problem has been made more acute with commit 0998d0631001
("device-core: Ensure drvdata = NULL when no driver is bound") as the
port data is now also NULL when dtr_rts is called resulting in further
oopses.

Reported-by: Chris Ruehl <chris.ruehl@gtsys.com.hk>
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <jhovold@gmail.com>
---

Resend with missing Signed-off-by added...

 drivers/usb/serial/ftdi_sio.c   | 20 +++++++++-----------
 drivers/usb/serial/mct_u232.c   | 22 +++++++++-------------
 drivers/usb/serial/quatech2.c   | 18 ++++++++----------
 drivers/usb/serial/sierra.c     |  8 +-------
 drivers/usb/serial/ssu100.c     | 19 ++++++++-----------
 drivers/usb/serial/usb-serial.c | 14 ++++++++++++--
 drivers/usb/serial/usb_wwan.c   |  8 +++-----
 7 files changed, 50 insertions(+), 59 deletions(-)

diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 90ceef1..d07fccf 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1886,24 +1886,22 @@ static void ftdi_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct ftdi_private *priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on && usb_control_msg(port->serial->dev,
+	/* Disable flow control */
+	if (!on) {
+		if (usb_control_msg(port->serial->dev,
 			    usb_sndctrlpipe(port->serial->dev, 0),
 			    FTDI_SIO_SET_FLOW_CTRL_REQUEST,
 			    FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
 			    0, priv->interface, NULL, 0,
 			    WDR_TIMEOUT) < 0) {
-			    dev_err(&port->dev, "error from flowcontrol urb\n");
+			dev_err(&port->dev, "error from flowcontrol urb\n");
 		}
-		/* drop RTS and DTR */
-		if (on)
-			set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
-		else
-			clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		set_mctrl(port, TIOCM_DTR | TIOCM_RTS);
+	else
+		clear_mctrl(port, TIOCM_DTR | TIOCM_RTS);
 }
 
 /*
diff --git a/drivers/usb/serial/mct_u232.c b/drivers/usb/serial/mct_u232.c
index b691175..d9c8651 100644
--- a/drivers/usb/serial/mct_u232.c
+++ b/drivers/usb/serial/mct_u232.c
@@ -499,19 +499,15 @@ static void mct_u232_dtr_rts(struct usb_serial_port *port, int on)
 	unsigned int control_state;
 	struct mct_u232_private *priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* drop DTR and RTS */
-		spin_lock_irq(&priv->lock);
-		if (on)
-			priv->control_state |= TIOCM_DTR | TIOCM_RTS;
-		else
-			priv->control_state &= ~(TIOCM_DTR | TIOCM_RTS);
-		control_state = priv->control_state;
-		spin_unlock_irq(&priv->lock);
-		mct_u232_set_modem_ctrl(port, control_state);
-	}
-	mutex_unlock(&port->serial->disc_mutex);
+	spin_lock_irq(&priv->lock);
+	if (on)
+		priv->control_state |= TIOCM_DTR | TIOCM_RTS;
+	else
+		priv->control_state &= ~(TIOCM_DTR | TIOCM_RTS);
+	control_state = priv->control_state;
+	spin_unlock_irq(&priv->lock);
+
+	mct_u232_set_modem_ctrl(port, control_state);
 }
 
 static void mct_u232_close(struct usb_serial_port *port)
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index d152be9..a8d5110 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -945,19 +945,17 @@ static void qt2_dtr_rts(struct usb_serial_port *port, int on)
 	struct usb_device *dev = port->serial->dev;
 	struct qt2_port_private *port_priv = usb_get_serial_port_data(port);
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on && qt2_setregister(dev, port_priv->device_port,
+	/* Disable flow control */
+	if (!on) {
+		if (qt2_setregister(dev, port_priv->device_port,
 					   UART_MCR, 0) < 0)
 			dev_warn(&port->dev, "error from flowcontrol urb\n");
-		/* drop RTS and DTR */
-		if (on)
-			update_mctrl(port_priv, TIOCM_DTR | TIOCM_RTS, 0);
-		else
-			update_mctrl(port_priv, 0, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		update_mctrl(port_priv, TIOCM_DTR | TIOCM_RTS, 0);
+	else
+		update_mctrl(port_priv, 0, TIOCM_DTR | TIOCM_RTS);
 }
 
 static void qt2_update_msr(struct usb_serial_port *port, unsigned char *ch)
diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c
index af06f2f..d4426c0 100644
--- a/drivers/usb/serial/sierra.c
+++ b/drivers/usb/serial/sierra.c
@@ -861,19 +861,13 @@ static int sierra_open(struct tty_struct *tty, struct usb_serial_port *port)
 
 static void sierra_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct usb_serial *serial = port->serial;
 	struct sierra_port_private *portdata;
 
 	portdata = usb_get_serial_port_data(port);
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
 
-	if (serial->dev) {
-		mutex_lock(&serial->disc_mutex);
-		if (!serial->disconnected)
-			sierra_send_setup(port);
-		mutex_unlock(&serial->disc_mutex);
-	}
+	sierra_send_setup(port);
 }
 
 static int sierra_startup(struct usb_serial *serial)
diff --git a/drivers/usb/serial/ssu100.c b/drivers/usb/serial/ssu100.c
index 4543ea3..d938396 100644
--- a/drivers/usb/serial/ssu100.c
+++ b/drivers/usb/serial/ssu100.c
@@ -506,19 +506,16 @@ static void ssu100_dtr_rts(struct usb_serial_port *port, int on)
 {
 	struct usb_device *dev = port->serial->dev;
 
-	mutex_lock(&port->serial->disc_mutex);
-	if (!port->serial->disconnected) {
-		/* Disable flow control */
-		if (!on &&
-		    ssu100_setregister(dev, 0, UART_MCR, 0) < 0)
+	/* Disable flow control */
+	if (!on) {
+		if (ssu100_setregister(dev, 0, UART_MCR, 0) < 0)
 			dev_err(&port->dev, "error from flowcontrol urb\n");
-		/* drop RTS and DTR */
-		if (on)
-			set_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
-		else
-			clear_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
 	}
-	mutex_unlock(&port->serial->disc_mutex);
+	/* drop RTS and DTR */
+	if (on)
+		set_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
+	else
+		clear_mctrl(dev, TIOCM_DTR | TIOCM_RTS);
 }
 
 static void ssu100_update_msr(struct usb_serial_port *port, u8 msr)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 64bda13..15af799 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -688,10 +688,20 @@ static int serial_carrier_raised(struct tty_port *port)
 static void serial_dtr_rts(struct tty_port *port, int on)
 {
 	struct usb_serial_port *p = container_of(port, struct usb_serial_port, port);
-	struct usb_serial_driver *drv = p->serial->type;
+	struct usb_serial *serial = p->serial;
+	struct usb_serial_driver *drv = serial->type;
 
-	if (drv->dtr_rts)
+	if (!drv->dtr_rts)
+		return;
+	/*
+	 * Work-around bug in the tty-layer which can result in dtr_rts
+	 * being called after a disconnect (and tty_unregister_device
+	 * has returned). Remove once bug has been squashed.
+	 */
+	mutex_lock(&serial->disc_mutex);
+	if (!serial->disconnected)
 		drv->dtr_rts(p, on);
+	mutex_unlock(&serial->disc_mutex);
 }
 
 static const struct tty_port_operations serial_port_ops = {
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 01c94aa..1355a6c 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -38,7 +38,6 @@
 
 void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
 {
-	struct usb_serial *serial = port->serial;
 	struct usb_wwan_port_private *portdata;
 	struct usb_wwan_intf_private *intfdata;
 
@@ -48,12 +47,11 @@ void usb_wwan_dtr_rts(struct usb_serial_port *port, int on)
 		return;
 
 	portdata = usb_get_serial_port_data(port);
-	mutex_lock(&serial->disc_mutex);
+	/* FIXME: locking */
 	portdata->rts_state = on;
 	portdata->dtr_state = on;
-	if (serial->dev)
-		intfdata->send_setup(port);
-	mutex_unlock(&serial->disc_mutex);
+
+	intfdata->send_setup(port);
 }
 EXPORT_SYMBOL(usb_wwan_dtr_rts);
 
-- 
1.8.1.1

[RFC 0/4] tty: port hangup and close issues

[Johan Hovold]

As mentioned in a previous mail there is a problem in the tty layer
which affects the USB-serial drivers. In summary, a call to dtr_rts()
can be made even after the device has been disconnected and the tty
device unregistered.

This can be worked around in usb-serial core by checking the
disconnected flag in dtr_rts() as we do in activate().

In my setup it can be almost perfectly reproduced by keeping the
usb-serial device open, while repeatedly opening and closing the port.
When the device is disconnected any open ports are properly hung up but
there is a window during disconnect where we can get a second open. This
call will fail in the serial driver because the disconnected flag is
checked in activate(). TTY core still proceeds to close the
never-successfully opened port, which involves the call to dtr_rts().

[ Note that this closing of an uninitialised port seems to be a bug in
  itself, which these patches aim to fix. ]

If the driver has port drain delay set, things get even worse, as we
then reschedule in tty_port_close_start() and usb-serial disconnect() can
proceed with unregistering the device. When scheduled back in, the device
is gone and the call to dtr_rts() will oops (as the usb-serial port
data is now NULL).

[ Note also that there has been a recent bug report, where dtr_rts() is
  called after tty device unregister when apparently only using minicom.
  This could indicate that there are more than one way to trigger this
  problem. ]

While trying to fix this bug I came across two related issues. First, it
turns out that dtr_rts() is normally not called at all during hang up,
and second, that the closing of uninitialised ports needlessly honours
drain delay.

1) With an open port (each open raising DTR/RST), dtr_rts() is never
   called to lower DTR/RTS when we get a hangup. This is because
   any open port reference will get the hung_up_tty_fops, which is later
   checked for in in tty_port_close_start before calling dtr_rts(). [ So
   it is only in the above somewhat construed case, with a second failed
   open that dtr_rts() is even called after a hangup. ]

   I propose to fix this by moving the HUPCL (DTR/RTS) handling from
   tty_port_close_start() to tty_shutdown() as well as protecting it
   with the ASYNC_INITIALISED flag. This way the signals are lowered on
   hangup as well as on the last close of a tty device (as before). This
   is also how things are currently handled in several serial drivers. 

   I've verified that all serial drivers that rely on
   tty_port_close_start() directly (rather than through tty_port_close())
   lowers DTR/RTS as part of their close() except for mxser.c and
   n_gsm.c, which I would need to fix.

   [ Note that some drivers currently seems broken with respect to this,
     as comments indicate that they do not expect tty_port_close_start to
     change the modem status. ]

   Note also that this would fix the usb-serial driver oops as a
   side-effect, as tty_port_shutdown() completes before the device is
   unregistered, and it would not be called again due to
   ASYNC_INITIALIZED.

2) Looking at tty_port_close_start() it seems to me that we should never
   do tty-driver callbacks on ports that have never been opened (as
   mentioned above). Currently, only the call to tty_wait_until_sent()
   is protected by the ASYNC_INITIALIZED flag, but I suggest this to be 
   extended to all driver callbacks as well as the drain-delay handling.

   As things stand today, a port with drain delay set could hang
   needlessly for up to two seconds on every failed open. The final
   patch below fixes this.

   Note that only protecting the driver call-backs could also in itself
   be enough to prevent the oops described above if it's decided best to
   keep HUPCL handling in tty_port_close_start().

The first, and third patches in the series are essentially clean-ups,
while the second patch fixes HUPCL handling and the last fixes the
closing of uninitialised ports.

Johan

   
Johan Hovold (4):
  tty: clean up port shutdown
  tty: fix DTR/RTS not being dropped on hang up
  tty: clean up port drain-delay handling
  tty: fix close of uninitialised ports

 drivers/tty/tty_port.c | 72 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 44 insertions(+), 28 deletions(-)

-- 
1.8.1.1

[RFC 1/4] tty: clean up port shutdown

[Johan Hovold]

Untangle port-shutdown logic and make sure the initialised flag is
always cleared for non-console ports.
---
 drivers/tty/tty_port.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index b7ff59d..57a061e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -199,9 +199,14 @@ EXPORT_SYMBOL(tty_port_tty_set);
 static void tty_port_shutdown(struct tty_port *port)
 {
 	mutex_lock(&port->mutex);
-	if (port->ops->shutdown && !port->console &&
-		test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags))
+	if (port->console)
+		goto out;
+
+	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		if (port->ops->shutdown)
 			port->ops->shutdown(port);
+	}
+out:
 	mutex_unlock(&port->mutex);
 }
 
-- 
1.8.1.1

[RFC 2/4] tty: fix DTR/RTS not being dropped on hang up

[Johan Hovold]

Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
hang up.

Currently a hung up port will return immediately from
tty_port_close_start leaving DTR/RTS unchanged.
---
 drivers/tty/tty_port.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 57a061e..ffe3689 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
 
 static void tty_port_shutdown(struct tty_port *port)
 {
+	struct tty_struct *tty = port->tty;
+
 	mutex_lock(&port->mutex);
 	if (port->console)
 		goto out;
 
 	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/*
+		 * Drop DTR/RTS if HUPCL is set. This causes any attached
+		 * modem to hang up the line.
+		 */
+		if (!tty || tty->termios.c_cflag & HUPCL)
+			tty_port_lower_dtr_rts(port);
+
 		if (port->ops->shutdown)
 			port->ops->shutdown(port);
 	}
@@ -225,15 +234,13 @@ void tty_port_hangup(struct tty_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 	port->count = 0;
 	port->flags &= ~ASYNC_NORMAL_ACTIVE;
-	if (port->tty) {
+	if (port->tty)
 		set_bit(TTY_IO_ERROR, &port->tty->flags);
-		tty_kref_put(port->tty);
-	}
-	port->tty = NULL;
 	spin_unlock_irqrestore(&port->lock, flags);
+	tty_port_shutdown(port);
+	tty_port_tty_set(port, NULL);
 	wake_up_interruptible(&port->open_wait);
 	wake_up_interruptible(&port->delta_msr_wait);
-	tty_port_shutdown(port);
 }
 EXPORT_SYMBOL(tty_port_hangup);
 
@@ -452,11 +459,6 @@ int tty_port_close_start(struct tty_port *port,
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-	/* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
-	   hang up the line */
-	if (tty->termios.c_cflag & HUPCL)
-		tty_port_lower_dtr_rts(port);
-
 	/* Don't call port->drop for the last reference. Callers will want
 	   to drop the last active reference in ->shutdown() or the tty
 	   shutdown path */
-- 
1.8.1.1

[RFC v2 2/4] tty: fix DTR/RTS not being dropped on hang up

[Johan Hovold]

Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
hang up.

Currently a hung up port will return immediately from
tty_port_close_start leaving DTR/RTS unchanged.
---

v2: fix tty refcounting in shutdown

 drivers/tty/tty_port.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 57a061e..506f579 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -198,15 +198,25 @@ EXPORT_SYMBOL(tty_port_tty_set);
 
 static void tty_port_shutdown(struct tty_port *port)
 {
+	struct tty_struct *tty = tty_port_tty_get(port);
+
 	mutex_lock(&port->mutex);
 	if (port->console)
 		goto out;
 
 	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/*
+		 * Drop DTR/RTS if HUPCL is set. This causes any attached
+		 * modem to hang up the line.
+		 */
+		if (!tty || tty->termios.c_cflag & HUPCL)
+			tty_port_lower_dtr_rts(port);
+
 		if (port->ops->shutdown)
 			port->ops->shutdown(port);
 	}
 out:
+	tty_kref_put(tty);
 	mutex_unlock(&port->mutex);
 }
 
@@ -225,15 +235,13 @@ void tty_port_hangup(struct tty_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 	port->count = 0;
 	port->flags &= ~ASYNC_NORMAL_ACTIVE;
-	if (port->tty) {
+	if (port->tty)
 		set_bit(TTY_IO_ERROR, &port->tty->flags);
-		tty_kref_put(port->tty);
-	}
-	port->tty = NULL;
 	spin_unlock_irqrestore(&port->lock, flags);
+	tty_port_shutdown(port);
+	tty_port_tty_set(port, NULL);
 	wake_up_interruptible(&port->open_wait);
 	wake_up_interruptible(&port->delta_msr_wait);
-	tty_port_shutdown(port);
 }
 EXPORT_SYMBOL(tty_port_hangup);
 
@@ -452,11 +460,6 @@ int tty_port_close_start(struct tty_port *port,
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-	/* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
-	   hang up the line */
-	if (tty->termios.c_cflag & HUPCL)
-		tty_port_lower_dtr_rts(port);

Re: [RFC 2/4] tty: fix DTR/RTS not being dropped on hang up

[Peter Hurley]

On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
> hang up.
> 
> Currently a hung up port will return immediately from
> tty_port_close_start leaving DTR/RTS unchanged.
> ---
>  drivers/tty/tty_port.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 57a061e..ffe3689 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
>  
>  static void tty_port_shutdown(struct tty_port *port)
>  {
> +	struct tty_struct *tty = port->tty;
> +
>  	mutex_lock(&port->mutex);
>  	if (port->console)
>  		goto out;
>  
>  	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> +		/*
> +		 * Drop DTR/RTS if HUPCL is set. This causes any attached
> +		 * modem to hang up the line.
> +		 */
> +		if (!tty || tty->termios.c_cflag & HUPCL)
> +			tty_port_lower_dtr_rts(port);
> +

port->ops->shutdown() requires the hardware to reset anyway, including
the DTR/RTS state.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/4] tty: fix DTR/RTS not being dropped on hang up

[Peter Hurley]

On Wed, 2013-02-13 at 14:32 -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> > Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
> > hang up.
> > 
> > Currently a hung up port will return immediately from
> > tty_port_close_start leaving DTR/RTS unchanged.
> > ---
> >  drivers/tty/tty_port.c | 22 ++++++++++++----------
> >  1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 57a061e..ffe3689 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
> >  
> >  static void tty_port_shutdown(struct tty_port *port)
> >  {
> > +	struct tty_struct *tty = port->tty;
> > +
> >  	mutex_lock(&port->mutex);
> >  	if (port->console)
> >  		goto out;
> >  
> >  	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> > +		/*
> > +		 * Drop DTR/RTS if HUPCL is set. This causes any attached
> > +		 * modem to hang up the line.
> > +		 */
> > +		if (!tty || tty->termios.c_cflag & HUPCL)
> > +			tty_port_lower_dtr_rts(port);
> > +
> 
> port->ops->shutdown() requires the hardware to reset anyway, including
> the DTR/RTS state.

Ok, after reviewing this further, I like this more.

For one, there is more flexibility for tty drivers that have a custom
close() routine (ie, one that doesn't call tty_port_close()). At the
same time, every call site of tty_port_close_start() needs to be checked
to ensure it does not assume (or does not require) DTR to be dropped:

char/pcmcia/synclink_cs.c:	if (tty_port_close_start(port, tty, filp) == 0)
tty/synclinkmp.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)
tty/rocket.c:	if (tty_port_close_start(port, tty, filp) == 0)
tty/amiserial.c:	if (tty_port_close_start(port, tty, filp) == 0)
tty/n_gsm.c:	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
tty/mxser.c:	if (tty_port_close_start(port, tty, filp) == 0)
tty/synclink_gt.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)
tty/serial/serial_core.c:	if (tty_port_close_start(port, tty, filp) == 0)
tty/synclink.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)			 

In the case of the serial core, this actually fixes the code excerpt
below where tty_port_close_start() prematurely drops DTR before the uart
can stop_rx() or wait_until_sent().

static void uart_close(struct tty_struct *tty, struct file *filp)
{
	struct uart_state *state = tty->driver_data;
	struct tty_port *port;
	struct uart_port *uport;
	unsigned long flags;

	if (!state)
		return;

	uport = state->uart_port;
	port = &state->port;

	pr_debug("uart_close(%d) called\n", uport->line);

	if (tty_port_close_start(port, tty, filp) == 0)
		return;

	/*
	 * At this point, we stop accepting input.  To do this, we
	 * disable the receive line status interrupts.
	 */
	if (port->flags & ASYNC_INITIALIZED) {
		unsigned long flags;
		spin_lock_irqsave(&uport->lock, flags);
		uport->ops->stop_rx(uport);
		spin_unlock_irqrestore(&uport->lock, flags);
		/*
		 * Before we drop DTR, make sure the UART transmitter
		 * has completely drained; this is especially
		 * important if there is a transmit FIFO!
		 */
		uart_wait_until_sent(tty, uport->timeout);
	}

	mutex_lock(&port->mutex);
	uart_shutdown(tty, state);
	uart_flush_buffer(tty);


Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/4] tty: fix DTR/RTS not being dropped on hang up

[Johan Hovold]

On Wed, Feb 13, 2013 at 08:11:13PM -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 14:32 -0500, Peter Hurley wrote:
> > On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> > > Move HUPCL handling to port shutdown so that DTR/RTS is dropped also on
> > > hang up.
> > > 
> > > Currently a hung up port will return immediately from
> > > tty_port_close_start leaving DTR/RTS unchanged.
> > > ---
> > >  drivers/tty/tty_port.c | 22 ++++++++++++----------
> > >  1 file changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > > index 57a061e..ffe3689 100644
> > > --- a/drivers/tty/tty_port.c
> > > +++ b/drivers/tty/tty_port.c
> > > @@ -198,11 +198,20 @@ EXPORT_SYMBOL(tty_port_tty_set);
> > >  
> > >  static void tty_port_shutdown(struct tty_port *port)
> > >  {
> > > +	struct tty_struct *tty = port->tty;
> > > +
> > >  	mutex_lock(&port->mutex);
> > >  	if (port->console)
> > >  		goto out;
> > >  
> > >  	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> > > +		/*
> > > +		 * Drop DTR/RTS if HUPCL is set. This causes any attached
> > > +		 * modem to hang up the line.
> > > +		 */
> > > +		if (!tty || tty->termios.c_cflag & HUPCL)
> > > +			tty_port_lower_dtr_rts(port);
> > > +
> > 
> > port->ops->shutdown() requires the hardware to reset anyway, including
> > the DTR/RTS state.
> 
> Ok, after reviewing this further, I like this more.
> 
> For one, there is more flexibility for tty drivers that have a custom
> close() routine (ie, one that doesn't call tty_port_close()). At the
> same time, every call site of tty_port_close_start() needs to be checked
> to ensure it does not assume (or does not require) DTR to be dropped:
> 
> char/pcmcia/synclink_cs.c:	if (tty_port_close_start(port, tty, filp) == 0)
> tty/synclinkmp.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)
> tty/rocket.c:	if (tty_port_close_start(port, tty, filp) == 0)
> tty/amiserial.c:	if (tty_port_close_start(port, tty, filp) == 0)
> tty/n_gsm.c:	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
> tty/mxser.c:	if (tty_port_close_start(port, tty, filp) == 0)
> tty/synclink_gt.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)
> tty/serial/serial_core.c:	if (tty_port_close_start(port, tty, filp) == 0)
> tty/synclink.c:	if (tty_port_close_start(&info->port, tty, filp) == 0)			 
> 
> In the case of the serial core, this actually fixes the code excerpt
> below where tty_port_close_start() prematurely drops DTR before the uart
> can stop_rx() or wait_until_sent().

Yeah, I mentioned both of these points in the cover letter of the
series. From a quick browse it seemed that I would have to fix n_gsm.c
and mxser.c, but I'll have a closer look at this later.

Thanks,
Johan

> static void uart_close(struct tty_struct *tty, struct file *filp)
> {
> 	struct uart_state *state = tty->driver_data;
> 	struct tty_port *port;
> 	struct uart_port *uport;
> 	unsigned long flags;
> 
> 	if (!state)
> 		return;
> 
> 	uport = state->uart_port;
> 	port = &state->port;
> 
> 	pr_debug("uart_close(%d) called\n", uport->line);
> 
> 	if (tty_port_close_start(port, tty, filp) == 0)
> 		return;
> 
> 	/*
> 	 * At this point, we stop accepting input.  To do this, we
> 	 * disable the receive line status interrupts.
> 	 */
> 	if (port->flags & ASYNC_INITIALIZED) {
> 		unsigned long flags;
> 		spin_lock_irqsave(&uport->lock, flags);
> 		uport->ops->stop_rx(uport);
> 		spin_unlock_irqrestore(&uport->lock, flags);
> 		/*
> 		 * Before we drop DTR, make sure the UART transmitter
> 		 * has completely drained; this is especially
> 		 * important if there is a transmit FIFO!
> 		 */
> 		uart_wait_until_sent(tty, uport->timeout);
> 	}
> 
> 	mutex_lock(&port->mutex);
> 	uart_shutdown(tty, state);
> 	uart_flush_buffer(tty);
> 
> 
> Regards,
> Peter Hurley
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 3/4] tty: clean up port drain-delay handling

[Johan Hovold]

Move port drain-delay handling to a separate function.
---
 drivers/tty/tty_port.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index ffe3689..0a5e955 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -407,6 +407,23 @@ int tty_port_block_til_ready(struct tty_port *port,
 }
 EXPORT_SYMBOL(tty_port_block_til_ready);
 
+static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
+{
+	unsigned int bps = tty_get_baud_rate(tty);
+	long timeout;
+
+	if (!port->drain_delay)
+		return;
+
+	if (bps > 1200) {
+		timeout = (HZ * 10 * port->drain_delay) / bps;
+		timeout = max_t(long, timeout, HZ / 10);
+	} else {
+		timeout = 2 * HZ;
+	}
+	schedule_timeout_interruptible(timeout);
+}
+
 int tty_port_close_start(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
@@ -445,17 +462,7 @@ int tty_port_close_start(struct tty_port *port,
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
 			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	if (port->drain_delay) {
-		unsigned int bps = tty_get_baud_rate(tty);
-		long timeout;
-
-		if (bps > 1200)
-			timeout = max_t(long,
-				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
-		else
-			timeout = 2 * HZ;
-		schedule_timeout_interruptible(timeout);
-	}
+	tty_port_drain_delay(port, tty);
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

Re: [RFC 3/4] tty: clean up port drain-delay handling

[Peter Hurley]

On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> Move port drain-delay handling to a separate function.
> ---
>  drivers/tty/tty_port.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index ffe3689..0a5e955 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -407,6 +407,23 @@ int tty_port_block_til_ready(struct tty_port *port,
>  }
>  EXPORT_SYMBOL(tty_port_block_til_ready);
>  
> +static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
> +{
> +	unsigned int bps = tty_get_baud_rate(tty);
> +	long timeout;


> +	if (!port->drain_delay)
> +		return;
               ^^^^^^^^^^^^
How about drop this and ...

> +	if (bps > 1200) {
> +		timeout = (HZ * 10 * port->drain_delay) / bps;
> +		timeout = max_t(long, timeout, HZ / 10);
> +	} else {
> +		timeout = 2 * HZ;
> +	}
> +	schedule_timeout_interruptible(timeout);
> +}
> +
>  int tty_port_close_start(struct tty_port *port,
>  				struct tty_struct *tty, struct file *filp)
>  {
> @@ -445,17 +462,7 @@ int tty_port_close_start(struct tty_port *port,
>  	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
>  			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
>  		tty_wait_until_sent_from_close(tty, port->closing_wait);
> -	if (port->drain_delay) {
> -		unsigned int bps = tty_get_baud_rate(tty);
> -		long timeout;
> -
> -		if (bps > 1200)
> -			timeout = max_t(long,
> -				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
> -		else
> -			timeout = 2 * HZ;
> -		schedule_timeout_interruptible(timeout);
> -	}
> +	tty_port_drain_delay(port, tty);

....
	if (port->drain_delay)
		tty_port_drain_delay(port, tty);

>  	/* Flush the ldisc buffering */
>  	tty_ldisc_flush(tty);
>  

Re: [RFC 3/4] tty: clean up port drain-delay handling

[Johan Hovold]

On Wed, Feb 13, 2013 at 08:39:31PM -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 18:27 +0100, Johan Hovold wrote:
> > Move port drain-delay handling to a separate function.
> > ---
> >  drivers/tty/tty_port.c | 29 ++++++++++++++++++-----------
> >  1 file changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index ffe3689..0a5e955 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -407,6 +407,23 @@ int tty_port_block_til_ready(struct tty_port *port,
> >  }
> >  EXPORT_SYMBOL(tty_port_block_til_ready);
> >  
> > +static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
> > +{
> > +	unsigned int bps = tty_get_baud_rate(tty);
> > +	long timeout;
> 
> 
> > +	if (!port->drain_delay)
> > +		return;
>                ^^^^^^^^^^^^
> How about drop this and ...
> 
> > +	if (bps > 1200) {
> > +		timeout = (HZ * 10 * port->drain_delay) / bps;
> > +		timeout = max_t(long, timeout, HZ / 10);
> > +	} else {
> > +		timeout = 2 * HZ;
> > +	}
> > +	schedule_timeout_interruptible(timeout);
> > +}
> > +
> >  int tty_port_close_start(struct tty_port *port,
> >  				struct tty_struct *tty, struct file *filp)
> >  {
> > @@ -445,17 +462,7 @@ int tty_port_close_start(struct tty_port *port,
> >  	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
> >  			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
> >  		tty_wait_until_sent_from_close(tty, port->closing_wait);
> > -	if (port->drain_delay) {
> > -		unsigned int bps = tty_get_baud_rate(tty);
> > -		long timeout;
> > -
> > -		if (bps > 1200)
> > -			timeout = max_t(long,
> > -				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
> > -		else
> > -			timeout = 2 * HZ;
> > -		schedule_timeout_interruptible(timeout);
> > -	}
> > +	tty_port_drain_delay(port, tty);
> 
> ....
> 	if (port->drain_delay)
> 		tty_port_drain_delay(port, tty);

Yeah, I agree. Checking at the call site would perhaps be more clear in
this case.

Thanks,
Johan

[RFC 4/4] tty: fix close of uninitialised ports

[Johan Hovold]

Make sure we do not make tty driver callbacks or wait for port to drain
on uninitialised ports (e.g. when open failed) in
tty_port_close_start().

No callbacks should be made on a port that has never been opened.
Neither does it make any sense to add drain delay for an uninitialised
port.

Currently a drain delay of up to two seconds could be added when a tty
fails to open.
---
 drivers/tty/tty_port.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 0a5e955..46edb98 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -456,13 +456,15 @@ int tty_port_close_start(struct tty_port *port,
 	set_bit(ASYNCB_CLOSING, &port->flags);
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);
-	/* Don't block on a stalled port, just pull the chain */
-	if (tty->flow_stopped)
-		tty_driver_flush_buffer(tty);
-	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
-			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	tty_port_drain_delay(port, tty);
+
+	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/* Don't block on a stalled port, just pull the chain */
+		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_port_drain_delay(port, tty);
+	}
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

Re: [RFC 0/4] tty: port hangup and close issues

[Alan Cox]

> [ Note that this closing of an uninitialised port seems to be a bug in
>   itself, which these patches aim to fix. ]

You don't want to be cc'ing me on these - not my problem any more.

(but you might want to fix the fact you reference port->tty without the
lock or refcounts 8))

Alan

Re: [RFC 0/4] tty: port hangup and close issues

[Johan Hovold]

On Wed, Feb 13, 2013 at 05:36:30PM +0000, Alan Cox wrote:
> > [ Note that this closing of an uninitialised port seems to be a bug in
> >   itself, which these patches aim to fix. ]
> 
> You don't want to be cc'ing me on these - not my problem any more.
> 
> (but you might want to fix the fact you reference port->tty without the
> lock or refcounts 8))

Thanks. :) Removing you from Cc.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] TTY: port hangup and close fixes

[Johan Hovold]

These patches against tty-next fix a few issues with tty-port hangup and
close.

The first and third patch are essentially clean ups.

The second patch makes sure DTR is dropped also on hangup and that DTR
is only dropped for initialised ports (where is could have been raised
in the first place).

The fourth and final patch, make sure no tty callbacks are made from
tty_port_close_start when the port has not been initialised (successfully
opened). This was previously only done for wait_until_sent but there's
no reason to call flush_buffer or to honour port drain delay either.
The latter could cause a failed open to stall for up to two seconds.

As a side-effect, this patches also fix an issue in USB-serial where we
could get tty-port callbacks on an uninitialised port after having hung
up and unregistered a device after disconnect.

Johan


Changes since RFC-series:
 - fix up the two driver relying on tty_port_close_start directly but
   that did not manage DTR themselves


Johan Hovold (4):
  TTY: clean up port shutdown
  TTY: fix DTR not being dropped on hang up
  TTY: clean up port drain-delay handling
  TTY: fix close of uninitialised ports

 drivers/tty/mxser.c    |  4 +++
 drivers/tty/n_gsm.c    |  4 +++
 drivers/tty/tty_port.c | 71 ++++++++++++++++++++++++++++++--------------------
 3 files changed, 51 insertions(+), 28 deletions(-)

-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] TTY: clean up port shutdown

[Johan Hovold]

Untangle port-shutdown logic and make sure the initialised flag is
always cleared for non-console ports.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index b7ff59d..57a061e 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -199,9 +199,14 @@ EXPORT_SYMBOL(tty_port_tty_set);
 static void tty_port_shutdown(struct tty_port *port)
 {
 	mutex_lock(&port->mutex);
-	if (port->ops->shutdown && !port->console &&
-		test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags))
+	if (port->console)
+		goto out;
+
+	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		if (port->ops->shutdown)
 			port->ops->shutdown(port);
+	}
+out:
 	mutex_unlock(&port->mutex);
 }
 
-- 
1.8.1.1

[PATCH 2/4] TTY: fix DTR not being dropped on hang up

[Johan Hovold]

Move HUPCL handling to port shutdown so that DTR is dropped also on hang
up (tty_port_close is a noop for hung-up ports).

Also do not try to drop DTR for uninitialised ports where it has never
been raised (e.g. after a failed open).

Nine drivers currently call tty_port_close_start directly (rather than
through tty_port_close) and seven of them lower DTR as part of their
close (if the port has been initialised). Fixup the remaining two
drivers so that it continues to be lowered also on normal (non-HUP)
close. [ Note that most of those other seven drivers did not expect DTR
to have been dropped by tty_port_close_start in the first place. ]

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/mxser.c    |  4 ++++
 drivers/tty/n_gsm.c    |  4 ++++
 drivers/tty/tty_port.c | 23 +++++++++++++----------
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
index 484b6a3..c547887 100644
--- a/drivers/tty/mxser.c
+++ b/drivers/tty/mxser.c
@@ -1084,6 +1084,10 @@ static void mxser_close(struct tty_struct *tty, struct file *filp)
 	mutex_lock(&port->mutex);
 	mxser_close_port(port);
 	mxser_flush_buffer(tty);
+	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		if (tty->termios.c_cflag & HUPCL)
+			tty_port_lower_dtr_rts(port);
+	}
 	mxser_shutdown_port(port);
 	clear_bit(ASYNCB_INITIALIZED, &port->flags);
 	mutex_unlock(&port->mutex);
diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
index 4a43ef5d7..049013e 100644
--- a/drivers/tty/n_gsm.c
+++ b/drivers/tty/n_gsm.c
@@ -2968,6 +2968,10 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
 	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
 		goto out;
 	gsm_dlci_begin_close(dlci);
+	if (test_bit(ASYNCB_INITIALIZED, &dlci->port.flags)) {
+		if (tty->termios.c_cflag & HUPCL)
+			tty_port_lower_dtr_rts(&dlci->port);
+	}
 	tty_port_close_end(&dlci->port, tty);
 	tty_port_tty_set(&dlci->port, NULL);
 out:
diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 57a061e..506f579 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -198,15 +198,25 @@ EXPORT_SYMBOL(tty_port_tty_set);
 
 static void tty_port_shutdown(struct tty_port *port)
 {
+	struct tty_struct *tty = tty_port_tty_get(port);
+
 	mutex_lock(&port->mutex);
 	if (port->console)
 		goto out;
 
 	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/*
+		 * Drop DTR/RTS if HUPCL is set. This causes any attached
+		 * modem to hang up the line.
+		 */
+		if (!tty || tty->termios.c_cflag & HUPCL)
+			tty_port_lower_dtr_rts(port);
+
 		if (port->ops->shutdown)
 			port->ops->shutdown(port);
 	}
 out:
+	tty_kref_put(tty);
 	mutex_unlock(&port->mutex);
 }
 
@@ -225,15 +235,13 @@ void tty_port_hangup(struct tty_port *port)
 	spin_lock_irqsave(&port->lock, flags);
 	port->count = 0;
 	port->flags &= ~ASYNC_NORMAL_ACTIVE;
-	if (port->tty) {
+	if (port->tty)
 		set_bit(TTY_IO_ERROR, &port->tty->flags);
-		tty_kref_put(port->tty);
-	}
-	port->tty = NULL;
 	spin_unlock_irqrestore(&port->lock, flags);
+	tty_port_shutdown(port);
+	tty_port_tty_set(port, NULL);
 	wake_up_interruptible(&port->open_wait);
 	wake_up_interruptible(&port->delta_msr_wait);
-	tty_port_shutdown(port);
 }
 EXPORT_SYMBOL(tty_port_hangup);
 
@@ -452,11 +460,6 @@ int tty_port_close_start(struct tty_port *port,
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-	/* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
-	   hang up the line */
-	if (tty->termios.c_cflag & HUPCL)
-		tty_port_lower_dtr_rts(port);
-
 	/* Don't call port->drop for the last reference. Callers will want
 	   to drop the last active reference in ->shutdown() or the tty
 	   shutdown path */
-- 
1.8.1.1

Re: [PATCH 2/4] TTY: fix DTR not being dropped on hang up

[Peter Hurley]

On Wed, 2013-02-20 at 17:02 +0100, Johan Hovold wrote:
> Move HUPCL handling to port shutdown so that DTR is dropped also on hang
> up (tty_port_close is a noop for hung-up ports).
> 
> Also do not try to drop DTR for uninitialised ports where it has never
> been raised (e.g. after a failed open).
> 
> Nine drivers currently call tty_port_close_start directly (rather than
> through tty_port_close) and seven of them lower DTR as part of their
> close (if the port has been initialised). Fixup the remaining two
> drivers so that it continues to be lowered also on normal (non-HUP)
> close. [ Note that most of those other seven drivers did not expect DTR
> to have been dropped by tty_port_close_start in the first place. ]
> 
> Signed-off-by: Johan Hovold <jhovold@gmail.com>
> ---
>  drivers/tty/mxser.c    |  4 ++++
>  drivers/tty/n_gsm.c    |  4 ++++
>  drivers/tty/tty_port.c | 23 +++++++++++++----------
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
> index 484b6a3..c547887 100644
> --- a/drivers/tty/mxser.c
> +++ b/drivers/tty/mxser.c
> @@ -1084,6 +1084,10 @@ static void mxser_close(struct tty_struct *tty, struct file *filp)
>  	mutex_lock(&port->mutex);
>  	mxser_close_port(port);
>  	mxser_flush_buffer(tty);
> +	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
> +		if (tty->termios.c_cflag & HUPCL)
> +			tty_port_lower_dtr_rts(port);
> +	}
>  	mxser_shutdown_port(port);
>  	clear_bit(ASYNCB_INITIALIZED, &port->flags);
>  	mutex_unlock(&port->mutex);
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index 4a43ef5d7..049013e 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2968,6 +2968,10 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
>  	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
>  		goto out;
>  	gsm_dlci_begin_close(dlci);
> +	if (test_bit(ASYNCB_INITIALIZED, &dlci->port.flags)) {
> +		if (tty->termios.c_cflag & HUPCL)
> +			tty_port_lower_dtr_rts(&dlci->port);
> +	}
>  	tty_port_close_end(&dlci->port, tty);
>  	tty_port_tty_set(&dlci->port, NULL);
>  out:
> diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> index 57a061e..506f579 100644
> --- a/drivers/tty/tty_port.c
> +++ b/drivers/tty/tty_port.c
> @@ -198,15 +198,25 @@ EXPORT_SYMBOL(tty_port_tty_set);
>  
>  static void tty_port_shutdown(struct tty_port *port)
>  {
> +	struct tty_struct *tty = tty_port_tty_get(port);
> +

I know I said this was done, but if this hasn't been pushed into
tty-next yet, I'd rather this was:

static void tty_port_shutdown(struct tty_port *port,
			      struct tty_struct *tty)

and not get the port reference. Both call sites already ensure that
there is a kref on the tty or tty is NULL.

(I ended up re-looking at this patch to double-check that it won't drop
DTR for the console, which it doesn't so that is good).

>  	mutex_lock(&port->mutex);
>  	if (port->console)
>  		goto out;
>  
>  	if (test_and_clear_bit(ASYNCB_INITIALIZED, &port->flags)) {
> +		/*
> +		 * Drop DTR/RTS if HUPCL is set. This causes any attached
> +		 * modem to hang up the line.
> +		 */
> +		if (!tty || tty->termios.c_cflag & HUPCL)
> +			tty_port_lower_dtr_rts(port);
> +
>  		if (port->ops->shutdown)
>  			port->ops->shutdown(port);
>  	}
>  out:
> +	tty_kref_put(tty);
>  	mutex_unlock(&port->mutex);
>  }
>  
> @@ -225,15 +235,13 @@ void tty_port_hangup(struct tty_port *port)
>  	spin_lock_irqsave(&port->lock, flags);
>  	port->count = 0;
>  	port->flags &= ~ASYNC_NORMAL_ACTIVE;
> -	if (port->tty) {
> +	if (port->tty)
>  		set_bit(TTY_IO_ERROR, &port->tty->flags);
> -		tty_kref_put(port->tty);
> -	}
> -	port->tty = NULL;
>  	spin_unlock_irqrestore(&port->lock, flags);
> +	tty_port_shutdown(port);
> +	tty_port_tty_set(port, NULL);
>  	wake_up_interruptible(&port->open_wait);
>  	wake_up_interruptible(&port->delta_msr_wait);
> -	tty_port_shutdown(port);
>  }
>  EXPORT_SYMBOL(tty_port_hangup);
>  
> @@ -452,11 +460,6 @@ int tty_port_close_start(struct tty_port *port,
>  	/* Flush the ldisc buffering */
>  	tty_ldisc_flush(tty);
>  
> -	/* Drop DTR/RTS if HUPCL is set. This causes any attached modem to
> -	   hang up the line */
> -	if (tty->termios.c_cflag & HUPCL)
> -		tty_port_lower_dtr_rts(port);
> -
>  	/* Don't call port->drop for the last reference. Callers will want
>  	   to drop the last active reference in ->shutdown() or the tty
>  	   shutdown path */

Re: [PATCH 2/4] TTY: fix DTR not being dropped on hang up

[Johan Hovold]

On Sat, Feb 23, 2013 at 09:18:02AM -0500, Peter Hurley wrote:
> On Wed, 2013-02-20 at 17:02 +0100, Johan Hovold wrote:
> > Move HUPCL handling to port shutdown so that DTR is dropped also on hang
> > up (tty_port_close is a noop for hung-up ports).
> > 
> > Also do not try to drop DTR for uninitialised ports where it has never
> > been raised (e.g. after a failed open).
> > 
> > Nine drivers currently call tty_port_close_start directly (rather than
> > through tty_port_close) and seven of them lower DTR as part of their
> > close (if the port has been initialised). Fixup the remaining two
> > drivers so that it continues to be lowered also on normal (non-HUP)
> > close. [ Note that most of those other seven drivers did not expect DTR
> > to have been dropped by tty_port_close_start in the first place. ]
> > 
> > Signed-off-by: Johan Hovold <jhovold@gmail.com>
> > ---
> >  drivers/tty/mxser.c    |  4 ++++
> >  drivers/tty/n_gsm.c    |  4 ++++
> >  drivers/tty/tty_port.c | 23 +++++++++++++----------
> >  3 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/tty/mxser.c b/drivers/tty/mxser.c
> > index 484b6a3..c547887 100644
> > --- a/drivers/tty/mxser.c
> > +++ b/drivers/tty/mxser.c
> > @@ -1084,6 +1084,10 @@ static void mxser_close(struct tty_struct *tty, struct file *filp)
> >  	mutex_lock(&port->mutex);
> >  	mxser_close_port(port);
> >  	mxser_flush_buffer(tty);
> > +	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
> > +		if (tty->termios.c_cflag & HUPCL)
> > +			tty_port_lower_dtr_rts(port);
> > +	}
> >  	mxser_shutdown_port(port);
> >  	clear_bit(ASYNCB_INITIALIZED, &port->flags);
> >  	mutex_unlock(&port->mutex);
> > diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> > index 4a43ef5d7..049013e 100644
> > --- a/drivers/tty/n_gsm.c
> > +++ b/drivers/tty/n_gsm.c
> > @@ -2968,6 +2968,10 @@ static void gsmtty_close(struct tty_struct *tty, struct file *filp)
> >  	if (tty_port_close_start(&dlci->port, tty, filp) == 0)
> >  		goto out;
> >  	gsm_dlci_begin_close(dlci);
> > +	if (test_bit(ASYNCB_INITIALIZED, &dlci->port.flags)) {
> > +		if (tty->termios.c_cflag & HUPCL)
> > +			tty_port_lower_dtr_rts(&dlci->port);
> > +	}
> >  	tty_port_close_end(&dlci->port, tty);
> >  	tty_port_tty_set(&dlci->port, NULL);
> >  out:
> > diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
> > index 57a061e..506f579 100644
> > --- a/drivers/tty/tty_port.c
> > +++ b/drivers/tty/tty_port.c
> > @@ -198,15 +198,25 @@ EXPORT_SYMBOL(tty_port_tty_set);
> >  
> >  static void tty_port_shutdown(struct tty_port *port)
> >  {
> > +	struct tty_struct *tty = tty_port_tty_get(port);
> > +
> 
> I know I said this was done, but if this hasn't been pushed into
> tty-next yet, I'd rather this was:
> 
> static void tty_port_shutdown(struct tty_port *port,
> 			      struct tty_struct *tty)
> 
> and not get the port reference. Both call sites already ensure that
> there is a kref on the tty or tty is NULL.
> 
> (I ended up re-looking at this patch to double-check that it won't drop
> DTR for the console, which it doesn't so that is good).

I'll send a v2.

Thanks, 
Johan

[PATCH 3/4] TTY: clean up port drain-delay handling

[Johan Hovold]

Move port drain-delay handling to a separate function.

Signed-off-by: Johan Hovold <jhovold@gmail.com>
---
 drivers/tty/tty_port.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index 506f579..fcb4ccb 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -408,6 +408,20 @@ int tty_port_block_til_ready(struct tty_port *port,
 }
 EXPORT_SYMBOL(tty_port_block_til_ready);
 
+static void tty_port_drain_delay(struct tty_port *port, struct tty_struct *tty)
+{
+	unsigned int bps = tty_get_baud_rate(tty);
+	long timeout;
+
+	if (bps > 1200) {
+		timeout = (HZ * 10 * port->drain_delay) / bps;
+		timeout = max_t(long, timeout, HZ / 10);
+	} else {
+		timeout = 2 * HZ;
+	}
+	schedule_timeout_interruptible(timeout);
+}
+
 int tty_port_close_start(struct tty_port *port,
 				struct tty_struct *tty, struct file *filp)
 {
@@ -446,17 +460,8 @@ int tty_port_close_start(struct tty_port *port,
 	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
 			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
 		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	if (port->drain_delay) {
-		unsigned int bps = tty_get_baud_rate(tty);
-		long timeout;
-
-		if (bps > 1200)
-			timeout = max_t(long,
-				(HZ * 10 * port->drain_delay) / bps, HZ / 10);
-		else
-			timeout = 2 * HZ;
-		schedule_timeout_interruptible(timeout);
-	}
+	if (port->drain_delay)
+		tty_port_drain_delay(port, tty);
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

[PATCH 4/4] TTY: fix close of uninitialised ports

[Johan Hovold]

Make sure we do not make tty-driver callbacks or wait for port to drain
on uninitialised ports (e.g. when open failed) in
tty_port_close_start().

No callback, such as flush_buffer or wait_until_sent, needs to be made
on a port that has never been opened. Neither does it make much sense to
add drain delay for an uninitialised port.

Currently a drain delay of up to two seconds could be added when a tty
fails to open.

Signed-off-by: Johan Hovold <jhovold-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/tty/tty_port.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/tty_port.c b/drivers/tty/tty_port.c
index fcb4ccb..2ff454c 100644
--- a/drivers/tty/tty_port.c
+++ b/drivers/tty/tty_port.c
@@ -454,14 +454,16 @@ int tty_port_close_start(struct tty_port *port,
 	set_bit(ASYNCB_CLOSING, &port->flags);
 	tty->closing = 1;
 	spin_unlock_irqrestore(&port->lock, flags);
-	/* Don't block on a stalled port, just pull the chain */
-	if (tty->flow_stopped)
-		tty_driver_flush_buffer(tty);
-	if (test_bit(ASYNCB_INITIALIZED, &port->flags) &&
-			port->closing_wait != ASYNC_CLOSING_WAIT_NONE)
-		tty_wait_until_sent_from_close(tty, port->closing_wait);
-	if (port->drain_delay)
-		tty_port_drain_delay(port, tty);
+
+	if (test_bit(ASYNCB_INITIALIZED, &port->flags)) {
+		/* Don't block on a stalled port, just pull the chain */
+		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);
+		if (port->drain_delay)
+			tty_port_drain_delay(port, tty);
+	}
 	/* Flush the ldisc buffering */
 	tty_ldisc_flush(tty);
 
-- 
1.8.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] TTY: port hangup and close fixes

[Peter Hurley]

On Wed, 2013-02-20 at 17:02 +0100, Johan Hovold wrote:
> These patches against tty-next fix a few issues with tty-port hangup and
> close.
> 
> The first and third patch are essentially clean ups.
> 
> The second patch makes sure DTR is dropped also on hangup and that DTR
> is only dropped for initialised ports (where is could have been raised
> in the first place).
> 
> The fourth and final patch, make sure no tty callbacks are made from
> tty_port_close_start when the port has not been initialised (successfully
> opened). This was previously only done for wait_until_sent but there's
> no reason to call flush_buffer or to honour port drain delay either.
> The latter could cause a failed open to stall for up to two seconds.
> 
> As a side-effect, this patches also fix an issue in USB-serial where we
> could get tty-port callbacks on an uninitialised port after having hung
> up and unregistered a device after disconnect.
> 
> Johan

Looks good to me. No further objections :)

> Changes since RFC-series:
>  - fix up the two driver relying on tty_port_close_start directly but
>    that did not manage DTR themselves
> 
> 
> Johan Hovold (4):
>   TTY: clean up port shutdown
>   TTY: fix DTR not being dropped on hang up
>   TTY: clean up port drain-delay handling
>   TTY: fix close of uninitialised ports
> 
>  drivers/tty/mxser.c    |  4 +++
>  drivers/tty/n_gsm.c    |  4 +++
>  drivers/tty/tty_port.c | 71 ++++++++++++++++++++++++++++++--------------------
>  3 files changed, 51 insertions(+), 28 deletions(-)
> 

Re: USB Ooops PL2303 when unplug while use (linux v3.7.3)

[Peter Hurley]

On Wed, 2013-02-13 at 15:25 +0100, Johan Hovold wrote:
> On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> > I file a report for you, please have a look when you have time.
> 
> Thanks for the bug report, Chris.
> 
> You have come across what looks like a known issue, which since it's
> discovery last summer has been made worse by an unrelated change.
> 
> A similar oops was reported and its cause identified in this thread:
> 
> 	http://marc.info/?l=linux-usb&m=133837337927749&w=2
> 
> It turns out that the tty-layer may call the driver's dtr_rts even after
> the device has been disconnected and the tty device unregistered. Since
> last summer another change has made the problem worse by setting the
> port data to NULL which results in even more drivers hitting the
> problem.

The tty driver's close() routine must be called even if the open()
failed because the tty layer doesn't know if the driver left unfinished
business and is expecting to receive a close() to cleanup even if it
failed the open(). This behavior was just recently documented in
include/linux/tty_driver.h (ie., is in linux-next).

> While waiting for input from the tty-guru Alan Cox, and as the immediate
> cause of that oops was remedied (by moving the offending interface
> access in the driver in question), the problem was unfortunately
> forgotten (or rather down-prioritised) until now.

Looks to me like a bug the usb serial mini-port interface design.
A usb bus disconnect has the pl2303 (and every other) mini-port freeing
the private data (before unregistering with tty anyway -- not that
putting that first would fix the problem).

static int usb_serial_device_remove(struct device *dev)
{
	struct usb_serial_driver *driver;
	struct usb_serial_port *port;
	int retval = 0;
	int minor;

	port = to_usb_serial_port(dev);
	if (!port)
		return -ENODEV;

	/* make sure suspend/resume doesn't race against port_remove */
	usb_autopm_get_interface(port->serial->interface);

	device_remove_file(&port->dev, &dev_attr_port_number);

	driver = port->serial->type;
	if (driver->port_remove)
====>		retval = driver->port_remove(port);

	minor = port->number;
	tty_unregister_device(usb_serial_tty_driver, minor);
	dev_info(dev, "%s converter now disconnected from ttyUSB%d\n",
		 driver->description, minor);

	usb_autopm_put_interface(port->serial->interface);
	return retval;
}

The pl2303 mini-port dutifully:

static int pl2303_port_remove(struct usb_serial_port *port)
{
	struct pl2303_private *priv;

	priv = usb_get_serial_port_data(port);
===>	kfree(priv);

	return 0;
}

while the tty layer still has outstanding references to the port.

static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
{
=====>	struct pl2303_private *priv = usb_get_serial_port_data(port);
	unsigned long flags;
	u8 control;

	[...]


The tty layer (and its port implementation) can't protect the pl2303
mini-port from this behavior/interface design.

It's the glue layer's responsibility to correctly manage the differing
lifetimes of its bus devices with tty devices (and tty_ports, if used).

Meaning, that if the physical device disconnects from the bus, the ports
must simply become in-operative; they can't disappear.

BTW, just fixing this one part won't work because the usb serial driver
is also abruptly deleting the usb_serial_port device as well:

static void usb_serial_disconnect(struct usb_interface *interface)
{
	[...]

	for (i = 0; i < serial->num_ports; ++i) {
		port = serial->port[i];
		if (port) {
			struct tty_struct *tty = tty_port_tty_get(&port->port);
			if (tty) {
				tty_vhangup(tty);
				tty_kref_put(tty);
			}
			kill_traffic(port);
			cancel_work_sync(&port->work);
			if (device_is_registered(&port->dev))
========>			device_del(&port->dev);
		}
	}
	
	[...]
}

Ummm, wasn't that where the port private data pointer was?

Regards,
Peter Hurley

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB Ooops PL2303 when unplug while use (linux v3.7.3)

[Johan Hovold]

[ Remembered to drop Alan Cox from Cc. ]

On Wed, Feb 13, 2013 at 01:40:08PM -0500, Peter Hurley wrote:
> On Wed, 2013-02-13 at 15:25 +0100, Johan Hovold wrote:
> > On Fri, Feb 01, 2013 at 10:44:25AM +0800, Chris Ruehl wrote:
> > > I file a report for you, please have a look when you have time.
> > 
> > Thanks for the bug report, Chris.
> > 
> > You have come across what looks like a known issue, which since it's
> > discovery last summer has been made worse by an unrelated change.
> > 
> > A similar oops was reported and its cause identified in this thread:
> > 
> > 	http://marc.info/?l=linux-usb&m=133837337927749&w=2
> > 
> > It turns out that the tty-layer may call the driver's dtr_rts even after
> > the device has been disconnected and the tty device unregistered. Since
> > last summer another change has made the problem worse by setting the
> > port data to NULL which results in even more drivers hitting the
> > problem.
> 
> The tty driver's close() routine must be called even if the open()
> failed because the tty layer doesn't know if the driver left unfinished
> business and is expecting to receive a close() to cleanup even if it
> failed the open(). This behavior was just recently documented in
> include/linux/tty_driver.h (ie., is in linux-next).

It's correct that tty-driver close() is called from tty_release() as part
of a failed open (which I mentioned elsewhere), but the tty-port
implementation has never called the tty-port callback shutdown() on
ports that fail to open. [ This is implemented using ASYNC_INITIALIZED. ]

In usb-serial, shutdown() is implemented as a call to the serial-driver
close(), and the serial drivers are expected to clean up any failed
open in their error paths instead (as is any driver using tty ports).

We have a situation where we are asked to re-open a HUPPED port (before
it's been fully closed). The hang-up was initiated by usb-serial core
which has received the disconnect() call. We set a disconnected flag
before calling vhangup() and refuse any further open (activate())
attempts.

This prevent DTR/RTS from being raised and also shutdown() from being
called. So it does not seem to make any sense for the tty-port
implementation to try to lower DTR/RTS on this never opened (or
initialised) port. [ Nor to honour port drain delay, for that matter. ]

What I'm trying to achieve with my RFC-patches -- apart from fixing a
few related issues -- is to get the tty-port implementation to uphold
the invariant of never making tty-port callbacks on a port that has
never been initialised (i.e. activate() has failed) just like we do with
shutdown() today -- at least during the error path of tty open().

> > While waiting for input from the tty-guru Alan Cox, and as the immediate
> > cause of that oops was remedied (by moving the offending interface
> > access in the driver in question), the problem was unfortunately
> > forgotten (or rather down-prioritised) until now.
> 
> Looks to me like a bug the usb serial mini-port interface design.
> A usb bus disconnect has the pl2303 (and every other) mini-port freeing
> the private data (before unregistering with tty anyway -- not that
> putting that first would fix the problem).
> 
> static int usb_serial_device_remove(struct device *dev)
> {
> 	struct usb_serial_driver *driver;
> 	struct usb_serial_port *port;
> 	int retval = 0;
> 	int minor;
> 
> 	port = to_usb_serial_port(dev);
> 	if (!port)
> 		return -ENODEV;
> 
> 	/* make sure suspend/resume doesn't race against port_remove */
> 	usb_autopm_get_interface(port->serial->interface);
> 
> 	device_remove_file(&port->dev, &dev_attr_port_number);
> 
> 	driver = port->serial->type;
> 	if (driver->port_remove)
> ====>		retval = driver->port_remove(port);
>
> 	minor = port->number;
> 	tty_unregister_device(usb_serial_tty_driver, minor);
> 	dev_info(dev, "%s converter now disconnected from ttyUSB%d\n",
> 		 driver->description, minor);
> 
> 	usb_autopm_put_interface(port->serial->interface);
> 	return retval;
> }
> 
> The pl2303 mini-port dutifully:
> 
> static int pl2303_port_remove(struct usb_serial_port *port)
> {
> 	struct pl2303_private *priv;
> 
> 	priv = usb_get_serial_port_data(port);
> ===>	kfree(priv);
> 
> 	return 0;
> }
> 
> while the tty layer still has outstanding references to the port.
> 
> static void pl2303_dtr_rts(struct usb_serial_port *port, int on)
> {
> =====>	struct pl2303_private *priv = usb_get_serial_port_data(port);
> 	unsigned long flags;
> 	u8 control;
> 
> 	[...]
> 
> 
> The tty layer (and its port implementation) can't protect the pl2303
> mini-port from this behavior/interface design.
> 
> It's the glue layer's responsibility to correctly manage the differing
> lifetimes of its bus devices with tty devices (and tty_ports, if used).
> 
> Meaning, that if the physical device disconnects from the bus, the ports
> must simply become in-operative; they can't disappear.

Agreed, the usb-serial port implementation and disconnect handling
appears broken.

I'll see if I can fix this up.

This patch ("USB: serial: fix null-pointer dereferences on disconnect")
will take care of the dtr_rts callback on a disconnected port, either
way.

> BTW, just fixing this one part won't work because the usb serial driver
> is also abruptly deleting the usb_serial_port device as well:
> 
> static void usb_serial_disconnect(struct usb_interface *interface)
> {
> 	[...]
> 
> 	for (i = 0; i < serial->num_ports; ++i) {
> 		port = serial->port[i];
> 		if (port) {
> 			struct tty_struct *tty = tty_port_tty_get(&port->port);
> 			if (tty) {
> 				tty_vhangup(tty);
> 				tty_kref_put(tty);
> 			}
> 			kill_traffic(port);
> 			cancel_work_sync(&port->work);
> 			if (device_is_registered(&port->dev))
> ========>			device_del(&port->dev);
> 		}
> 	}
> 	
> 	[...]
> }
> 
> Ummm, wasn't that where the port private data pointer was?

Indeed.

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Thu, Feb 14, 2013 at 06:43:36PM +0100, Johan Hovold wrote:
> On Wed, Feb 13, 2013 at 01:40:08PM -0500, Peter Hurley wrote:

[...]

> > Looks to me like a bug the usb serial mini-port interface design.
> > A usb bus disconnect has the pl2303 (and every other) mini-port freeing
> > the private data (before unregistering with tty anyway -- not that
> > putting that first would fix the problem).

[...]

> > The tty layer (and its port implementation) can't protect the pl2303
> > mini-port from this behavior/interface design.
> > 
> > It's the glue layer's responsibility to correctly manage the differing
> > lifetimes of its bus devices with tty devices (and tty_ports, if used).
> > 
> > Meaning, that if the physical device disconnects from the bus, the ports
> > must simply become in-operative; they can't disappear.

[...]

> > BTW, just fixing this one part won't work because the usb serial driver
> > is also abruptly deleting the usb_serial_port device as well:
> > 
> > static void usb_serial_disconnect(struct usb_interface *interface)
> > {
> > 	[...]
> > 
> > 	for (i = 0; i < serial->num_ports; ++i) {
> > 		port = serial->port[i];
> > 		if (port) {
> > 			struct tty_struct *tty = tty_port_tty_get(&port->port);
> > 			if (tty) {
> > 				tty_vhangup(tty);
> > 				tty_kref_put(tty);
> > 			}
> > 			kill_traffic(port);
> > 			cancel_work_sync(&port->work);
> > 			if (device_is_registered(&port->dev))
> > ========>			device_del(&port->dev);
> > 		}
> > 	}
> > 	
> > 	[...]
> > }
> > 
> > Ummm, wasn't that where the port private data pointer was?
> 
> Indeed.

We keep the usb-serial-port structure around until the last tty
reference is dropped, but the port private data is freed as part of
unregistering from the bus in disconnect. [ The subdrivers aren't
supposed to access the ports after port_remove is called as part of
unregistration. ]

This wasn't always the case, but unregistering in serial_cleanup (when
the last tty reference is dropped) had other issues. In particular, we
would end up unregistering the parent device before its child (the
interface and usb device are unregistered when disconnect returns)
resulting in broken uevent devpaths:

KERNEL[794.849051] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1/1-4.1:1.0 (usb)
KERNEL[794.851649] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1 (usb)
KERNEL[795.115623] remove   /1-4.1:1.0/ttyUSB0/tty/ttyUSB0 (tty)
KERNEL[795.117429] remove   /1-4.1:1.0/ttyUSB0 (usb-serial)

This was reported here

	https://bugs.freedesktop.org/show_bug.cgi?id=20703

and fixed by Alan Stern in 2d93148ab6 (USB: serial: fix lifetime and
locking problems) by moving unregistration to disconnect.

So we end up with an unregistered device still possibly referenced by
tty instead, and I suspect we can't do much else than deal with any
post-disconnect callbacks. [ These should be few, especially with my
latest TTY-patches applied. ]

Alan, do you see any way around this? It's not possible (or desirable)
to pin the parent device (interface) until the last reference is
dropped, is it?

Other drivers, such as cdc-acm, do unregister when the last reference is
dropped, but could potentially also generate broken uevents:

KERNEL[3042.388951] remove   /devices/pci0000:00/0000:00:1d.7/usb2/2-1/2-1:3.1 (usb)
KERNEL[3042.390242] remove   /2-1:3.1/tty/ttyACM0 (tty)

[ To trigger this I had to move the tty_kref_put to the end of
  disconnect(). As the tty ref is dropped in a workqueue, the disconnect
  then returns before cleanup is called, but any outstanding reference
  would have the same effect. ]

Johan

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Alan Stern]

On Fri, 22 Feb 2013, Johan Hovold wrote:

> We keep the usb-serial-port structure around until the last tty
> reference is dropped, but the port private data is freed as part of
> unregistering from the bus in disconnect. [ The subdrivers aren't
> supposed to access the ports after port_remove is called as part of
> unregistration. ]
> 
> This wasn't always the case, but unregistering in serial_cleanup (when
> the last tty reference is dropped) had other issues. In particular, we
> would end up unregistering the parent device before its child (the
> interface and usb device are unregistered when disconnect returns)
> resulting in broken uevent devpaths:
> 
> KERNEL[794.849051] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1/1-4.1:1.0 (usb)
> KERNEL[794.851649] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1 (usb)
> KERNEL[795.115623] remove   /1-4.1:1.0/ttyUSB0/tty/ttyUSB0 (tty)
> KERNEL[795.117429] remove   /1-4.1:1.0/ttyUSB0 (usb-serial)
> 
> This was reported here
> 
> 	https://bugs.freedesktop.org/show_bug.cgi?id=20703
> 
> and fixed by Alan Stern in 2d93148ab6 (USB: serial: fix lifetime and
> locking problems) by moving unregistration to disconnect.
> 
> So we end up with an unregistered device still possibly referenced by
> tty instead, and I suspect we can't do much else than deal with any
> post-disconnect callbacks. [ These should be few, especially with my
> latest TTY-patches applied. ]
> 
> Alan, do you see any way around this? It's not possible (or desirable)
> to pin the parent device (interface) until the last reference is
> dropped, is it?

On the contrary, it is customary to pin data structures until the last 
reference to them is gone.  That's what krefs are for.

On the other hand, the port private data was owned entirely by the
serial sub-driver.  Neither the serial core nor the tty layer is able
to use it meaningfully -- they don't even know what type of structure
it is.

Therefore freeing the structure when the port is removed should be 
harmless -- unless the subdriver is called after the structure is 
deallocated.

This means there still is one bug remaining.  In
usb_serial_device_remove(), the call to tty_unregister_device() should
occur _before_ the call to driver->port_remove(), not _after_.  Do you
think changing the order cause any new problems?

Alan Stern

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Fri, Feb 22, 2013 at 10:17:51AM -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
> 
> > We keep the usb-serial-port structure around until the last tty
> > reference is dropped, but the port private data is freed as part of
> > unregistering from the bus in disconnect. [ The subdrivers aren't
> > supposed to access the ports after port_remove is called as part of
> > unregistration. ]
> > 
> > This wasn't always the case, but unregistering in serial_cleanup (when
> > the last tty reference is dropped) had other issues. In particular, we
> > would end up unregistering the parent device before its child (the
> > interface and usb device are unregistered when disconnect returns)
> > resulting in broken uevent devpaths:
> > 
> > KERNEL[794.849051] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1/1-4.1:1.0 (usb)
> > KERNEL[794.851649] remove   /devices/pci0000:00/0000:00:1a.7/usb1/1-4/1-4.1 (usb)
> > KERNEL[795.115623] remove   /1-4.1:1.0/ttyUSB0/tty/ttyUSB0 (tty)
> > KERNEL[795.117429] remove   /1-4.1:1.0/ttyUSB0 (usb-serial)
> > 
> > This was reported here
> > 
> > 	https://bugs.freedesktop.org/show_bug.cgi?id=20703
> > 
> > and fixed by Alan Stern in 2d93148ab6 (USB: serial: fix lifetime and
> > locking problems) by moving unregistration to disconnect.
> > 
> > So we end up with an unregistered device still possibly referenced by
> > tty instead, and I suspect we can't do much else than deal with any
> > post-disconnect callbacks. [ These should be few, especially with my
> > latest TTY-patches applied. ]
> > 
> > Alan, do you see any way around this? It's not possible (or desirable)
> > to pin the parent device (interface) until the last reference is
> > dropped, is it?
> 
> On the contrary, it is customary to pin data structures until the last 
> reference to them is gone.  That's what krefs are for.

I was referring to the usb device in the device hierarchy, which
apparently is not pinned despite the outstanding reference we have in
struct usb_serial.

There is an unconditional call to device_del in usb_disconnect which
unlinks the parent usb device from the device hierarchy resulting in the
broken devpaths above if we do not unregister the usb-serial port (and
tty device) in disconnect.

> On the other hand, the port private data was owned entirely by the
> serial sub-driver.  Neither the serial core nor the tty layer is able
> to use it meaningfully -- they don't even know what type of structure
> it is.
> 
> Therefore freeing the structure when the port is removed should be 
> harmless -- unless the subdriver is called after the structure is 
> deallocated.

Which could happen (and is happening), unless we defer port unregister
until the last tty reference is dropped -- but then we get the broken
uevents.

> This means there still is one bug remaining.  In
> usb_serial_device_remove(), the call to tty_unregister_device() should
> occur _before_ the call to driver->port_remove(), not _after_.  Do you
> think changing the order cause any new problems?

Yes, Peter noticed that one too. Changing the order shouldn't cause any
new issues as far as I can see. I'll cook up a patch for this one as
well, but just to be clear: this is not directly related to the problem
discussed above as there may be outstanding tty references long after
both functions return (not that anyone has claimed anything else).

Johan

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Alan Stern]

On Fri, 22 Feb 2013, Johan Hovold wrote:

> > > So we end up with an unregistered device still possibly referenced by
> > > tty instead, and I suspect we can't do much else than deal with any
> > > post-disconnect callbacks. [ These should be few, especially with my
> > > latest TTY-patches applied. ]
> > > 
> > > Alan, do you see any way around this? It's not possible (or desirable)
> > > to pin the parent device (interface) until the last reference is
> > > dropped, is it?
> > 
> > On the contrary, it is customary to pin data structures until the last 
> > reference to them is gone.  That's what krefs are for.
> 
> I was referring to the usb device in the device hierarchy, which
> apparently is not pinned despite the outstanding reference we have in
> struct usb_serial.

Are you talking about the usb_device or the usb_interface?  The
usb_serial structure _does_ pin the usb_device structure.  But it
doesn't pin the usb_interface.  I don't know why things were done this
way; maybe it's a mistake.

Anyway, keeping a pointer to a non-pinned data structure after
unregistration is okay, provided you know you will never dereference
the pointer.  If you don't know this in the case of the usb_interface,
pinning is acceptable -- depending on _how_ the usb_interface is
accessed.  For example, no URBs should be submitted for any of the
interface's endpoints.

> There is an unconditional call to device_del in usb_disconnect which
> unlinks the parent usb device from the device hierarchy resulting in the
> broken devpaths above if we do not unregister the usb-serial port (and
> tty device) in disconnect.

Sure.  But unregistering is different from deallocation.  It's not
clear what your point is.

> > On the other hand, the port private data was owned entirely by the
> > serial sub-driver.  Neither the serial core nor the tty layer is able
> > to use it meaningfully -- they don't even know what type of structure
> > it is.
> > 
> > Therefore freeing the structure when the port is removed should be 
> > harmless -- unless the subdriver is called after the structure is 
> > deallocated.
> 
> Which could happen (and is happening), unless we defer port unregister
> until the last tty reference is dropped -- but then we get the broken
> uevents.

Unregistration should not be deferred.  We mustn't have those broken 
devpaths.

> > This means there still is one bug remaining.  In
> > usb_serial_device_remove(), the call to tty_unregister_device() should
> > occur _before_ the call to driver->port_remove(), not _after_.  Do you
> > think changing the order cause any new problems?
> 
> Yes, Peter noticed that one too. Changing the order shouldn't cause any
> new issues as far as I can see. I'll cook up a patch for this one as
> well, but just to be clear: this is not directly related to the problem
> discussed above as there may be outstanding tty references long after
> both functions return (not that anyone has claimed anything else).

This is related to the problem of the port's private data being
accessed after it is deallocated.  The only way that can happen is if
the tty layer calls the subdriver after the private data structure is
freed -- and you said above that this does happen.

But if change things so that the structure isn't freed until after the
port is unregistered from the tty layer, this would mean that the tty
layer is trying to do stuff to an unregistered port.  That would be a
bug in the tty layer.

I'm not saying such bugs don't exist.  However, if they do exist then 
the tty layer needs to be fixed, not the usb-serial layer.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Greg KH]

On Fri, Feb 22, 2013 at 12:35:32PM -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
> 
> > > > So we end up with an unregistered device still possibly referenced by
> > > > tty instead, and I suspect we can't do much else than deal with any
> > > > post-disconnect callbacks. [ These should be few, especially with my
> > > > latest TTY-patches applied. ]
> > > > 
> > > > Alan, do you see any way around this? It's not possible (or desirable)
> > > > to pin the parent device (interface) until the last reference is
> > > > dropped, is it?
> > > 
> > > On the contrary, it is customary to pin data structures until the last 
> > > reference to them is gone.  That's what krefs are for.
> > 
> > I was referring to the usb device in the device hierarchy, which
> > apparently is not pinned despite the outstanding reference we have in
> > struct usb_serial.
> 
> Are you talking about the usb_device or the usb_interface?  The
> usb_serial structure _does_ pin the usb_device structure.  But it
> doesn't pin the usb_interface.  I don't know why things were done this
> way; maybe it's a mistake.

Ick, yeah, that was a mistake, probably been there since the very
beginning, sorry about that.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Peter Hurley]

On Fri, 2013-02-22 at 12:35 -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
> 
> > > > So we end up with an unregistered device still possibly referenced by
> > > > tty instead, and I suspect we can't do much else than deal with any
> > > > post-disconnect callbacks. [ These should be few, especially with my
> > > > latest TTY-patches applied. ]
> > > > 
> > > > Alan, do you see any way around this? It's not possible (or desirable)
> > > > to pin the parent device (interface) until the last reference is
> > > > dropped, is it?
> > > 
> > > On the contrary, it is customary to pin data structures until the last 
> > > reference to them is gone.  That's what krefs are for.
> > 
> > I was referring to the usb device in the device hierarchy, which
> > apparently is not pinned despite the outstanding reference we have in
> > struct usb_serial.
> 
> Are you talking about the usb_device or the usb_interface?  The
> usb_serial structure _does_ pin the usb_device structure.  But it
> doesn't pin the usb_interface.  I don't know why things were done this
> way; maybe it's a mistake.
> 
> Anyway, keeping a pointer to a non-pinned data structure after
> unregistration is okay, provided you know you will never dereference
> the pointer.  If you don't know this in the case of the usb_interface,
> pinning is acceptable -- depending on _how_ the usb_interface is
> accessed.  For example, no URBs should be submitted for any of the
> interface's endpoints.
> 
> > There is an unconditional call to device_del in usb_disconnect which
> > unlinks the parent usb device from the device hierarchy resulting in the
> > broken devpaths above if we do not unregister the usb-serial port (and
> > tty device) in disconnect.
> 
> Sure.  But unregistering is different from deallocation.  It's not
> clear what your point is.
> 
> > > On the other hand, the port private data was owned entirely by the
> > > serial sub-driver.  Neither the serial core nor the tty layer is able
> > > to use it meaningfully -- they don't even know what type of structure
> > > it is.
> > > 
> > > Therefore freeing the structure when the port is removed should be 
> > > harmless -- unless the subdriver is called after the structure is 
> > > deallocated.
> > 
> > Which could happen (and is happening), unless we defer port unregister
> > until the last tty reference is dropped -- but then we get the broken
> > uevents.
> 
> Unregistration should not be deferred.  We mustn't have those broken 
> devpaths.
> 
> > > This means there still is one bug remaining.  In
> > > usb_serial_device_remove(), the call to tty_unregister_device() should
> > > occur _before_ the call to driver->port_remove(), not _after_.  Do you
> > > think changing the order cause any new problems?
> > 
> > Yes, Peter noticed that one too. Changing the order shouldn't cause any
> > new issues as far as I can see. I'll cook up a patch for this one as
> > well, but just to be clear: this is not directly related to the problem
> > discussed above as there may be outstanding tty references long after
> > both functions return (not that anyone has claimed anything else).
> 
> This is related to the problem of the port's private data being
> accessed after it is deallocated.  The only way that can happen is if
> the tty layer calls the subdriver after the private data structure is
> freed -- and you said above that this does happen.
> 
> But if change things so that the structure isn't freed until after the
> port is unregistered from the tty layer, this would mean that the tty
> layer is trying to do stuff to an unregistered port.  That would be a
> bug in the tty layer.
> 
> I'm not saying such bugs don't exist.  However, if they do exist then 
> the tty layer needs to be fixed, not the usb-serial layer.

ISTM the usb_serial_port private data should not be freed until
port_release().

If usb traffic isn't halted until port_release() ...

static void port_release(struct device *dev)
{
	struct usb_serial_port *port = to_usb_serial_port(dev);
	int i;

	dev_dbg(dev, "%s\n", __func__);

	/*
	 * Stop all the traffic before cancelling the work, so that
	 * nobody will restart it by calling usb_serial_port_softint.
	 */
====>	kill_traffic(port);
	cancel_work_sync(&port->work);

then what happens here if ->port_remove() has already been called?

static void garmin_write_bulk_callback(struct urb *urb)
{
	struct usb_serial_port *port = urb->context;

	if (port) {
		struct garmin_data *garmin_data_p =
					usb_get_serial_port_data(port);

		if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {

====>			if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
				gsp_send_ack(garmin_data_p,
					((__u8 *)urb->transfer_buffer)[4]);
			}

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Alan Stern]

On Fri, 22 Feb 2013, Peter Hurley wrote:

> ISTM the usb_serial_port private data should not be freed until
> port_release().

I don't think that's right.  In general, a driver doesn't know what's 
going to happen to a device after the two are unbound from each other.  
Another driver may get bound to that same device, and may overwrite the 
old private-data pointer with its own new one.

The same reasoning applies here.  The serial subdriver isn't involved 
in the creation of the usb_serial_port structure, and therefore it 
shouldn't be involved in that structure's destruction.

> If usb traffic isn't halted until port_release() ...
> 
> static void port_release(struct device *dev)
> {
> 	struct usb_serial_port *port = to_usb_serial_port(dev);
> 	int i;
> 
> 	dev_dbg(dev, "%s\n", __func__);
> 
> 	/*
> 	 * Stop all the traffic before cancelling the work, so that
> 	 * nobody will restart it by calling usb_serial_port_softint.
> 	 */
> ====>	kill_traffic(port);
> 	cancel_work_sync(&port->work);
> 
> then what happens here if ->port_remove() has already been called?
> 
> static void garmin_write_bulk_callback(struct urb *urb)
> {
> 	struct usb_serial_port *port = urb->context;
> 
> 	if (port) {
> 		struct garmin_data *garmin_data_p =
> 					usb_get_serial_port_data(port);
> 
> 		if (GARMIN_LAYERID_APPL == getLayerId(urb->transfer_buffer)) {
> 
> ====>			if (garmin_data_p->mode == MODE_GARMIN_SERIAL) {
> 				gsp_send_ack(garmin_data_p,
> 					((__u8 *)urb->transfer_buffer)[4]);
> 			}

Clearly this is a mistake.  Probably it is the result of historical
confusion between the overall USB device and the multiple serial ports
embodied within it.  At any rate, it is clear that all port-related USB
activity should be stopped when the port is unregistered.

Whether this can be carried out by the usb-serial core or needs help 
from the subdriver, I leave up to you guys.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Fri, Feb 22, 2013 at 01:21:24PM -0500, Peter Hurley wrote:
> On Fri, 2013-02-22 at 12:35 -0500, Alan Stern wrote:
> > On Fri, 22 Feb 2013, Johan Hovold wrote:
> ISTM the usb_serial_port private data should not be freed until
> port_release().
> 
> If usb traffic isn't halted until port_release() ...
> 
> static void port_release(struct device *dev)
> {
> 	struct usb_serial_port *port = to_usb_serial_port(dev);
> 	int i;
> 
> 	dev_dbg(dev, "%s\n", __func__);
> 
> 	/*
> 	 * Stop all the traffic before cancelling the work, so that
> 	 * nobody will restart it by calling usb_serial_port_softint.
> 	 */
> ====>	kill_traffic(port);
> 	cancel_work_sync(&port->work);
> 
> then what happens here if ->port_remove() has already been called?

I agree with Alan on this one. In fact, the call to kill_traffic in
port_release is wrong and I'm removing it as part of a disconnect clean
up series I'm preparing.

Exactly when the private port data is freed isn't the main issue here as
the subdrivers mustn't access the ports after the device has been
unregistered, and it's up to tty and usb-serial core to prevent any port
related calls after disconnect.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Fri, Feb 22, 2013 at 12:35:32PM -0500, Alan Stern wrote:
> On Fri, 22 Feb 2013, Johan Hovold wrote:
> > > > So we end up with an unregistered device still possibly referenced by
> > > > tty instead, and I suspect we can't do much else than deal with any
> > > > post-disconnect callbacks. [ These should be few, especially with my
> > > > latest TTY-patches applied. ]
> > > > 
> > > > Alan, do you see any way around this? It's not possible (or desirable)
> > > > to pin the parent device (interface) until the last reference is
> > > > dropped, is it?
> > > 
> > > On the contrary, it is customary to pin data structures until the last 
> > > reference to them is gone.  That's what krefs are for.
> > 
> > I was referring to the usb device in the device hierarchy, which
> > apparently is not pinned despite the outstanding reference we have in
> > struct usb_serial.
> 
> Are you talking about the usb_device or the usb_interface?  The
> usb_serial structure _does_ pin the usb_device structure.  But it
> doesn't pin the usb_interface.  I don't know why things were done this
> way; maybe it's a mistake.
> 
> Anyway, keeping a pointer to a non-pinned data structure after
> unregistration is okay, provided you know you will never dereference
> the pointer.  If you don't know this in the case of the usb_interface,
> pinning is acceptable -- depending on _how_ the usb_interface is
> accessed.  For example, no URBs should be submitted for any of the
> interface's endpoints.
> 
> > There is an unconditional call to device_del in usb_disconnect which
> > unlinks the parent usb device from the device hierarchy resulting in the
> > broken devpaths above if we do not unregister the usb-serial port (and
> > tty device) in disconnect.
> 
> Sure.  But unregistering is different from deallocation.  It's not
> clear what your point is.

I'm not primarily concerned with deallocation, and perhaps my choice of
wording was misleading when I use "pinning" in reference to the device
hierarchy and unregistration. I was basically asking whether it is
possible to defer unregistration of the interface (parent device) until
the last tty reference is dropped.

> > > On the other hand, the port private data was owned entirely by the
> > > serial sub-driver.  Neither the serial core nor the tty layer is able
> > > to use it meaningfully -- they don't even know what type of structure
> > > it is.
> > > 
> > > Therefore freeing the structure when the port is removed should be 
> > > harmless -- unless the subdriver is called after the structure is 
> > > deallocated.
> > 
> > Which could happen (and is happening), unless we defer port unregister
> > until the last tty reference is dropped -- but then we get the broken
> > uevents.
> 
> Unregistration should not be deferred.  We mustn't have those broken 
> devpaths.

And here is your answer it seems.

So to repeat, it is not possible to defer unregistration of the parent
(usb interface) until the child (usb-serial port) is unregistered so
that we could defer the latter to when the last tty ref is dropped.

> > > This means there still is one bug remaining.  In
> > > usb_serial_device_remove(), the call to tty_unregister_device() should
> > > occur _before_ the call to driver->port_remove(), not _after_.  Do you
> > > think changing the order cause any new problems?
> > 
> > Yes, Peter noticed that one too. Changing the order shouldn't cause any
> > new issues as far as I can see. I'll cook up a patch for this one as
> > well, but just to be clear: this is not directly related to the problem
> > discussed above as there may be outstanding tty references long after
> > both functions return (not that anyone has claimed anything else).
> 
> This is related to the problem of the port's private data being
> accessed after it is deallocated.  The only way that can happen is if
> the tty layer calls the subdriver after the private data structure is
> freed -- and you said above that this does happen.
> 
> But if change things so that the structure isn't freed until after the
> port is unregistered from the tty layer, this would mean that the tty
> layer is trying to do stuff to an unregistered port.  That would be a
> bug in the tty layer.

Yes, I acknowledged that it is a bug, but it's not the one I'm
triggering.

I think the confusion stems from what tty_unregister_device actually
implies. You seem to, and I used to, think that this calls works as a
barrier so that no further tty callbacks can be made once it returns.
However, this is not the case.

As long as there are outstanding tty refs, tty will happily call back
even after tty_unregister_device returns. Unless we all agree that this
a bug in tty, it's a bug in usb-serial which should instead defer
unregistration until the last reference is dropped (but that gives us
the broken uevents unless it could be worked around).

> I'm not saying such bugs don't exist.  However, if they do exist then 
> the tty layer needs to be fixed, not the usb-serial layer.

Fair enough. 

Note also that we have at least two drivers on each side of this
argument; ubs-serial unregistering at disconnect, and cdc-acm
unregistering when the last tty ref is dropped. One of them must be
wrong.

Johan

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Sat, Feb 23, 2013 at 05:23:43PM +0100, Johan Hovold wrote:
> On Fri, Feb 22, 2013 at 12:35:32PM -0500, Alan Stern wrote:
> > > Yes, Peter noticed that one too. Changing the order shouldn't cause any
> > > new issues as far as I can see. I'll cook up a patch for this one as
> > > well, but just to be clear: this is not directly related to the problem
> > > discussed above as there may be outstanding tty references long after
> > > both functions return (not that anyone has claimed anything else).
> > 
> > This is related to the problem of the port's private data being
> > accessed after it is deallocated.  The only way that can happen is if
> > the tty layer calls the subdriver after the private data structure is
> > freed -- and you said above that this does happen.
> > 
> > But if change things so that the structure isn't freed until after the
> > port is unregistered from the tty layer, this would mean that the tty
> > layer is trying to do stuff to an unregistered port.  That would be a
> > bug in the tty layer.
> 
> Yes, I acknowledged that it is a bug, but it's not the one I'm
> triggering.

This was ambiguous. I meant to acknowledge that port_release should be
called after unregistration, but what I'm triggering is the different
bug that tty callbacks are made after unregistration (and proceed to
discuss whether this is indeed to be considered a bug in tty or
usb-serial below).

> I think the confusion stems from what tty_unregister_device actually
> implies. You seem to, and I used to, think that this calls works as a
> barrier so that no further tty callbacks can be made once it returns.
> However, this is not the case.
> 
> As long as there are outstanding tty refs, tty will happily call back
> even after tty_unregister_device returns. Unless we all agree that this
> a bug in tty, it's a bug in usb-serial which should instead defer
> unregistration until the last reference is dropped (but that gives us
> the broken uevents unless it could be worked around).
> 
> > I'm not saying such bugs don't exist.  However, if they do exist then 
> > the tty layer needs to be fixed, not the usb-serial layer.
> 
> Fair enough. 
> 
> Note also that we have at least two drivers on each side of this
> argument; ubs-serial unregistering at disconnect, and cdc-acm
> unregistering when the last tty ref is dropped. One of them must be
> wrong.
> 
> Johan

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Alan Stern]

On Sat, 23 Feb 2013, Johan Hovold wrote:

> I'm not primarily concerned with deallocation, and perhaps my choice of
> wording was misleading when I use "pinning" in reference to the device
> hierarchy and unregistration. I was basically asking whether it is
> possible to defer unregistration of the interface (parent device) until
> the last tty reference is dropped.

> > Unregistration should not be deferred.  We mustn't have those broken 
> > devpaths.
> 
> And here is your answer it seems.

Right.  More precisely, registration mustn't be deferred _and_ we
mustn't have any broken devpaths.  Currently usb-serial gets one of
these wrong and cdc-acm gets the other one wrong.

> So to repeat, it is not possible to defer unregistration of the parent
> (usb interface) until the child (usb-serial port) is unregistered so
> that we could defer the latter to when the last tty ref is dropped.

No.  The USB stack is designed under the assumption that outstanding
references pin data structures in memory but they don't pin anything
else.  In particular, release routines are not supposed to call things
like device_del() (although there may be some cases where this does
happen).

What _is_ true is this: We do not want to defer unregistration of
either entity until some final ref is dropped.  After all, userspace
can continue to hold references indefinitely, whereas registration
should occur in a timely manner.  For example, aren't the hotplug
events for device removal triggered by unregistration?

> > This is related to the problem of the port's private data being
> > accessed after it is deallocated.  The only way that can happen is if
> > the tty layer calls the subdriver after the private data structure is
> > freed -- and you said above that this does happen.
> > 
> > But if change things so that the structure isn't freed until after the
> > port is unregistered from the tty layer, this would mean that the tty
> > layer is trying to do stuff to an unregistered port.  That would be a
> > bug in the tty layer.
> 
> Yes, I acknowledged that it is a bug, but it's not the one I'm
> triggering.

> This was ambiguous. I meant to acknowledge that port_release should be
> called after unregistration, but what I'm triggering is the different
> bug that tty callbacks are made after unregistration (and proceed to
> discuss whether this is indeed to be considered a bug in tty or
> usb-serial below).

> I think the confusion stems from what tty_unregister_device actually
> implies. You seem to, and I used to, think that this calls works as a
> barrier so that no further tty callbacks can be made once it returns.
> However, this is not the case.

Sounds like a design weakness, if not quite a flaw, in the tty layer.  
As a general rule, responsibility for tricky things like barriers
always belongs in the core layer where it can be done once and done
correctly, rather than being replicated throughout multiple drivers
which are likely to get it wrong.

> As long as there are outstanding tty refs, tty will happily call back
> even after tty_unregister_device returns. Unless we all agree that this
> a bug in tty, it's a bug in usb-serial which should instead defer
> unregistration until the last reference is dropped (but that gives us
> the broken uevents unless it could be worked around).

No, unregistration cannot be deferred.  However the usb-serial core 
could try to compensate for the problem by refusing to pass callbacks 
through to the subdriver after the port has been unregistered.  I don't 
know if that would work -- it wouldn't if some of the callback routines 
are located in the subdriver itself as opposed to usb-serial.c.

> > I'm not saying such bugs don't exist.  However, if they do exist then 
> > the tty layer needs to be fixed, not the usb-serial layer.
> 
> Fair enough. 
> 
> Note also that we have at least two drivers on each side of this
> argument; ubs-serial unregistering at disconnect, and cdc-acm
> unregistering when the last tty ref is dropped. One of them must be
> wrong.

cdc-acm is wrong.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Johan Hovold]

On Sat, Feb 23, 2013 at 12:31:47PM -0500, Alan Stern wrote:
> On Sat, 23 Feb 2013, Johan Hovold wrote:
> > I'm not primarily concerned with deallocation, and perhaps my choice of
> > wording was misleading when I use "pinning" in reference to the device
> > hierarchy and unregistration. I was basically asking whether it is
> > possible to defer unregistration of the interface (parent device) until
> > the last tty reference is dropped.
> 
> > > Unregistration should not be deferred.  We mustn't have those broken 
> > > devpaths.
> > 
> > And here is your answer it seems.
> 
> Right.  More precisely, registration mustn't be deferred _and_ we
> mustn't have any broken devpaths.  Currently usb-serial gets one of
> these wrong and cdc-acm gets the other one wrong.

Maybe I misunderstand you, but it seems to me that ubs-serial gets both
right and cdc-acm both wrong, as non-deferred registration guarantees
non-broken uevent devpaths?

The uevent devpaths from cdc-acm are usually correct, but an outstanding
tty ref when disconnect returns would break them (as my test case
showed).

But perhaps you're not referring to the uevent devpaths, but rather the
refcounted parent references, in which case I agree with you.

> > So to repeat, it is not possible to defer unregistration of the parent
> > (usb interface) until the child (usb-serial port) is unregistered so
> > that we could defer the latter to when the last tty ref is dropped.
> 
> No.  The USB stack is designed under the assumption that outstanding
> references pin data structures in memory but they don't pin anything
> else.  In particular, release routines are not supposed to call things
> like device_del() (although there may be some cases where this does
> happen).

As I suspected. Thanks for verifying.

> What _is_ true is this: We do not want to defer unregistration of
> either entity until some final ref is dropped.  After all, userspace
> can continue to hold references indefinitely, whereas registration
> should occur in a timely manner.  For example, aren't the hotplug
> events for device removal triggered by unregistration?

Indeed, this is where we get the broken uevent devpaths.

> > > This is related to the problem of the port's private data being
> > > accessed after it is deallocated.  The only way that can happen is if
> > > the tty layer calls the subdriver after the private data structure is
> > > freed -- and you said above that this does happen.
> > > 
> > > But if change things so that the structure isn't freed until after the
> > > port is unregistered from the tty layer, this would mean that the tty
> > > layer is trying to do stuff to an unregistered port.  That would be a
> > > bug in the tty layer.
> > 
> > Yes, I acknowledged that it is a bug, but it's not the one I'm
> > triggering.
> 
> > This was ambiguous. I meant to acknowledge that port_release should be
> > called after unregistration, but what I'm triggering is the different
> > bug that tty callbacks are made after unregistration (and proceed to
> > discuss whether this is indeed to be considered a bug in tty or
> > usb-serial below).
> 
> > I think the confusion stems from what tty_unregister_device actually
> > implies. You seem to, and I used to, think that this calls works as a
> > barrier so that no further tty callbacks can be made once it returns.
> > However, this is not the case.
> 
> Sounds like a design weakness, if not quite a flaw, in the tty layer.  
> As a general rule, responsibility for tricky things like barriers
> always belongs in the core layer where it can be done once and done
> correctly, rather than being replicated throughout multiple drivers
> which are likely to get it wrong.

Agreed.

> > As long as there are outstanding tty refs, tty will happily call back
> > even after tty_unregister_device returns. Unless we all agree that this
> > a bug in tty, it's a bug in usb-serial which should instead defer
> > unregistration until the last reference is dropped (but that gives us
> > the broken uevents unless it could be worked around).
> 
> No, unregistration cannot be deferred.  However the usb-serial core 
> could try to compensate for the problem by refusing to pass callbacks 
> through to the subdriver after the port has been unregistered.  I don't 
> know if that would work -- it wouldn't if some of the callback routines 
> are located in the subdriver itself as opposed to usb-serial.c.

It should be ok as all tty callbacks are made through usb serial core.

I have a pretty good overview over what callbacks we can be expecting
when, and those that we cannot prevent tty from making, we'll deal with
in usb-serial core.

My fix for the dtr_rts case is in 3.9-rc, and I try to reduce some of
the callbacks with my TTY patches (which would for example make the
dtr_rts fix unnecessary).

> > > I'm not saying such bugs don't exist.  However, if they do exist then 
> > > the tty layer needs to be fixed, not the usb-serial layer.
> > 
> > Fair enough. 
> > 
> > Note also that we have at least two drivers on each side of this
> > argument; ubs-serial unregistering at disconnect, and cdc-acm
> > unregistering when the last tty ref is dropped. One of them must be
> > wrong.
> 
> cdc-acm is wrong.

I'll fix this then.

Thanks,
Johan

Re: USB: serial: port lifetimes (was: Re: USB Ooops PL2303 when unplug while use (linux v3.7.3))

[Alan Stern]

On Sat, 23 Feb 2013, Johan Hovold wrote:

> > Right.  More precisely, registration mustn't be deferred _and_ we
> > mustn't have any broken devpaths.  Currently usb-serial gets one of
> > these wrong and cdc-acm gets the other one wrong.
> 
> Maybe I misunderstand you, but it seems to me that ubs-serial gets both
> right and cdc-acm both wrong, as non-deferred registration guarantees
> non-broken uevent devpaths?

Sorry, yes, you are right.  I wasn't thinking clearly.

> > No, unregistration cannot be deferred.  However the usb-serial core 
> > could try to compensate for the problem by refusing to pass callbacks 
> > through to the subdriver after the port has been unregistered.  I don't 
> > know if that would work -- it wouldn't if some of the callback routines 
> > are located in the subdriver itself as opposed to usb-serial.c.
> 
> It should be ok as all tty callbacks are made through usb serial core.
> 
> I have a pretty good overview over what callbacks we can be expecting
> when, and those that we cannot prevent tty from making, we'll deal with
> in usb-serial core.
> 
> My fix for the dtr_rts case is in 3.9-rc, and I try to reduce some of
> the callbacks with my TTY patches (which would for example make the
> dtr_rts fix unnecessary).

So you could do it that way.  But IMO changing the tty layer would be
preferable, if it can be done without too much difficulty.  That would
allow both usb-serial and cdc-acm to be fixed fairly easily.

Alan Stern