[PATCHv1 00/14] Bluetooth: Change socket lock to l2cap_chan lock

[PATCHv1 00/14] Bluetooth: Change socket lock to l2cap_chan lock

[Andrei Emeltchenko]

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

Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
protocol above L2CAP without creating sockets.

Changes:
	* PATCHv1: Added extra line (per Gustavo comment)
	* RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
	following recommendations from review.
	* RFCv5: Fixed locking bug in l2cap_data_channel, added locks in 
	l2cap_sock_shutdown function, fixed several styles issues.
	* RFCv4: Better split patches so they looks more clear and obvious,
	taking coments about naming change and delete unused vars. See diff change
	from the previous version below:
	* 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.

Andrei Emeltchenko (14):
  Bluetooth: trivial: Fix long line
  Bluetooth: Revert to mutexes from RCU list
  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 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    |  392 +++++++++++++++++++++++++----------------
 net/bluetooth/l2cap_sock.c    |   34 +++-
 3 files changed, 281 insertions(+), 156 deletions(-)

-- 
1.7.9

[PATCHv1 01/14] Bluetooth: trivial: Fix long line

[Andrei Emeltchenko]

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

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
 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 37736c6..63539f9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -2720,7 +2720,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.9

[PATCHv1 02/14] Bluetooth: Revert to mutexes from RCU list

[Andrei Emeltchenko]

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

Usage of RCU list looks not reasonalbe for a number of reasons:
our code sleep and we had to use socket spinlocks. Most parts
of code are updaters thus there is little sense to use RCU.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap_core.c |  166 ++++++++++++++++++++++++++------------------
 net/bluetooth/l2cap_sock.c |   10 +++
 2 files changed, 109 insertions(+), 67 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 63539f9..8e8e9e9 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,32 @@ 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;
 }
 
@@ -228,11 +212,13 @@ static void l2cap_chan_timeout(struct work_struct *work)
 {
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							chan_timer.work);
+	struct l2cap_conn *conn = chan->conn;
 	struct sock *sk = chan->sk;
 	int reason;
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
+	mutex_lock(&conn->chan_lock);
 	lock_sock(sk);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
@@ -248,6 +234,8 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	release_sock(sk);
 
 	chan->ops->close(chan->data);
+	mutex_unlock(&conn->chan_lock);
+
 	l2cap_chan_put(chan);
 }
 
@@ -331,7 +319,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.
@@ -348,8 +338,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);
 
@@ -400,10 +389,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);
 	}
 }
@@ -718,13 +709,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);
@@ -804,7 +795,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.
@@ -916,9 +907,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);
@@ -938,7 +929,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 */
@@ -948,16 +939,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)
@@ -984,6 +975,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;
@@ -993,6 +986,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)
@@ -1050,6 +1045,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);
 
@@ -1792,9 +1788,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;
@@ -1810,7 +1806,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 ---- */
@@ -2600,6 +2596,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) */
@@ -2673,6 +2670,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);
@@ -2714,6 +2712,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);
@@ -2723,17 +2722,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;
+		}
 	}
 
+	err = 0;
+
 	sk = chan->sk;
+	lock_sock(sk);
 
 	switch (result) {
 	case L2CAP_CR_SUCCESS:
@@ -2760,7 +2768,11 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	}
 
 	release_sock(sk);
-	return 0;
+
+unlock:
+	mutex_unlock(&conn->chan_lock);
+
+	return err;
 }
 
 static inline void set_default_fcs(struct l2cap_chan *chan)
@@ -2793,6 +2805,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		return -ENOENT;
 
 	sk = chan->sk;
+	lock_sock(sk);
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
 		struct l2cap_cmd_rej_cid rej;
@@ -2905,6 +2918,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		return 0;
 
 	sk = chan->sk;
+	lock_sock(sk);
 
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
@@ -3006,11 +3020,16 @@ 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;
+	}
 
 	sk = chan->sk;
+	lock_sock(sk);
 
 	rsp.dcid = cpu_to_le16(chan->scid);
 	rsp.scid = cpu_to_le16(chan->dcid);
@@ -3022,6 +3041,9 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 	release_sock(sk);
 
 	chan->ops->close(chan->data);
+
+	mutex_unlock(&conn->chan_lock);
+
 	return 0;
 }
 
@@ -3037,16 +3059,24 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 
 	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;
+	}
 
 	sk = chan->sk;
