[RFCv3 00/16] Bluetooth: Change socket lock to l2cap_chan lock

[RFCv3 00/16] Bluetooth: Change socket lock to l2cap_chan lock

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Those small patches which dealing with locks need to be applied at once,
if needed they can be merged together. Please voice opinion about the split.

Changes:
	* RFCv3: Split the big patch to several small (I believe logical) chunks,
	remove unneded locks from cleanup_listen, use the same arguments for
	locked/unlocked socket error functions.
	* RFCv2: Convert l2cap channel list back to mutex from RCU list.

This is DRAFT RFC introducing l2cap channel lock. Please make suggestion how to
make it better.

Andrei Emeltchenko (16):
  Bluetooth: trivial: Fix long line
  Bluetooth: Revert to mutexes from RCU list
  Bluetooth: Do not use sk lock in get_chan functions
  Bluetooth: Add l2cap_chan_lock
  Bluetooth: Add locked and unlocked state_change
  Bluetooth: Add socket error function
  Bluetooth: Add unlocked __l2cap_chan_add function
  Bluetooth: Use chan lock in timers
  Bluetooth: Use chan lock in L2CAP sig commands
  Bluetooth: Use chan lock in chan delete functions
  Bluetooth: Use chan lock in L2CAP conn start
  Bluetooth: Use chan lock in receiving data
  Bluetooth: Change locking logic for conn/chan ready
  Bluetooth: Change locking logic in security_cfm
  Bluetooth: Use l2cap chan lock in socket connect
  Bluetooth: Remove socket lock check

 include/net/bluetooth/l2cap.h |   11 ++
 net/bluetooth/l2cap_core.c    |  388 +++++++++++++++++++++++++----------------
 net/bluetooth/l2cap_sock.c    |   23 ++-
 3 files changed, 268 insertions(+), 154 deletions(-)

-- 
1.7.8.3

[RFCv3 01/16] Bluetooth: trivial: Fix long line

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f1a6b3c..8dfccb3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2743,7 +2743,8 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	result = __le16_to_cpu(rsp->result);
 	status = __le16_to_cpu(rsp->status);
 
-	BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x", dcid, scid, result, status);
+	BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
+						dcid, scid, result, status);
 
 	if (scid) {
 		chan = l2cap_get_chan_by_scid(conn, scid);
-- 
1.7.8.3

[RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Usage of RCU list looks not reasonalbe for a number of reasons:
our code sleep and we have to use socket spinlocks, some parts
of code are updaters thus we need to use mutexes anyway.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |  108 ++++++++++++++++++++++----------------------
 1 files changed, 54 insertions(+), 54 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8dfccb3..ae08944 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
 
 static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
 {
-	struct l2cap_chan *c, *r = NULL;
-
-	rcu_read_lock();
+	struct l2cap_chan *c;
 
-	list_for_each_entry_rcu(c, &conn->chan_l, list) {
-		if (c->dcid == cid) {
-			r = c;
-			break;
-		}
+	list_for_each_entry(c, &conn->chan_l, list) {
+		if (c->dcid == cid)
+			return c;
 	}
-
-	rcu_read_unlock();
-	return r;
+	return NULL;
 }
 
 static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
 {
-	struct l2cap_chan *c, *r = NULL;
-
-	rcu_read_lock();
+	struct l2cap_chan *c;
 
-	list_for_each_entry_rcu(c, &conn->chan_l, list) {
-		if (c->scid == cid) {
-			r = c;
-			break;
-		}
+	list_for_each_entry(c, &conn->chan_l, list) {
+		if (c->scid == cid)
+			return c;
 	}
-
-	rcu_read_unlock();
-	return r;
+	return NULL;
 }
 
 /* Find channel with given SCID.
@@ -115,36 +103,34 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
 {
 	struct l2cap_chan *c;
 
+	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_scid(conn, cid);
 	if (c)
 		lock_sock(c->sk);
+	mutex_unlock(&conn->chan_lock);
 	return c;
 }
 
 static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
 {
-	struct l2cap_chan *c, *r = NULL;
-
-	rcu_read_lock();
+	struct l2cap_chan *c;
 
-	list_for_each_entry_rcu(c, &conn->chan_l, list) {
-		if (c->ident == ident) {
-			r = c;
-			break;
-		}
+	list_for_each_entry(c, &conn->chan_l, list) {
+		if (c->ident == ident)
+			return c;
 	}
-
-	rcu_read_unlock();
-	return r;
+	return NULL;
 }
 
 static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
 {
 	struct l2cap_chan *c;
 
+	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_ident(conn, ident);
 	if (c)
 		lock_sock(c->sk);
+	mutex_unlock(&conn->chan_lock);
 	return c;
 }
 
@@ -259,6 +245,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	BT_DBG("chan %p state %d", chan, chan->state);
 
+	mutex_lock(&chan->conn->chan_lock);
 	lock_sock(sk);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -272,6 +259,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	l2cap_chan_close(chan, reason);
 
 	release_sock(sk);
+	mutex_unlock(&chan->conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	l2cap_chan_put(chan);
@@ -357,7 +345,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
-	list_add_rcu(&chan->list, &conn->chan_l);
+	mutex_lock(&conn->chan_lock);
+	list_add(&chan->list, &conn->chan_l);
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* Delete channel.
@@ -374,8 +364,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 
 	if (conn) {
 		/* Delete from channel list */
-		list_del_rcu(&chan->list);
-		synchronize_rcu();
+		list_del(&chan->list);
 
 		l2cap_chan_put(chan);
 
@@ -426,10 +415,12 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
 	/* Close not yet accepted channels */
 	while ((sk = bt_accept_dequeue(parent, NULL))) {
 		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+
 		__clear_chan_timer(chan);
 		lock_sock(sk);
 		l2cap_chan_close(chan, ECONNRESET);
 		release_sock(sk);
+
 		chan->ops->close(chan->data);
 	}
 }
@@ -743,13 +734,13 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 /* ---- L2CAP connections ---- */
 static void l2cap_conn_start(struct l2cap_conn *conn)
 {
-	struct l2cap_chan *chan;
+	struct l2cap_chan *chan, *tmp;
 
 	BT_DBG("conn %p", conn);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
 		bh_lock_sock(sk);
@@ -829,7 +820,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 		bh_unlock_sock(sk);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* Find socket with cid and source bdaddr.
@@ -941,9 +932,9 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 	if (conn->hcon->out && conn->hcon->type == LE_LINK)
 		smp_conn_security(conn, conn->hcon->pending_sec_level);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+	list_for_each_entry(chan, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
 		bh_lock_sock(sk);
@@ -963,7 +954,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 		bh_unlock_sock(sk);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* Notify sockets that we cannot guaranty reliability anymore */
@@ -973,16 +964,16 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
 
 	BT_DBG("conn %p", conn);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+	list_for_each_entry(chan, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
 		if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
 			sk->sk_err = err;
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 static void l2cap_info_timeout(struct work_struct *work)
@@ -1009,6 +1000,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	kfree_skb(conn->rx_skb);
 
+	mutex_lock(&conn->chan_lock);
+
 	/* Kill channels */
 	list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
 		sk = chan->sk;
@@ -1018,6 +1011,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 		chan->ops->close(chan->data);
 	}
 
+	mutex_unlock(&conn->chan_lock);
+
 	hci_chan_del(conn->hchan);
 
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
@@ -1075,6 +1070,7 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
 	conn->feat_mask = 0;
 
 	spin_lock_init(&conn->lock);
+	mutex_init(&conn->chan_lock);
 
 	INIT_LIST_HEAD(&conn->chan_l);
 
@@ -1815,9 +1811,9 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
 
 	BT_DBG("conn %p", conn);
 
-	rcu_read_lock();
+	mutex_lock(&conn->chan_lock);
 
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
+	list_for_each_entry(chan, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 		if (chan->chan_type != L2CAP_CHAN_RAW)
 			continue;
@@ -1833,7 +1829,7 @@ static void l2cap_raw_recv(struct l2cap_conn *conn, struct sk_buff *skb)
 			kfree_skb(nskb);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 }
 
 /* ---- L2CAP signalling commands ---- */
@@ -2756,6 +2752,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 			return -EFAULT;
 	}
 
+	mutex_lock(&conn->chan_lock);
 	sk = chan->sk;
 
 	switch (result) {
@@ -2783,6 +2780,8 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	}
 
 	release_sock(sk);
+	mutex_unlock(&conn->chan_lock);
+
 	return 0;
 }
 
@@ -3033,6 +3032,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 	if (!chan)
 		return 0;
 
+	mutex_lock(&conn->chan_lock);
 	sk = chan->sk;
 
 	rsp.dcid = cpu_to_le16(chan->scid);
@@ -3043,6 +3043,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 
 	l2cap_chan_del(chan, ECONNRESET);
 	release_sock(sk);
+	mutex_unlock(&conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	return 0;
@@ -3064,6 +3065,7 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 	if (!chan)
 		return 0;
 
+	mutex_lock(&conn->chan_lock);
 	sk = chan->sk;
 
 	l2cap_chan_del(chan, 0);
@@ -4515,11 +4517,9 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 		cancel_delayed_work(&conn->security_timer);
 	}
 
-	rcu_read_lock();
-
-	list_for_each_entry_rcu(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
+	mutex_lock(&conn->chan_lock);
 
+	list_for_each_entry(chan, &conn->chan_l, list) {
 		bh_lock_sock(sk);
 
 		BT_DBG("chan->scid %d", chan->scid);
@@ -4597,7 +4597,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 		bh_unlock_sock(sk);
 	}
 
-	rcu_read_unlock();
+	mutex_unlock(&conn->chan_lock);
 
 	return 0;
 }
-- 
1.7.8.3

[RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

When needed we take explicit lock.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index ae08944..dcf4792 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
 
 	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_scid(conn, cid);
-	if (c)
-		lock_sock(c->sk);
 	mutex_unlock(&conn->chan_lock);
+
 	return c;
 }
 
@@ -128,9 +127,8 @@ static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn
 
 	mutex_lock(&conn->chan_lock);
 	c = __l2cap_get_chan_by_ident(conn, ident);
-	if (c)
-		lock_sock(c->sk);
 	mutex_unlock(&conn->chan_lock);
+
 	return c;
 }
 
-- 
1.7.8.3

[RFCv3 04/16] Bluetooth: Add l2cap_chan_lock

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Channel lock will be used to lock L2CAP channels which are locked
currently by socket locks.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/l2cap.h |   11 +++++++++++
 net/bluetooth/l2cap_core.c    |    2 ++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 42fdbb8..f9fe348 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -497,6 +497,7 @@ struct l2cap_chan {
 
 	void		*data;
 	struct l2cap_ops *ops;
+	struct mutex		lock;
 };
 
 struct l2cap_ops {
@@ -609,6 +610,16 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
 		kfree(c);
 }
 
+static inline void l2cap_chan_lock(struct l2cap_chan *chan)
+{
+	mutex_lock(&chan->lock);
+}
+
+static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
+{
+	mutex_unlock(&chan->lock);
+}
+
 static inline void l2cap_set_timer(struct l2cap_chan *chan,
 					struct delayed_work *work, long timeout)
 {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index dcf4792..f8804ed 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -271,6 +271,8 @@ struct l2cap_chan *l2cap_chan_create(struct sock *sk)
 	if (!chan)
 		return NULL;
 
+	mutex_init(&chan->lock);
+
 	chan->sk = sk;
 
 	write_lock(&chan_list_lock);
-- 
1.7.8.3

[RFCv3 05/16] Bluetooth: Add locked and unlocked state_change

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Split to locked and unlocked versions of l2cap_state_change helping
to remove socket locks from l2cap code.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 net/bluetooth/l2cap_core.c |   41 +++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f8804ed..a23c306 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -225,7 +225,7 @@ static char *state_to_string(int state)
 	return "invalid state";
 }
 
-static void l2cap_state_change(struct l2cap_chan *chan, int state)
+static void __l2cap_state_change(struct l2cap_chan *chan, int state)
 {
 	BT_DBG("%p %s -> %s", chan, state_to_string(chan->state),
 						state_to_string(state));
@@ -234,6 +234,15 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
 	chan->ops->state_change(chan->data, state);
 }
 
+static void l2cap_state_change(struct l2cap_chan *chan, int state)
+{
+	struct sock *sk = chan->sk;
+
+	lock_sock(sk);
+	__l2cap_state_change(chan, state);
+	release_sock(sk);
+}
+
 static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -372,7 +381,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		hci_conn_put(conn->hcon);
 	}
 
-	l2cap_state_change(chan, BT_CLOSED);
+	__l2cap_state_change(chan, BT_CLOSED);
 	sock_set_flag(sk, SOCK_ZAPPED);
 
 	if (err)
@@ -436,7 +445,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 	case BT_LISTEN:
 		l2cap_chan_cleanup_listen(sk);
 
-		l2cap_state_change(chan, BT_CLOSED);
+		__l2cap_state_change(chan, BT_CLOSED);
 		sock_set_flag(sk, SOCK_ZAPPED);
 		break;
 
@@ -727,7 +736,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 	l2cap_send_cmd(conn, l2cap_get_ident(conn),
 			L2CAP_DISCONN_REQ, sizeof(req), &req);
 
-	l2cap_state_change(chan, BT_DISCONN);
+	__l2cap_state_change(chan, BT_DISCONN);
 	sk->sk_err = err;
 }
 
