* [PATCH v2 2/3] Bluetooth: Rename hci_acl_disconn
2013-01-30 14:50 [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine Andre Guedes
@ 2013-01-30 14:50 ` Andre Guedes
2013-02-20 17:41 ` Gustavo Padovan
2013-01-30 14:50 ` [PATCH v2 3/3] Bluetooth: Reduce critical section in sco_conn_ready Andre Guedes
2013-01-31 17:39 ` [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine Gustavo Padovan
2 siblings, 1 reply; 5+ messages in thread
From: Andre Guedes @ 2013-01-30 14:50 UTC (permalink / raw)
To: linux-bluetooth
As hci_acl_disconn function basically sends the HCI Disconnect Command
and it is used to disconnect ACL, SCO and LE links, renaming it to
hci_disconnect is more suitable.
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
include/net/bluetooth/hci_core.h | 2 +-
net/bluetooth/hci_conn.c | 4 ++--
net/bluetooth/hci_core.c | 2 +-
net/bluetooth/hci_event.c | 4 ++--
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 90cf75a..787d3b9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -574,7 +574,7 @@ static inline struct hci_conn *hci_conn_hash_lookup_state(struct hci_dev *hdev,
return NULL;
}
-void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
+void hci_disconnect(struct hci_conn *conn, __u8 reason);
void hci_setup_sync(struct hci_conn *conn, __u16 handle);
void hci_sco_setup(struct hci_conn *conn, __u8 status);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 4925a02..b9f9016 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -117,7 +117,7 @@ static void hci_acl_create_connection_cancel(struct hci_conn *conn)
hci_send_cmd(conn->hdev, HCI_OP_CREATE_CONN_CANCEL, sizeof(cp), &cp);
}
-void hci_acl_disconn(struct hci_conn *conn, __u8 reason)
+void hci_disconnect(struct hci_conn *conn, __u8 reason)
{
struct hci_cp_disconnect cp;
@@ -253,7 +253,7 @@ static void hci_conn_disconnect(struct hci_conn *conn)
hci_amp_disconn(conn, reason);
break;
default:
- hci_acl_disconn(conn, reason);
+ hci_disconnect(conn, reason);
break;
}
}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 618ca1a..388c456 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2398,7 +2398,7 @@ static void hci_link_tx_to(struct hci_dev *hdev, __u8 type)
if (c->type == type && c->sent) {
BT_ERR("%s killing stalled connection %pMR",
hdev->name, &c->dst);
- hci_acl_disconn(c, HCI_ERROR_REMOTE_USER_TERM);
+ hci_disconnect(c, HCI_ERROR_REMOTE_USER_TERM);
}
}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d4fcba6..5c78480 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2399,7 +2399,7 @@ static void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
if (ev->status && conn->state == BT_CONNECTED) {
- hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
+ hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
hci_conn_put(conn);
goto unlock;
}
@@ -3472,7 +3472,7 @@ static void hci_key_refresh_complete_evt(struct hci_dev *hdev,
clear_bit(HCI_CONN_ENCRYPT_PEND, &conn->flags);
if (ev->status && conn->state == BT_CONNECTED) {
- hci_acl_disconn(conn, HCI_ERROR_AUTH_FAILURE);
+ hci_disconnect(conn, HCI_ERROR_AUTH_FAILURE);
hci_conn_put(conn);
goto unlock;
}
--
1.8.1.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 2/3] Bluetooth: Rename hci_acl_disconn
2013-01-30 14:50 ` [PATCH v2 2/3] Bluetooth: Rename hci_acl_disconn Andre Guedes
@ 2013-02-20 17:41 ` Gustavo Padovan
0 siblings, 0 replies; 5+ messages in thread
From: Gustavo Padovan @ 2013-02-20 17:41 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
Hi Andre,
* Andre Guedes <andre.guedes@openbossa.org> [2013-01-30 11:50:56 -0300]:
> As hci_acl_disconn function basically sends the HCI Disconnect Command
> and it is used to disconnect ACL, SCO and LE links, renaming it to
> hci_disconnect is more suitable.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h | 2 +-
> net/bluetooth/hci_conn.c | 4 ++--
> net/bluetooth/hci_core.c | 2 +-
> net/bluetooth/hci_event.c | 4 ++--
> 4 files changed, 6 insertions(+), 6 deletions(-)
Patch has been applied to bluetooth-next. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 3/3] Bluetooth: Reduce critical section in sco_conn_ready
2013-01-30 14:50 [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine Andre Guedes
2013-01-30 14:50 ` [PATCH v2 2/3] Bluetooth: Rename hci_acl_disconn Andre Guedes
@ 2013-01-30 14:50 ` Andre Guedes
2013-01-31 17:39 ` [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine Gustavo Padovan
2 siblings, 0 replies; 5+ messages in thread
From: Andre Guedes @ 2013-01-30 14:50 UTC (permalink / raw)
To: linux-bluetooth
This patch reduces the critical section protected by sco_conn_lock in
sco_conn_ready function. The lock is acquired only when it is really
needed.
This patch fixes the following lockdep warning which is generated
when the host terminates a SCO connection.
Today, this warning is a false positive. There is no way those
two threads reported by lockdep are running at the same time since
hdev->workqueue (where rx_work is queued) is single-thread. However,
if somehow this behavior is changed in future, we will have a
potential deadlock.
======================================================
[ INFO: possible circular locking dependency detected ]
3.8.0-rc1+ #7 Not tainted
-------------------------------------------------------
kworker/u:1H/1018 is trying to acquire lock:
(&(&conn->lock)->rlock){+.+...}, at: [<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
but task is already holding lock:
(slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}:
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa003436e>] sco_connect_cfm+0xbe/0x350 [bluetooth]
[<ffffffffa0015d6c>] hci_event_packet+0xd3c/0x29b0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
-> #0 (&(&conn->lock)->rlock){+.+...}:
[<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
[<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
[<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
[<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
lock(&(&conn->lock)->rlock);
lock(slock-AF_BLUETOOTH-BTPROTO_SCO);
lock(&(&conn->lock)->rlock);
*** DEADLOCK ***
4 locks held by kworker/u:1H/1018:
#0: (hdev->name#2){.+.+.+}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
#1: ((&hdev->rx_work)){+.+.+.}, at: [<ffffffff8104d5f8>] process_one_work+0x258/0x4f0
#2: (&hdev->lock){+.+.+.}, at: [<ffffffffa000fbe9>] hci_disconn_complete_evt.isra.54+0x59/0x3c0 [bluetooth]
#3: (slock-AF_BLUETOOTH-BTPROTO_SCO){+.+...}, at: [<ffffffffa0033d5a>] sco_conn_del+0x8a/0xe0 [bluetooth]
stack backtrace:
Pid: 1018, comm: kworker/u:1H Not tainted 3.8.0-rc1+ #7
Call Trace:
[<ffffffff813e92f9>] print_circular_bug+0x1fb/0x20c
[<ffffffff81082215>] __lock_acquire+0x1465/0x1c70
[<ffffffff81083011>] lock_acquire+0xb1/0xe0
[<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffff813efd01>] _raw_spin_lock+0x41/0x80
[<ffffffffa0033ba6>] ? sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033ba6>] sco_chan_del+0x66/0x190 [bluetooth]
[<ffffffffa0033d6d>] sco_conn_del+0x9d/0xe0 [bluetooth]
[<ffffffffa0034653>] sco_disconn_cfm+0x53/0x60 [bluetooth]
[<ffffffffa000fef3>] hci_disconn_complete_evt.isra.54+0x363/0x3c0 [bluetooth]
[<ffffffffa000fbd0>] ? hci_disconn_complete_evt.isra.54+0x40/0x3c0 [bluetooth]
[<ffffffffa00150f7>] hci_event_packet+0xc7/0x29b0 [bluetooth]
[<ffffffff81202e90>] ? __dynamic_pr_debug+0x80/0x90
[<ffffffff8133ff7d>] ? kfree_skb+0x2d/0x40
[<ffffffffa0021644>] ? hci_send_to_monitor+0x1a4/0x1c0 [bluetooth]
[<ffffffffa0004583>] hci_rx_work+0x133/0x870 [bluetooth]
[<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
[<ffffffff8104d65f>] process_one_work+0x2bf/0x4f0
[<ffffffff8104d5f8>] ? process_one_work+0x258/0x4f0
[<ffffffff8104fdc1>] ? worker_thread+0x51/0x3e0
[<ffffffffa0004450>] ? hci_tx_work+0x800/0x800 [bluetooth]
[<ffffffff81050022>] worker_thread+0x2b2/0x3e0
[<ffffffff8104fd70>] ? busy_worker_rebind_fn+0x100/0x100
[<ffffffff81056021>] kthread+0xd1/0xe0
[<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
[<ffffffff813f14bc>] ret_from_fork+0x7c/0xb0
[<ffffffff81055f50>] ? flush_kthread_worker+0xc0/0xc0
Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
This lockdep warning has been around for a long time. I could test
until Linux 3.4, but it seems it is older than that. However, in
current bluetooth-next, this warning has been masked by commit
53502d69be49e3dd5bc95ab0f2deeaea260bd617 which introduced a bug in
SCO hci_conn timeout routine.
The bug in SCO hci_conn timeout has been fixed by patch 01/03 from
this patchset.
net/bluetooth/sco.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 531a93d..e435641 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -900,8 +900,6 @@ static void sco_conn_ready(struct sco_conn *conn)
BT_DBG("conn %p", conn);
- sco_conn_lock(conn);
-
if (sk) {
sco_sock_clear_timer(sk);
bh_lock_sock(sk);
@@ -909,9 +907,13 @@ static void sco_conn_ready(struct sco_conn *conn)
sk->sk_state_change(sk);
bh_unlock_sock(sk);
} else {
+ sco_conn_lock(conn);
+
parent = sco_get_sock_listen(conn->src);
- if (!parent)
- goto done;
+ if (!parent) {
+ sco_conn_unlock(conn);
+ return;
+ }
bh_lock_sock(parent);
@@ -919,7 +921,8 @@ static void sco_conn_ready(struct sco_conn *conn)
BTPROTO_SCO, GFP_ATOMIC);
if (!sk) {
bh_unlock_sock(parent);
- goto done;
+ sco_conn_unlock(conn);
+ return;
}
sco_sock_init(sk, parent);
@@ -939,10 +942,9 @@ static void sco_conn_ready(struct sco_conn *conn)
parent->sk_data_ready(parent, 1);
bh_unlock_sock(parent);
- }
-done:
- sco_conn_unlock(conn);
+ sco_conn_unlock(conn);
+ }
}
/* ----- SCO interface with lower layer (HCI) ----- */
--
1.8.1.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine
2013-01-30 14:50 [PATCH v2 1/3] Bluetooth: Fix hci_conn timeout routine Andre Guedes
2013-01-30 14:50 ` [PATCH v2 2/3] Bluetooth: Rename hci_acl_disconn Andre Guedes
2013-01-30 14:50 ` [PATCH v2 3/3] Bluetooth: Reduce critical section in sco_conn_ready Andre Guedes
@ 2013-01-31 17:39 ` Gustavo Padovan
2 siblings, 0 replies; 5+ messages in thread
From: Gustavo Padovan @ 2013-01-31 17:39 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
Hi Andre,
* Andre Guedes <andre.guedes@openbossa.org> [2013-01-30 11:50:55 -0300]:
> If occurs a LE or SCO hci_conn timeout and the connection is already
> established (BT_CONNECTED state), the connection is not terminated as
> expected. This bug can be reproduced using l2test or scotest tool.
> Once the connection is established, kill l2test/scotest and the
> connection won't be terminated.
>
> This patch fixes hci_conn_disconnect helper so it is able to
> terminate LE and SCO connections, as well as ACL.
>
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
I just pushed this patch to bluetooth.git, this also means I'm not applying
the hci_acl_disconn() rename patch until the next release cycle. A rename
patch is not suitable for 3.8 anymore. Thanks.
Gustavo
^ permalink raw reply [flat|nested] 5+ messages in thread