+	lock_sock(sk);
 
 	l2cap_chan_del(chan, 0);
 	release_sock(sk);
 
 	chan->ops->close(chan->data);
+
+	mutex_unlock(&conn->chan_lock);
+
 	return 0;
 }
 
@@ -4205,6 +4235,7 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
 	}
 
 	sk = chan->sk;
+	lock_sock(sk);
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
@@ -4492,9 +4523,9 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 		cancel_delayed_work(&conn->security_timer);
 	}
 
-	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);
@@ -4574,7 +4605,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;
 }
@@ -4635,6 +4666,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 b48d6c1..1273fcb 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -796,6 +796,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 {
 	struct sock *sk = sock->sk;
 	struct l2cap_chan *chan;
+	struct l2cap_conn *conn;
 	int err = 0;
 
 	BT_DBG("sock %p, sk %p", sock, sk);
@@ -804,6 +805,10 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		return 0;
 
 	chan = l2cap_pi(sk)->chan;
+	conn = chan->conn;
+
+	if (conn)
+		mutex_lock(&conn->chan_lock);
 
 	lock_sock(sk);
 	if (!sk->sk_shutdown) {
@@ -811,6 +816,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 			err = __l2cap_wait_ack(sk);
 
 		sk->sk_shutdown = SHUTDOWN_MASK;
+
 		l2cap_chan_close(chan, 0);
 
 		if (sock_flag(sk, SOCK_LINGER) && sk->sk_lingertime)
@@ -822,6 +828,10 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = -sk->sk_err;
 
 	release_sock(sk);
+
+	if (conn)
+		mutex_unlock(&conn->chan_lock);
+
 	return err;
 }
 
-- 
1.7.9

[PATCHv1 03/14] Bluetooth: Add l2cap_chan_lock

[Andrei Emeltchenko]

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 bbb0e21..d6d8ec8 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 8e8e9e9..e39eba1 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -247,6 +247,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.9

Re: [PATCHv1 03/14] Bluetooth: Add l2cap_chan_lock

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:54 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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(-)

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

Re: [PATCHv1 03/14] Bluetooth: Add l2cap_chan_lock

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:54:56 +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(-)

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>

	Gustavo

[PATCHv1 04/14] Bluetooth: Add locked and unlocked state_change

[Andrei Emeltchenko]

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 e39eba1..4638dbb 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -199,7 +199,7 @@ static u16 l2cap_alloc_cid(struct l2cap_conn *conn)
 	return 0;
 }
 
-static void l2cap_state_change(struct l2cap_chan *chan, int state)
+static void __l2cap_state_change(struct l2cap_chan *chan, int state)
 {
 	BT_DBG("chan %p %s -> %s", chan, state_to_string(chan->state),
 						state_to_string(state));
@@ -208,6 +208,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,
@@ -348,7 +357,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)
@@ -413,7 +422,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;
 
@@ -704,7 +713,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;
 }
 
@@ -770,7 +779,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);
 				}
@@ -873,7 +882,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:
@@ -890,7 +899,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)
@@ -922,7 +931,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)
@@ -1196,14 +1205,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);
 	}
@@ -2650,22 +2659,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;
 	}
@@ -4584,12 +4593,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.9

Re: [PATCHv1 04/14] Bluetooth: Add locked and unlocked state_change

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:54 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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(-)

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

Re: [PATCHv1 04/14] Bluetooth: Add locked and unlocked state_change

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:54:57 +0200]:

> 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(-)

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>

	Gustavo

[PATCHv1 05/14] Bluetooth: Add socket error function

[Andrei Emeltchenko]

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

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

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 4638dbb..c0640b7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -217,6 +217,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
 	release_sock(sk);
 }
 
+static inline void __l2cap_chan_set_err(struct l2cap_chan *chan, int err)
+{
+	struct sock *sk = chan->sk;
+
+	sk->sk_err = err;
+}
+
+static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
+{
+	struct sock *sk = chan->sk;
+
+	lock_sock(sk);
+	__l2cap_chan_set_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,
@@ -361,7 +377,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_chan_set_err(chan, err);
 
 	if (parent) {
 		bt_accept_unlink(sk);
@@ -694,14 +710,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);
@@ -714,7 +727,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_chan_set_err(chan, err);
 }
 
 /* ---- L2CAP connections ---- */