@@ -793,7 +802,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 						parent->sk_data_ready(parent, 0);
 
 				} else {
-					l2cap_state_change(chan, BT_CONFIG);
+					__l2cap_state_change(chan, BT_CONFIG);
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
 					rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
 				}
@@ -896,7 +905,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
-	l2cap_state_change(chan, BT_CONNECTED);
+	__l2cap_state_change(chan, BT_CONNECTED);
 	parent->sk_data_ready(parent, 0);
 
 clean:
@@ -913,7 +922,7 @@ static void l2cap_chan_ready(struct l2cap_chan *chan)
 	chan->conf_state = 0;
 	__clear_chan_timer(chan);
 
-	l2cap_state_change(chan, BT_CONNECTED);
+	__l2cap_state_change(chan, BT_CONNECTED);
 	sk->sk_state_change(sk);
 
 	if (parent)
@@ -945,7 +954,7 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 
 		} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
-			l2cap_state_change(chan, BT_CONNECTED);
+			__l2cap_state_change(chan, BT_CONNECTED);
 			sk->sk_state_change(sk);
 
 		} else if (chan->state == BT_CONNECT)
@@ -1219,14 +1228,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	l2cap_chan_add(conn, chan);
 
-	l2cap_state_change(chan, BT_CONNECT);
+	__l2cap_state_change(chan, BT_CONNECT);
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
 	if (hcon->state == BT_CONNECTED) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
 			if (l2cap_chan_check_security(chan))
-				l2cap_state_change(chan, BT_CONNECTED);
+				__l2cap_state_change(chan, BT_CONNECTED);
 		} else
 			l2cap_do_start(chan);
 	}
@@ -2670,22 +2679,22 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE) {
 		if (l2cap_chan_check_security(chan)) {
 			if (bt_sk(sk)->defer_setup) {
-				l2cap_state_change(chan, BT_CONNECT2);
+				__l2cap_state_change(chan, BT_CONNECT2);
 				result = L2CAP_CR_PEND;
 				status = L2CAP_CS_AUTHOR_PEND;
 				parent->sk_data_ready(parent, 0);
 			} else {
-				l2cap_state_change(chan, BT_CONFIG);
+				__l2cap_state_change(chan, BT_CONFIG);
 				result = L2CAP_CR_SUCCESS;
 				status = L2CAP_CS_NO_INFO;
 			}
 		} else {
-			l2cap_state_change(chan, BT_CONNECT2);
+			__l2cap_state_change(chan, BT_CONNECT2);
 			result = L2CAP_CR_PEND;
 			status = L2CAP_CS_AUTHEN_PEND;
 		}
 	} else {
-		l2cap_state_change(chan, BT_CONNECT2);
+		__l2cap_state_change(chan, BT_CONNECT2);
 		result = L2CAP_CR_PEND;
 		status = L2CAP_CS_NO_INFO;
 	}
@@ -4574,12 +4583,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 					if (parent)
 						parent->sk_data_ready(parent, 0);
 				} else {
-					l2cap_state_change(chan, BT_CONFIG);
+					__l2cap_state_change(chan, BT_CONFIG);
 					res = L2CAP_CR_SUCCESS;
 					stat = L2CAP_CS_NO_INFO;
 				}
 			} else {
-				l2cap_state_change(chan, BT_DISCONN);
+				__l2cap_state_change(chan, BT_DISCONN);
 				__set_chan_timer(chan,
 					msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
 				res = L2CAP_CR_SEC_BLOCK;
-- 
1.7.8.3

[RFCv3 06/16] Bluetooth: Add socket error function

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Use locked and unlocked versions to help removing socket
locks from l2cap.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index a23c306..e6515b3 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -243,6 +243,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
 	release_sock(sk);
 }
 
+static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
+{
+	struct sock *sk = chan->sk;
+
+	sk->sk_err = err;
+}
+
+static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
+{
+	struct sock *sk = chan->sk;
+
+	lock_sock(sk);
+	__l2cap_set_sock_err(chan, err);
+	release_sock(sk);
+}
+
 static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
@@ -385,7 +401,7 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	sock_set_flag(sk, SOCK_ZAPPED);
 
 	if (err)
-		sk->sk_err = err;
+		__l2cap_set_sock_err(chan, err);
 
 	if (parent) {
 		bt_accept_unlink(sk);
@@ -717,14 +733,11 @@ static inline int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
 
 static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *chan, int err)
 {
-	struct sock *sk;
 	struct l2cap_disconn_req req;
 
 	if (!conn)
 		return;
 
-	sk = chan->sk;
-
 	if (chan->mode == L2CAP_MODE_ERTM) {
 		__clear_retrans_timer(chan);
 		__clear_monitor_timer(chan);
@@ -737,7 +750,7 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 			L2CAP_DISCONN_REQ, sizeof(req), &req);
 
 	__l2cap_state_change(chan, BT_DISCONN);
-	sk->sk_err = err;
+	__l2cap_set_sock_err(chan, err);
 }
 
 /* ---- L2CAP connections ---- */
@@ -976,10 +989,8 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
 	mutex_lock(&conn->chan_lock);
 
 	list_for_each_entry(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
-
 		if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
-			sk->sk_err = err;
+			l2cap_set_sock_err(chan, err);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -2994,7 +3005,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		sk->sk_err = ECONNRESET;
+		l2cap_set_sock_err(chan, ECONNRESET);
+
 		__set_chan_timer(chan,
 				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
 		l2cap_send_disconn_req(conn, chan, ECONNRESET);
-- 
1.7.8.3

[RFCv3 07/16] Bluetooth: Add unlocked __l2cap_chan_add function

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e6515b3..21f9f5d 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -324,7 +324,7 @@ void l2cap_chan_destroy(struct l2cap_chan *chan)
 	l2cap_chan_put(chan);
 }
 
-static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 {
 	BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
 			chan->psm, chan->dcid);
@@ -370,8 +370,13 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 	l2cap_chan_hold(chan);
 
-	mutex_lock(&conn->chan_lock);
 	list_add(&chan->list, &conn->chan_l);
+}
+
+void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
+{
+	mutex_lock(&conn->chan_lock);
+	__l2cap_chan_add(conn, chan);
 	mutex_unlock(&conn->chan_lock);
 }
 
-- 
1.7.8.3

[RFCv3 08/16] Bluetooth: Use chan lock in timers

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change locking in delayed works to use chan locks instead of sk lock.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 21f9f5d..bcc1c1c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -263,13 +263,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							chan_timer.work);
-	struct sock *sk = chan->sk;
 	int reason;
 
 	BT_DBG("chan %p state %d", chan, chan->state);
 
 	mutex_lock(&chan->conn->chan_lock);
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
 		reason = ECONNREFUSED;
