tty related lockdep trace during bootup on 3.2-rc2

tty related lockdep trace during bootup on 3.2-rc2

[Dave Jones]

>From Linus' current tree...

related to 5dc2470c602da8851907ec18942cd876c3b4ecc1 maybe ?

	Dave

[   40.778011] ======================================================
[   40.780010] [ INFO: possible circular locking dependency detected ]
[   40.780010] 3.2.0-rc2+ #8
[   40.780010] -------------------------------------------------------
[   40.780010] modem-manager/1141 is trying to acquire lock:
[   40.780010]  (big_tty_mutex){+.+.+.}, at: [<ffffffff81682807>] tty_lock+0x17/0x19
[   40.780010] 
[   40.780010] but task is already holding lock:
[   40.780010]  (open_mutex){+.+...}, at: [<ffffffffa032b321>] acm_tty_close+0x41/0xc0 [cdc_acm]
[   40.780010] 
[   40.780010] which lock already depends on the new lock.
[   40.780010] 
[   40.780010] 
[   40.780010] the existing dependency chain (in reverse order) is:
[   40.780010] 
[   40.780010] -> #1 (open_mutex){+.+...}:
[   40.780010]        [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
[   40.780010]        [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
[   40.780010]        [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
[   40.780010]        [<ffffffffa032b715>] acm_tty_open+0x35/0x230 [cdc_acm]
[   40.780010]        [<ffffffff813c1087>] tty_open+0x247/0x5d0
[   40.780010]        [<ffffffff811b5408>] chrdev_open+0x258/0x350
[   40.780010]        [<ffffffff811ad9a4>] __dentry_open+0x384/0x550
[   40.780010]        [<ffffffff811af194>] nameidata_to_filp+0x74/0x80
[   40.780010]        [<ffffffff811c0b6c>] do_last+0x26c/0x920
[   40.780010]        [<ffffffff811c1335>] path_openat+0xd5/0x3e0
[   40.780010]        [<ffffffff811c1762>] do_filp_open+0x42/0xa0
[   40.780010]        [<ffffffff811af297>] do_sys_open+0xf7/0x1d0
[   40.780010]        [<ffffffff811af390>] sys_open+0x20/0x30
[   40.780010]        [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b
[   40.780010] 
[   40.780010] -> #0 (big_tty_mutex){+.+.+.}:
[   40.780010]        [<ffffffff810c112e>] __lock_acquire+0x16ce/0x1c40
[   40.780010]        [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
[   40.780010]        [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
[   40.780010]        [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
[   40.780010]        [<ffffffff81682807>] tty_lock+0x17/0x19
[   40.780010]        [<ffffffff813c91ad>] tty_port_close_start+0x17d/0x210
[   40.780010]        [<ffffffffa032b32f>] acm_tty_close+0x4f/0xc0 [cdc_acm]
[   40.780010]        [<ffffffff813c09e7>] tty_release+0x167/0x5c0
[   40.780010]        [<ffffffff811b252e>] fput+0xfe/0x2d0
[   40.780010]        [<ffffffff811adee9>] filp_close+0x69/0x90
[   40.780010]        [<ffffffff811ae1d0>] sys_close+0xc0/0x1a0
[   40.780010]        [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b
[   40.780010] 
[   40.780010] other info that might help us debug this:
[   40.780010] 
[   40.780010]  Possible unsafe locking scenario:
[   40.780010] 
[   40.780010]        CPU0                    CPU1
[   40.780010]        ----                    ----
[   40.780010]   lock(open_mutex);
[   40.780010]                                lock(big_tty_mutex);
[   40.780010]                                lock(open_mutex);
[   40.780010]   lock(big_tty_mutex);
[   40.780010] 
[   40.780010]  *** DEADLOCK ***
[   40.780010] 
[   40.780010] 1 lock held by modem-manager/1141:
[   40.780010]  #0:  (open_mutex){+.+...}, at: [<ffffffffa032b321>] acm_tty_close+0x41/0xc0 [cdc_acm]
[   40.780010] 
[   40.780010] stack backtrace:
[   40.780010] Pid: 1141, comm: modem-manager Not tainted 3.2.0-rc2+ #8
[   40.780010] Call Trace:
[   40.780010]  [<ffffffff81675378>] print_circular_bug+0x202/0x213
[   40.780010]  [<ffffffff810c112e>] __lock_acquire+0x16ce/0x1c40
[   40.780010]  [<ffffffff81021f62>] ? native_sched_clock+0x22/0x70
[   40.780010]  [<ffffffff810ae2c5>] ? sched_clock_local+0x25/0x90
[   40.780010]  [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
[   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
[   40.780010]  [<ffffffff810c180a>] ? lock_release_non_nested+0x16a/0x350
[   40.780010]  [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
[   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
[   40.780010]  [<ffffffff810c2796>] ? mark_held_locks+0x86/0x150
[   40.780010]  [<ffffffff816807ae>] ? mutex_unlock+0xe/0x10
[   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
[   40.780010]  [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
[   40.780010]  [<ffffffff81682807>] tty_lock+0x17/0x19
[   40.780010]  [<ffffffff813c91ad>] tty_port_close_start+0x17d/0x210
[   40.780010]  [<ffffffffa032b32f>] acm_tty_close+0x4f/0xc0 [cdc_acm]
[   40.780010]  [<ffffffff813c09e7>] tty_release+0x167/0x5c0
[   40.780010]  [<ffffffff81021fb9>] ? sched_clock+0x9/0x10
[   40.780010]  [<ffffffff810ae2c5>] ? sched_clock_local+0x25/0x90
[   40.780010]  [<ffffffff811b252e>] fput+0xfe/0x2d0
[   40.780010]  [<ffffffff811adee9>] filp_close+0x69/0x90
[   40.780010]  [<ffffffff811ae1d0>] sys_close+0xc0/0x1a0
[   40.780010]  [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b

Re: tty related lockdep trace during bootup on 3.2-rc2

[Havard Skinnemoen]

On Tue, Nov 22, 2011 at 7:38 PM, Dave Jones <davej@redhat.com> wrote:
> From Linus' current tree...
>
> related to 5dc2470c602da8851907ec18942cd876c3b4ecc1 maybe ?

Yes, probably. I did have a bad feeling about the locking, but it
seemed to behave well during testing. Wonder why I didn't see this.

So what's happening is that tty_open() holds big_tty_mutex while
calling acm_tty_open which takes open_lock, and acm_tty_close holds
open_lock while calling tty_port_close_start which takes
big_tty_mutex?

Not sure how to solve this. Not taking the lock before calling
tty_port_close_start means the tty_port may get freed before it
returns.

Havard

Re: tty related lockdep trace during bootup on 3.2-rc2

[Cong Wang]

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

On Wed, Nov 23, 2011 at 11:38 AM, Dave Jones <davej@redhat.com> wrote:
> From Linus' current tree...
>
> related to 5dc2470c602da8851907ec18942cd876c3b4ecc1 maybe ?
>
>        Dave
>
> [   40.778011] ======================================================
> [   40.780010] [ INFO: possible circular locking dependency detected ]
> [   40.780010] 3.2.0-rc2+ #8
> [   40.780010] -------------------------------------------------------
> [   40.780010] modem-manager/1141 is trying to acquire lock:
> [   40.780010]  (big_tty_mutex){+.+.+.}, at: [<ffffffff81682807>] tty_lock+0x17/0x19
> [   40.780010]
> [   40.780010] but task is already holding lock:
> [   40.780010]  (open_mutex){+.+...}, at: [<ffffffffa032b321>] acm_tty_close+0x41/0xc0 [cdc_acm]
> [   40.780010]
> [   40.780010] which lock already depends on the new lock.
> [   40.780010]
> [   40.780010]
> [   40.780010] the existing dependency chain (in reverse order) is:
> [   40.780010]
> [   40.780010] -> #1 (open_mutex){+.+...}:
> [   40.780010]        [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
> [   40.780010]        [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
> [   40.780010]        [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
> [   40.780010]        [<ffffffffa032b715>] acm_tty_open+0x35/0x230 [cdc_acm]
> [   40.780010]        [<ffffffff813c1087>] tty_open+0x247/0x5d0
> [   40.780010]        [<ffffffff811b5408>] chrdev_open+0x258/0x350
> [   40.780010]        [<ffffffff811ad9a4>] __dentry_open+0x384/0x550
> [   40.780010]        [<ffffffff811af194>] nameidata_to_filp+0x74/0x80
> [   40.780010]        [<ffffffff811c0b6c>] do_last+0x26c/0x920
> [   40.780010]        [<ffffffff811c1335>] path_openat+0xd5/0x3e0
> [   40.780010]        [<ffffffff811c1762>] do_filp_open+0x42/0xa0
> [   40.780010]        [<ffffffff811af297>] do_sys_open+0xf7/0x1d0
> [   40.780010]        [<ffffffff811af390>] sys_open+0x20/0x30
> [   40.780010]        [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b
> [   40.780010]
> [   40.780010] -> #0 (big_tty_mutex){+.+.+.}:
> [   40.780010]        [<ffffffff810c112e>] __lock_acquire+0x16ce/0x1c40
> [   40.780010]        [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
> [   40.780010]        [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
> [   40.780010]        [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
> [   40.780010]        [<ffffffff81682807>] tty_lock+0x17/0x19
> [   40.780010]        [<ffffffff813c91ad>] tty_port_close_start+0x17d/0x210
> [   40.780010]        [<ffffffffa032b32f>] acm_tty_close+0x4f/0xc0 [cdc_acm]
> [   40.780010]        [<ffffffff813c09e7>] tty_release+0x167/0x5c0
> [   40.780010]        [<ffffffff811b252e>] fput+0xfe/0x2d0
> [   40.780010]        [<ffffffff811adee9>] filp_close+0x69/0x90
> [   40.780010]        [<ffffffff811ae1d0>] sys_close+0xc0/0x1a0
> [   40.780010]        [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b
> [   40.780010]
> [   40.780010] other info that might help us debug this:
> [   40.780010]
> [   40.780010]  Possible unsafe locking scenario:
> [   40.780010]
> [   40.780010]        CPU0                    CPU1
> [   40.780010]        ----                    ----
> [   40.780010]   lock(open_mutex);
> [   40.780010]                                lock(big_tty_mutex);
> [   40.780010]                                lock(open_mutex);
> [   40.780010]   lock(big_tty_mutex);
> [   40.780010]
> [   40.780010]  *** DEADLOCK ***
> [   40.780010]
> [   40.780010] 1 lock held by modem-manager/1141:
> [   40.780010]  #0:  (open_mutex){+.+...}, at: [<ffffffffa032b321>] acm_tty_close+0x41/0xc0 [cdc_acm]
> [   40.780010]
> [   40.780010] stack backtrace:
> [   40.780010] Pid: 1141, comm: modem-manager Not tainted 3.2.0-rc2+ #8
> [   40.780010] Call Trace:
> [   40.780010]  [<ffffffff81675378>] print_circular_bug+0x202/0x213
> [   40.780010]  [<ffffffff810c112e>] __lock_acquire+0x16ce/0x1c40
> [   40.780010]  [<ffffffff81021f62>] ? native_sched_clock+0x22/0x70
> [   40.780010]  [<ffffffff810ae2c5>] ? sched_clock_local+0x25/0x90
> [   40.780010]  [<ffffffff810c1d9d>] lock_acquire+0x9d/0x210
> [   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
> [   40.780010]  [<ffffffff810c180a>] ? lock_release_non_nested+0x16a/0x350
> [   40.780010]  [<ffffffff8168003e>] __mutex_lock_common+0x5e/0x4f0
> [   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
> [   40.780010]  [<ffffffff810c2796>] ? mark_held_locks+0x86/0x150
> [   40.780010]  [<ffffffff816807ae>] ? mutex_unlock+0xe/0x10
> [   40.780010]  [<ffffffff81682807>] ? tty_lock+0x17/0x19
> [   40.780010]  [<ffffffff81680604>] mutex_lock_nested+0x44/0x50
> [   40.780010]  [<ffffffff81682807>] tty_lock+0x17/0x19
> [   40.780010]  [<ffffffff813c91ad>] tty_port_close_start+0x17d/0x210
> [   40.780010]  [<ffffffffa032b32f>] acm_tty_close+0x4f/0xc0 [cdc_acm]
> [   40.780010]  [<ffffffff813c09e7>] tty_release+0x167/0x5c0
> [   40.780010]  [<ffffffff81021fb9>] ? sched_clock+0x9/0x10
> [   40.780010]  [<ffffffff810ae2c5>] ? sched_clock_local+0x25/0x90
> [   40.780010]  [<ffffffff811b252e>] fput+0xfe/0x2d0
> [   40.780010]  [<ffffffff811adee9>] filp_close+0x69/0x90
> [   40.780010]  [<ffffffff811ae1d0>] sys_close+0xc0/0x1a0
> [   40.780010]  [<ffffffff81689f82>] system_call_fastpath+0x16/0x1b
>

Will the following untested patch fix this problem?

------->
tty_port_close_start() will acquire big_tty_mutex, so don't
call it with  open_mutex held.

Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>

[-- Attachment #2: tty-fix.patch --]
[-- Type: text/x-patch, Size: 745 bytes --]

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e8c564a..3a97fec 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -570,8 +570,8 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
 	if (!acm)
 		return;
 
-	mutex_lock(&open_mutex);
 	if (tty_port_close_start(&acm->port, tty, filp) == 0) {
+		mutex_lock(&open_mutex);
 		if (!acm->dev) {
 			tty_port_tty_set(&acm->port, NULL);
 			acm_tty_unregister(acm);
@@ -580,6 +580,7 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
 		mutex_unlock(&open_mutex);
 		return;
 	}
+	mutex_lock(&open_mutex);
 	acm_port_down(acm);
 	tty_port_close_end(&acm->port, tty);
 	tty_port_tty_set(&acm->port, NULL);

Re: tty related lockdep trace during bootup on 3.2-rc2

[Cong Wang]

On Wed, Nov 23, 2011 at 3:28 PM, Havard Skinnemoen
<hskinnemoen@google.com> wrote:
> On Tue, Nov 22, 2011 at 7:38 PM, Dave Jones <davej@redhat.com> wrote:
>> From Linus' current tree...
>>
>> related to 5dc2470c602da8851907ec18942cd876c3b4ecc1 maybe ?
>
> Yes, probably. I did have a bad feeling about the locking, but it
> seemed to behave well during testing. Wonder why I didn't see this.
>
> So what's happening is that tty_open() holds big_tty_mutex while
> calling acm_tty_open which takes open_lock, and acm_tty_close holds
> open_lock while calling tty_port_close_start which takes
> big_tty_mutex?

Yes, exactly.

>
> Not sure how to solve this. Not taking the lock before calling
> tty_port_close_start means the tty_port may get freed before it
> returns.

But...

static void acm_tty_hangup(struct tty_struct *tty)
{
        struct acm *acm = tty->driver_data;
        tty_port_hangup(&acm->port); //<---------------- No open_mutex
protected neither...
        mutex_lock(&open_mutex);
        acm_port_down(acm);
        mutex_unlock(&open_mutex);
}

Re: tty related lockdep trace during bootup on 3.2-rc2

[Jiri Slaby]

On 11/23/2011 08:28 AM, Havard Skinnemoen wrote:
> On Tue, Nov 22, 2011 at 7:38 PM, Dave Jones <davej@redhat.com> wrote:
>> From Linus' current tree...
>>
>> related to 5dc2470c602da8851907ec18942cd876c3b4ecc1 maybe ?
> 
> Yes, probably. I did have a bad feeling about the locking, but it
> seemed to behave well during testing. Wonder why I didn't see this.
> 
> So what's happening is that tty_open() holds big_tty_mutex while
> calling acm_tty_open which takes open_lock, and acm_tty_close holds
> open_lock while calling tty_port_close_start which takes
> big_tty_mutex?
> 
> Not sure how to solve this. Not taking the lock before calling
> tty_port_close_start means the tty_port may get freed before it
> returns.

Oh, how it can be? You should vhangup the device on disconnect. Then you
will be sure all close calls were performed before freeing the port.
Then you can free the port, right?

Like:
* forbid further opens
* tty_vhangup
* stop the device
* free the device state

Also it looks like you have a leak in disconnect when there are users
still. If I am looking correctly?

thanks,
-- 
js
suse labs

Re: tty related lockdep trace during bootup on 3.2-rc2

[Jiri Slaby]

On 11/23/2011 11:12 AM, Jiri Slaby wrote:
> Also it looks like you have a leak in disconnect when there are users
> still. If I am looking correctly?

No, you are freeing it on the last close...

thanks,
-- 
js
suse labs

Re: tty related lockdep trace during bootup on 3.2-rc2

[Havard Skinnemoen]

On Tue, Nov 22, 2011 at 11:29 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> Will the following untested patch fix this problem?

It will probably silence the warning, but it will reopen the race I
was trying to fix.

Havard

Re: tty related lockdep trace during bootup on 3.2-rc2

[Havard Skinnemoen]

On Wed, Nov 23, 2011 at 2:12 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> Oh, how it can be? You should vhangup the device on disconnect. Then you
> will be sure all close calls were performed before freeing the port.
> Then you can free the port, right?
>
> Like:
> * forbid further opens
> * tty_vhangup
> * stop the device
> * free the device state

Ok, interesting. It seems pretty different from what the driver is
currently doing. I'll see if I can figure out how to rework the
disconnect code.

I guess one remaining concern is that someone might still hold the
device open after hangup, so if we free the device state, which
includes the tty_port, something bad might happen, no?

> Also it looks like you have a leak in disconnect when there are users
> still. If I am looking correctly?

Funny you should say that. I'm also trying to investigate a case in
which the driver appears to leak tty devices, i.e. after a few days of
stress testing, /sys/class/tty is full of dangling ttyACMx symlinks
and new devices are ignored because acm_table is full.

Havard

[RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

We can't hold open_lock while calling tty_port_close_start, which takes
big_tty_mutex, because open_lock and big_tty_mutex are taken in the
opposite order when opening the device.

This means we need some other way of preventing the device state from
being freed before we're done cleaning up. Using the existing kref in
tty_port, we can let the TTY side and the USB side indenpendently signal
that they're done with the object, and free it when both are done.

Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
How about something along these lines? I haven't tested it yet, and in fact I'm
a bit worried about the possible lack of symmetry between the tty_port_get() in
acm_tty_open() and the tty_port_put() in acm_tty_cleanup(). Is there any better
way to do this?

 drivers/usb/class/cdc-acm.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e8c564a..c09f3f7 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -62,9 +62,6 @@ static DEFINE_MUTEX(open_mutex);
 
 #define ACM_READY(acm)	(acm && acm->dev && acm->port.count)
 
-static const struct tty_port_operations acm_port_ops = {
-};
-
 /*
  * Functions for ACM control messages.
  */
@@ -501,6 +498,9 @@ static int acm_tty_open(struct tty_struct *tty, struct file *filp)
 	if (acm_submit_read_urbs(acm, GFP_KERNEL))
 		goto bail_out;
 
+	/* Get a reference for the TTY device. Released on the last close */
+	tty_port_get(&acm->port);
+
 	set_bit(ASYNCB_INITIALIZED, &acm->port.flags);
 	rv = tty_port_block_til_ready(&acm->port, tty, filp);
 
@@ -519,8 +519,9 @@ early_bail:
 	return -EIO;
 }
 
-static void acm_tty_unregister(struct acm *acm)
+static void acm_port_destruct(struct tty_port *port)
 {
+	struct acm *acm = container_of(port, struct acm, port);
 	int i;
 
 	tty_unregister_device(acm_tty_driver, acm->minor);
@@ -552,6 +553,12 @@ static void acm_port_down(struct acm *acm)
 	}
 }
 
+static void acm_tty_cleanup(struct tty_struct *tty)
+{
+	struct acm *acm = tty->driver_data;
+	tty_port_put(&acm->port);
+}
+
 static void acm_tty_hangup(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
@@ -567,19 +574,10 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
 
 	/* Perform the closing process and see if we need to do the hardware
 	   shutdown */
-	if (!acm)
-		return;
-
-	mutex_lock(&open_mutex);
 	if (tty_port_close_start(&acm->port, tty, filp) == 0) {
-		if (!acm->dev) {
-			tty_port_tty_set(&acm->port, NULL);
-			acm_tty_unregister(acm);
-			tty->driver_data = NULL;
-		}
-		mutex_unlock(&open_mutex);
 		return;
 	}
+	mutex_lock(&open_mutex);
 	acm_port_down(acm);
 	tty_port_close_end(&acm->port, tty);
 	tty_port_tty_set(&acm->port, NULL);
@@ -788,6 +786,10 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	}
 }
 
+static const struct tty_port_operations acm_port_ops = {
+	.destruct =		acm_port_destruct,
+};
+
 /*
  * USB probe and disconnect routines.
  */
@@ -1206,6 +1208,9 @@ skip_countries:
 
 	dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", minor);
 
+	/* Grab a reference for the USB device. Released on disconnect */
+	tty_port_get(&acm->port);
+
 	acm_set_control(acm, acm->ctrlout);
 
 	acm->line.dwDTERate = cpu_to_le32(9600);
@@ -1287,18 +1292,14 @@ static void acm_disconnect(struct usb_interface *intf)
 		usb_driver_release_interface(&acm_driver, intf == acm->control ?
 					acm->data : acm->control);
 
-	if (acm->port.count == 0) {
-		acm_tty_unregister(acm);
-		mutex_unlock(&open_mutex);
-		return;
-	}
-
 	mutex_unlock(&open_mutex);
 	tty = tty_port_tty_get(&acm->port);
 	if (tty) {
 		tty_hangup(tty);
 		tty_kref_put(tty);
 	}
+
+	tty_port_put(&acm->port);
 }
 
 #ifdef CONFIG_PM
@@ -1596,6 +1597,7 @@ static struct usb_driver acm_driver = {
 static const struct tty_operations acm_ops = {
 	.open =			acm_tty_open,
 	.close =		acm_tty_close,
+	.cleanup =		acm_tty_cleanup,
 	.hangup =		acm_tty_hangup,
 	.write =		acm_tty_write,
 	.write_room =		acm_tty_write_room,
-- 
1.7.3.1

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

On Wed, Nov 23, 2011 at 11:22 AM, Alan Cox <alan@linux.intel.com> wrote:
>        take lock
>        mark device disconnected
>        drop lock
>        for each tty on the device {
>                tty_port_tty_get
>                if (tty) {
>                        tty_vhangup(tty);
>                        tty_kref_put(tty);
>        }
>        then do the USB disconnect processing
>        and drop our USB side kref

Ok, by USB side kref do you mean the tty_port_get/put calls I
introduced in this patch, or a kref associated with the USB device
itself?

> and on the open side they provide their own install() method which
> nicely avoids all the nasty locking problems and lets them use the
> standard tty_port_open/close/hangup etc rather than the partial ones.

Right, that makes sense.

> It might be worth turning the ACM driver to work the same way as it'll
> save doing that work later to achieve the same use of
> tty_port_open/close/..
>
> The main thing is the use of install - which lets you plug the USB and
> tty bits together during the lookup of the device.

Ok, so the basic idea is:
  * USB side: tty_port_get() in probe, tty_port_put() in disconnect
  * TTY side: tty_port_get() in install, tty_port_put() in remove

And I need to check if the device is properly marked as disconnected,
switch to vhangup, and possibly reorder things a bit as well. Right?

Havard

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Alan Cox]

O> This means we need some other way of preventing the device state from
> being freed before we're done cleaning up. Using the existing kref in
> tty_port, we can let the TTY side and the USB side indenpendently
> signal that they're done with the object, and free it when both are
> done.

I would have thought doing a vhangup was the best approach. The way the
USB serial drivers do it on a cable pull is

	take lock
	mark device disconnected
	drop lock
	for each tty on the device {
		tty_port_tty_get
		if (tty) {
			tty_vhangup(tty);
			tty_kref_put(tty);
	}
	then do the USB disconnect processing
	and drop our USB side kref


and on the open side they provide their own install() method which
nicely avoids all the nasty locking problems and lets them use the
standard tty_port_open/close/hangup etc rather than the partial ones.

It might be worth turning the ACM driver to work the same way as it'll
save doing that work later to achieve the same use of
tty_port_open/close/..

The main thing is the use of install - which lets you plug the USB and
tty bits together during the lookup of the device.

Alan

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Oliver Neukum]

Am Mittwoch, 23. November 2011, 20:22:33 schrieb Alan Cox:
> and on the open side they provide their own install() method which
> nicely avoids all the nasty locking problems and lets them use the
> standard tty_port_open/close/hangup etc rather than the partial ones.
> 
> It might be worth turning the ACM driver to work the same way as it'll
> save doing that work later to achieve the same use of
> tty_port_open/close/..
> 
> The main thing is the use of install - which lets you plug the USB and
> tty bits together during the lookup of the device.

Which driver is the best example?

	Regards
		Oliver

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Alan Cox]

> Ok, by USB side kref do you mean the tty_port_get/put calls I
> introduced in this patch, or a kref associated with the USB device
> itself?

The USB serial driver krefs its USB related objects too

> Ok, so the basic idea is:
>   * USB side: tty_port_get() in probe, tty_port_put() in disconnect
>   * TTY side: tty_port_get() in install, tty_port_put() in remove
> 
> And I need to check if the device is properly marked as disconnected,
> switch to vhangup, and possibly reorder things a bit as well. Right?

You shouldn't need to do the port_get in the probe or the put in the
disconnect path. The install/remove one is sufficient for all the
tty_port data because the only way a new open can occur is if the
install succeeds. So providing install and remove are suitably locked
(see the usb_serial.c code) the other cases shouldn't matter.

Any data owned by the USB part needs to be handled by the USB driver.
In the case of usb-serial it keeps its own krefs for that because the
lifetime of the port/USB serial data is not the same as the lifetime of
the tty itself.

The lifetime model is intended to be

object discovered
	Port created

	one or more iterations of
	{
		tty_install (refcounts the port and USB data)
		tty opened
		tty created

		[activity]

		tty closed final time
		tty destroyed (drops a reference on the port and USB
		data)
	}

	Port destroyed

and if the object is destroyed while the tty is opened tty_vhangup
ensures that all the linkages are broken and the port itself can be
destroyed after the last user.

That is tty_struct has a lifetime of open - close (plus a bit for any
active receive etc) while the tty_port is usually embedded in the USB
related material and has a life time of object creation to the point
that the object has gone away AND there are no ttys in existence using
it, hung up or otherwise. The tty "cleanup" hook may be useful for
that. It is called asynchronously as part of the the destructor for the
tty object.

In some ways its easier to read usb-serial.c than describe it

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Jiri Slaby]

On 11/23/2011 07:53 PM, Havard Skinnemoen wrote:
> --- a/drivers/usb/class/cdc-acm.c
> +++ b/drivers/usb/class/cdc-acm.c
...
> @@ -567,19 +574,10 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
>  
>  	/* Perform the closing process and see if we need to do the hardware
>  	   shutdown */
> -	if (!acm)
> -		return;
> -
> -	mutex_lock(&open_mutex);
>  	if (tty_port_close_start(&acm->port, tty, filp) == 0) {

Note that port->count is protected by port->lock usually. Till now it
used to be protected by open_mutex in your driver. As of now it is not
protected by anything. (Well, BTM is still there to save you, but...)

thanks,
-- 
js
suse labs

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

On Wed, Nov 23, 2011 at 11:44 AM, Alan Cox <alan@linux.intel.com> wrote:
> Any data owned by the USB part needs to be handled by the USB driver.
> In the case of usb-serial it keeps its own krefs for that because the
> lifetime of the port/USB serial data is not the same as the lifetime of
> the tty itself.

Yes, exactly. But since tty_port already has a kref, why not use that?
It doesn't have any other use currently.

Hmm, a quick grep tells me tty_port_get() and tty_port_put() are
completely unused. What was the intention behind them?

Havard

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

On Wed, Nov 23, 2011 at 11:55 AM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 11/23/2011 07:53 PM, Havard Skinnemoen wrote:
>> --- a/drivers/usb/class/cdc-acm.c
>> +++ b/drivers/usb/class/cdc-acm.c
> ...
>> @@ -567,19 +574,10 @@ static void acm_tty_close(struct tty_struct *tty, struct file *filp)
>>
>>       /* Perform the closing process and see if we need to do the hardware
>>          shutdown */
>> -     if (!acm)
>> -             return;
>> -
>> -     mutex_lock(&open_mutex);
>>       if (tty_port_close_start(&acm->port, tty, filp) == 0) {
>
> Note that port->count is protected by port->lock usually. Till now it
> used to be protected by open_mutex in your driver. As of now it is not
> protected by anything. (Well, BTM is still there to save you, but...)

tty_port_close_start() takes port->lock, so if we try to do that,
we'll turn a potential deadlock into a real one. Or did you mean
something else?

I think I'll try to follow Alan's advice and use tty_port_close()
instead of the split functions. it shouldn't be too hard once we get
the lifecycle issues out of the way.

Havard

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Jiri Slaby]

On 11/23/2011 10:08 PM, Havard Skinnemoen wrote:
> tty_port_close_start() takes port->lock, so if we try to do that,
> we'll turn a potential deadlock into a real one. Or did you mean
> something else?

I mean the other uses of port->count in your driver.

> I think I'll try to follow Alan's advice and use tty_port_close()
> instead of the split functions.

Yes, that's indeed the best approach. However you have to use them all:
tty_port_open, tty_port_hangup, tty_port_close. OR you would have to do
the locking of count properly.

thanks,
-- 
js
suse labs

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Jiri Slaby]

On 11/23/2011 10:11 PM, Jiri Slaby wrote:
> On 11/23/2011 10:08 PM, Havard Skinnemoen wrote:
>> tty_port_close_start() takes port->lock, so if we try to do that,
>> we'll turn a potential deadlock into a real one. Or did you mean
>> something else?
> 
> I mean the other uses of port->count in your driver.
> 
>> I think I'll try to follow Alan's advice and use tty_port_close()
>> instead of the split functions.
> 
> Yes, that's indeed the best approach. However you have to use them all:
> tty_port_open, tty_port_hangup, tty_port_close. OR you would have to do
> the locking of count properly.

And I see now that the driver uses tty_port_hangup (count under
port->lock) and tty_port_close_start (count under open_mutex and
port->lock). But doesn't use tty_port_open (count under open_mutex and
acm->mutex). Hence there was always a race hangup vs. open wrt. port
counting.

thanks,
-- 
js
suse labs

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Alan Cox]

On Wed, 23 Nov 2011 13:03:27 -0800
Havard Skinnemoen <hskinnemoen@google.com> wrote:

> On Wed, Nov 23, 2011 at 11:44 AM, Alan Cox <alan@linux.intel.com>
> wrote:
> > Any data owned by the USB part needs to be handled by the USB
> > driver. In the case of usb-serial it keeps its own krefs for that
> > because the lifetime of the port/USB serial data is not the same as
> > the lifetime of the tty itself.
> 
> Yes, exactly. But since tty_port already has a kref, why not use that?
> It doesn't have any other use currently.

Indeed
 
> Hmm, a quick grep tells me tty_port_get() and tty_port_put() are
> completely unused. What was the intention behind them?

To provide a way to use the kref nicely.

Re: [RFC] cdc-acm: Fix potential deadlock (lockdep warning)

[Alan Cox]

On Wed, 23 Nov 2011 20:34:24 +0100
Oliver Neukum <oliver@neukum.org> wrote:

> Am Mittwoch, 23. November 2011, 20:22:33 schrieb Alan Cox:
> > and on the open side they provide their own install() method which
> > nicely avoids all the nasty locking problems and lets them use the
> > standard tty_port_open/close/hangup etc rather than the partial
> > ones.
> > 
> > It might be worth turning the ACM driver to work the same way as
> > it'll save doing that work later to achieve the same use of
> > tty_port_open/close/..
> > 
> > The main thing is the use of install - which lets you plug the USB
> > and tty bits together during the lookup of the device.
> 
> Which driver is the best example?

All the usb-serial drivers use the same core usb-serial.c to manage
all these bits. They just plug in additional I/O methods. So just look
at usb-serial.c

[RFC v2] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

Rework the locking and lifecycle management in the cdc-acm driver.
Instead of using a global mutex to prevent the 'acm' object from being
freed, use the tty_port kref to keep the device alive when either the
USB side or TTY side is still active.

This allows us to use the global mutex purely for protecting the
acm_table, while use acm->mutex to guard against disconnect during
TTY port activation and shutdown.

The USB-side kref is taken during port initialization in probe(), and
released at the end of disconnect(). The TTY-side kref is taken in
install() and released in cleanup(). On disconnect, tty_vhangup() is
called instead of tty_hangup() to ensure the TTY hangup processing is
completed before the USB device is taken down.

The TTY open and close handlers have been gutted and replaced with
tty_port_open() and tty_port_close() respectively. The driver-specific
code which used to be there was spread across install(), activate() and
shutdown().

Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
I've tested this briefly, and it seems to behave. With DEBUG defined, I see all
the expected functions being called on open/close/disconnect/etc. and pulling
the cable while the device is open doesn't cause anything bad to happen.
Suspend/resume works with the device plugged in. It did get reset on resume,
but that could be because either it or the hub lost power at some point (I saw
the battery indicator flash briefly.)

Please let me know what you think. The fact that it survives my basic testing
of course doesn't mean I got all the locking right. I'll put it through some
more rigorous testing this week.

 drivers/usb/class/cdc-acm.c |  326 +++++++++++++++++++++++++------------------
 drivers/usb/class/cdc-acm.h |    1 +
 2 files changed, 189 insertions(+), 138 deletions(-)

diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e8c564a..d638ca4 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -58,12 +58,64 @@ static struct usb_driver acm_driver;
 static struct tty_driver *acm_tty_driver;
 static struct acm *acm_table[ACM_TTY_MINORS];
 
-static DEFINE_MUTEX(open_mutex);
+static DEFINE_MUTEX(acm_table_lock);
 
 #define ACM_READY(acm)	(acm && acm->dev && acm->port.count)
 
-static const struct tty_port_operations acm_port_ops = {
-};
+/*
+ * acm_table accessors
+ */
+
+/*
+ * Look up an ACM structure by index. If found and not disconnected, increment
+ * its refcount and return it with its mutex held.
+ */
+static struct acm *acm_get_by_index(unsigned index)
+{
+	struct acm *acm;
+
+	mutex_lock(&acm_table_lock);
+	acm = acm_table[index];
+	if (acm) {
+		mutex_lock(&acm->mutex);
+		if (acm->disconnected) {
+			mutex_unlock(&acm->mutex);
+			acm = NULL;
+		} else {
+			tty_port_get(&acm->port);
+			mutex_unlock(&acm->mutex);
+		}
+	}
+	mutex_unlock(&acm_table_lock);
+	return acm;
+}
+
+/*
+ * Try to find an available minor number and if found, associate it with 'acm'.
+ */
+static int acm_alloc_minor(struct acm *acm)
+{
+	int minor;
+
+	mutex_lock(&acm_table_lock);
+	for (minor = 0; minor < ACM_TTY_MINORS; minor++) {
+		if (!acm_table[minor]) {
+			acm_table[minor] = acm;
+			break;
+		}
+	}
+	mutex_unlock(&acm_table_lock);
+
+	return minor;
+}
+
+/* Release the minor number associated with 'acm'.  */
+static void acm_release_minor(struct acm *acm)
+{
+	mutex_lock(&acm_table_lock);
+	acm_table[acm->minor] = NULL;
+	mutex_unlock(&acm_table_lock);
+}
 
 /*
  * Functions for ACM control messages.
@@ -453,93 +505,122 @@ static void acm_softint(struct work_struct *work)
  * TTY handlers
  */
 
-static int acm_tty_open(struct tty_struct *tty, struct file *filp)
+static int acm_tty_install(struct tty_driver *driver, struct tty_struct *tty)
 {
 	struct acm *acm;
-	int rv = -ENODEV;
-
-	mutex_lock(&open_mutex);
+	int retval;
 
-	acm = acm_table[tty->index];
-	if (!acm || !acm->dev)
-		goto out;
-	else
-		rv = 0;
+	dev_dbg(tty->dev, "%s\n", __func__);
 
-	dev_dbg(&acm->control->dev, "%s\n", __func__);
+	acm = acm_get_by_index(tty->index);
+	if (!acm)
+		return -ENODEV;
 
-	set_bit(TTY_NO_WRITE_SPLIT, &tty->flags);
+	retval = tty_init_termios(tty);
+	if (retval)
+		goto error_init_termios;
 
 	tty->driver_data = acm;
-	tty_port_tty_set(&acm->port, tty);
 
-	if (usb_autopm_get_interface(acm->control) < 0)
-		goto early_bail;
-	else
-		acm->control->needs_remote_wakeup = 1;
+	/* Final install (we use the default method) */
+	tty_driver_kref_get(driver);
+	tty->count++;
+	driver->ttys[tty->index] = tty;
+
+	return 0;
+
+error_init_termios:
+	tty_port_put(&acm->port);
+	return retval;
+}
+
+static int acm_tty_open(struct tty_struct *tty, struct file *filp)
+{
+	struct acm *acm = tty->driver_data;
+
+	dev_dbg(tty->dev, "%s\n", __func__);
+
+	return tty_port_open(&acm->port, tty, filp);
+}
+
+static int acm_port_activate(struct tty_port *port, struct tty_struct *tty)
+{
+	struct acm *acm = container_of(port, struct acm, port);
+	int retval = -ENODEV;
+
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
 
 	mutex_lock(&acm->mutex);
-	if (acm->port.count++) {
-		mutex_unlock(&acm->mutex);
-		usb_autopm_put_interface(acm->control);
-		goto out;
-	}
+	if (acm->disconnected)
+		goto disconnected;
+
+	retval = usb_autopm_get_interface(acm->control);
+	if (retval)
+		goto error_get_interface;
+
+	/*
+	 * FIXME: Why do we need this? Allocating 64K of physically contiguous
+	 * memory is really nasty...
+	 */
+	set_bit(TTY_NO_WRITE_SPLIT, &tty->flags);
+	acm->control->needs_remote_wakeup = 1;
 
 	acm->ctrlurb->dev = acm->dev;
 	if (usb_submit_urb(acm->ctrlurb, GFP_KERNEL)) {
 		dev_err(&acm->control->dev,
 			"%s - usb_submit_urb(ctrl irq) failed\n", __func__);
-		goto bail_out;
+		goto error_submit_urb;
 	}
 
-	if (0 > acm_set_control(acm, acm->ctrlout = ACM_CTRL_DTR | ACM_CTRL_RTS) &&
+	acm->ctrlout = ACM_CTRL_DTR | ACM_CTRL_RTS;
+	if (acm_set_control(acm, acm->ctrlout < 0) &&
 	    (acm->ctrl_caps & USB_CDC_CAP_LINE))
-		goto bail_out;
+		goto error_set_control;
 
 	usb_autopm_put_interface(acm->control);
 
 	if (acm_submit_read_urbs(acm, GFP_KERNEL))
-		goto bail_out;
-
-	set_bit(ASYNCB_INITIALIZED, &acm->port.flags);
-	rv = tty_port_block_til_ready(&acm->port, tty, filp);
+		goto error_submit_read_urbs;
 
 	mutex_unlock(&acm->mutex);
-out:
-	mutex_unlock(&open_mutex);
-	return rv;
 
-bail_out:
-	acm->port.count--;
-	mutex_unlock(&acm->mutex);
+	return 0;
+
+error_submit_read_urbs:
+	acm->ctrlout = 0;
+	acm_set_control(acm, acm->ctrlout);
+error_set_control:
+	usb_kill_urb(acm->ctrlurb);
+error_submit_urb:
 	usb_autopm_put_interface(acm->control);
-early_bail:
-	mutex_unlock(&open_mutex);
-	tty_port_tty_set(&acm->port, NULL);
-	return -EIO;
+error_get_interface:
+disconnected:
+	mutex_unlock(&acm->mutex);
+	return retval;
 }
 
-static void acm_tty_unregister(struct acm *acm)
+static void acm_port_destruct(struct tty_port *port)
 {
-	int i;
+	struct acm *acm = container_of(port, struct acm, port);
+
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
 
 	tty_unregister_device(acm_tty_driver, acm->minor);
+	acm_release_minor(acm);
 	usb_put_intf(acm->control);
-	acm_table[acm->minor] = NULL;
-	usb_free_urb(acm->ctrlurb);
-	for (i = 0; i < ACM_NW; i++)
-		usb_free_urb(acm->wb[i].urb);
-	for (i = 0; i < acm->rx_buflimit; i++)
-		usb_free_urb(acm->read_urbs[i]);
 	kfree(acm->country_codes);
 	kfree(acm);
 }
 
-static void acm_port_down(struct acm *acm)
+static void acm_port_shutdown(struct tty_port *port)
 {
+	struct acm *acm = container_of(port, struct acm, port);
 	int i;
 
-	if (acm->dev) {
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
+
+	mutex_lock(&acm->mutex);
+	if (!acm->disconnected) {
 		usb_autopm_get_interface(acm->control);
 		acm_set_control(acm, acm->ctrlout = 0);
 		usb_kill_urb(acm->ctrlurb);
@@ -550,40 +631,28 @@ static void acm_port_down(struct acm *acm)
 		acm->control->needs_remote_wakeup = 0;
 		usb_autopm_put_interface(acm->control);
 	}
+	mutex_unlock(&acm->mutex);
+}
+
+static void acm_tty_cleanup(struct tty_struct *tty)
+{
+	struct acm *acm = tty->driver_data;
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
+	tty_port_put(&acm->port);
 }
 
 static void acm_tty_hangup(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
 	tty_port_hangup(&acm->port);
-	mutex_lock(&open_mutex);
-	acm_port_down(acm);
-	mutex_unlock(&open_mutex);
 }
 
 static void acm_tty_close(struct tty_struct *tty, struct file *filp)
 {
 	struct acm *acm = tty->driver_data;
-
-	/* Perform the closing process and see if we need to do the hardware
-	   shutdown */
-	if (!acm)
-		return;
-
-	mutex_lock(&open_mutex);
-	if (tty_port_close_start(&acm->port, tty, filp) == 0) {
-		if (!acm->dev) {
-			tty_port_tty_set(&acm->port, NULL);
-			acm_tty_unregister(acm);
-			tty->driver_data = NULL;
-		}
-		mutex_unlock(&open_mutex);
-		return;
-	}
-	acm_port_down(acm);
-	tty_port_close_end(&acm->port, tty);
-	tty_port_tty_set(&acm->port, NULL);
-	mutex_unlock(&open_mutex);
+	dev_dbg(&acm->control->dev, "%s\n", __func__);
+	tty_port_close(&acm->port, tty, filp);
 }
 
 static int acm_tty_write(struct tty_struct *tty,
@@ -595,8 +664,6 @@ static int acm_tty_write(struct tty_struct *tty,
 	int wbn;
 	struct acm_wb *wb;
 
-	if (!ACM_READY(acm))
-		return -EINVAL;
 	if (!count)
 		return 0;
 
@@ -625,8 +692,6 @@ static int acm_tty_write(struct tty_struct *tty,
 static int acm_tty_write_room(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
-	if (!ACM_READY(acm))
-		return -EINVAL;
 	/*
 	 * Do not let the line discipline to know that we have a reserve,
 	 * or it might get too enthusiastic.
@@ -637,7 +702,11 @@ static int acm_tty_write_room(struct tty_struct *tty)
 static int acm_tty_chars_in_buffer(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
-	if (!ACM_READY(acm))
+	/*
+	 * if the device was unplugged then any remaining characters fell out
+	 * of the connector ;)
+	 */
+	if (acm->disconnected)
 		return 0;
 	/*
 	 * This is inaccurate (overcounts), but it works.
@@ -649,9 +718,6 @@ static void acm_tty_throttle(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
 
-	if (!ACM_READY(acm))
-		return;
-
 	spin_lock_irq(&acm->read_lock);
 	acm->throttle_req = 1;
 	spin_unlock_irq(&acm->read_lock);
@@ -662,9 +728,6 @@ static void acm_tty_unthrottle(struct tty_struct *tty)
 	struct acm *acm = tty->driver_data;
 	unsigned int was_throttled;
 
-	if (!ACM_READY(acm))
-		return;
-
 	spin_lock_irq(&acm->read_lock);
 	was_throttled = acm->throttled;
 	acm->throttled = 0;
@@ -679,8 +742,7 @@ static int acm_tty_break_ctl(struct tty_struct *tty, int state)
 {
 	struct acm *acm = tty->driver_data;
 	int retval;
-	if (!ACM_READY(acm))
-		return -EINVAL;
+
 	retval = acm_send_break(acm, state ? 0xffff : 0);
 	if (retval < 0)
 		dev_dbg(&acm->control->dev, "%s - send break failed\n",
@@ -692,9 +754,6 @@ static int acm_tty_tiocmget(struct tty_struct *tty)
 {
 	struct acm *acm = tty->driver_data;
 
-	if (!ACM_READY(acm))
-		return -EINVAL;
-
 	return (acm->ctrlout & ACM_CTRL_DTR ? TIOCM_DTR : 0) |
 	       (acm->ctrlout & ACM_CTRL_RTS ? TIOCM_RTS : 0) |
 	       (acm->ctrlin  & ACM_CTRL_DSR ? TIOCM_DSR : 0) |
@@ -709,9 +768,6 @@ static int acm_tty_tiocmset(struct tty_struct *tty,
 	struct acm *acm = tty->driver_data;
 	unsigned int newctrl;
 
-	if (!ACM_READY(acm))
-		return -EINVAL;
-
 	newctrl = acm->ctrlout;
 	set = (set & TIOCM_DTR ? ACM_CTRL_DTR : 0) |
 					(set & TIOCM_RTS ? ACM_CTRL_RTS : 0);
@@ -728,11 +784,6 @@ static int acm_tty_tiocmset(struct tty_struct *tty,
 static int acm_tty_ioctl(struct tty_struct *tty,
 					unsigned int cmd, unsigned long arg)
 {
-	struct acm *acm = tty->driver_data;
-
-	if (!ACM_READY(acm))
-		return -EINVAL;
-
 	return -ENOIOCTLCMD;
 }
 
@@ -756,9 +807,6 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	struct usb_cdc_line_coding newline;
 	int newctrl = acm->ctrlout;
 
-	if (!ACM_READY(acm))
-		return;
-
 	newline.dwDTERate = cpu_to_le32(tty_get_baud_rate(tty));
 	newline.bCharFormat = termios->c_cflag & CSTOPB ? 2 : 0;
 	newline.bParityType = termios->c_cflag & PARENB ?
@@ -788,6 +836,12 @@ static void acm_tty_set_termios(struct tty_struct *tty,
 	}
 }
 
+static const struct tty_port_operations acm_port_ops = {
+	.shutdown = acm_port_shutdown,
+	.activate = acm_port_activate,
+	.destruct = acm_port_destruct,
+};
+
 /*
  * USB probe and disconnect routines.
  */
@@ -1047,12 +1101,6 @@ skip_normal_probe:
 	}
 made_compressed_probe:
 	dev_dbg(&intf->dev, "interfaces are valid\n");
-	for (minor = 0; minor < ACM_TTY_MINORS && acm_table[minor]; minor++);
-
-	if (minor == ACM_TTY_MINORS) {
-		dev_err(&intf->dev, "no more free acm devices\n");
-		return -ENODEV;
-	}
 
 	acm = kzalloc(sizeof(struct acm), GFP_KERNEL);
 	if (acm == NULL) {
@@ -1060,6 +1108,13 @@ made_compressed_probe:
 		goto alloc_fail;
 	}
 
+	minor = acm_alloc_minor(acm);
+	if (minor == ACM_TTY_MINORS) {
+		dev_err(&intf->dev, "no more free acm devices\n");
+		kfree(acm);
+		return -ENODEV;
+	}
+
 	ctrlsize = usb_endpoint_maxp(epctrl);
 	readsize = usb_endpoint_maxp(epread) *
 				(quirks == SINGLE_RX_URB ? 1 : 2);
@@ -1218,8 +1273,6 @@ skip_countries:
 	usb_get_intf(control_interface);
 	tty_register_device(acm_tty_driver, minor, &control_interface->dev);
 
-	acm_table[minor] = acm;
-
 	return 0;
 alloc_fail7:
 	for (i = 0; i < ACM_NW; i++)
@@ -1234,6 +1287,7 @@ alloc_fail5:
 alloc_fail4:
 	usb_free_coherent(usb_dev, ctrlsize, acm->ctrl_buffer, acm->ctrl_dma);
 alloc_fail2:
+	acm_release_minor(acm);
 	kfree(acm);
 alloc_fail:
 	return -ENOMEM;
@@ -1259,12 +1313,16 @@ static void acm_disconnect(struct usb_interface *intf)
 	struct acm *acm = usb_get_intfdata(intf);
 	struct usb_device *usb_dev = interface_to_usbdev(intf);
 	struct tty_struct *tty;
+	int i;
+
+	dev_dbg(&intf->dev, "%s\n", __func__);
 
 	/* sibling interface is already cleaning up */
 	if (!acm)
 		return;
 
-	mutex_lock(&open_mutex);
+	mutex_lock(&acm->mutex);
+	acm->disconnected = true;
 	if (acm->country_codes) {
 		device_remove_file(&acm->control->dev,
 				&dev_attr_wCountryCodes);
@@ -1272,33 +1330,32 @@ static void acm_disconnect(struct usb_interface *intf)
 				&dev_attr_iCountryCodeRelDate);
 	}
 	device_remove_file(&acm->control->dev, &dev_attr_bmCapabilities);
-	acm->dev = NULL;
 	usb_set_intfdata(acm->control, NULL);
 	usb_set_intfdata(acm->data, NULL);
+	mutex_unlock(&acm->mutex);
+
+	tty = tty_port_tty_get(&acm->port);
+	if (tty) {
+		tty_vhangup(tty);
+		tty_kref_put(tty);
+	}
 
 	stop_data_traffic(acm);
 
+	usb_free_urb(acm->ctrlurb);
+	for (i = 0; i < ACM_NW; i++)
+		usb_free_urb(acm->wb[i].urb);
+	for (i = 0; i < acm->rx_buflimit; i++)
+		usb_free_urb(acm->read_urbs[i]);
 	acm_write_buffers_free(acm);
-	usb_free_coherent(usb_dev, acm->ctrlsize, acm->ctrl_buffer,
-			  acm->ctrl_dma);
+	usb_free_coherent(usb_dev, acm->ctrlsize, acm->ctrl_buffer, acm->ctrl_dma);
 	acm_read_buffers_free(acm);
 
 	if (!acm->combined_interfaces)
 		usb_driver_release_interface(&acm_driver, intf == acm->control ?
 					acm->data : acm->control);
 
-	if (acm->port.count == 0) {
-		acm_tty_unregister(acm);
-		mutex_unlock(&open_mutex);
-		return;
-	}
-
-	mutex_unlock(&open_mutex);
-	tty = tty_port_tty_get(&acm->port);
-	if (tty) {
-		tty_hangup(tty);
-		tty_kref_put(tty);
-	}
+	tty_port_put(&acm->port);
 }
 
 #ifdef CONFIG_PM
@@ -1325,16 +1382,10 @@ static int acm_suspend(struct usb_interface *intf, pm_message_t message)
 
 	if (cnt)
 		return 0;
-	/*
-	we treat opened interfaces differently,
-	we must guard against open
-	*/
-	mutex_lock(&acm->mutex);
 
-	if (acm->port.count)
+	if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags))
 		stop_data_traffic(acm);
 
-	mutex_unlock(&acm->mutex);
 	return 0;
 }
 
@@ -1353,8 +1404,7 @@ static int acm_resume(struct usb_interface *intf)
 	if (cnt)
 		return 0;
 
-	mutex_lock(&acm->mutex);
-	if (acm->port.count) {
+	if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags)) {
 		rv = usb_submit_urb(acm->ctrlurb, GFP_NOIO);
 
 		spin_lock_irq(&acm->write_lock);
@@ -1378,7 +1428,6 @@ static int acm_resume(struct usb_interface *intf)
 	}
 
 err_out:
-	mutex_unlock(&acm->mutex);
 	return rv;
 }
 
@@ -1387,15 +1436,14 @@ static int acm_reset_resume(struct usb_interface *intf)
 	struct acm *acm = usb_get_intfdata(intf);
 	struct tty_struct *tty;
 
-	mutex_lock(&acm->mutex);
-	if (acm->port.count) {
+	if (test_bit(ASYNCB_INITIALIZED, &acm->port.flags)) {
 		tty = tty_port_tty_get(&acm->port);
 		if (tty) {
 			tty_hangup(tty);
 			tty_kref_put(tty);
 		}
 	}
-	mutex_unlock(&acm->mutex);
+
 	return acm_resume(intf);
 }
 
@@ -1594,8 +1642,10 @@ static struct usb_driver acm_driver = {
  */
 
 static const struct tty_operations acm_ops = {
+	.install =		acm_tty_install,
 	.open =			acm_tty_open,
 	.close =		acm_tty_close,
+	.cleanup =		acm_tty_cleanup,
 	.hangup =		acm_tty_hangup,
 	.write =		acm_tty_write,
 	.write_room =		acm_tty_write_room,
diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h
index ca7937f..35ef887 100644
--- a/drivers/usb/class/cdc-acm.h
+++ b/drivers/usb/class/cdc-acm.h
@@ -101,6 +101,7 @@ struct acm {
 	int transmitting;
 	spinlock_t write_lock;
 	struct mutex mutex;
+	bool disconnected;
 	struct usb_cdc_line_coding line;		/* bits, stop, parity */
 	struct work_struct work;			/* work queue entry for line discipline waking up */
 	unsigned int ctrlin;				/* input control lines (DCD, DSR, RI, break, overruns) */
-- 
1.7.3.1

Re: [RFC v2] cdc-acm: Fix potential deadlock (lockdep warning)

[Havard Skinnemoen]

On Sun, Nov 27, 2011 at 1:37 PM, Havard Skinnemoen
<hskinnemoen@google.com> wrote:
> -       if (0 > acm_set_control(acm, acm->ctrlout = ACM_CTRL_DTR | ACM_CTRL_RTS) &&
> +       acm->ctrlout = ACM_CTRL_DTR | ACM_CTRL_RTS;
> +       if (acm_set_control(acm, acm->ctrlout < 0) &&

Ok, so this is obviously broken. Unfortunately, my test setup doesn't
use the control signals in any way.

I'm sure I broke something else, so I'm going to wait a bit for others
to have a look before I post an updated patch.

Havard