@@ -953,10 +966,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_chan_set_err(chan, err);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -2988,7 +2999,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		sk->sk_err = ECONNRESET;
+		__l2cap_chan_set_err(chan, ECONNRESET);
+
 		__set_chan_timer(chan,
 				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
 		l2cap_send_disconn_req(conn, chan, ECONNRESET);
-- 
1.7.9

Re: [PATCHv1 05/14] Bluetooth: Add socket error function

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:54 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Use locked and unlocked versions to help removing socket
> locks from l2cap core functions.
>
> 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 4638dbb..c0640b7 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -217,6 +217,22 @@ static void l2cap_state_change(struct l2cap_chan *chan, int state)
>        release_sock(sk);
>  }
>
> +static inline void __l2cap_chan_set_err(struct l2cap_chan *chan, int err)
> +{
> +       struct sock *sk = chan->sk;
> +
> +       sk->sk_err = err;
> +}
> +
> +static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
> +{
> +       struct sock *sk = chan->sk;
> +
> +       lock_sock(sk);
> +       __l2cap_chan_set_err(chan, err);
> +       release_sock(sk);
> +}
> +

I don't remember now, but if we don't even use the locked version,
please remove it. Overall it looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

<snip>

Regards,

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

Re: [PATCHv1 05/14] Bluetooth: Add socket error function

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:54:58 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Use locked and unlocked versions to help removing socket
> locks from l2cap core functions.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
>

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>


	Gustavo

[PATCHv1 06/14] Bluetooth: Add unlocked __l2cap_chan_add function

[Andrei Emeltchenko]

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 c0640b7..0e4f4cb 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -300,7 +300,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);
@@ -346,8 +346,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.9

Re: [PATCHv1 06/14] Bluetooth: Add unlocked __l2cap_chan_add function

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:54 AM, Andrei Emeltchenko
<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 c0640b7..0e4f4cb 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -300,7 +300,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);
> @@ -346,8 +346,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);
>  }

Same comment applies here. If we don't use the locked version, don't
bother adding it.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

Re: [PATCHv1 06/14] Bluetooth: Add unlocked __l2cap_chan_add function

[Andrei Emeltchenko]

Hi Ulisses,

On Tue, Feb 21, 2012 at 03:01:55PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Tue, Feb 21, 2012 at 8:54 AM, Andrei Emeltchenko
> <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 c0640b7..0e4f4cb 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -300,7 +300,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);
> > @@ -346,8 +346,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);
> >  }
> 
> Same comment applies here. If we don't use the locked version, don't
> bother adding it.

It is used for example in the code below:

   1    470  net/bluetooth/a2mp.c <<open_a2mp_chan>>
             l2cap_chan_add(conn, chan);
   2    917  net/bluetooth/l2cap_core.c <<l2cap_le_conn_ready>>
             l2cap_chan_add(conn, chan);
   3   1258  net/bluetooth/l2cap_core.c <<l2cap_chan_connect>>
             l2cap_chan_add(conn, chan);

Best regards 
Andrei Emeltchenko 

Re: [PATCHv1 06/14] Bluetooth: Add unlocked __l2cap_chan_add function

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:54:59 +0200]:

> 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(-)

Please add a message description to this patch, apart from that

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>


	Gustavo

[PATCHv1 07/14] Bluetooth: Use chan lock in timers

[Andrei Emeltchenko]

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

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

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0e4f4cb..c008608 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -238,13 +238,12 @@ static void l2cap_chan_timeout(struct work_struct *work)
 	struct l2cap_chan *chan = container_of(work, struct l2cap_chan,
 							chan_timer.work);
 	struct l2cap_conn *conn = chan->conn;
-	struct sock *sk = chan->sk;
 	int reason;
 
 	BT_DBG("chan %p state %s", chan, state_to_string(chan->state));
 
 	mutex_lock(&conn->chan_lock);
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	if (chan->state == BT_CONNECTED || chan->state == BT_CONFIG)
 		reason = ECONNREFUSED;
@@ -256,7 +255,7 @@ static void l2cap_chan_timeout(struct work_struct *work)
 
 	l2cap_chan_close(chan, reason);
 
-	release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	chan->ops->close(chan->data);
 	mutex_unlock(&conn->chan_lock);
@@ -1277,14 +1276,14 @@ 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);
+		l2cap_chan_unlock(chan);
 		return;
 	}
 
@@ -1292,25 +1291,26 @@ static void l2cap_monitor_timeout(struct work_struct *work)
 	__set_monitor_timer(chan);
 
 	l2cap_send_rr_or_rnr(chan, L2CAP_CTRL_POLL);
-	release_sock(sk);
+	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)
@@ -2001,9 +2001,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.9