@@ -281,7 +280,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	l2cap_chan_close(chan, reason);
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	mutex_unlock(&chan->conn->chan_lock);
 
 	chan->ops->close(chan->data);
@@ -1300,40 +1299,39 @@ static void l2cap_monitor_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							monitor_timer.work);
-	struct sock *sk = chan->sk;
-
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	if (chan->retry_count >= chan->remote_max_tx) {
 		l2cap_send_disconn_req(chan->conn, chan, ECONNABORTED);
-		release_sock(sk);
-		return;
+		goto done;
 	}
 
 	chan->retry_count++;
 	__set_monitor_timer(chan);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+done:
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_retrans_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							retrans_timer.work);
-	struct sock *sk = chan->sk;
-
 	BT_DBG("chan %p", chan);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
+
 	chan->retry_count = 1;
 	__set_monitor_timer(chan);
 
 	set_bit(CONN_WAIT_F, &chan->conn_state);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 }
 
 static void l2cap_drop_acked_frames(struct l2cap_chan *chan)
@@ -2022,9 +2020,11 @@ static void l2cap_ack_timeout(struct work_struct *work)
 
 	BT_DBG("chan %p", chan);
 
-	lock_sock(chan->sk);
+	l2cap_chan_lock(chan);
+
 	__l2cap_send_ack(chan);
-	release_sock(chan->sk);
+
+	l2cap_chan_unlock(chan);
 
 	l2cap_chan_put(chan);
 }
-- 
1.7.8.3

[RFCv3 09/16] Bluetooth: Use chan lock in L2CAP sig commands

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Convert sk lock to chan lock for L2CAP signalling commands.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   76 +++++++++++++++++++++++++++++++-------------
 1 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index bcc1c1c..eb04405 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -737,6 +737,7 @@ static inline int l2cap_mode_supported(__u8 mode, __u32 feat_mask)
 
 static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *chan, int err)
 {
+	struct sock *sk = chan->sk;
 	struct l2cap_disconn_req req;
 
 	if (!conn)
@@ -753,8 +754,10 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn, struct l2cap_chan *c
 	l2cap_send_cmd(conn, l2cap_get_ident(conn),
 			L2CAP_DISCONN_REQ, sizeof(req), &req);
 
+	lock_sock(sk);
 	__l2cap_state_change(chan, BT_DISCONN);
 	__l2cap_set_sock_err(chan, err);
+	release_sock(sk);
 }
 
 /* ---- L2CAP connections ---- */
@@ -2644,6 +2647,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	parent = pchan->sk;
 
+	mutex_lock(&conn->chan_lock);
 	lock_sock(parent);
 
 	/* Check if the ACL is secure enough (if not SDP) */
@@ -2684,7 +2688,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	bt_accept_enqueue(parent, sk);
 
-	l2cap_chan_add(conn, chan);
+	__l2cap_chan_add(conn, chan);
 
 	dcid = chan->scid;
 
@@ -2717,6 +2721,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 response:
 	release_sock(parent);
+	mutex_unlock(&conn->chan_lock);
 
 sendresp:
 	rsp.scid   = cpu_to_le16(scid);
@@ -2758,6 +2763,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	struct l2cap_chan *chan;
 	struct sock *sk;
 	u8 req[128];
+	int err;
 
 	scid   = __le16_to_cpu(rsp->scid);
 	dcid   = __le16_to_cpu(rsp->dcid);
@@ -2767,17 +2773,26 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	BT_DBG("dcid 0x%4.4x scid 0x%4.4x result 0x%2.2x status 0x%2.2x",
 						dcid, scid, result, status);
 
+	mutex_lock(&conn->chan_lock);
+
 	if (scid) {
-		chan = l2cap_get_chan_by_scid(conn, scid);
-		if (!chan)
-			return -EFAULT;
+		chan = __l2cap_get_chan_by_scid(conn, scid);
+		if (!chan) {
+			err = -EFAULT;
+			goto unlock;
+		}
 	} else {
-		chan = l2cap_get_chan_by_ident(conn, cmd->ident);
-		if (!chan)
-			return -EFAULT;
+		chan = __l2cap_get_chan_by_ident(conn, cmd->ident);
+		if (!chan) {
+			err = -EFAULT;
+			goto unlock;
+		}
 	}
 
-	mutex_lock(&conn->chan_lock);
+	err = 0;
+
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	switch (result) {
@@ -2804,10 +2819,11 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		break;
 	}
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
+unlock:
 	mutex_unlock(&conn->chan_lock);
 
-	return 0;
+	return err;
 }
 
 static inline void set_default_fcs(struct l2cap_chan *chan)
@@ -2839,6 +2855,8 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return -ENOENT;
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
@@ -2928,7 +2946,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 unlock:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -2951,6 +2969,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return 0;
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
 
 	switch (result) {
@@ -3037,7 +3057,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	}
 
 done:
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 	return 0;
 }
 
@@ -3054,21 +3074,29 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 
 	BT_DBG("scid 0x%4.4x dcid 0x%4.4x", scid, dcid);
 
-	chan = l2cap_get_chan_by_scid(conn, dcid);
-	if (!chan)
+	mutex_lock(&conn->chan_lock);
+
+	chan = __l2cap_get_chan_by_scid(conn, dcid);
+	if (!chan) {
+		mutex_unlock(&conn->chan_lock);
 		return 0;
+	}
+
+	l2cap_chan_lock(chan);
 
-	mutex_lock(&conn->chan_lock);
 	sk = chan->sk;
 
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_DISCONN_RSP, sizeof(rsp), &rsp);
 
+	lock_sock(sk);
 	sk->sk_shutdown = SHUTDOWN_MASK;
+	release_sock(sk);
 
 	l2cap_chan_del(chan, ECONNRESET);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
 	mutex_unlock(&conn->chan_lock);
 
 	chan->ops->close(chan->data);
@@ -3080,22 +3108,26 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 	struct l2cap_disconn_rsp *rsp = (struct l2cap_disconn_rsp *) data;
 	u16 dcid, scid;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 
 	scid = __le16_to_cpu(rsp->scid);
 	dcid = __le16_to_cpu(rsp->dcid);
 
 	BT_DBG("dcid 0x%4.4x scid 0x%4.4x", dcid, scid);
 
-	chan = l2cap_get_chan_by_scid(conn, scid);
-	if (!chan)
+	mutex_lock(&conn->chan_lock);
+
+	chan = __l2cap_get_chan_by_scid(conn, scid);
+	if (!chan) {
+		mutex_unlock(&conn->chan_lock);
 		return 0;
+	}
 
-	mutex_lock(&conn->chan_lock);
-	sk = chan->sk;
+	l2cap_chan_lock(chan);
 
 	l2cap_chan_del(chan, 0);
-	release_sock(sk);
+
+	l2cap_chan_unlock(chan);
+	mutex_unlock(&conn->chan_lock);
 
 	chan->ops->close(chan->data);
 	return 0;
-- 
1.7.8.3

[RFCv3 10/16] Bluetooth: Use chan lock in chan delete functions

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change sk lock to chan lock in functions deleting L2CAP
channels.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   21 +++++++++++++++------
 net/bluetooth/l2cap_sock.c |    2 ++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index eb04405..2a41e81 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -401,6 +401,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 		hci_conn_put(conn->hcon);
 	}
 
+	lock_sock(sk);
+
 	__l2cap_state_change(chan, BT_CLOSED);
 	sock_set_flag(sk, SOCK_ZAPPED);
 
@@ -413,6 +415,8 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	} else
 		sk->sk_state_change(sk);
 
+	release_sock(sk);
+
 	if (!(test_bit(CONF_OUTPUT_DONE, &chan->conf_state) &&
 			test_bit(CONF_INPUT_DONE, &chan->conf_state)))
 		return;
@@ -445,10 +449,10 @@ static void l2cap_chan_cleanup_listen(struct sock *parent)
 	while ((sk = bt_accept_dequeue(parent, NULL))) {
 		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
 
+		l2cap_chan_lock(chan);
 		__clear_chan_timer(chan);
-		lock_sock(sk);
 		l2cap_chan_close(chan, ECONNRESET);
-		release_sock(sk);
+		l2cap_chan_unlock(chan);
 
 		chan->ops->close(chan->data);
 	}
@@ -463,10 +467,12 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 
 	switch (chan->state) {
 	case BT_LISTEN:
+		lock_sock(sk);
 		l2cap_chan_cleanup_listen(sk);
 
 		__l2cap_state_change(chan, BT_CLOSED);
 		sock_set_flag(sk, SOCK_ZAPPED);
+		release_sock(sk);
 		break;
 
 	case BT_CONNECTED:
@@ -509,7 +515,9 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 		break;
 
 	default:
+		lock_sock(sk);
 		sock_set_flag(sk, SOCK_ZAPPED);
+		release_sock(sk);
 		break;
 	}
 }
@@ -1018,7 +1026,6 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
 	struct l2cap_chan *chan, *l;
-	struct sock *sk;
 
 	if (!conn)
 		return;
@@ -1031,10 +1038,12 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
 
 	/* Kill channels */
 	list_for_each_entry_safe(chan, l, &conn->chan_l, list) {
-		sk = chan->sk;
-		lock_sock(sk);
+		l2cap_chan_lock(chan);
+
 		l2cap_chan_del(chan, err);
-		release_sock(sk);
+
+		l2cap_chan_unlock(chan);
+
 		chan->ops->close(chan->data);
 	}
 
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 1636029..85e4fd7 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -809,7 +809,9 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 			err = __l2cap_wait_ack(sk);
 
 		sk->sk_shutdown = SHUTDOWN_MASK;
+		release_sock(sk);
 		l2cap_chan_close(chan, 0);