Re: [PATCHv1 07/14] Bluetooth: Use chan lock in timers

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Change locking in delayed works to chan lock instead of sk lock.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   26 ++++++++++++++------------
>  1 files changed, 14 insertions(+), 12 deletions(-)

Looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

[PATCHv1 08/14] Bluetooth: Use chan lock in L2CAP sig commands

[Andrei Emeltchenko]

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 |   61 +++++++++++++++++++++++++------------------
 net/bluetooth/l2cap_sock.c |    7 +++-
 2 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c008608..ab6bede 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -377,6 +377,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);
 
@@ -389,6 +391,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;
@@ -421,10 +425,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);
 	}
@@ -440,10 +444,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:
@@ -486,7 +492,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;
 	}
 }
@@ -714,6 +722,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)
@@ -730,8 +739,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_chan_set_err(chan, err);
+	release_sock(sk);
 }
 
 /* ---- L2CAP connections ---- */
@@ -992,7 +1003,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;
@@ -1005,10 +1015,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);
 	}
 
@@ -2666,7 +2678,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;
 
@@ -2739,7 +2751,6 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
 	u16 scid, dcid, result, status;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	u8 req[128];
 	int err;
 
@@ -2769,8 +2780,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	err = 0;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	switch (result) {
 	case L2CAP_CR_SUCCESS:
@@ -2796,7 +2806,7 @@ 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);
@@ -2821,7 +2831,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	u16 dcid, flags;
 	u8 rsp[64];
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	int len;
 
 	dcid  = __le16_to_cpu(req->dcid);
@@ -2833,8 +2842,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return -ENOENT;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
 		struct l2cap_cmd_rej_cid rej;
@@ -2923,7 +2931,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;
 }
 
@@ -2932,7 +2940,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	struct l2cap_conf_rsp *rsp = (struct l2cap_conf_rsp *)data;
 	u16 scid, flags, result;
 	struct l2cap_chan *chan;
-	struct sock *sk;
 	int len = cmd->len - sizeof(*rsp);
 
 	scid   = __le16_to_cpu(rsp->scid);
@@ -2946,8 +2953,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (!chan)
 		return 0;
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	switch (result) {
 	case L2CAP_CONF_SUCCESS:
@@ -3006,7 +3012,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		__l2cap_chan_set_err(chan, ECONNRESET);
+		l2cap_chan_set_err(chan, ECONNRESET);
 
 		__set_chan_timer(chan,
 				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
@@ -3033,7 +3039,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;
 }
 
@@ -3058,17 +3064,21 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
 		return 0;
 	}
 
+	l2cap_chan_lock(chan);
+
 	sk = chan->sk;
-	lock_sock(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);
 
 	chan->ops->close(chan->data);
 
@@ -3082,7 +3092,6 @@ 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);
@@ -3097,11 +3106,11 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
 		return 0;
 	}
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	l2cap_chan_del(chan, 0);
-	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 1273fcb..db38fae 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -809,15 +809,18 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 
 	if (conn)
 		mutex_lock(&conn->chan_lock);
-
+	l2cap_chan_lock(chan);
 	lock_sock(sk);
+
 	if (!sk->sk_shutdown) {
 		if (chan->mode == L2CAP_MODE_ERTM)
 			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,
@@ -828,7 +831,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
 		err = -sk->sk_err;
 
 	release_sock(sk);
-
+	l2cap_chan_unlock(chan);
 	if (conn)
 		mutex_unlock(&conn->chan_lock);
 
-- 
1.7.9

Re: [PATCHv1 08/14] Bluetooth: Use chan lock in L2CAP sig commands

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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 |   61 +++++++++++++++++++++++++------------------
>  net/bluetooth/l2cap_sock.c |    7 +++-
>  2 files changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c008608..ab6bede 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -377,6 +377,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);
>
> @@ -389,6 +391,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;

You're making l2cap_chan_del() deal with sock lock so please remove
the comment above the function signature. It doesn't make sense to
leave old comments around.

> @@ -421,10 +425,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);
>        }

Ok.

> @@ -440,10 +444,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:

Do we need to lock sock around l2cap_chan_cleanup_listen() as well?
l2cap_chan_close() will call l2cap_chan_del() which now does
lock_sock/release_sock, right?

> @@ -486,7 +492,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;
>        }
>  }

Hmm, this indeed looks correct. However, it'd be good to later revisit
this as we want core without any sock handling, right?