+		lock_sock(sk);
 
 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
 			err = bt_sock_wait_state(sk, BT_CLOSED,
-- 
1.7.8.3

[RFCv3 11/16] Bluetooth: Use chan lock in L2CAP conn start

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change sk lock to chan lock in l2cap_conn_start. bh_locks were used
because of being RCU critical section.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2a41e81..419e2a6 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -780,10 +780,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 	list_for_each_entry_safe(chan, tmp, &conn->chan_l, list) {
 		struct sock *sk = chan->sk;
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -792,17 +792,15 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (!l2cap_chan_check_security(chan) ||
 					!__l2cap_no_conn_pending(chan)) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
 			if (!l2cap_mode_supported(chan->mode, conn->feat_mask)
 					&& test_bit(CONF_STATE2_DEVICE,
 					&chan->conf_state)) {
-				/* l2cap_chan_close() calls list_del(chan)
-				 * so release the lock */
 				l2cap_chan_close(chan, ECONNRESET);
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -822,6 +820,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			rsp.dcid = cpu_to_le16(chan->scid);
 
 			if (l2cap_chan_check_security(chan)) {
+				lock_sock(sk);
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
@@ -834,6 +833,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
 					rsp.status = cpu_to_le16(L2CAP_CS_NO_INFO);
 				}
+				release_sock(sk);
 			} else {
 				rsp.result = cpu_to_le16(L2CAP_CR_PEND);
 				rsp.status = cpu_to_le16(L2CAP_CS_AUTHEN_PEND);
@@ -844,7 +844,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (test_bit(CONF_REQ_SENT, &chan->conf_state) ||
 					rsp.result != L2CAP_CR_SUCCESS) {
-				bh_unlock_sock(sk);
+				l2cap_chan_unlock(chan);
 				continue;
 			}
 
@@ -854,7 +854,7 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 			chan->num_conf_req++;
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
-- 
1.7.8.3

[RFCv3 12/16] Bluetooth: Use chan lock in receiving data

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change locking logic when receiving ACL and L2CAP data.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |    7 +++----
 net/bluetooth/l2cap_sock.c |   11 +++++++++--
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 419e2a6..e03c5df 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4285,7 +4285,6 @@ drop:
 static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk_buff *skb)
 {
 	struct l2cap_chan *chan;
-	struct sock *sk = NULL;
 	u32 control;
 	u16 tx_seq;
 	int len;
@@ -4296,7 +4295,7 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
 		goto drop;
 	}
 
-	sk = chan->sk;
+	l2cap_chan_lock(chan);
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
@@ -4367,8 +4366,7 @@ drop:
 	kfree_skb(skb);
 
 done:
-	if (sk)
-		release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	return 0;
 }
@@ -4725,6 +4723,7 @@ int l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 
 		if (chan && chan->sk) {
 			struct sock *sk = chan->sk;
+			lock_sock(sk);
 
 			if (chan->imtu < len - L2CAP_HDR_SIZE) {
 				BT_ERR("Frame exceeding recv MTU (len %d, "
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 85e4fd7..dea5418 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -864,8 +864,12 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 	struct sock *sk = data;
 	struct l2cap_pinfo *pi = l2cap_pi(sk);
 
-	if (pi->rx_busy_skb)
-		return -ENOMEM;
+	lock_sock(sk);
+
+	if (pi->rx_busy_skb) {
+		err = -ENOMEM;
+		goto done;
+	}
 
 	err = sock_queue_rcv_skb(sk, skb);
 
@@ -884,6 +888,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 		err = 0;
 	}
 
+done:
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.8.3

[RFCv3 13/16] Bluetooth: Change locking logic for conn/chan ready

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

In l2cap_conn_ready and l2cap_chan_ready change locking logic and
use explicit socket when needed.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e03c5df..284c5c9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -943,7 +943,11 @@ clean:
 static void l2cap_chan_ready(struct l2cap_chan *chan)
 {
 	struct sock *sk = chan->sk;
-	struct sock *parent = bt_sk(sk)->parent;
+	struct sock *parent;
+
+	lock_sock(sk);
+
+	parent = bt_sk(sk)->parent;
 
 	BT_DBG("sk %p, parent %p", sk, parent);
 
@@ -955,6 +959,8 @@ static void l2cap_chan_ready(struct l2cap_chan *chan)
 
 	if (parent)
 		parent->sk_data_ready(parent, 0);
+
+	release_sock(sk);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -972,23 +978,25 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 	mutex_lock(&conn->chan_lock);
 
 	list_for_each_entry(chan, &conn->chan_l, list) {
-		struct sock *sk = chan->sk;
 
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		if (conn->hcon->type == LE_LINK) {
 			if (smp_conn_security(conn, chan->sec_level))
 				l2cap_chan_ready(chan);
 
 		} else if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
+			struct sock *sk = chan->sk;
 			__clear_chan_timer(chan);
+			lock_sock(sk);
 			__l2cap_state_change(chan, BT_CONNECTED);
 			sk->sk_state_change(sk);
+			release_sock(sk);
 
 		} else if (chan->state == BT_CONNECT)
 			l2cap_do_start(chan);
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
-- 
1.7.8.3

[RFCv3 14/16] Bluetooth: Change locking logic in security_cfm

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Change bh_ locking functions to mutex_locks since we can now sleep.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   15 ++++++++++-----
 1 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 284c5c9..2b97cf7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4593,7 +4593,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 	mutex_lock(&conn->chan_lock);
 
 	list_for_each_entry(chan, &conn->chan_l, list) {
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		BT_DBG("chan->scid %d", chan->scid);
 
@@ -4603,19 +4603,19 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				l2cap_chan_ready(chan);
 			}
 
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (test_bit(CONF_CONNECT_PEND, &chan->conf_state)) {
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
 		if (!status && (chan->state == BT_CONNECTED ||
 						chan->state == BT_CONFIG)) {
 			l2cap_check_encryption(chan, encrypt);
-			bh_unlock_sock(sk);
+			l2cap_chan_unlock(chan);
 			continue;
 		}
 
@@ -4636,9 +4636,12 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 					msecs_to_jiffies(L2CAP_DISC_TIMEOUT));
 			}
 		} else if (chan->state == BT_CONNECT2) {
+			struct sock *sk = chan->sk;
 			struct l2cap_conn_rsp rsp;
 			__u16 res, stat;
 
+			lock_sock(sk);
+
 			if (!status) {
 				if (bt_sk(sk)->defer_setup) {
 					struct sock *parent = bt_sk(sk)->parent;
@@ -4659,6 +4662,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 				stat = L2CAP_CS_NO_INFO;
 			}
 
+			release_sock(sk);
+
 			rsp.scid   = cpu_to_le16(chan->dcid);
 			rsp.dcid   = cpu_to_le16(chan->scid);
 			rsp.result = cpu_to_le16(res);
@@ -4667,7 +4672,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 							sizeof(rsp), &rsp);
 		}
 
-		bh_unlock_sock(sk);
+		l2cap_chan_unlock(chan);
 	}
 
 	mutex_unlock(&conn->chan_lock);
-- 
1.7.8.3

[RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_core.c |   24 ++++++++++++++++++------
 net/bluetooth/l2cap_sock.c |    2 ++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2b97cf7..bb390ec 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1182,7 +1182,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	hci_dev_lock(hdev);
 
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	/* PSM must be odd and lsb of upper byte must be 0 */
 	if ((__le16_to_cpu(psm) & 0x0101) != 0x0001 && !cid &&
@@ -1209,18 +1209,20 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 		goto done;
 	}
 
+	lock_sock(sk);
+
 	switch (sk->sk_state) {
 	case BT_CONNECT:
 	case BT_CONNECT2:
 	case BT_CONFIG:
 		/* Already connecting */
 		err = 0;
-		goto done;
+		goto sock_release;
 
 	case BT_CONNECTED:
 		/* Already connected */
 		err = -EISCONN;
-		goto done;
+		goto sock_release;
 
 	case BT_OPEN:
 	case BT_BOUND:
@@ -1229,11 +1231,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	default:
 		err = -EBADFD;
-		goto done;
+		goto sock_release;
 	}
 
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, dst);
+
+	release_sock(sk);
+
 	chan->psm = psm;
 	chan->dcid = cid;
 
@@ -1261,23 +1266,30 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 	/* Update source addr of the socket */
 	bacpy(src, conn->src);
 
+	l2cap_chan_unlock(chan);
 	l2cap_chan_add(conn, chan);
+	l2cap_chan_lock(chan);
 
-	__l2cap_state_change(chan, BT_CONNECT);
+	l2cap_state_change(chan, BT_CONNECT);
 	__set_chan_timer(chan, sk->sk_sndtimeo);
 
 	if (hcon->state == BT_CONNECTED) {
 		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
 			__clear_chan_timer(chan);
 			if (l2cap_chan_check_security(chan))
-				__l2cap_state_change(chan, BT_CONNECTED);
+				l2cap_state_change(chan, BT_CONNECTED);
 		} else
 			l2cap_do_start(chan);
 	}
 
 	err = 0;
+	goto done;
+
+sock_release:
+	release_sock(sk);
 
 done:
+	l2cap_chan_unlock(chan);
 	hci_dev_unlock(hdev);
 	hci_dev_put(hdev);
 	return err;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index dea5418..ddac4cb 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -127,6 +127,8 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 	if (err)
 		goto done;
 
+	lock_sock(sk);
+
 	err = bt_sock_wait_state(sk, BT_CONNECTED,
 			sock_sndtimeo(sk, flags & O_NONBLOCK));
 done:
-- 
1.7.8.3

[RFCv3 16/16] Bluetooth: Remove socket lock check

[Emeltchenko Andrei]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Simplify code so that we do not need to check whether socket is locked.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/l2cap_sock.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ddac4cb..358975a 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -125,15 +125,15 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
 
 	err = l2cap_chan_connect(chan, la.l2_psm, la.l2_cid, &la.l2_bdaddr);
 	if (err)
-		goto done;
+		return err;
 
 	lock_sock(sk);
 
 	err = bt_sock_wait_state(sk, BT_CONNECTED,
 			sock_sndtimeo(sk, flags & O_NONBLOCK));
-done:
-	if (sock_owned_by_user(sk))
-		release_sock(sk);
+
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.8.3

Re: [RFCv3 01/16] Bluetooth: trivial: Fix long line

[Marcel Holtmann]

Hi Andrei,

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Marcel Holtmann]