> @@ -714,6 +722,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)
> @@ -730,8 +739,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_chan_set_err(chan, err);
> +       release_sock(sk);
>  }
>
>  /* ---- L2CAP connections ---- */

It seems we didn't have any locking around these before. Why? Do we
really need it now?

<snip>

Regards,

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

Re: [PATCHv1 08/14] Bluetooth: Use chan lock in L2CAP sig commands

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:01 +0200]:

> 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 |   61 +++++++++++++++++++++++++------------------
>  net/bluetooth/l2cap_sock.c |    7 +++-
>  2 files changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c008608..ab6bede 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -377,6 +377,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);
>  
> @@ -389,6 +391,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;
> @@ -421,10 +425,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);
>  	}
> @@ -440,10 +444,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:
> @@ -486,7 +492,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;
>  	}
>  }
> @@ -714,6 +722,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)
> @@ -730,8 +739,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_chan_set_err(chan, err);
> +	release_sock(sk);
>  }
>  
>  /* ---- L2CAP connections ---- */
> @@ -992,7 +1003,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;
> @@ -1005,10 +1015,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);
>  	}
>  
> @@ -2666,7 +2678,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;
>  
> @@ -2739,7 +2751,6 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  	struct l2cap_conn_rsp *rsp = (struct l2cap_conn_rsp *) data;
>  	u16 scid, dcid, result, status;
>  	struct l2cap_chan *chan;
> -	struct sock *sk;
>  	u8 req[128];
>  	int err;
>  
> @@ -2769,8 +2780,7 @@ static inline int l2cap_connect_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  
>  	err = 0;
>  
> -	sk = chan->sk;
> -	lock_sock(sk);
> +	l2cap_chan_lock(chan);
>  
>  	switch (result) {
>  	case L2CAP_CR_SUCCESS:
> @@ -2796,7 +2806,7 @@ 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);
> @@ -2821,7 +2831,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	u16 dcid, flags;
>  	u8 rsp[64];
>  	struct l2cap_chan *chan;
> -	struct sock *sk;
>  	int len;
>  
>  	dcid  = __le16_to_cpu(req->dcid);
> @@ -2833,8 +2842,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	if (!chan)
>  		return -ENOENT;
>  
> -	sk = chan->sk;
> -	lock_sock(sk);
> +	l2cap_chan_lock(chan);
>  
>  	if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
>  		struct l2cap_cmd_rej_cid rej;
> @@ -2923,7 +2931,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;
>  }
>  
> @@ -2932,7 +2940,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	struct l2cap_conf_rsp *rsp = (struct l2cap_conf_rsp *)data;
>  	u16 scid, flags, result;
>  	struct l2cap_chan *chan;
> -	struct sock *sk;
>  	int len = cmd->len - sizeof(*rsp);
>  
>  	scid   = __le16_to_cpu(rsp->scid);
> @@ -2946,8 +2953,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	if (!chan)
>  		return 0;
>  
> -	sk = chan->sk;
> -	lock_sock(sk);
> +	l2cap_chan_lock(chan);
>  
>  	switch (result) {
>  	case L2CAP_CONF_SUCCESS:
> @@ -3006,7 +3012,7 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  		}
>  
>  	default:
> -		__l2cap_chan_set_err(chan, ECONNRESET);
> +		l2cap_chan_set_err(chan, ECONNRESET);
>  
>  		__set_chan_timer(chan,
>  				msecs_to_jiffies(L2CAP_DISC_REJ_TIMEOUT));
> @@ -3033,7 +3039,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;
>  }
>  
> @@ -3058,17 +3064,21 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn, struct l2cap_cmd
>  		return 0;
>  	}
>  
> +	l2cap_chan_lock(chan);
> +
>  	sk = chan->sk;
> -	lock_sock(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);
>  
>  	chan->ops->close(chan->data);
>  
> @@ -3082,7 +3092,6 @@ 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);
> @@ -3097,11 +3106,11 @@ static inline int l2cap_disconnect_rsp(struct l2cap_conn *conn, struct l2cap_cmd
>  		return 0;
>  	}
>  
> -	sk = chan->sk;
> -	lock_sock(sk);
> +	l2cap_chan_lock(chan);
>  
>  	l2cap_chan_del(chan, 0);
> -	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 1273fcb..db38fae 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -809,15 +809,18 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
>  
>  	if (conn)
>  		mutex_lock(&conn->chan_lock);
> -

Don't remove this line.

> +	l2cap_chan_lock(chan);
>  	lock_sock(sk);
> +
>  	if (!sk->sk_shutdown) {
>  		if (chan->mode == L2CAP_MODE_ERTM)
>  			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,
> @@ -828,7 +831,7 @@ static int l2cap_sock_shutdown(struct socket *sock, int how)
>  		err = -sk->sk_err;
>  
>  	release_sock(sk);
> -

Same here.

	Gustavo

[PATCHv1 09/14] Bluetooth: Use chan lock in L2CAP conn start

[Andrei Emeltchenko]

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 ab6bede..d5380ce 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -757,10 +757,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;
 		}
 
@@ -769,17 +769,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;
 			}
 
@@ -799,6 +797,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);
@@ -811,6 +810,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);
@@ -821,7 +821,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;
 			}
 
@@ -831,7 +831,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.9

Re: [PATCHv1 09/14] Bluetooth: Use chan lock in L2CAP conn start

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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(-)

Looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

[PATCHv1 10/14] Bluetooth: Use chan lock in receiving data

[Andrei Emeltchenko]

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 |   11 +++++------
 net/bluetooth/l2cap_sock.c |   11 +++++++++--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d5380ce..24c7729 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4262,7 +4262,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;
@@ -4270,11 +4269,12 @@ static inline int l2cap_data_channel(struct l2cap_conn *conn, u16 cid, struct sk
 	chan = l2cap_get_chan_by_scid(conn, cid);
 	if (!chan) {
 		BT_DBG("unknown cid 0x%4.4x", cid);
-		goto drop;
+		/* Drop packet and return */
+		kfree(skb);
+		return 0;
 	}
 
-	sk = chan->sk;
-	lock_sock(sk);
+	l2cap_chan_lock(chan);
 
 	BT_DBG("chan %p, len %d", chan, skb->len);
 
@@ -4345,8 +4345,7 @@ drop:
 	kfree_skb(skb);
 
 done:
-	if (sk)
-		release_sock(sk);
+	l2cap_chan_unlock(chan);
 
 	return 0;
 }
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index db38fae..a278858 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -877,8 +877,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);
 
@@ -897,6 +901,9 @@ static int l2cap_sock_recv_cb(void *data, struct sk_buff *skb)
 		err = 0;
 	}
 
+done:
+	release_sock(sk);
+
 	return err;
 }
 
-- 
1.7.9

Re: [PATCHv1 10/14] Bluetooth: Use chan lock in receiving data

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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 |   11 +++++------
>  net/bluetooth/l2cap_sock.c |   11 +++++++++--
>  2 files changed, 14 insertions(+), 8 deletions(-)

Looks good to me.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

[PATCHv1 11/14] Bluetooth: Change locking logic for conn/chan ready

[Andrei Emeltchenko]

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 24c7729..7baee68 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -920,7 +920,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);
 
@@ -932,6 +936,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)
@@ -949,23 +955,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.9

Re: [PATCHv1 11/14] Bluetooth: Change locking logic for conn/chan ready

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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(-)

Looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

[PATCHv1 12/14] Bluetooth: Change locking logic in security_cfm

[Andrei Emeltchenko]

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 |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7baee68..49aa226 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4572,9 +4572,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) {
-		struct sock *sk = chan->sk;
-
-		bh_lock_sock(sk);
+		l2cap_chan_lock(chan);
 
 		BT_DBG("chan->scid %d", chan->scid);
 
@@ -4584,19 +4582,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;
 		}
 
@@ -4617,9 +4615,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;
@@ -4640,6 +4641,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);
@@ -4648,7 +4651,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.9

Re: [PATCHv1 12/14] Bluetooth: Change locking logic in security_cfm

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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 |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)

Looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

Re: [PATCHv1 12/14] Bluetooth: Change locking logic in security_cfm

[Gustavo Padovan]

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:05 +0200]:

> 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 |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 7baee68..49aa226 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4572,9 +4572,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) {
> -		struct sock *sk = chan->sk;
> -
> -		bh_lock_sock(sk);
> +		l2cap_chan_lock(chan);
>  
>  		BT_DBG("chan->scid %d", chan->scid);
>  
> @@ -4584,19 +4582,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;
>  		}
>  
> @@ -4617,9 +4615,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;
> @@ -4640,6 +4641,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);
> @@ -4648,7 +4651,7 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
>  							sizeof(rsp), &rsp);
>  		}
>  
> -		bh_unlock_sock(sk);
> +		l2cap_chan_unlock(chan);