Hi Andrei,

> Usage of RCU list looks not reasonalbe for a number of reasons:
> our code sleep and we have to use socket spinlocks, some parts
> of code are updaters thus we need to use mutexes anyway.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |  108 ++++++++++++++++++++++----------------------
>  1 files changed, 54 insertions(+), 54 deletions(-)

I am fine with this, but I wanna get an ack from Ulisses as well.

Regards

Marcel

Re: [RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Marcel Holtmann]

Hi Andrei,

> When needed we take explicit lock.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ae08944..dcf4792 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
>  
>  	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_scid(conn, cid);
> -	if (c)
> -		lock_sock(c->sk);
>  	mutex_unlock(&conn->chan_lock);
> +
>  	return c;
>  }

so this is dangerous since now we are creating an imbalance with the
socket locks. Are we fixing that one later on?

Regards

Marcel

Re: [RFCv3 06/16] Bluetooth: Add socket error function

[Marcel Holtmann]

Hi Andrei,

> Use locked and unlocked versions to help removing socket
> locks from l2cap.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index a23c306..e6515b3 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -243,6 +243,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
>  	release_sock(sk);
>  }
>  
> +static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> +{
> +	struct sock *sk = chan->sk;
> +
> +	sk->sk_err = err;
> +}
> +
> +static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> +{
> +	struct sock *sk = chan->sk;
> +
> +	lock_sock(sk);
> +	__l2cap_set_sock_err(chan, err);
> +	release_sock(sk);
> +}

I think we better call this l2cap_chan_set_sock_err(). Sounds kinda
long, but makes it a bit more clearer what it does.

Regards

Marcel

Re: [RFCv3 07/16] Bluetooth: Add unlocked __l2cap_chan_add function

[Marcel Holtmann]

Hi Andrei,

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 08/16] Bluetooth: Use chan lock in timers

[Marcel Holtmann]

Hi Andrei,

> Change locking in delayed works to use chan locks instead of sk lock.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   30 +++++++++++++++---------------
>  1 files changed, 15 insertions(+), 15 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 09/16] Bluetooth: Use chan lock in L2CAP sig commands

[Marcel Holtmann]

Hi Andrei,

> Convert sk lock to chan lock for L2CAP signalling commands.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   76 +++++++++++++++++++++++++++++++-------------
>  1 files changed, 54 insertions(+), 22 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 10/16] Bluetooth: Use chan lock in chan delete functions

[Marcel Holtmann]

Hi Andrei,

> Change sk lock to chan lock in functions deleting L2CAP
> channels.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   21 +++++++++++++++------
>  net/bluetooth/l2cap_sock.c |    2 ++
>  2 files changed, 17 insertions(+), 6 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 11/16] Bluetooth: Use chan lock in L2CAP conn start

[Marcel Holtmann]

Hi Andrei,

> Change sk lock to chan lock in l2cap_conn_start. bh_locks were used
> because of being RCU critical section.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 12/16] Bluetooth: Use chan lock in receiving data

[Marcel Holtmann]

Hi Andrei,

> Change locking logic when receiving ACL and L2CAP data.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    7 +++----
>  net/bluetooth/l2cap_sock.c |   11 +++++++++--
>  2 files changed, 12 insertions(+), 6 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 13/16] Bluetooth: Change locking logic for conn/chan ready

[Marcel Holtmann]

Hi Andrei,

> In l2cap_conn_ready and l2cap_chan_ready change locking logic and
> use explicit socket when needed.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 14/16] Bluetooth: Change locking logic in security_cfm

[Marcel Holtmann]

Hi Andrei,

> Change bh_ locking functions to mutex_locks since we can now sleep.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   15 ++++++++++-----
>  1 files changed, 10 insertions(+), 5 deletions(-)

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Emeltchenko Andrei]

Hi Marcel,

On Thu, Feb 09, 2012 at 03:28:45PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > When needed we take explicit lock.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |    6 ++----
> >  1 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index ae08944..dcf4792 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
> >  
> >  	mutex_lock(&conn->chan_lock);
> >  	c = __l2cap_get_chan_by_scid(conn, cid);
> > -	if (c)
> > -		lock_sock(c->sk);
> >  	mutex_unlock(&conn->chan_lock);
> > +
> >  	return c;
> >  }
> 
> so this is dangerous since now we are creating an imbalance with the
> socket locks. Are we fixing that one later on?

Yes, that will be fixed later on. Do you think that needs to be merged
with the other code?

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Marcel Holtmann]

Hi Andrei,

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   24 ++++++++++++++++++------
>  net/bluetooth/l2cap_sock.c |    2 ++
>  2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 2b97cf7..bb390ec 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1182,7 +1182,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>  
>  	hci_dev_lock(hdev);
>  
> -	lock_sock(sk);
> +	l2cap_chan_lock(chan);
>  
>  	/* PSM must be odd and lsb of upper byte must be 0 */
>  	if ((__le16_to_cpu(psm) & 0x0101) != 0x0001 && !cid &&
> @@ -1209,18 +1209,20 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>  		goto done;
>  	}
>  
> +	lock_sock(sk);
> +
>  	switch (sk->sk_state) {
>  	case BT_CONNECT:
>  	case BT_CONNECT2:
>  	case BT_CONFIG:
>  		/* Already connecting */
>  		err = 0;
> -		goto done;
> +		goto sock_release;
>  
>  	case BT_CONNECTED:
>  		/* Already connected */
>  		err = -EISCONN;
> -		goto done;
> +		goto sock_release;
>  
>  	case BT_OPEN:
>  	case BT_BOUND:
> @@ -1229,11 +1231,14 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>  
>  	default:
>  		err = -EBADFD;
> -		goto done;
> +		goto sock_release;
>  	}
>  
>  	/* Set destination address and psm */
>  	bacpy(&bt_sk(sk)->dst, dst);
> +
> +	release_sock(sk);
> +
>  	chan->psm = psm;
>  	chan->dcid = cid;
>  
> @@ -1261,23 +1266,30 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>  	/* Update source addr of the socket */
>  	bacpy(src, conn->src);
>  
> +	l2cap_chan_unlock(chan);
>  	l2cap_chan_add(conn, chan);
> +	l2cap_chan_lock(chan);
>  
> -	__l2cap_state_change(chan, BT_CONNECT);
> +	l2cap_state_change(chan, BT_CONNECT);
>  	__set_chan_timer(chan, sk->sk_sndtimeo);
>  
>  	if (hcon->state == BT_CONNECTED) {
>  		if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>  			__clear_chan_timer(chan);
>  			if (l2cap_chan_check_security(chan))
> -				__l2cap_state_change(chan, BT_CONNECTED);
> +				l2cap_state_change(chan, BT_CONNECTED);
>  		} else
>  			l2cap_do_start(chan);
>  	}
>  
>  	err = 0;
> +	goto done;
> +
> +sock_release:
> +	release_sock(sk);

I don't like this jumping. Just use release_sock(sk) directly in the
error path above.

And I think we need to simplify this function a little bit and maybe
split it into more logical pieces.

Regards

Marcel

Re: [RFCv3 16/16] Bluetooth: Remove socket lock check

[Marcel Holtmann]

Hi Andrei,

> Simplify code so that we do not need to check whether socket is locked.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_sock.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index ddac4cb..358975a 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -125,15 +125,15 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
>  
>  	err = l2cap_chan_connect(chan, la.l2_psm, la.l2_cid, &la.l2_bdaddr);
>  	if (err)
> -		goto done;
> +		return err;
>  
>  	lock_sock(sk);
>  
>  	err = bt_sock_wait_state(sk, BT_CONNECTED,
>  			sock_sndtimeo(sk, flags & O_NONBLOCK));
> -done:
> -	if (sock_owned_by_user(sk))
> -		release_sock(sk);
> +
> +	release_sock(sk);
> +

is this code change really equivalent?

Regards

Marcel

Re: [RFCv3 16/16] Bluetooth: Remove socket lock check

[Emeltchenko Andrei]

Hi Marcel,

On Thu, Feb 09, 2012 at 03:43:40PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
> 
> > Simplify code so that we do not need to check whether socket is locked.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_sock.c |    8 ++++----
> >  1 files changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index ddac4cb..358975a 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -125,15 +125,15 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
> >  
> >  	err = l2cap_chan_connect(chan, la.l2_psm, la.l2_cid, &la.l2_bdaddr);
> >  	if (err)
> > -		goto done;
> > +		return err;
> >  
> >  	lock_sock(sk);
> >  
> >  	err = bt_sock_wait_state(sk, BT_CONNECTED,
> >  			sock_sndtimeo(sk, flags & O_NONBLOCK));
> > -done:
> > -	if (sock_owned_by_user(sk))
> > -		release_sock(sk);
> > +
> > +	release_sock(sk);
> > +
> 
> is this code change really equivalent?

I think so, we used to have possibility to get sk locked
in l2cap_chan_connect but now it is not locked so we explicitly lock and
unlock.

I do not like that check sock_owned_by_user, this means we do not know is
socket locked or not.

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Ulisses Furquim]

Hi Andrei,

On Thu, Feb 9, 2012 at 12:27 PM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andrei,
>
>> Usage of RCU list looks not reasonalbe for a number of reasons:
>> our code sleep and we have to use socket spinlocks, some parts
>> of code are updaters thus we need to use mutexes anyway.
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> ---
>> =A0net/bluetooth/l2cap_core.c | =A0108 ++++++++++++++++++++++-----------=
-----------
>> =A01 files changed, 54 insertions(+), 54 deletions(-)
>
> I am fine with this, but I wanna get an ack from Ulisses as well.

Please, refer to my previous review because it seems you haven't
changed anything at all. For instance, l2cap_disconnect_rsp() is still
missing an unlock of chan_lock and I see no changes to l2cap_sock.c
which are needed. Moreover, you need to use the unlocked versions of
the chan lookup functions and check if we still need the locked ones.
If I wasn't clear in some points please just ask in that thread.

Regards,

--=20
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 07/16] Bluetooth: Add unlocked __l2cap_chan_add function

[Ulisses Furquim]

Hi Andrei,

On Thu, Feb 9, 2012 at 12:17 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index e6515b3..21f9f5d 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -324,7 +324,7 @@ void l2cap_chan_destroy(struct l2cap_chan *chan)
>        l2cap_chan_put(chan);
>  }
>
> -static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> +void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>  {
>        BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
>                        chan->psm, chan->dcid);
> @@ -370,8 +370,13 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>
>        l2cap_chan_hold(chan);
>
> -       mutex_lock(&conn->chan_lock);
>        list_add(&chan->list, &conn->chan_l);
> +}
> +
> +void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> +{
> +       mutex_lock(&conn->chan_lock);
> +       __l2cap_chan_add(conn, chan);
>        mutex_unlock(&conn->chan_lock);
>  }
>
> --
> 1.7.8.3

As far as I see we don't need the locked version of l2cap_chan_add().
What I said in my previous review was to remove the lock and unlock
around the list_add() and add the prefix __ to l2cap_chan_add() then
use it like that. You'll see that all places where you have
l2cap_chan_add() you have already grabbed chan_lock. Ok? Adding the
locked version and not using it afterwards makes no sense at all.
Another thing is that this change to l2cap_chan_add() really belongs
to patch number 2 in your series.

Best regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-09 16:40:39 +0200]:

> Hi Marcel,
> 
> On Thu, Feb 09, 2012 at 03:28:45PM +0100, Marcel Holtmann wrote:
> > Hi Andrei,
> > 
> > > When needed we take explicit lock.
> > > 
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  net/bluetooth/l2cap_core.c |    6 ++----
> > >  1 files changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > > index ae08944..dcf4792 100644
> > > --- a/net/bluetooth/l2cap_core.c
> > > +++ b/net/bluetooth/l2cap_core.c
> > > @@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
> > >  
> > >  	mutex_lock(&conn->chan_lock);
> > >  	c = __l2cap_get_chan_by_scid(conn, cid);
> > > -	if (c)
> > > -		lock_sock(c->sk);
> > >  	mutex_unlock(&conn->chan_lock);
> > > +
> > >  	return c;
> > >  }
> > 
> > so this is dangerous since now we are creating an imbalance with the
> > socket locks. Are we fixing that one later on?
> 
> Yes, that will be fixed later on. Do you think that needs to be merged
> with the other code?

Please merge such changes, I don't merge a code that put us in a bad state.

	Gustavo

Re: [RFCv3 06/16] Bluetooth: Add socket error function

[Gustavo Padovan]

Hi Marcel,

* Marcel Holtmann <marcel@holtmann.org> [2012-02-09 15:30:59 +0100]:

> Hi Andrei,
> 
> > Use locked and unlocked versions to help removing socket
> > locks from l2cap.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |   30 +++++++++++++++++++++---------
> >  1 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index a23c306..e6515b3 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -243,6 +243,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
> >  	release_sock(sk);
> >  }
> >  
> > +static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > +{
> > +	struct sock *sk = chan->sk;
> > +
> > +	sk->sk_err = err;
> > +}
> > +
> > +static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > +{
> > +	struct sock *sk = chan->sk;
> > +
> > +	lock_sock(sk);
> > +	__l2cap_set_sock_err(chan, err);
> > +	release_sock(sk);
> > +}
> 
> I think we better call this l2cap_chan_set_sock_err(). Sounds kinda
> long, but makes it a bit more clearer what it does.

Or l2cap_chan_set_err() since this will be made generic once the separation
between core and sock is done.

	Gustavo

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Ulisses Furquim]

Hi Andrei,

On Thu, Feb 9, 2012 at 12:17 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> @@ -1261,23 +1266,30 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>        /* Update source addr of the socket */
>        bacpy(src, conn->src);
>
> +       l2cap_chan_unlock(chan);
>        l2cap_chan_add(conn, chan);
> +       l2cap_chan_lock(chan);

Hum, do we really need to do this? Maybe l2cap_chan_add() can receive
chan already locked?

> -       __l2cap_state_change(chan, BT_CONNECT);
> +       l2cap_state_change(chan, BT_CONNECT);

Why? Is there a problem moving the release_sock() call to we don't
call the locked function here?

>        __set_chan_timer(chan, sk->sk_sndtimeo);
>
>        if (hcon->state == BT_CONNECTED) {
>                if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>                        __clear_chan_timer(chan);
>                        if (l2cap_chan_check_security(chan))
> -                               __l2cap_state_change(chan, BT_CONNECTED);
> +                               l2cap_state_change(chan, BT_CONNECTED);

And here as well.

>                } else
>                        l2cap_do_start(chan);
>        }
>
>        err = 0;
> +       goto done;
> +
> +sock_release:
> +       release_sock(sk);
>
>  done:
> +       l2cap_chan_unlock(chan);
>        hci_dev_unlock(hdev);
>        hci_dev_put(hdev);
>        return err;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index dea5418..ddac4cb 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -127,6 +127,8 @@ static int l2cap_sock_connect(struct socket *sock, struct sockaddr *addr, int al
>        if (err)
>                goto done;
>
> +       lock_sock(sk);
> +
>        err = bt_sock_wait_state(sk, BT_CONNECTED,
>                        sock_sndtimeo(sk, flags & O_NONBLOCK));
>  done:
> --
> 1.7.8.3

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Ulisses Furquim]

Hi Andrei,

On Thu, Feb 9, 2012 at 12:17 PM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> When needed we take explicit lock.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ae08944..dcf4792 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
>
>        mutex_lock(&conn->chan_lock);
>        c = __l2cap_get_chan_by_scid(conn, cid);
> -       if (c)
> -               lock_sock(c->sk);
>        mutex_unlock(&conn->chan_lock);
> +
>        return c;
>  }
>
> @@ -128,9 +127,8 @@ static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn
>
>        mutex_lock(&conn->chan_lock);
>        c = __l2cap_get_chan_by_ident(conn, ident);
> -       if (c)
> -               lock_sock(c->sk);
>        mutex_unlock(&conn->chan_lock);
> +
>        return c;
>  }
>
> --
> 1.7.8.3