I don't like the idea of this locking change in multiple separate patches, it
could put the stack in a broken and racy state if we stop in the middle of
these patches.
I would say to merge all these patches into one, they were a lot reviewed
alone, there is no problem in create a big one with all the changes together.

	Gustavo

Re: [PATCHv1 12/14] Bluetooth: Change locking logic in security_cfm

[Andrei Emeltchenko]

Hi Gustavo,

On Tue, Feb 21, 2012 at 7:56 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:05 +0200]:
>
>> 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>

> I don't like the idea of this locking change in multiple separate patches, it
> could put the stack in a broken and racy state if we stop in the middle of
> these patches.
> I would say to merge all these patches into one, they were a lot reviewed
> alone, there is no problem in create a big one with all the changes together.

I saw the same comments from Ulisses. I will merge all those patches
together and send new patch series.

Regards,
Andrei

Re: [PATCHv1 12/14] Bluetooth: Change locking logic in security_cfm

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 6:44 PM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi Gustavo,
>
> On Tue, Feb 21, 2012 at 7:56 PM, Gustavo Padovan <padovan@profusion.mobi> wrote:
>> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:05 +0200]:
>>
>>> 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>
>
>> I don't like the idea of this locking change in multiple separate patches, it
>> could put the stack in a broken and racy state if we stop in the middle of
>> these patches.
>> I would say to merge all these patches into one, they were a lot reviewed
>> alone, there is no problem in create a big one with all the changes together.
>
> I saw the same comments from Ulisses. I will merge all those patches
> together and send new patch series.

Well, yes, but Marcel wasn't really worried about it, though. It's
good if we merge this soon and move on with other patches touching
L2CAP code.

Best regards,

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

[PATCHv1 13/14] Bluetooth: Use l2cap chan lock in socket connect

[Andrei Emeltchenko]

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

Function l2cap_chan_connect does not return with locked socket
anymore. So we take explicit lock in l2cap_sock_connect.

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

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 49aa226..9e0af6e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1159,7 +1159,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 &&
@@ -1186,17 +1186,21 @@ 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;
+		release_sock(sk);
 		goto done;
 
 	case BT_CONNECTED:
 		/* Already connected */
 		err = -EISCONN;
+		release_sock(sk);
 		goto done;
 
 	case BT_OPEN:
@@ -1206,11 +1210,15 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
 
 	default:
 		err = -EBADFD;
+		release_sock(sk);
 		goto done;
 	}
 
 	/* Set destination address and psm */
 	bacpy(&bt_sk(sk)->dst, dst);
+
+	release_sock(sk);
+
 	chan->psm = psm;
 	chan->dcid = cid;
 
@@ -1238,23 +1246,25 @@ 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;
-
 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 a278858..bbc1747 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.9

Re: [PATCHv1 13/14] Bluetooth: Use l2cap chan lock in socket connect

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Function l2cap_chan_connect does not return with locked socket
> anymore. So we take explicit lock in l2cap_sock_connect.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   18 ++++++++++++++----
>  net/bluetooth/l2cap_sock.c |    2 ++
>  2 files changed, 16 insertions(+), 4 deletions(-)

Looks good.

Reviewed-by: Ulisses Furquim <ulisses@profusion.mobi>

Regards,

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

Re: [PATCHv1 13/14] Bluetooth: Use l2cap chan lock in socket connect

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:06 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Function l2cap_chan_connect does not return with locked socket
> anymore. So we take explicit lock in l2cap_sock_connect.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/l2cap_core.c |   18 ++++++++++++++----
>  net/bluetooth/l2cap_sock.c |    2 ++
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 49aa226..9e0af6e 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1159,7 +1159,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 &&
> @@ -1186,17 +1186,21 @@ 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;
> +		release_sock(sk);
>  		goto done;
>  
>  	case BT_CONNECTED:
>  		/* Already connected */
>  		err = -EISCONN;
> +		release_sock(sk);
>  		goto done;
>  
>  	case BT_OPEN:
> @@ -1206,11 +1210,15 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
>  
>  	default:
>  		err = -EBADFD;
> +		release_sock(sk);
>  		goto done;
>  	}
>  
>  	/* Set destination address and psm */
>  	bacpy(&bt_sk(sk)->dst, dst);
> +
> +	release_sock(sk);
> +
>  	chan->psm = psm;
>  	chan->dcid = cid;
>  
> @@ -1238,23 +1246,25 @@ 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);

What are you trying to do here?

>  
> -	__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;
> -

Do not remove this line, please.

	Gustavo

Re: [PATCHv1 13/14] Bluetooth: Use l2cap chan lock in socket connect

[Andrei Emeltchenko]

Hi Gustavo,

On Tue, Feb 21, 2012 at 03:46:57PM -0200, Gustavo Padovan wrote:
> Hi Andrei,
> 
> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:06 +0200]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > Function l2cap_chan_connect does not return with locked socket
> > anymore. So we take explicit lock in l2cap_sock_connect.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/l2cap_core.c |   18 ++++++++++++++----
> >  net/bluetooth/l2cap_sock.c |    2 ++
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 49aa226..9e0af6e 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1159,7 +1159,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 &&
> > @@ -1186,17 +1186,21 @@ 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;
> > +		release_sock(sk);
> >  		goto done;
> >  
> >  	case BT_CONNECTED:
> >  		/* Already connected */
> >  		err = -EISCONN;
> > +		release_sock(sk);
> >  		goto done;
> >  
> >  	case BT_OPEN:
> > @@ -1206,11 +1210,15 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid, bdaddr_t *d
> >  
> >  	default:
> >  		err = -EBADFD;
> > +		release_sock(sk);
> >  		goto done;
> >  	}
> >  
> >  	/* Set destination address and psm */
> >  	bacpy(&bt_sk(sk)->dst, dst);
> > +
> > +	release_sock(sk);
> > +
> >  	chan->psm = psm;
> >  	chan->dcid = cid;
> >  
> > @@ -1238,23 +1246,25 @@ 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);
> 
> What are you trying to do here?

I am following the same locking sequence, otherwise lockdep warns.

> > -	__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;
> > -
> 
> Do not remove this line, please.

OK.

Best regards 
Andrei Emeltchenko 

[PATCHv1 14/14] Bluetooth: Remove socket lock check

[Andrei Emeltchenko]

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 bbc1747..e2fc24b 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.9

Re: [PATCHv1 14/14] Bluetooth: Remove socket lock check

[Ulisses Furquim]

Hi Andrei,

On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> 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 bbc1747..e2fc24b 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;
>  }

I don't remember your answer so again are sure about this? Why can we
remove this now? (I may be missing something here).

Regards,

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

Re: [PATCHv1 14/14] Bluetooth: Remove socket lock check

[Andrei Emeltchenko]

Hi Ulisses,

On Tue, Feb 21, 2012 at 03:37:32PM -0200, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Tue, Feb 21, 2012 at 8:55 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > 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 bbc1747..e2fc24b 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;
> >  }
> 
> I don't remember your answer so again are sure about this? Why can we
> remove this now? (I may be missing something here).

l2cap_chan_connect used to return with locked socket and apparently we
were screwed up and did not know is socket locked or not. That's why we used
check to verify that socket was locked.

Best regards 
Andrei Emeltchenko 

Re: [PATCHv1 14/14] Bluetooth: Remove socket lock check

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-02-21 12:55:07 +0200]:

> 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 bbc1747..e2fc24b 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;

Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>

	Gustavo

Re: [PATCHv1 00/14] Bluetooth: Change socket lock to l2cap_chan lock

[Johan Hedberg]

Hi Andrei,

On Tue, Feb 21, 2012, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Changing socket lock to L2CAP chan lock in L2CAP code. Needed for implementing
> protocol above L2CAP without creating sockets.
> 
> Changes:
> 	* PATCHv1: Added extra line (per Gustavo comment)
> 	* RFCv6: Same code but patches 2,3 and 4 from RFCv5 are merged together
> 	following recommendations from review.
> 	* RFCv5: Fixed locking bug in l2cap_data_channel, added locks in 
> 	l2cap_sock_shutdown function, fixed several styles issues.
> 	* RFCv4: Better split patches so they looks more clear and obvious,
> 	taking coments about naming change and delete unused vars. See diff change
> 	from the previous version below:
> 	* 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.
> 
> Andrei Emeltchenko (14):
>   Bluetooth: trivial: Fix long line
>   Bluetooth: Revert to mutexes from RCU list
>   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 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    |  392 +++++++++++++++++++++++++----------------
>  net/bluetooth/l2cap_sock.c    |   34 +++-
>  3 files changed, 281 insertions(+), 156 deletions(-)

Patches 1-5 have been applied to my bluetooth-next tree, so no need to
resend them. Thanks.

Johan