This isn't what I said, maybe I wasn't clear enough, sorry. I said to
change the usage from the locked versions to unlocked ones and in the
same patch add the explicit locking of chan_lock. This will be better
to read and maintain specially since we're adding more locks in all
these paths now. Then later if we don't use the locked versions
anymore we can just remove them. Ok?

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-09 16:17:23 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Usage of RCU list looks not reasonalbe for a number of reasons:
> our code sleep and we have to use socket spinlocks, some parts
> of code are updaters thus we need to use mutexes anyway.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |  108 ++++++++++++++++++++++----------------------
>  1 files changed, 54 insertions(+), 54 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 8dfccb3..ae08944 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
>  
>  static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
>  {
> -	struct l2cap_chan *c, *r = NULL;
> -
> -	rcu_read_lock();
> +	struct l2cap_chan *c;
>  
> -	list_for_each_entry_rcu(c, &conn->chan_l, list) {
> -		if (c->dcid == cid) {
> -			r = c;
> -			break;
> -		}
> +	list_for_each_entry(c, &conn->chan_l, list) {
> +		if (c->dcid == cid)
> +			return c;
>  	}
> -
> -	rcu_read_unlock();
> -	return r;
> +	return NULL;
>  }
>  
>  static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
>  {
> -	struct l2cap_chan *c, *r = NULL;
> -
> -	rcu_read_lock();
> +	struct l2cap_chan *c;
>  
> -	list_for_each_entry_rcu(c, &conn->chan_l, list) {
> -		if (c->scid == cid) {
> -			r = c;
> -			break;
> -		}
> +	list_for_each_entry(c, &conn->chan_l, list) {
> +		if (c->scid == cid)
> +			return c;
>  	}
> -
> -	rcu_read_unlock();
> -	return r;
> +	return NULL;
>  }
>  
>  /* Find channel with given SCID.
> @@ -115,36 +103,34 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
>  {
>  	struct l2cap_chan *c;
>  
> +	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_scid(conn, cid);
>  	if (c)
>  		lock_sock(c->sk);
> +	mutex_unlock(&conn->chan_lock);
>  	return c;
>  }
>  
>  static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
>  {
> -	struct l2cap_chan *c, *r = NULL;
> -
> -	rcu_read_lock();
> +	struct l2cap_chan *c;
>  
> -	list_for_each_entry_rcu(c, &conn->chan_l, list) {
> -		if (c->ident == ident) {
> -			r = c;
> -			break;
> -		}
> +	list_for_each_entry(c, &conn->chan_l, list) {
> +		if (c->ident == ident)
> +			return c;
>  	}
> -
> -	rcu_read_unlock();
> -	return r;
> +	return NULL;
>  }
>  
>  static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
>  {
>  	struct l2cap_chan *c;
>  
> +	mutex_lock(&conn->chan_lock);
>  	c = __l2cap_get_chan_by_ident(conn, ident);
>  	if (c)
>  		lock_sock(c->sk);
> +	mutex_unlock(&conn->chan_lock);
>  	return c;
>  }
>  
> @@ -259,6 +245,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>  
>  	BT_DBG("chan %p state %d", chan, chan->state);
>  
> +	mutex_lock(&chan->conn->chan_lock);
>  	lock_sock(sk);
>  
>  	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
> @@ -272,6 +259,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>  	l2cap_chan_close(chan, reason);
>  
>  	release_sock(sk);
> +	mutex_unlock(&chan->conn->chan_lock);
>  
>  	chan->ops->close(chan->data);
>  	l2cap_chan_put(chan);
> @@ -357,7 +345,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>  
>  	l2cap_chan_hold(chan);
>  
> -	list_add_rcu(&chan->list, &conn->chan_l);
> +	mutex_lock(&conn->chan_lock);
> +	list_add(&chan->list, &conn->chan_l);
> +	mutex_unlock(&conn->chan_lock);

Check the commit that added RCU to our list, I think you are missing to revert
some parts of it.

	Gustavo

Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Ulisses Furquim]

Hi Gustavo,

On Thu, Feb 9, 2012 at 4:37 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> Hi Andrei,
>
> * Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-09 16:17:23 +0200]:
>
>> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>
>> Usage of RCU list looks not reasonalbe for a number of reasons:
>> our code sleep and we have to use socket spinlocks, some parts
>> of code are updaters thus we need to use mutexes anyway.
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> ---
>>  net/bluetooth/l2cap_core.c |  108 ++++++++++++++++++++++----------------------
>>  1 files changed, 54 insertions(+), 54 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
>> index 8dfccb3..ae08944 100644
>> --- a/net/bluetooth/l2cap_core.c
>> +++ b/net/bluetooth/l2cap_core.c
>> @@ -77,36 +77,24 @@ static void l2cap_send_disconn_req(struct l2cap_conn *conn,
>>
>>  static struct l2cap_chan *__l2cap_get_chan_by_dcid(struct l2cap_conn *conn, u16 cid)
>>  {
>> -     struct l2cap_chan *c, *r = NULL;
>> -
>> -     rcu_read_lock();
>> +     struct l2cap_chan *c;
>>
>> -     list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> -             if (c->dcid == cid) {
>> -                     r = c;
>> -                     break;
>> -             }
>> +     list_for_each_entry(c, &conn->chan_l, list) {
>> +             if (c->dcid == cid)
>> +                     return c;
>>       }
>> -
>> -     rcu_read_unlock();
>> -     return r;
>> +     return NULL;
>>  }
>>
>>  static struct l2cap_chan *__l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 cid)
>>  {
>> -     struct l2cap_chan *c, *r = NULL;
>> -
>> -     rcu_read_lock();
>> +     struct l2cap_chan *c;
>>
>> -     list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> -             if (c->scid == cid) {
>> -                     r = c;
>> -                     break;
>> -             }
>> +     list_for_each_entry(c, &conn->chan_l, list) {
>> +             if (c->scid == cid)
>> +                     return c;
>>       }
>> -
>> -     rcu_read_unlock();
>> -     return r;
>> +     return NULL;
>>  }
>>
>>  /* Find channel with given SCID.
>> @@ -115,36 +103,34 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
>>  {
>>       struct l2cap_chan *c;
>>
>> +     mutex_lock(&conn->chan_lock);
>>       c = __l2cap_get_chan_by_scid(conn, cid);
>>       if (c)
>>               lock_sock(c->sk);
>> +     mutex_unlock(&conn->chan_lock);
>>       return c;
>>  }
>>
>>  static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
>>  {
>> -     struct l2cap_chan *c, *r = NULL;
>> -
>> -     rcu_read_lock();
>> +     struct l2cap_chan *c;
>>
>> -     list_for_each_entry_rcu(c, &conn->chan_l, list) {
>> -             if (c->ident == ident) {
>> -                     r = c;
>> -                     break;
>> -             }
>> +     list_for_each_entry(c, &conn->chan_l, list) {
>> +             if (c->ident == ident)
>> +                     return c;
>>       }
>> -
>> -     rcu_read_unlock();
>> -     return r;
>> +     return NULL;
>>  }
>>
>>  static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn, u8 ident)
>>  {
>>       struct l2cap_chan *c;
>>
>> +     mutex_lock(&conn->chan_lock);
>>       c = __l2cap_get_chan_by_ident(conn, ident);
>>       if (c)
>>               lock_sock(c->sk);
>> +     mutex_unlock(&conn->chan_lock);
>>       return c;
>>  }
>>
>> @@ -259,6 +245,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>>
>>       BT_DBG("chan %p state %d", chan, chan->state);
>>
>> +     mutex_lock(&chan->conn->chan_lock);
>>       lock_sock(sk);
>>
>>       if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
>> @@ -272,6 +259,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
>>       l2cap_chan_close(chan, reason);
>>
>>       release_sock(sk);
>> +     mutex_unlock(&chan->conn->chan_lock);
>>
>>       chan->ops->close(chan->data);
>>       l2cap_chan_put(chan);
>> @@ -357,7 +345,9 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
>>
>>       l2cap_chan_hold(chan);
>>
>> -     list_add_rcu(&chan->list, &conn->chan_l);
>> +     mutex_lock(&conn->chan_lock);
>> +     list_add(&chan->list, &conn->chan_l);
>> +     mutex_unlock(&conn->chan_lock);
>
> Check the commit that added RCU to our list, I think you are missing to revert
> some parts of it.

He's undoing it a little bit different. Check my comments in the
previous series he sent.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 04/16] Bluetooth: Add l2cap_chan_lock

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-09 16:17:25 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Channel lock will be used to lock L2CAP channels which are locked
> currently by socket locks.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Acked-by: Marcel Holtmann <marcel@holtmann.org>
> ---
>  include/net/bluetooth/l2cap.h |   11 +++++++++++
>  net/bluetooth/l2cap_core.c    |    2 ++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 42fdbb8..f9fe348 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -497,6 +497,7 @@ struct l2cap_chan {
>  
>  	void		*data;
>  	struct l2cap_ops *ops;
> +	struct mutex		lock;
>  };
>  
>  struct l2cap_ops {
> @@ -609,6 +610,16 @@ static inline void l2cap_chan_put(struct l2cap_chan *c)
>  		kfree(c);
>  }
>  
> +static inline void l2cap_chan_lock(struct l2cap_chan *chan)
> +{
> +	mutex_lock(&chan->lock);
> +}
> +
> +static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
> +{
> +	mutex_unlock(&chan->lock);
> +}
> +

I would call this just l2cap_lock(chan), the shorter the better. We will
always use "chan" in the parameter, so it is easy to understand.

	Gustavo

Re: [RFCv3 02/16] Bluetooth: Revert to mutexes from RCU list

[Emeltchenko Andrei]

Hi Ulisses,

On Thu, Feb 09, 2012 at 03:54:44PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Thu, Feb 9, 2012 at 12:27 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Andrei,
> >
> >> Usage of RCU list looks not reasonalbe for a number of reasons:
> >> our code sleep and we have to use socket spinlocks, some parts
> >> of code are updaters thus we need to use mutexes anyway.
> >>
> >> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >> ---
> >>  net/bluetooth/l2cap_core.c |  108 ++++++++++++++++++++++----------------------
> >>  1 files changed, 54 insertions(+), 54 deletions(-)
> >
> > I am fine with this, but I wanna get an ack from Ulisses as well.
> 
> Please, refer to my previous review because it seems you haven't
> changed anything at all. For instance, l2cap_disconnect_rsp() is still

those changes are in other patch:
[RFCv3 09/16] Bluetooth: Use chan lock in L2CAP sig commands

I was thinking that this patch change RCU to mutex chan list locks
and the changes which change socket lock to chan lock are different.

> missing an unlock of chan_lock and I see no changes to l2cap_sock.c
> which are needed. Moreover, you need to use the unlocked versions of
> the chan lookup functions and check if we still need the locked ones.

I will check need for locking function but at least compiler did not warn
me about unused function.

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 03/16] Bluetooth: Do not use sk lock in get_chan functions

[Emeltchenko Andrei]

On Thu, Feb 09, 2012 at 04:33:58PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Thu, Feb 9, 2012 at 12:17 PM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >
> > When needed we take explicit lock.
> >
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |    6 ++----
> >  1 files changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index ae08944..dcf4792 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -105,9 +105,8 @@ static struct l2cap_chan *l2cap_get_chan_by_scid(struct l2cap_conn *conn, u16 ci
> >
> >        mutex_lock(&conn->chan_lock);
> >        c = __l2cap_get_chan_by_scid(conn, cid);
> > -       if (c)
> > -               lock_sock(c->sk);
> >        mutex_unlock(&conn->chan_lock);
> > +
> >        return c;
> >  }
> >
> > @@ -128,9 +127,8 @@ static inline struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn
> >
> >        mutex_lock(&conn->chan_lock);
> >        c = __l2cap_get_chan_by_ident(conn, ident);
> > -       if (c)
> > -               lock_sock(c->sk);
> >        mutex_unlock(&conn->chan_lock);
> > +
> >        return c;
> >  }
> >
> > --
> > 1.7.8.3
> 
> This isn't what I said, maybe I wasn't clear enough, sorry. I said to
> change the usage from the locked versions to unlocked ones and in the
> same patch add the explicit locking of chan_lock. This will be better
> to read and maintain specially since we're adding more locks in all
> these paths now. Then later if we don't use the locked versions
> anymore we can just remove them. Ok?

We understand different locked and unlocked functions. Since I have
changed locks those are locked ones but locked with conn->chan_lock.

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 04/16] Bluetooth: Add l2cap_chan_lock

[Emeltchenko Andrei]

Hi Gustavo,

On Thu, Feb 09, 2012 at 05:24:30PM -0200, Gustavo Padovan wrote:
> > +static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
> > +{
> > +	mutex_unlock(&chan->lock);
> > +}
> > +
> 
> I would call this just l2cap_lock(chan), the shorter the better. We will
> always use "chan" in the parameter, so it is easy to understand.

See other comment to patch 6 where Marcel suggests to add chan to function
name.

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 06/16] Bluetooth: Add socket error function

[Emeltchenko Andrei]

Hi Marcel, Gustavo,

On Thu, Feb 09, 2012 at 04:13:31PM -0200, Gustavo Padovan wrote:
> > > +static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > > +{
> > > +	struct sock *sk = chan->sk;
> > > +
> > > +	sk->sk_err = err;
> > > +}
> > > +
> > > +static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > > +{
> > > +	struct sock *sk = chan->sk;
> > > +
> > > +	lock_sock(sk);
> > > +	__l2cap_set_sock_err(chan, err);
> > > +	release_sock(sk);
> > > +}
> > 
> > I think we better call this l2cap_chan_set_sock_err(). Sounds kinda
> > long, but makes it a bit more clearer what it does.
> 
> Or l2cap_chan_set_err() since this will be made generic once the separation
> between core and sock is done.

So which of the proposed names better to use? Both looks OK.

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 07/16] Bluetooth: Add unlocked __l2cap_chan_add function

[Emeltchenko Andrei]

Hi Ulisses,

On Thu, Feb 09, 2012 at 03:54:56PM -0200, Ulisses Furquim wrote:
> > -static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> > +void __l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> >  {
> >        BT_DBG("conn %p, psm 0x%2.2x, dcid 0x%4.4x", conn,
> >                        chan->psm, chan->dcid);
> > @@ -370,8 +370,13 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> >
> >        l2cap_chan_hold(chan);
> >
> > -       mutex_lock(&conn->chan_lock);
> >        list_add(&chan->list, &conn->chan_l);
> > +}
> > +
> > +void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
> > +{
> > +       mutex_lock(&conn->chan_lock);
> > +       __l2cap_chan_add(conn, chan);
> >        mutex_unlock(&conn->chan_lock);
> >  }
> >
> > --
> > 1.7.8.3
> 
> As far as I see we don't need the locked version of l2cap_chan_add().

It is still used.

> What I said in my previous review was to remove the lock and unlock
> around the list_add() and add the prefix __ to l2cap_chan_add() then
> use it like that. You'll see that all places where you have
> l2cap_chan_add() you have already grabbed chan_lock. Ok? Adding the

Not always.

> locked version and not using it afterwards makes no sense at all.
> Another thing is that this change to l2cap_chan_add() really belongs
> to patch number 2 in your series.

Yes I can merge this patch to patch number 2 but does it really belongs to
"changing from RCU to mutex locks"?

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Emeltchenko Andrei]

Hi Ulisses,

On Thu, Feb 09, 2012 at 04:25:11PM -0200, Ulisses Furquim wrote:
> >        bacpy(src, conn->src);
> >
> > +       l2cap_chan_unlock(chan);
> >        l2cap_chan_add(conn, chan);
> > +       l2cap_chan_lock(chan);
> 
> Hum, do we really need to do this? Maybe l2cap_chan_add() can receive
> chan already locked?

Then we have here order of locking changed and I have lockdep warnings.

And here l2cap_chan_add used locked.

> 
> > -       __l2cap_state_change(chan, BT_CONNECT);
> > +       l2cap_state_change(chan, BT_CONNECT);
> 
> Why? Is there a problem moving the release_sock() call to we don't
> call the locked function here?
> 
> >        __set_chan_timer(chan, sk->sk_sndtimeo);
> >
> >        if (hcon->state == BT_CONNECTED) {
> >                if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> >                        __clear_chan_timer(chan);
> >                        if (l2cap_chan_check_security(chan))
> > -                               __l2cap_state_change(chan, BT_CONNECTED);
> > +                               l2cap_state_change(chan, BT_CONNECTED);
> 
> And here as well.

Then we would need to release lock before l2cap_do_start.



> 
> >                } else
> >                        l2cap_do_start(chan);


Best regards 
Andrei Emeltchenko 

Re: [RFCv3 06/16] Bluetooth: Add socket error function

[Gustavo Padovan]

Hi Andrei,

* Emeltchenko Andrei <Andrei.Emeltchenko.news@gmail.com> [2012-02-10 10:46:36 +0200]:

> Hi Marcel, Gustavo,
> 
> On Thu, Feb 09, 2012 at 04:13:31PM -0200, Gustavo Padovan wrote:
> > > > +static inline void __l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > > > +{
> > > > +	struct sock *sk = chan->sk;
> > > > +
> > > > +	sk->sk_err = err;
> > > > +}
> > > > +
> > > > +static inline void l2cap_set_sock_err(struct l2cap_chan *chan, int err)
> > > > +{
> > > > +	struct sock *sk = chan->sk;
> > > > +
> > > > +	lock_sock(sk);
> > > > +	__l2cap_set_sock_err(chan, err);
> > > > +	release_sock(sk);
> > > > +}
> > > 
> > > I think we better call this l2cap_chan_set_sock_err(). Sounds kinda
> > > long, but makes it a bit more clearer what it does.
> > 
> > Or l2cap_chan_set_err() since this will be made generic once the separation
> > between core and sock is done.
> 
> So which of the proposed names better to use? Both looks OK.

Please go with l2cap_chan_set_err(), so we avoid a new rename when l2cap sock
is completely removed from core.c

	Gustavo

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Ulisses Furquim]

Hi Andrei,

On Fri, Feb 10, 2012 at 7:18 AM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Thu, Feb 09, 2012 at 04:25:11PM -0200, Ulisses Furquim wrote:
>> >        bacpy(src, conn->src);
>> >
>> > +       l2cap_chan_unlock(chan);
>> >        l2cap_chan_add(conn, chan);
>> > +       l2cap_chan_lock(chan);
>>
>> Hum, do we really need to do this? Maybe l2cap_chan_add() can receive
>> chan already locked?
>
> Then we have here order of locking changed and I have lockdep warnings.
>
> And here l2cap_chan_add used locked.

Why the locked version and not __l2cap_chan_add()?

>>
>> > -       __l2cap_state_change(chan, BT_CONNECT);
>> > +       l2cap_state_change(chan, BT_CONNECT);
>>
>> Why? Is there a problem moving the release_sock() call to we don't
>> call the locked function here?
>>
>> >        __set_chan_timer(chan, sk->sk_sndtimeo);
>> >
>> >        if (hcon->state == BT_CONNECTED) {
>> >                if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>> >                        __clear_chan_timer(chan);
>> >                        if (l2cap_chan_check_security(chan))
>> > -                               __l2cap_state_change(chan, BT_CONNECTED);
>> > +                               l2cap_state_change(chan, BT_CONNECTED);
>>
>> And here as well.
>
> Then we would need to release lock before l2cap_do_start.

Sure.

>>
>> >                } else
>> >                        l2cap_do_start(chan);
>
>
> Best regards
> Andrei Emeltchenko

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Emeltchenko Andrei]

Hi Ulisses,

On Fri, Feb 10, 2012 at 04:31:26PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Fri, Feb 10, 2012 at 7:18 AM, Emeltchenko Andrei
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > Hi Ulisses,
> >
> > On Thu, Feb 09, 2012 at 04:25:11PM -0200, Ulisses Furquim wrote:
> >> >        bacpy(src, conn->src);
> >> >
> >> > +       l2cap_chan_unlock(chan);
> >> >        l2cap_chan_add(conn, chan);
> >> > +       l2cap_chan_lock(chan);
> >>
> >> Hum, do we really need to do this? Maybe l2cap_chan_add() can receive
> >> chan already locked?
> >
> > Then we have here order of locking changed and I have lockdep warnings.
> >
> > And here l2cap_chan_add used locked.
> 
> Why the locked version and not __l2cap_chan_add()?

Because we need to lock addition channel to chan list and we are locked
only with chan->lock.

> >> > -       __l2cap_state_change(chan, BT_CONNECT);
> >> > +       l2cap_state_change(chan, BT_CONNECT);
> >>
> >> Why? Is there a problem moving the release_sock() call to we don't
> >> call the locked function here?
> >>
> >> >        __set_chan_timer(chan, sk->sk_sndtimeo);
> >> >
> >> >        if (hcon->state == BT_CONNECTED) {
> >> >                if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
> >> >                        __clear_chan_timer(chan);
> >> >                        if (l2cap_chan_check_security(chan))
> >> > -                               __l2cap_state_change(chan, BT_CONNECTED);
> >> > +                               l2cap_state_change(chan, BT_CONNECTED);
> >>
> >> And here as well.
> >
> > Then we would need to release lock before l2cap_do_start.
> 
> Sure.

I will check what can be done, currently including wide locks/unlocks would
significantly reduce readability of this part of the code. 

Best regards 
Andrei Emeltchenko 

Re: [RFCv3 15/16] Bluetooth: Use l2cap chan lock in socket connect

[Ulisses Furquim]

Hi Andrei,

On Mon, Feb 13, 2012 at 5:47 AM, Emeltchenko Andrei
<Andrei.Emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Fri, Feb 10, 2012 at 04:31:26PM -0200, Ulisses Furquim wrote:
>> Hi Andrei,
>>
>> On Fri, Feb 10, 2012 at 7:18 AM, Emeltchenko Andrei
>> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> > Hi Ulisses,
>> >
>> > On Thu, Feb 09, 2012 at 04:25:11PM -0200, Ulisses Furquim wrote:
>> >> >        bacpy(src, conn->src);
>> >> >
>> >> > +       l2cap_chan_unlock(chan);
>> >> >        l2cap_chan_add(conn, chan);
>> >> > +       l2cap_chan_lock(chan);
>> >>
>> >> Hum, do we really need to do this? Maybe l2cap_chan_add() can receive
>> >> chan already locked?
>> >
>> > Then we have here order of locking changed and I have lockdep warnings.
>> >
>> > And here l2cap_chan_add used locked.
>>
>> Why the locked version and not __l2cap_chan_add()?
>
> Because we need to lock addition channel to chan list and we are locked
> only with chan->lock.

Ugh, you're right.

>> >> > -       __l2cap_state_change(chan, BT_CONNECT);
>> >> > +       l2cap_state_change(chan, BT_CONNECT);
>> >>
>> >> Why? Is there a problem moving the release_sock() call to we don't
>> >> call the locked function here?
>> >>
>> >> >        __set_chan_timer(chan, sk->sk_sndtimeo);
>> >> >
>> >> >        if (hcon->state == BT_CONNECTED) {
>> >> >                if (chan->chan_type != L2CAP_CHAN_CONN_ORIENTED) {
>> >> >                        __clear_chan_timer(chan);
>> >> >                        if (l2cap_chan_check_security(chan))
>> >> > -                               __l2cap_state_change(chan, BT_CONNECTED);
>> >> > +                               l2cap_state_change(chan, BT_CONNECTED);
>> >>
>> >> And here as well.
>> >
>> > Then we would need to release lock before l2cap_do_start.
>>
>> Sure.
>
> I will check what can be done, currently including wide locks/unlocks would
> significantly reduce readability of this part of the code.

Do you think? Having fewer lock and unlock calls actually should be
less error prone IMO. And we don't have any lock with a lot of
contention that we need to be so minimal in our critical sections.

Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs