[RFC 0/8] Another step in l2cap_core/sock separation

[RFC 0/8] Another step in l2cap_core/sock separation

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

This patchset does a bit more of clean up in l2cap core, it was inially based
on the two patches Andrei has sent to the list today.

Vinicus, you might want check patch 4/8 especifically.

Comments are welcome.

Gustavo Padovan (8):
  Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c
  Bluetooth: Remove extra l2cap_state_change(BT_CONNECTED)
  Bluetooth: Add l2cap_chan->ops->ready()
  Bluetooth: Use l2cap_chan_ready() in LE path
  Bluetooth: Use chan->state instead of sk->sk_state
  Bluetooth: Move check for backlog size to l2cap_sock.c
  Bluetooth: Create DEFER_SETUP flag in conf_state
  Bluetooth: Add chan->ops->authorize

 include/net/bluetooth/l2cap.h |    4 ++
 net/bluetooth/l2cap_core.c    |  121 +++++++++--------------------------------
 net/bluetooth/l2cap_sock.c    |  111 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 139 insertions(+), 97 deletions(-)

-- 
1.7.10.1

[RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This remove a bit more of socket code from l2cap core, this calls set the
SOCK_ZAPPED and do some clean up depending on the socket state.

Reported-by: Mat Martineau <mathewm@codeaurora.org>
Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   55 ++++++-----------------------------
 net/bluetooth/l2cap_sock.c    |   63 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 73 insertions(+), 46 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 0142257..29f7b06 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -526,6 +526,7 @@ struct l2cap_ops {
 	struct l2cap_chan	*(*new_connection) (void *data);
 	int			(*recv) (void *data, struct sk_buff *skb);
 	void			(*close) (void *data);
+	void			(*finalize) (void *data, int err);
 	void			(*state_change) (void *data, int state);
 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
 					       unsigned long len, int nb);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e31b005..e7a598c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -493,9 +493,7 @@ static void l2cap_chan_add(struct l2cap_conn *conn, struct l2cap_chan *chan)
 
 static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 {
-	struct sock *sk = chan->sk;
 	struct l2cap_conn *conn = chan->conn;
-	struct sock *parent = bt_sk(sk)->parent;
 
 	__clear_chan_timer(chan);
 
@@ -511,21 +509,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);
-
-	if (err)
-		__l2cap_chan_set_err(chan, err);
-
-	if (parent) {
-		bt_accept_unlink(sk);
-		parent->sk_data_ready(parent, 0);
-	} else
-		sk->sk_state_change(sk);
-
-	release_sock(sk);
+	if (chan->ops->finalize)
+		chan->ops->finalize(chan->data, err);
 
 	if (test_bit(CONF_NOT_COMPLETE, &chan->conf_state))
 		return;
@@ -554,25 +539,6 @@ static void l2cap_chan_del(struct l2cap_chan *chan, int err)
 	return;
 }
 
-static void l2cap_chan_cleanup_listen(struct sock *parent)
-{
-	struct sock *sk;
-
-	BT_DBG("parent %p", parent);
-
-	/* Close not yet accepted channels */
-	while ((sk = bt_accept_dequeue(parent, NULL))) {
-		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
-
-		l2cap_chan_lock(chan);
-		__clear_chan_timer(chan);
-		l2cap_chan_close(chan, ECONNRESET);
-		l2cap_chan_unlock(chan);
-
-		chan->ops->close(chan->data);
-	}
-}
-
 void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 {
 	struct l2cap_conn *conn = chan->conn;
@@ -583,12 +549,8 @@ 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);
+		if (chan->ops->finalize)
+			chan->ops->finalize(chan->data, 0);
 		break;
 
 	case BT_CONNECTED:
@@ -630,9 +592,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 		break;
 
 	default:
-		lock_sock(sk);
-		sock_set_flag(sk, SOCK_ZAPPED);
-		release_sock(sk);
+		if (chan->ops->finalize)
+			chan->ops->finalize(chan->data, 0);
 		break;
 	}
 }
@@ -3416,7 +3377,9 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	/* Check if we already have channel with that dcid */
 	if (__l2cap_get_chan_by_dcid(conn, scid)) {
-		sock_set_flag(sk, SOCK_ZAPPED);
+		if (chan->ops->finalize)
+			chan->ops->finalize(chan->data, 0);
+
 		chan->ops->close(chan->data);
 		goto response;
 	}
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 4d36605..0302cb4 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -872,6 +872,25 @@ static int l2cap_sock_release(struct socket *sock)
 	return err;
 }
 
+static void l2cap_sock_cleanup_listen(struct sock *parent)
+{
+	struct sock *sk;
+
+	BT_DBG("parent %p", parent);
+
+	/* Close not yet accepted channels */
+	while ((sk = bt_accept_dequeue(parent, NULL))) {
+		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
+
+		l2cap_chan_lock(chan);
+		__clear_chan_timer(chan);
+		l2cap_chan_close(chan, ECONNRESET);
+		l2cap_chan_unlock(chan);
+
+		l2cap_sock_kill(sk);
+	}
+}
+
 static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
 {
 	struct sock *sk, *parent = data;
@@ -931,6 +950,49 @@ static void l2cap_sock_close_cb(void *data)
 	l2cap_sock_kill(sk);
 }
 
+static void l2cap_sock_finalize_cb(void *data, int err)
+{
+	struct sock *sk = data;
+	struct sock *parent;
+	struct l2cap_chan *chan;
+
+	lock_sock(sk);
+
+	parent = bt_sk(sk)->parent;
+	chan = l2cap_pi(sk)->chan;
+
+	sock_set_flag(sk, SOCK_ZAPPED);
+
+	switch (chan->state) {
+	case BT_OPEN:
+	case BT_BOUND:
+	case BT_CLOSED:
+		break;
+	case BT_LISTEN:
+		l2cap_sock_cleanup_listen(sk);
+		sk->sk_state = BT_CLOSED;
+		chan->state = BT_CLOSED;
+
+		break;
+	default:
+		sk->sk_state = BT_CLOSED;
+		chan->state = BT_CLOSED;
+
+		sk->sk_err = err;
+
+		if (parent) {
+			bt_accept_unlink(sk);
+			parent->sk_data_ready(parent, 0);
+		} else {
+			sk->sk_state_change(sk);
+		}
+
+		break;
+	}
+
+	release_sock(sk);
+}
+
 static void l2cap_sock_state_change_cb(void *data, int state)
 {
 	struct sock *sk = data;
@@ -959,6 +1021,7 @@ static struct l2cap_ops l2cap_chan_ops = {
 	.new_connection	= l2cap_sock_new_connection_cb,
 	.recv		= l2cap_sock_recv_cb,
 	.close		= l2cap_sock_close_cb,
+	.finalize	= l2cap_sock_finalize_cb,
 	.state_change	= l2cap_sock_state_change_cb,
 	.alloc_skb	= l2cap_sock_alloc_skb_cb,
 };
-- 
1.7.10.1

[RFC 2/8] Bluetooth: Remove extra l2cap_state_change(BT_CONNECTED)

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This is already performed inside l2cap_chan_ready(), so we don't need it
here again.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index e7a598c..f77b134 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3608,8 +3608,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
 		set_default_fcs(chan);
 
-		l2cap_state_change(chan, BT_CONNECTED);
-
 		if (chan->mode == L2CAP_MODE_ERTM ||
 		    chan->mode == L2CAP_MODE_STREAMING)
 			err = l2cap_ertm_init(chan);
@@ -3740,7 +3738,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 	if (test_bit(CONF_OUTPUT_DONE, &chan->conf_state)) {
 		set_default_fcs(chan);
 
-		l2cap_state_change(chan, BT_CONNECTED);
 		if (chan->mode == L2CAP_MODE_ERTM ||
 		    chan->mode == L2CAP_MODE_STREAMING)
 			err = l2cap_ertm_init(chan);
-- 
1.7.10.1

[RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This move socket specific code to l2cap_sock.c.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   18 +++---------------
 net/bluetooth/l2cap_sock.c    |   21 +++++++++++++++++++++
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 29f7b06..d1f6819 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -530,6 +530,7 @@ struct l2cap_ops {
 	void			(*state_change) (void *data, int state);
 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
 					       unsigned long len, int nb);
+	void			(*ready) (void *data);
 };
 
 struct l2cap_conn {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index f77b134..7d671ad 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -931,26 +931,14 @@ static void l2cap_send_conn_req(struct l2cap_chan *chan)
 
 static void l2cap_chan_ready(struct l2cap_chan *chan)
 {
-	struct sock *sk = chan->sk;
-	struct sock *parent;
-
-	lock_sock(sk);
-
-	parent = bt_sk(sk)->parent;
-
-	BT_DBG("sk %p, parent %p", sk, parent);
-
 	/* This clears all conf flags, including CONF_NOT_COMPLETE */
 	chan->conf_state = 0;
 	__clear_chan_timer(chan);
 
-	__l2cap_state_change(chan, BT_CONNECTED);
-	sk->sk_state_change(sk);
-
-	if (parent)
-		parent->sk_data_ready(parent, 0);
+	chan->state = BT_CONNECTED;
 
-	release_sock(sk);
+	if (chan->ops->ready)
+		chan->ops->ready(chan->data);
 }
 
 static void l2cap_do_start(struct l2cap_chan *chan)
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0302cb4..8a3620b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1016,6 +1016,26 @@ static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
 	return skb;
 }
 
+static void l2cap_sock_ready_cb(void *data)
+{
+	struct sock *sk = data;
+	struct sock *parent;
+
+	lock_sock(sk);
+
+	parent = bt_sk(sk)->parent;
+
+	BT_DBG("sk %p, parent %p", sk, parent);
+
+	sk->sk_state = BT_CONNECTED;
+	sk->sk_state_change(sk);
+
+	if (parent)
+		parent->sk_data_ready(parent, 0);
+
+	release_sock(sk);
+}
+
 static struct l2cap_ops l2cap_chan_ops = {
 	.name		= "L2CAP Socket Interface",
 	.new_connection	= l2cap_sock_new_connection_cb,
@@ -1024,6 +1044,7 @@ static struct l2cap_ops l2cap_chan_ops = {
 	.finalize	= l2cap_sock_finalize_cb,
 	.state_change	= l2cap_sock_state_change_cb,
 	.alloc_skb	= l2cap_sock_alloc_skb_cb,
+	.ready		= l2cap_sock_ready_cb,
 };
 
 static void l2cap_sock_destruct(struct sock *sk)
-- 
1.7.10.1

[RFC 4/8] Bluetooth: Use l2cap_chan_ready() in LE path

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This replace code in l2cap_le_conn_ready() by a similar code in
l2cap_chan_ready().

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7d671ad..15ee8a5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1177,10 +1177,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	l2cap_chan_add(conn, chan);
 
-	__set_chan_timer(chan, sk->sk_sndtimeo);
-
-	__l2cap_state_change(chan, BT_CONNECTED);
-	parent->sk_data_ready(parent, 0);
+	l2cap_chan_ready(chan);
 
 clean:
 	release_sock(parent);
-- 
1.7.10.1

[RFC 5/8] Bluetooth: Use chan->state instead of sk->sk_state

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

These vars are kept in sync so we can use chan->state here.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 15ee8a5..9416c60 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1444,7 +1444,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 
 	lock_sock(sk);
 
-	switch (sk->sk_state) {
+	switch (chan->state) {
 	case BT_CONNECT:
 	case BT_CONNECT2:
 	case BT_CONFIG:
-- 
1.7.10.1

[RFC 6/8] Bluetooth: Move check for backlog size to l2cap_sock.c

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Remove socket specific code from l2cap_core.c

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |   12 ------------
 net/bluetooth/l2cap_sock.c |    6 ++++++
 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9416c60..46e6fb7 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1156,12 +1156,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 
 	lock_sock(parent);
 
-	/* Check for backlog size */
-	if (sk_acceptq_is_full(parent)) {
-		BT_DBG("backlog full %d", parent->sk_ack_backlog);
-		goto clean;
-	}
-
 	chan = pchan->ops->new_connection(pchan->data);
 	if (!chan)
 		goto clean;
@@ -3348,12 +3342,6 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	result = L2CAP_CR_NO_MEM;
 
-	/* Check for backlog size */
-	if (sk_acceptq_is_full(parent)) {
-		BT_DBG("backlog full %d", parent->sk_ack_backlog);
-		goto response;
-	}
-
 	chan = pchan->ops->new_connection(pchan->data);
 	if (!chan)
 		goto response;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 8a3620b..bc409a2 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -895,6 +895,12 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
 {
 	struct sock *sk, *parent = data;
 
+	/* Check for backlog size */
+	if (sk_acceptq_is_full(parent)) {
+		BT_DBG("backlog full %d", parent->sk_ack_backlog);
+		return NULL;
+	}
+
 	sk = l2cap_sock_alloc(sock_net(parent), NULL, BTPROTO_L2CAP,
 								GFP_ATOMIC);
 	if (!sk)
-- 
1.7.10.1

[RFC 7/8] Bluetooth: Create DEFER_SETUP flag in conf_state

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Remove another socket usage from l2cap_core.c

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   12 ++++++------
 net/bluetooth/l2cap_sock.c    |    9 +++++++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index d1f6819..4f81d08 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -595,6 +595,7 @@ enum {
 	CONF_LOC_CONF_PEND,
 	CONF_REM_CONF_PEND,
 	CONF_NOT_COMPLETE,
+	CONF_DEFER_SETUP,
 };
 
 #define L2CAP_CONF_MAX_CONF_REQ 2
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 46e6fb7..1305b3b 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -569,7 +569,7 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
 			struct l2cap_conn_rsp rsp;
 			__u16 result;
 
-			if (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags))
+			if (test_bit(CONF_DEFER_SETUP, &chan->conf_state))
 				result = L2CAP_CR_SEC_BLOCK;
 			else
 				result = L2CAP_CR_BAD_PSM;
@@ -1056,8 +1056,8 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 
 			if (l2cap_chan_check_security(chan)) {
 				lock_sock(sk);
-				if (test_bit(BT_SK_DEFER_SETUP,
-					     &bt_sk(sk)->flags)) {
+				if (test_bit(CONF_DEFER_SETUP,
+					     &chan->conf_state)) {
 					struct sock *parent = bt_sk(sk)->parent;
 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
 					rsp.status = cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
@@ -3376,7 +3376,7 @@ 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 (test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
+			if (test_bit(CONF_DEFER_SETUP, &chan->conf_state)) {
 				__l2cap_state_change(chan, BT_CONNECT2);
 				result = L2CAP_CR_PEND;
 				status = L2CAP_CS_AUTHOR_PEND;
@@ -5406,8 +5406,8 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 			lock_sock(sk);
 
 			if (!status) {
-				if (test_bit(BT_SK_DEFER_SETUP,
-					     &bt_sk(sk)->flags)) {
+				if (test_bit(CONF_DEFER_SETUP,
+					     &chan->conf_state)) {
 					struct sock *parent = bt_sk(sk)->parent;
 					res = L2CAP_CR_PEND;
 					stat = L2CAP_CS_AUTHOR_PEND;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index bc409a2..dac7b2c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -623,10 +623,15 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
 			break;
 		}
 
-		if (opt)
+		if (opt) {
 			set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
-		else
+			set_bit(CONF_DEFER_SETUP, &chan->conf_state);
+		} else {
 			clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
+			clear_bit(CONF_DEFER_SETUP, &chan->conf_state);
+		}
+
+
 		break;
 
 	case BT_FLUSHABLE:
-- 
1.7.10.1

[RFC 8/8] Bluetooth: Add chan->ops->authorize

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

When DEFER_SETUP is set authorize() will trigger an authorization request
to the userspace.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   14 ++++++--------
 net/bluetooth/l2cap_sock.c    |   12 ++++++++++++
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 4f81d08..94e375a 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -531,6 +531,7 @@ struct l2cap_ops {
 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
 					       unsigned long len, int nb);
 	void			(*ready) (void *data);
+	void			(*authorize) (void *data);
 };
 
 struct l2cap_conn {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 1305b3b..22ba699 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1058,12 +1058,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
 				lock_sock(sk);
 				if (test_bit(CONF_DEFER_SETUP,
 					     &chan->conf_state)) {
-					struct sock *parent = bt_sk(sk)->parent;
 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
 					rsp.status = cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
-					if (parent)
-						parent->sk_data_ready(parent, 0);
-
+					if(chan->ops->authorize)
+						chan->ops->authorize(chan->data);
 				} else {
 					__l2cap_state_change(chan, BT_CONFIG);
 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
@@ -3380,7 +3378,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 				__l2cap_state_change(chan, BT_CONNECT2);
 				result = L2CAP_CR_PEND;
 				status = L2CAP_CS_AUTHOR_PEND;
-				parent->sk_data_ready(parent, 0);
+				if(chan->ops->authorize)
+					chan->ops->authorize(chan->data);
 			} else {
 				__l2cap_state_change(chan, BT_CONFIG);
 				result = L2CAP_CR_SUCCESS;
@@ -5408,11 +5407,10 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 			if (!status) {
 				if (test_bit(CONF_DEFER_SETUP,
 					     &chan->conf_state)) {
-					struct sock *parent = bt_sk(sk)->parent;
 					res = L2CAP_CR_PEND;
 					stat = L2CAP_CS_AUTHOR_PEND;
-					if (parent)
-						parent->sk_data_ready(parent, 0);
+					if(chan->ops->authorize)
+						chan->ops->authorize(chan->data);
 				} else {
 					__l2cap_state_change(chan, BT_CONFIG);
 					res = L2CAP_CR_SUCCESS;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index dac7b2c..d2a81f1 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1047,6 +1047,17 @@ static void l2cap_sock_ready_cb(void *data)
 	release_sock(sk);
 }
 
+static void l2cap_sock_authorize_cb(void *data)
+{
+	struct sock *sk = data;
+	struct sock *parent;
+
+	parent = bt_sk(sk)->parent;
+
+	if (parent)
+		parent->sk_data_ready(parent, 0);
+}
+
 static struct l2cap_ops l2cap_chan_ops = {
 	.name		= "L2CAP Socket Interface",
 	.new_connection	= l2cap_sock_new_connection_cb,
@@ -1056,6 +1067,7 @@ static struct l2cap_ops l2cap_chan_ops = {
 	.state_change	= l2cap_sock_state_change_cb,
 	.alloc_skb	= l2cap_sock_alloc_skb_cb,
 	.ready		= l2cap_sock_ready_cb,
+	.authorize	= l2cap_sock_authorize_cb,
 };
 
 static void l2cap_sock_destruct(struct sock *sk)
-- 
1.7.10.1

[RFC 1/3] Bluetooth: check for already existent channel before create new one

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Move this check to before the channel time creation simplifies the code
and avoid memory allocation if the channel already exist.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 22ba699..c9de4f5 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3340,21 +3340,16 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 
 	result = L2CAP_CR_NO_MEM;
 
+	/* Check if we already have channel with that dcid */
+	if (__l2cap_get_chan_by_dcid(conn, scid))
+		goto response;
+
 	chan = pchan->ops->new_connection(pchan->data);
 	if (!chan)
 		goto response;
 
 	sk = chan->sk;
 
-	/* Check if we already have channel with that dcid */
-	if (__l2cap_get_chan_by_dcid(conn, scid)) {
-		if (chan->ops->finalize)
-			chan->ops->finalize(chan->data, 0);
-
-		chan->ops->close(chan->data);
-		goto response;
-	}
-
 	hci_conn_hold(conn->hcon);
 
 	bacpy(&bt_sk(sk)->src, conn->src);
-- 
1.7.10.1

[RFC 2/3] Bluetooth: Move bt_accept_enqueue() call to l2cap_sock.c

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

bt_accept_enqueue() can be easily placed at the end of
l2cap_sock_new_connection_cb().

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |    4 ----
 net/bluetooth/l2cap_sock.c |    2 ++
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index c9de4f5..5ff294f9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1165,8 +1165,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	bacpy(&bt_sk(sk)->src, conn->src);
 	bacpy(&bt_sk(sk)->dst, conn->dst);
 
-	bt_accept_enqueue(parent, sk);
-
 	l2cap_chan_add(conn, chan);
 
 	l2cap_chan_ready(chan);
@@ -3357,8 +3355,6 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	chan->psm  = psm;
 	chan->dcid = scid;
 
-	bt_accept_enqueue(parent, sk);
-
 	__l2cap_chan_add(conn, chan);
 
 	dcid = chan->scid;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index d2a81f1..f4c3e2c 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -915,6 +915,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
 
 	l2cap_sock_init(sk, parent);
 
+	bt_accept_enqueue(parent, sk);
+
 	return l2cap_pi(sk)->chan;
 }
 
-- 
1.7.10.1

[RFC 3/3] Blueooth: Remove parent socket usage from l2cap_core.c

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

We can lock the parent lock only inside the new_connection() call, then we
just use the l2cap_chan_lock() in core code.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |   16 ++++++----------
 net/bluetooth/l2cap_sock.c |    9 ++++++++-
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 5ff294f9..314ce74 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1139,7 +1139,7 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
 
 static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 {
-	struct sock *parent, *sk;
+	struct sock *sk;
 	struct l2cap_chan *chan, *pchan;
 
 	BT_DBG("");
@@ -1150,9 +1150,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	if (!pchan)
 		return;
 
-	parent = pchan->sk;
-
-	lock_sock(parent);
+	l2cap_chan_lock(pchan);
 
 	chan = pchan->ops->new_connection(pchan->data);
 	if (!chan)
@@ -1170,7 +1168,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
 	l2cap_chan_ready(chan);
 
 clean:
-	release_sock(parent);
+	l2cap_chan_unlock(pchan);
 }
 
 static void l2cap_conn_ready(struct l2cap_conn *conn)
@@ -3308,7 +3306,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
 	struct l2cap_conn_rsp rsp;
 	struct l2cap_chan *chan = NULL, *pchan;
-	struct sock *parent, *sk = NULL;
+	struct sock *sk = NULL;
 	int result, status = L2CAP_CS_NO_INFO;
 
 	u16 dcid = 0, scid = __le16_to_cpu(req->scid);
@@ -3323,10 +3321,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		goto sendresp;
 	}
 
-	parent = pchan->sk;
-
 	mutex_lock(&conn->chan_lock);
-	lock_sock(parent);
+	l2cap_chan_lock(pchan);
 
 	/* Check if the ACL is secure enough (if not SDP) */
 	if (psm != cpu_to_le16(0x0001) &&
@@ -3388,7 +3384,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 	}
 
 response:
-	release_sock(parent);
+	l2cap_chan_unlock(pchan);
 	mutex_unlock(&conn->chan_lock);
 
 sendresp:
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index f4c3e2c..be3e934 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -900,16 +900,21 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
 {
 	struct sock *sk, *parent = data;
 
+	lock_sock(parent);
+
 	/* Check for backlog size */
 	if (sk_acceptq_is_full(parent)) {
 		BT_DBG("backlog full %d", parent->sk_ack_backlog);
+		release_sock(parent);
 		return NULL;
 	}
 
 	sk = l2cap_sock_alloc(sock_net(parent), NULL, BTPROTO_L2CAP,
 								GFP_ATOMIC);
-	if (!sk)
+	if (!sk) {
+		release_sock(parent);
 		return NULL;
+	}
 
 	bt_sock_reclassify_lock(sk, BTPROTO_L2CAP);
 
@@ -917,6 +922,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
 
 	bt_accept_enqueue(parent, sk);
 
+	release_sock(parent);
+
 	return l2cap_pi(sk)->chan;
 }
 
-- 
1.7.10.1

Re: [RFC 0/8] Another step in l2cap_core/sock separation

[Gustavo Padovan]

* Gustavo Padovan <gustavo@padovan.org> [2012-05-23 22:12:36 -0300]:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> This patchset does a bit more of clean up in l2cap core, it was inially based
> on the two patches Andrei has sent to the list today.

For those interested in this work, a good measure of how good we are is:

$ egrep -rn "\<sk\>" net/bluetooth/l2cap_core.c | wc -l

Before this patchset of 11 patches we were counting 113 here, now we down to 77
occurrences of "sk" in the file with bt_sk(sk)->{src,dst} and socket locks
being the biggest users.

	Gustavo

Re: [RFC 3/3] Blueooth: Remove parent socket usage from l2cap_core.c

[Gustavo Padovan]

This is Bluetooth: and not Blueooth:

* Gustavo Padovan <gustavo@padovan.org> [2012-05-24 03:02:54 -0300]:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> We can lock the parent lock only inside the new_connection() call, then we
> just use the l2cap_chan_lock() in core code.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/l2cap_core.c |   16 ++++++----------
>  net/bluetooth/l2cap_sock.c |    9 ++++++++-
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 5ff294f9..314ce74 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1139,7 +1139,7 @@ static struct l2cap_chan *l2cap_global_chan_by_scid(int state, u16 cid,
>  
>  static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  {
> -	struct sock *parent, *sk;
> +	struct sock *sk;
>  	struct l2cap_chan *chan, *pchan;
>  
>  	BT_DBG("");
> @@ -1150,9 +1150,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  	if (!pchan)
>  		return;
>  
> -	parent = pchan->sk;
> -
> -	lock_sock(parent);
> +	l2cap_chan_lock(pchan);
>  
>  	chan = pchan->ops->new_connection(pchan->data);
>  	if (!chan)
> @@ -1170,7 +1168,7 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  	l2cap_chan_ready(chan);
>  
>  clean:
> -	release_sock(parent);
> +	l2cap_chan_unlock(pchan);
>  }
>  
>  static void l2cap_conn_ready(struct l2cap_conn *conn)
> @@ -3308,7 +3306,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  	struct l2cap_conn_req *req = (struct l2cap_conn_req *) data;
>  	struct l2cap_conn_rsp rsp;
>  	struct l2cap_chan *chan = NULL, *pchan;
> -	struct sock *parent, *sk = NULL;
> +	struct sock *sk = NULL;
>  	int result, status = L2CAP_CS_NO_INFO;
>  
>  	u16 dcid = 0, scid = __le16_to_cpu(req->scid);
> @@ -3323,10 +3321,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  		goto sendresp;
>  	}
>  
> -	parent = pchan->sk;
> -
>  	mutex_lock(&conn->chan_lock);
> -	lock_sock(parent);
> +	l2cap_chan_lock(pchan);
>  
>  	/* Check if the ACL is secure enough (if not SDP) */
>  	if (psm != cpu_to_le16(0x0001) &&
> @@ -3388,7 +3384,7 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  	}
>  
>  response:
> -	release_sock(parent);
> +	l2cap_chan_unlock(pchan);
>  	mutex_unlock(&conn->chan_lock);
>  
>  sendresp:
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index f4c3e2c..be3e934 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -900,16 +900,21 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>  {
>  	struct sock *sk, *parent = data;
>  
> +	lock_sock(parent);
> +
>  	/* Check for backlog size */
>  	if (sk_acceptq_is_full(parent)) {
>  		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> +		release_sock(parent);
>  		return NULL;
>  	}
>  
>  	sk = l2cap_sock_alloc(sock_net(parent), NULL, BTPROTO_L2CAP,
>  								GFP_ATOMIC);
> -	if (!sk)
> +	if (!sk) {
> +		release_sock(parent);
>  		return NULL;
> +	}
>  
>  	bt_sock_reclassify_lock(sk, BTPROTO_L2CAP);
>  
> @@ -917,6 +922,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>  
>  	bt_accept_enqueue(parent, sk);
>  
> +	release_sock(parent);
> +
>  	return l2cap_pi(sk)->chan;
>  }
>  
> -- 
> 1.7.10.1
> 

	Gustavo

[RFC 1/2] Bluetooth: Create chan->ops->set_err()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Remove more sk occurrences from l2cap_core by enabling
us to set sk_err via chan ops.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/net/bluetooth/l2cap.h |    1 +
 net/bluetooth/l2cap_core.c    |   34 ++++++++++------------------------
 net/bluetooth/l2cap_sock.c    |   10 ++++++++++
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 94e375a..e734572 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -532,6 +532,7 @@ struct l2cap_ops {
 					       unsigned long len, int nb);
 	void			(*ready) (void *data);
 	void			(*authorize) (void *data);
+	void			(*set_err) (void *data, int err);
 };
 
 struct l2cap_conn {
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 314ce74..0d22695 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -192,22 +192,6 @@ 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 __set_retrans_timer(struct l2cap_chan *chan)
 {
 	if (!delayed_work_pending(&chan->monitor_timer) &&
@@ -989,7 +973,6 @@ 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)
@@ -1006,10 +989,9 @@ 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_state_change(chan, BT_DISCONN);
+	if(chan->ops->set_err)
+		chan->ops->set_err(chan->data, err);
 }
 
 /* ---- L2CAP connections ---- */
@@ -1220,8 +1202,11 @@ 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) {
-		if (test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
-			__l2cap_chan_set_err(chan, err);
+		if (!test_bit(FLAG_FORCE_RELIABLE, &chan->flags))
+			continue;
+
+		if(chan->ops->set_err)
+			chan->ops->set_err(chan->data, err);
 	}
 
 	mutex_unlock(&conn->chan_lock);
@@ -3682,7 +3667,8 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		}
 
 	default:
-		l2cap_chan_set_err(chan, ECONNRESET);
+		if(chan->ops->set_err)
+			chan->ops->set_err(chan->data, err);
 
 		__set_chan_timer(chan, L2CAP_DISC_REJ_TIMEOUT);
 		l2cap_send_disconn_req(conn, chan, ECONNRESET);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index be3e934..0a1a827 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1067,6 +1067,15 @@ static void l2cap_sock_authorize_cb(void *data)
 		parent->sk_data_ready(parent, 0);
 }
 
+static void l2cap_sock_set_err_cb(void *data, int err)
+{
+	struct sock *sk = data;
+
+	lock_sock(sk);
+	sk->sk_err = err;
+	release_sock(sk);
+}
+
 static struct l2cap_ops l2cap_chan_ops = {
 	.name		= "L2CAP Socket Interface",
 	.new_connection	= l2cap_sock_new_connection_cb,
@@ -1077,6 +1086,7 @@ static struct l2cap_ops l2cap_chan_ops = {
 	.alloc_skb	= l2cap_sock_alloc_skb_cb,
 	.ready		= l2cap_sock_ready_cb,
 	.authorize	= l2cap_sock_authorize_cb,
+	.set_err	= l2cap_sock_set_err_cb,
 };
 
 static void l2cap_sock_destruct(struct sock *sk)
-- 
1.7.10.1

[RFC 2/2] Bluetooth: Remove last usage of sk->sk_state_change()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

If the is not oriented we should call sk_state_change() just after set the
channel state to BT_CONNECTED.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 net/bluetooth/l2cap_core.c |    6 +-----
 net/bluetooth/l2cap_sock.c |    4 ++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 0d22695..6137b48 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -1176,12 +1176,8 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
 				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);
+			l2cap_state_change(chan, BT_CONNECTED);
 
 		} else if (chan->state == BT_CONNECT)
 			l2cap_do_start(chan);
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0a1a827..9fd9412 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1018,6 +1018,10 @@ static void l2cap_sock_state_change_cb(void *data, int state)
 	struct sock *sk = data;
 
 	sk->sk_state = state;
+
+	if (state == BT_CONNECTED &&
+	    l2cap_pi(sk)->chan->chan_type != L2CAP_CHAN_CONN_ORIENTED)
+		sk->sk_state_change(sk);
 }
 
 static struct sk_buff *l2cap_sock_alloc_skb_cb(struct l2cap_chan *chan,
-- 
1.7.10.1

Re: [RFC 1/2] Bluetooth: Create chan->ops->set_err()

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, May 24, 2012 at 04:00:16AM -0300, Gustavo Padovan wrote:
> -	lock_sock(sk);
> -	__l2cap_state_change(chan, BT_DISCONN);
> -	__l2cap_chan_set_err(chan, err);
> -	release_sock(sk);
> +	l2cap_state_change(chan, BT_DISCONN);
> +	if(chan->ops->set_err)
> +		chan->ops->set_err(chan->data, err);

I do not know can it be done somehow better, currently we lock and unlock
sockets for each operation.

Best regards 
Andrei Emeltchenko 

Re: [RFC 2/2] Bluetooth: Remove last usage of sk->sk_state_change()

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, May 24, 2012 at 04:00:17AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> If the is not oriented we should call sk_state_change() just after set the
> channel state to BT_CONNECTED.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/l2cap_core.c |    6 +-----
>  net/bluetooth/l2cap_sock.c |    4 ++++
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 0d22695..6137b48 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1176,12 +1176,8 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
>  				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);
> +			l2cap_state_change(chan, BT_CONNECTED);
>  
>  		} else if (chan->state == BT_CONNECT)
>  			l2cap_do_start(chan);
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 0a1a827..9fd9412 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1018,6 +1018,10 @@ static void l2cap_sock_state_change_cb(void *data, int state)
>  	struct sock *sk = data;
>  
>  	sk->sk_state = state;
> +
> +	if (state == BT_CONNECTED &&
> +	    l2cap_pi(sk)->chan->chan_type != L2CAP_CHAN_CONN_ORIENTED)
> +		sk->sk_state_change(sk);

Somehow I feel that there is too much logic here. BTW: can we use
sk_state_change for every state_change callback?

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/3] Blueooth: Remove parent socket usage from l2cap_core.c

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, May 24, 2012 at 03:02:54AM -0300, Gustavo Padovan wrote:
> @@ -900,16 +900,21 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>  {
>  	struct sock *sk, *parent = data;
>  
> +	lock_sock(parent);
> +
>  	/* Check for backlog size */
>  	if (sk_acceptq_is_full(parent)) {
>  		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> +		release_sock(parent);
>  		return NULL;
>  	}

You seems to forget to post patch adding sk_acceptq_is_full to
l2cap_sock_new_connection_cb

Best regards 
Andrei Emeltchenko 

Re: [RFC 2/3] Bluetooth: Move bt_accept_enqueue() call to l2cap_sock.c

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, May 24, 2012 at 03:02:53AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> bt_accept_enqueue() can be easily placed at the end of
> l2cap_sock_new_connection_cb().
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/l2cap_core.c |    4 ----
>  net/bluetooth/l2cap_sock.c |    2 ++
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c9de4f5..5ff294f9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1165,8 +1165,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  	bacpy(&bt_sk(sk)->src, conn->src);
>  	bacpy(&bt_sk(sk)->dst, conn->dst);
>  
> -	bt_accept_enqueue(parent, sk);
> -

Shall we also move to l2cap_sock_new_connection_cb bacpys above?

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/3] Bluetooth: check for already existent channel before create new one

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, May 24, 2012 at 03:02:52AM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Move this check to before the channel time creation simplifies the code
> and avoid memory allocation if the channel already exist.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/l2cap_core.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 22ba699..c9de4f5 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3340,21 +3340,16 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  
>  	result = L2CAP_CR_NO_MEM;
>  
> +	/* Check if we already have channel with that dcid */
> +	if (__l2cap_get_chan_by_dcid(conn, scid))
> +		goto response;
> +
>  	chan = pchan->ops->new_connection(pchan->data);
>  	if (!chan)
>  		goto response;
>  
>  	sk = chan->sk;
>  
> -	/* Check if we already have channel with that dcid */
> -	if (__l2cap_get_chan_by_dcid(conn, scid)) {
> -		if (chan->ops->finalize)
> -			chan->ops->finalize(chan->data, 0);

What is finalize? Is this the first patch? We do not have finalize yet.

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:37PM -0300, Gustavo Padovan wrote:
>  void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>  {
>  	struct l2cap_conn *conn = chan->conn;
> @@ -583,12 +549,8 @@ 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);
> +		if (chan->ops->finalize)
> +			chan->ops->finalize(chan->data, 0);

Looks like you are handling all cases so this can be put outside of
switch.

>  		break;
>  
>  	case BT_CONNECTED:
> @@ -630,9 +592,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
>  		break;
>  
>  	default:
> -		lock_sock(sk);
> -		sock_set_flag(sk, SOCK_ZAPPED);
> -		release_sock(sk);
> +		if (chan->ops->finalize)
> +			chan->ops->finalize(chan->data, 0);
>  		break;
>  	}
>  }
> @@ -3416,7 +3377,9 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  
>  	/* Check if we already have channel with that dcid */
>  	if (__l2cap_get_chan_by_dcid(conn, scid)) {
> -		sock_set_flag(sk, SOCK_ZAPPED);
> +		if (chan->ops->finalize)
> +			chan->ops->finalize(chan->data, 0);
> +
>  		chan->ops->close(chan->data);
>  		goto response;
>  	}
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 4d36605..0302cb4 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -872,6 +872,25 @@ static int l2cap_sock_release(struct socket *sock)
>  	return err;
>  }
>  
> +static void l2cap_sock_cleanup_listen(struct sock *parent)
> +{
> +	struct sock *sk;
> +
> +	BT_DBG("parent %p", parent);
> +
> +	/* Close not yet accepted channels */
> +	while ((sk = bt_accept_dequeue(parent, NULL))) {
> +		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> +
> +		l2cap_chan_lock(chan);
> +		__clear_chan_timer(chan);
> +		l2cap_chan_close(chan, ECONNRESET);
> +		l2cap_chan_unlock(chan);
> +
> +		l2cap_sock_kill(sk);
> +	}
> +}
> +
>  static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>  {
>  	struct sock *sk, *parent = data;
> @@ -931,6 +950,49 @@ static void l2cap_sock_close_cb(void *data)
>  	l2cap_sock_kill(sk);
>  }
>  
> +static void l2cap_sock_finalize_cb(void *data, int err)
> +{
> +	struct sock *sk = data;

Why we cannot just use l2cap_sock_finalize_cb(struct sock *sk, int err)
what is the reason for extra assignment?

I also think finalize is not good name since it does not mean what is
actually finalized.

Best regards 
Andrei Emeltchenko 

> +	struct sock *parent;
> +	struct l2cap_chan *chan;
> +
> +	lock_sock(sk);
> +
> +	parent = bt_sk(sk)->parent;
> +	chan = l2cap_pi(sk)->chan;
> +
> +	sock_set_flag(sk, SOCK_ZAPPED);
> +
> +	switch (chan->state) {
> +	case BT_OPEN:
> +	case BT_BOUND:
> +	case BT_CLOSED:
> +		break;
> +	case BT_LISTEN:
> +		l2cap_sock_cleanup_listen(sk);
> +		sk->sk_state = BT_CLOSED;
> +		chan->state = BT_CLOSED;
> +
> +		break;
> +	default:
> +		sk->sk_state = BT_CLOSED;
> +		chan->state = BT_CLOSED;
> +
> +		sk->sk_err = err;
> +
> +		if (parent) {
> +			bt_accept_unlink(sk);
> +			parent->sk_data_ready(parent, 0);
> +		} else {
> +			sk->sk_state_change(sk);
> +		}
> +
> +		break;
> +	}
> +
> +	release_sock(sk);
> +}
> +

Re: [RFC 2/8] Bluetooth: Remove extra l2cap_state_change(BT_CONNECTED)

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:38PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> This is already performed inside l2cap_chan_ready(), so we don't need it
> here again.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Acked-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com> 

> ---
>  net/bluetooth/l2cap_core.c |    3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index e7a598c..f77b134 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3608,8 +3608,6 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
>  		set_default_fcs(chan);
>  
> -		l2cap_state_change(chan, BT_CONNECTED);
> -
>  		if (chan->mode == L2CAP_MODE_ERTM ||
>  		    chan->mode == L2CAP_MODE_STREAMING)
>  			err = l2cap_ertm_init(chan);
> @@ -3740,7 +3738,6 @@ static inline int l2cap_config_rsp(struct l2cap_conn *conn, struct l2cap_cmd_hdr
>  	if (test_bit(CONF_OUTPUT_DONE, &chan->conf_state)) {
>  		set_default_fcs(chan);
>  
> -		l2cap_state_change(chan, BT_CONNECTED);
>  		if (chan->mode == L2CAP_MODE_ERTM ||
>  		    chan->mode == L2CAP_MODE_STREAMING)
>  			err = l2cap_ertm_init(chan);
> -- 
> 1.7.10.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:39PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> This move socket specific code to l2cap_sock.c.
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  include/net/bluetooth/l2cap.h |    1 +
>  net/bluetooth/l2cap_core.c    |   18 +++---------------
>  net/bluetooth/l2cap_sock.c    |   21 +++++++++++++++++++++
>  3 files changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 29f7b06..d1f6819 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -530,6 +530,7 @@ struct l2cap_ops {
>  	void			(*state_change) (void *data, int state);
>  	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
>  					       unsigned long len, int nb);
> +	void			(*ready) (void *data);

Again, why void *data ?

Best regards 
Andrei Emeltchenko 

Re: [RFC 6/8] Bluetooth: Move check for backlog size to l2cap_sock.c

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:42PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Remove socket specific code from l2cap_core.c
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
>  net/bluetooth/l2cap_core.c |   12 ------------
>  net/bluetooth/l2cap_sock.c |    6 ++++++
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9416c60..46e6fb7 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1156,12 +1156,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
>  
>  	lock_sock(parent);
>  
> -	/* Check for backlog size */
> -	if (sk_acceptq_is_full(parent)) {
> -		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> -		goto clean;
> -	}
> -
>  	chan = pchan->ops->new_connection(pchan->data);
>  	if (!chan)
>  		goto clean;
> @@ -3348,12 +3342,6 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
>  
>  	result = L2CAP_CR_NO_MEM;
>  
> -	/* Check for backlog size */
> -	if (sk_acceptq_is_full(parent)) {
> -		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> -		goto response;
> -	}
> -
>  	chan = pchan->ops->new_connection(pchan->data);
>  	if (!chan)
>  		goto response;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 8a3620b..bc409a2 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -895,6 +895,12 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>  {
>  	struct sock *sk, *parent = data;
>  
> +	/* Check for backlog size */
> +	if (sk_acceptq_is_full(parent)) {
> +		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> +		return NULL;
> +	}
> +

Ok I see where comes from this change. I feel that this can be merged with
some other patch removing parent sk.

Best regards 
Andrei Emeltchenko 

Re: [RFC 7/8] Bluetooth: Create DEFER_SETUP flag in conf_state

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:43PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Remove another socket usage from l2cap_core.c

Good idea.

...
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -623,10 +623,15 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
>  			break;
>  		}
>  
> -		if (opt)
> +		if (opt) {
>  			set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> -		else
> +			set_bit(CONF_DEFER_SETUP, &chan->conf_state);
> +		} else {
>  			clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> +			clear_bit(CONF_DEFER_SETUP, &chan->conf_state);

Do we now have 2 similar flags?

> +		}
> +
> +

Remove extra line

Best regards 
Andrei Emeltchenko 

Re: [RFC 8/8] Bluetooth: Add chan->ops->authorize

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:44PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> When DEFER_SETUP is set authorize() will trigger an authorization request
> to the userspace.

Looks good.

> +static void l2cap_sock_authorize_cb(void *data)
> +{
> +	struct sock *sk = data;
> +	struct sock *parent;
> +
> +	parent = bt_sk(sk)->parent;

But why not:

l2cap_sock_authorize_cb(struct sock *sk)
{
	struct sock *parent = bt_sk(sk)->parent;

Best regards 
Andrei Emeltchenko 

Re: [RFC 0/8] Another step in l2cap_core/sock separation

[Andrei Emeltchenko]

Hi Gustavo,

On Wed, May 23, 2012 at 10:12:36PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Hi,
> 
> This patchset does a bit more of clean up in l2cap core, it was inially based
> on the two patches Andrei has sent to the list today.

Can this RFC be merged with other 2 RFC since otherwise I got lost. 

I also found that my mail reader indent your RFC in a strange way so I
thought that they are in the same series.

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Andrei Emeltchenko]

Hi,

On Thu, May 24, 2012 at 12:39:59PM +0300, Andrei Emeltchenko wrote:
> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:39PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > This move socket specific code to l2cap_sock.c.
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  include/net/bluetooth/l2cap.h |    1 +
> >  net/bluetooth/l2cap_core.c    |   18 +++---------------
> >  net/bluetooth/l2cap_sock.c    |   21 +++++++++++++++++++++
> >  3 files changed, 25 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> > index 29f7b06..d1f6819 100644
> > --- a/include/net/bluetooth/l2cap.h
> > +++ b/include/net/bluetooth/l2cap.h
> > @@ -530,6 +530,7 @@ struct l2cap_ops {
> >  	void			(*state_change) (void *data, int state);
> >  	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
> >  					       unsigned long len, int nb);
> > +	void			(*ready) (void *data);
> 
> Again, why void *data ?

I mean here that for fixed channels we do not need this function at this
point since initialization is different.

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Ulisses Furquim]

Hi Andrei,

On Thu, May 24, 2012 at 7:23 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi,
>
> On Thu, May 24, 2012 at 12:39:59PM +0300, Andrei Emeltchenko wrote:
>> Hi Gustavo,
>>
>> On Wed, May 23, 2012 at 10:12:39PM -0300, Gustavo Padovan wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> >
>> > This move socket specific code to l2cap_sock.c.
>> >
>> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>> > ---
>> >  include/net/bluetooth/l2cap.h |    1 +
>> >  net/bluetooth/l2cap_core.c    |   18 +++---------------
>> >  net/bluetooth/l2cap_sock.c    |   21 +++++++++++++++++++++
>> >  3 files changed, 25 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
>> > index 29f7b06..d1f6819 100644
>> > --- a/include/net/bluetooth/l2cap.h
>> > +++ b/include/net/bluetooth/l2cap.h
>> > @@ -530,6 +530,7 @@ struct l2cap_ops {
>> >     void                    (*state_change) (void *data, int state);
>> >     struct sk_buff          *(*alloc_skb) (struct l2cap_chan *chan,
>> >                                            unsigned long len, int nb);
>> > +   void                    (*ready) (void *data);
>>
>> Again, why void *data ?
>
> I mean here that for fixed channels we do not need this function at this
> point since initialization is different.

So? What do you mean? This needs to be generic, I think. It's an
abstraction after all.

Regards,

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

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Andrei Emeltchenko]

Hi Ulisses,

On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
> >> > +   void                    (*ready) (void *data);
> >>
> >> Again, why void *data ?
> >
> > I mean here that for fixed channels we do not need this function at this
> > point since initialization is different.
> 
> So? What do you mean? This needs to be generic, I think. It's an
> abstraction after all.

Fixed channels do not have configuration phase, they are created
CONNECTED (at least A2MP).

Best regards 
Andrei Emeltchenko 

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Ulisses Furquim]

Hi Andrei,

On Thu, May 24, 2012 at 8:30 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@gmail.com> wrote:
> Hi Ulisses,
>
> On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
>> >> > +   void                    (*ready) (void *data);
>> >>
>> >> Again, why void *data ?
>> >
>> > I mean here that for fixed channels we do not need this function at this
>> > point since initialization is different.
>>
>> So? What do you mean? This needs to be generic, I think. It's an
>> abstraction after all.
>
> Fixed channels do not have configuration phase, they are created
> CONNECTED (at least A2MP).

And your proposal is?

Regards,

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

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Andrei Emeltchenko]

Hi Ulisses,

On Thu, May 24, 2012 at 08:31:15AM -0300, Ulisses Furquim wrote:
> Hi Andrei,
> 
> On Thu, May 24, 2012 at 8:30 AM, Andrei Emeltchenko
> <andrei.emeltchenko.news@gmail.com> wrote:
> > Hi Ulisses,
> >
> > On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
> >> >> > +   void                    (*ready) (void *data);
> >> >>
> >> >> Again, why void *data ?
> >> >
> >> > I mean here that for fixed channels we do not need this function at this
> >> > point since initialization is different.
> >>
> >> So? What do you mean? This needs to be generic, I think. It's an
> >> abstraction after all.
> >
> > Fixed channels do not have configuration phase, they are created
> > CONNECTED (at least A2MP).
> 
> And your proposal is?

Fox fixed channels ready is not defined (at least now) so we can just use
exact type, see my patch in a minute.

Best regards 
Andrei Emeltchenko 

Re: [RFC 0/8] Another step in l2cap_core/sock separation

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 13:02:29 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:36PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Hi,
> > 
> > This patchset does a bit more of clean up in l2cap core, it was inially based
> > on the two patches Andrei has sent to the list today.
> 
> Can this RFC be merged with other 2 RFC since otherwise I got lost. 
> 
> I also found that my mail reader indent your RFC in a strange way so I
> thought that they are in the same series.

And actually they the same series, I just thought twice that I was done with
this patchset, but then I just found more work to do. 

	Gustavo

Re: [RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:35:59 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:37PM -0300, Gustavo Padovan wrote:
> >  void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >  {
> >  	struct l2cap_conn *conn = chan->conn;
> > @@ -583,12 +549,8 @@ 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);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> 
> Looks like you are handling all cases so this can be put outside of
> switch.

No, I'm not, look to the BT_CONNECTED/BT_CONFIG option.

> 
> >  		break;
> >  
> >  	case BT_CONNECTED:
> > @@ -630,9 +592,8 @@ void l2cap_chan_close(struct l2cap_chan *chan, int reason)
> >  		break;
> >  
> >  	default:
> > -		lock_sock(sk);
> > -		sock_set_flag(sk, SOCK_ZAPPED);
> > -		release_sock(sk);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> >  		break;
> >  	}
> >  }
> > @@ -3416,7 +3377,9 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
> >  
> >  	/* Check if we already have channel with that dcid */
> >  	if (__l2cap_get_chan_by_dcid(conn, scid)) {
> > -		sock_set_flag(sk, SOCK_ZAPPED);
> > +		if (chan->ops->finalize)
> > +			chan->ops->finalize(chan->data, 0);
> > +
> >  		chan->ops->close(chan->data);
> >  		goto response;
> >  	}
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 4d36605..0302cb4 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -872,6 +872,25 @@ static int l2cap_sock_release(struct socket *sock)
> >  	return err;
> >  }
> >  
> > +static void l2cap_sock_cleanup_listen(struct sock *parent)
> > +{
> > +	struct sock *sk;
> > +
> > +	BT_DBG("parent %p", parent);
> > +
> > +	/* Close not yet accepted channels */
> > +	while ((sk = bt_accept_dequeue(parent, NULL))) {
> > +		struct l2cap_chan *chan = l2cap_pi(sk)->chan;
> > +
> > +		l2cap_chan_lock(chan);
> > +		__clear_chan_timer(chan);
> > +		l2cap_chan_close(chan, ECONNRESET);
> > +		l2cap_chan_unlock(chan);
> > +
> > +		l2cap_sock_kill(sk);
> > +	}
> > +}
> > +
> >  static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
> >  {
> >  	struct sock *sk, *parent = data;
> > @@ -931,6 +950,49 @@ static void l2cap_sock_close_cb(void *data)
> >  	l2cap_sock_kill(sk);
> >  }
> >  
> > +static void l2cap_sock_finalize_cb(void *data, int err)
> > +{
> > +	struct sock *sk = data;
> 
> Why we cannot just use l2cap_sock_finalize_cb(struct sock *sk, int err)
> what is the reason for extra assignment?

Because we want to be generic here, we don't know who else will use finalize()
so we need to keep a void * here.

> 
> I also think finalize is not good name since it does not mean what is
> actually finalized.

And it is not, I failed to come with a better name here. The initial one was
delete, even worse.

	Gustavo

Re: [RFC 6/8] Bluetooth: Move check for backlog size to l2cap_sock.c

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:45:48 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:42PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Remove socket specific code from l2cap_core.c
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  net/bluetooth/l2cap_core.c |   12 ------------
> >  net/bluetooth/l2cap_sock.c |    6 ++++++
> >  2 files changed, 6 insertions(+), 12 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 9416c60..46e6fb7 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1156,12 +1156,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> >  
> >  	lock_sock(parent);
> >  
> > -	/* Check for backlog size */
> > -	if (sk_acceptq_is_full(parent)) {
> > -		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> > -		goto clean;
> > -	}
> > -
> >  	chan = pchan->ops->new_connection(pchan->data);
> >  	if (!chan)
> >  		goto clean;
> > @@ -3348,12 +3342,6 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
> >  
> >  	result = L2CAP_CR_NO_MEM;
> >  
> > -	/* Check for backlog size */
> > -	if (sk_acceptq_is_full(parent)) {
> > -		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> > -		goto response;
> > -	}
> > -
> >  	chan = pchan->ops->new_connection(pchan->data);
> >  	if (!chan)
> >  		goto response;
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 8a3620b..bc409a2 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -895,6 +895,12 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
> >  {
> >  	struct sock *sk, *parent = data;
> >  
> > +	/* Check for backlog size */
> > +	if (sk_acceptq_is_full(parent)) {
> > +		BT_DBG("backlog full %d", parent->sk_ack_backlog);
> > +		return NULL;
> > +	}
> > +
> 
> Ok I see where comes from this change. I feel that this can be merged with
> some other patch removing parent sk.

Let keep this patch as is, the other patch changes locking and I prefer a
separated patch for it.

	Gustavo

Re: [RFC 7/8] Bluetooth: Create DEFER_SETUP flag in conf_state

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:50:06 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:43PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > Remove another socket usage from l2cap_core.c
> 
> Good idea.
> 
> ...
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -623,10 +623,15 @@ static int l2cap_sock_setsockopt(struct socket *sock, int level, int optname, ch
> >  			break;
> >  		}
> >  
> > -		if (opt)
> > +		if (opt) {
> >  			set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> > -		else
> > +			set_bit(CONF_DEFER_SETUP, &chan->conf_state);
> > +		} else {
> >  			clear_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
> > +			clear_bit(CONF_DEFER_SETUP, &chan->conf_state);
> 
> Do we now have 2 similar flags?

Yes, and there is no other way to do this, BT_SK_DEFER_SETUP is for socket
related usage (mostly in af_bluetooth.c) and CONF_DEFER_SETUP is for
l2cap_core.c

> 
> > +		}
> > +
> > +
> 
> Remove extra line

Sure.

	Gustavo

Re: [RFC 1/2] Bluetooth: Create chan->ops->set_err()

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 11:26:04 +0300]:

> Hi Gustavo,
> 
> On Thu, May 24, 2012 at 04:00:16AM -0300, Gustavo Padovan wrote:
> > -	lock_sock(sk);
> > -	__l2cap_state_change(chan, BT_DISCONN);
> > -	__l2cap_chan_set_err(chan, err);
> > -	release_sock(sk);
> > +	l2cap_state_change(chan, BT_DISCONN);
> > +	if(chan->ops->set_err)
> > +		chan->ops->set_err(chan->data, err);
> 
> I do not know can it be done somehow better, currently we lock and unlock
> sockets for each operation.

I'm having trouble to get your point, could you please explain?

	Gustavo

Re: [RFC 2/2] Bluetooth: Remove last usage of sk->sk_state_change()

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 11:30:45 +0300]:

> Hi Gustavo,
> 
> On Thu, May 24, 2012 at 04:00:17AM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > If the is not oriented we should call sk_state_change() just after set the
> > channel state to BT_CONNECTED.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  net/bluetooth/l2cap_core.c |    6 +-----
> >  net/bluetooth/l2cap_sock.c |    4 ++++
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index 0d22695..6137b48 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1176,12 +1176,8 @@ static void l2cap_conn_ready(struct l2cap_conn *conn)
> >  				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);
> > +			l2cap_state_change(chan, BT_CONNECTED);
> >  
> >  		} else if (chan->state == BT_CONNECT)
> >  			l2cap_do_start(chan);
> > diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> > index 0a1a827..9fd9412 100644
> > --- a/net/bluetooth/l2cap_sock.c
> > +++ b/net/bluetooth/l2cap_sock.c
> > @@ -1018,6 +1018,10 @@ static void l2cap_sock_state_change_cb(void *data, int state)
> >  	struct sock *sk = data;
> >  
> >  	sk->sk_state = state;
> > +
> > +	if (state == BT_CONNECTED &&
> > +	    l2cap_pi(sk)->chan->chan_type != L2CAP_CHAN_CONN_ORIENTED)
> > +		sk->sk_state_change(sk);
> 
> Somehow I feel that there is too much logic here.

Yes, there is, I failed to find something better.

>BTW: can we use
> sk_state_change for every state_change callback?

No, it would be a bit expensive call sk_state_change() everytime here.

	Gustavo

Re: [RFC 8/8] Bluetooth: Add chan->ops->authorize

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:55:53 +0300]:

> Hi Gustavo,
> 
> On Wed, May 23, 2012 at 10:12:44PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > When DEFER_SETUP is set authorize() will trigger an authorization request
> > to the userspace.
> 
> Looks good.
> 
> > +static void l2cap_sock_authorize_cb(void *data)
> > +{
> > +	struct sock *sk = data;
> > +	struct sock *parent;
> > +
> > +	parent = bt_sk(sk)->parent;
> 
> But why not:
> 
> l2cap_sock_authorize_cb(struct sock *sk)
> {
> 	struct sock *parent = bt_sk(sk)->parent;

We need to be generic, I said that in the other patch already. The chan->sk
should disappear and the only thing l2cap_core.c will see is chan->data void *

	Gustavo

Re: [RFC 2/3] Bluetooth: Move bt_accept_enqueue() call to l2cap_sock.c

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 12:24:26 +0300]:

> Hi Gustavo,
> 
> On Thu, May 24, 2012 at 03:02:53AM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > bt_accept_enqueue() can be easily placed at the end of
> > l2cap_sock_new_connection_cb().
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  net/bluetooth/l2cap_core.c |    4 ----
> >  net/bluetooth/l2cap_sock.c |    2 ++
> >  2 files changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index c9de4f5..5ff294f9 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -1165,8 +1165,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> >  	bacpy(&bt_sk(sk)->src, conn->src);
> >  	bacpy(&bt_sk(sk)->dst, conn->dst);
> >  
> > -	bt_accept_enqueue(parent, sk);
> > -
> 
> Shall we also move to l2cap_sock_new_connection_cb bacpys above?

I don't think so, bacpy and bacmp are used everywhere, they need to fixed
differently.

	Gustavo

Re: [RFC 8/8] Bluetooth: Add chan->ops->authorize

[Mat Martineau]

Gustavo -

On Wed, 23 May 2012, Gustavo Padovan wrote:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> When DEFER_SETUP is set authorize() will trigger an authorization request
> to the userspace.
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> include/net/bluetooth/l2cap.h |    1 +
> net/bluetooth/l2cap_core.c    |   14 ++++++--------
> net/bluetooth/l2cap_sock.c    |   12 ++++++++++++
> 3 files changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 4f81d08..94e375a 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -531,6 +531,7 @@ struct l2cap_ops {
> 	struct sk_buff		*(*alloc_skb) (struct l2cap_chan *chan,
> 					       unsigned long len, int nb);
> 	void			(*ready) (void *data);
> +	void			(*authorize) (void *data);
> };

I'm trying to think of a name more specific to the deferred connection 
feature than "authorize"...  There's a lot of use of "auth" to refer 
to "authentication", so it could be confusing to throw "authorize" in 
to the mix.

"deferred_connect"? 
"pending_connection"?
"defer"?

>
> struct l2cap_conn {
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 1305b3b..22ba699 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1058,12 +1058,10 @@ static void l2cap_conn_start(struct l2cap_conn *conn)
> 				lock_sock(sk);
> 				if (test_bit(CONF_DEFER_SETUP,
> 					     &chan->conf_state)) {
> -					struct sock *parent = bt_sk(sk)->parent;
> 					rsp.result = cpu_to_le16(L2CAP_CR_PEND);
> 					rsp.status = cpu_to_le16(L2CAP_CS_AUTHOR_PEND);
> -					if (parent)
> -						parent->sk_data_ready(parent, 0);
> -
> +					if(chan->ops->authorize)
> +						chan->ops->authorize(chan->data);
> 				} else {
> 					__l2cap_state_change(chan, BT_CONFIG);
> 					rsp.result = cpu_to_le16(L2CAP_CR_SUCCESS);
> @@ -3380,7 +3378,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
> 				__l2cap_state_change(chan, BT_CONNECT2);
> 				result = L2CAP_CR_PEND;
> 				status = L2CAP_CS_AUTHOR_PEND;
> -				parent->sk_data_ready(parent, 0);
> +				if(chan->ops->authorize)
> +					chan->ops->authorize(chan->data);
> 			} else {
> 				__l2cap_state_change(chan, BT_CONFIG);
> 				result = L2CAP_CR_SUCCESS;
> @@ -5408,11 +5407,10 @@ int l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
> 			if (!status) {
> 				if (test_bit(CONF_DEFER_SETUP,
> 					     &chan->conf_state)) {
> -					struct sock *parent = bt_sk(sk)->parent;
> 					res = L2CAP_CR_PEND;
> 					stat = L2CAP_CS_AUTHOR_PEND;
> -					if (parent)
> -						parent->sk_data_ready(parent, 0);
> +					if(chan->ops->authorize)
> +						chan->ops->authorize(chan->data);
> 				} else {
> 					__l2cap_state_change(chan, BT_CONFIG);
> 					res = L2CAP_CR_SUCCESS;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index dac7b2c..d2a81f1 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1047,6 +1047,17 @@ static void l2cap_sock_ready_cb(void *data)
> 	release_sock(sk);
> }
>
> +static void l2cap_sock_authorize_cb(void *data)
> +{
> +	struct sock *sk = data;
> +	struct sock *parent;
> +
> +	parent = bt_sk(sk)->parent;
> +
> +	if (parent)
> +		parent->sk_data_ready(parent, 0);
> +}
> +
> static struct l2cap_ops l2cap_chan_ops = {
> 	.name		= "L2CAP Socket Interface",
> 	.new_connection	= l2cap_sock_new_connection_cb,
> @@ -1056,6 +1067,7 @@ static struct l2cap_ops l2cap_chan_ops = {
> 	.state_change	= l2cap_sock_state_change_cb,
> 	.alloc_skb	= l2cap_sock_alloc_skb_cb,
> 	.ready		= l2cap_sock_ready_cb,
> +	.authorize	= l2cap_sock_authorize_cb,
> };
>
> static void l2cap_sock_destruct(struct sock *sk)
> -- 
> 1.7.10.1


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Gustavo Padovan]

* Gustavo Padovan <gustavo@padovan.org> [2012-05-24 13:56:11 -0300]:

> > >  
> > > +static void l2cap_sock_finalize_cb(void *data, int err)
> > > +{
> > > +	struct sock *sk = data;
> > 
> > Why we cannot just use l2cap_sock_finalize_cb(struct sock *sk, int err)
> > what is the reason for extra assignment?
> 
> Because we want to be generic here, we don't know who else will use finalize()
> so we need to keep a void * here.
> 
> > 
> > I also think finalize is not good name since it does not mean what is
> > actually finalized.
> 
> And it is not, I failed to come with a better name here. The initial one was
> delete, even worse.

Maybe we can go with prepare_finalize() or prepare_close() here.

	Gustavo

Re: [RFC 1/2] Bluetooth: Create chan->ops->set_err()

[Mat Martineau]

Gustavo -

On Thu, 24 May 2012, Gustavo Padovan wrote:

> Hi Andrei,
>
> * Andrei Emeltchenko <andrei.emeltchenko.news@gmail.com> [2012-05-24 11:26:04 +0300]:
>
>> Hi Gustavo,
>>
>> On Thu, May 24, 2012 at 04:00:16AM -0300, Gustavo Padovan wrote:
>>> -	lock_sock(sk);
>>> -	__l2cap_state_change(chan, BT_DISCONN);
>>> -	__l2cap_chan_set_err(chan, err);
>>> -	release_sock(sk);
>>> +	l2cap_state_change(chan, BT_DISCONN);
>>> +	if(chan->ops->set_err)
>>> +		chan->ops->set_err(chan->data, err);
>>
>> I do not know can it be done somehow better, currently we lock and unlock
>> sockets for each operation.
>
> I'm having trouble to get your point, could you please explain?

What I see is that the lock used to be acquired once, with both 
state_change and set_err happening while locked.  With this change, 
the lock is acquired twice, once for state_change and once for 
set_err.  I'm not sure it's good to release the lock between 
state_change and set_err.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 1/8] Bluetooth: Move clean up code and set of SOCK_ZAPPED to l2cap_sock.c

[Vinicius Costa Gomes]

Hi,

On 14:14 Thu 24 May, Gustavo Padovan wrote:
> * Gustavo Padovan <gustavo@padovan.org> [2012-05-24 13:56:11 -0300]:
> 
> > > >  
> > > > +static void l2cap_sock_finalize_cb(void *data, int err)
> > > > +{
> > > > +	struct sock *sk = data;
> > > 
> > > Why we cannot just use l2cap_sock_finalize_cb(struct sock *sk, int err)
> > > what is the reason for extra assignment?
> > 
> > Because we want to be generic here, we don't know who else will use finalize()
> > so we need to keep a void * here.
> > 
> > > 
> > > I also think finalize is not good name since it does not mean what is
> > > actually finalized.
> > 
> > And it is not, I failed to come with a better name here. The initial one was
> > delete, even worse.
> 
> Maybe we can go with prepare_finalize() or prepare_close() here.

That sounds better, there's "teardown" too.

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


Cheers,
-- 
Vinicius

Re: [RFC 2/3] Bluetooth: Move bt_accept_enqueue() call to l2cap_sock.c

[Mat Martineau]

On Thu, 24 May 2012, Gustavo Padovan wrote:

> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> bt_accept_enqueue() can be easily placed at the end of
> l2cap_sock_new_connection_cb().
>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> ---
> net/bluetooth/l2cap_core.c |    4 ----
> net/bluetooth/l2cap_sock.c |    2 ++
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index c9de4f5..5ff294f9 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1165,8 +1165,6 @@ static void l2cap_le_conn_ready(struct l2cap_conn *conn)
> 	bacpy(&bt_sk(sk)->src, conn->src);
> 	bacpy(&bt_sk(sk)->dst, conn->dst);
>
> -	bt_accept_enqueue(parent, sk);
> -
> 	l2cap_chan_add(conn, chan);
>
> 	l2cap_chan_ready(chan);
> @@ -3357,8 +3355,6 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
> 	chan->psm  = psm;
> 	chan->dcid = scid;
>
> -	bt_accept_enqueue(parent, sk);
> -

Can you explain in the commit message why it is ok to move 
bt_accept_enqueue before the other initialization code in 
l2cap_connect_req?  It looks like some important setup happens there, 
but maybe the parent socket lock keeps things serialized.

> 	__l2cap_chan_add(conn, chan);
>
> 	dcid = chan->scid;
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index d2a81f1..f4c3e2c 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -915,6 +915,8 @@ static struct l2cap_chan *l2cap_sock_new_connection_cb(void *data)
>
> 	l2cap_sock_init(sk, parent);
>
> +	bt_accept_enqueue(parent, sk);
> +
> 	return l2cap_pi(sk)->chan;
> }
>
> -- 
> 1.7.10.1



--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Mat Martineau]

On Thu, 24 May 2012, Andrei Emeltchenko wrote:

> Hi Ulisses,
>
> On Thu, May 24, 2012 at 08:31:15AM -0300, Ulisses Furquim wrote:
>> Hi Andrei,
>>
>> On Thu, May 24, 2012 at 8:30 AM, Andrei Emeltchenko
>> <andrei.emeltchenko.news@gmail.com> wrote:
>>> Hi Ulisses,
>>>
>>> On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
>>>>>>> +   void                    (*ready) (void *data);
>>>>>>
>>>>>> Again, why void *data ?
>>>>>
>>>>> I mean here that for fixed channels we do not need this function at this
>>>>> point since initialization is different.
>>>>
>>>> So? What do you mean? This needs to be generic, I think. It's an
>>>> abstraction after all.
>>>
>>> Fixed channels do not have configuration phase, they are created
>>> CONNECTED (at least A2MP).
>>
>> And your proposal is?
>
> Fox fixed channels ready is not defined (at least now) so we can just use
> exact type, see my patch in a minute.

For l2cap_ops right now, every callback takes a void* except for 
alloc_skb.  alloc_skb could get by with a void* by using 
l2cap_pi(sk)->chan.

In any case, I think we can agree that some consistency in l2cap_ops 
would be good!  Either way will work because the void* can give the 
chan*, and with the chan* you have chan->data.  Let's pick either 
void* or l2cap_chan* for the callbacks and stick with it.

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Ulisses Furquim]

Hi Mat,

On Thu, May 24, 2012 at 2:48 PM, Mat Martineau <mathewm@codeaurora.org> wrote:
>
> On Thu, 24 May 2012, Andrei Emeltchenko wrote:
>
>> Hi Ulisses,
>>
>> On Thu, May 24, 2012 at 08:31:15AM -0300, Ulisses Furquim wrote:
>>>
>>> Hi Andrei,
>>>
>>> On Thu, May 24, 2012 at 8:30 AM, Andrei Emeltchenko
>>> <andrei.emeltchenko.news@gmail.com> wrote:
>>>>
>>>> Hi Ulisses,
>>>>
>>>> On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
>>>>>>>>
>>>>>>>> +   void                    (*ready) (void *data);
>>>>>>>
>>>>>>>
>>>>>>> Again, why void *data ?
>>>>>>
>>>>>>
>>>>>> I mean here that for fixed channels we do not need this function at
>>>>>> this
>>>>>> point since initialization is different.
>>>>>
>>>>>
>>>>> So? What do you mean? This needs to be generic, I think. It's an
>>>>> abstraction after all.
>>>>
>>>>
>>>> Fixed channels do not have configuration phase, they are created
>>>> CONNECTED (at least A2MP).
>>>
>>>
>>> And your proposal is?
>>
>>
>> Fox fixed channels ready is not defined (at least now) so we can just use
>> exact type, see my patch in a minute.
>
>
> For l2cap_ops right now, every callback takes a void* except for alloc_skb.
>  alloc_skb could get by with a void* by using l2cap_pi(sk)->chan.

IMO that should be changed to void*.

> In any case, I think we can agree that some consistency in l2cap_ops would
> be good!  Either way will work because the void* can give the chan*, and
> with the chan* you have chan->data.  Let's pick either void* or l2cap_chan*
> for the callbacks and stick with it.

Exactly. I'm ok with either void* or chan* for the same reason Mat
said. Let's have some consistency in l2cap_ops, please.

Best regards,

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

Re: [RFC 0/8] Another step in l2cap_core/sock separation

[Mat Martineau]

Gustavo -

On Thu, 24 May 2012, Andrei Emeltchenko wrote:

> Hi Gustavo,
>
> On Wed, May 23, 2012 at 10:12:36PM -0300, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>>
>> Hi,
>>
>> This patchset does a bit more of clean up in l2cap core, it was inially based
>> on the two patches Andrei has sent to the list today.
>
> Can this RFC be merged with other 2 RFC since otherwise I got lost.
>
> I also found that my mail reader indent your RFC in a strange way so I
> thought that they are in the same series.

I agree -- using "--chain-reply-to" has made this group of RFC patches
very difficult to read in a threaded email view.  Would you consider 
using --no-chain-reply-to with git send-email?

--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: [RFC 3/8] Bluetooth: Add l2cap_chan->ops->ready()

[Vinicius Costa Gomes]

Hi Andrei,

On 14:30 Thu 24 May, Andrei Emeltchenko wrote:
> Hi Ulisses,
> 
> On Thu, May 24, 2012 at 08:17:23AM -0300, Ulisses Furquim wrote:
> > >> > +   void                    (*ready) (void *data);
> > >>
> > >> Again, why void *data ?
> > >
> > > I mean here that for fixed channels we do not need this function at this
> > > point since initialization is different.
> > 
> > So? What do you mean? This needs to be generic, I think. It's an
> > abstraction after all.
> 
> Fixed channels do not have configuration phase, they are created
> CONNECTED (at least A2MP).

You are right, but having this abstraction seems useful.

In LE, for example, we (ab)use the CONFIG phase for blocking the channel
while SMP is running (negotiating the encryption of the link), so any 
applications are not able to send data in a lower security level than
what they requested.

> 
> Best regards 
> Andrei Emeltchenko 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers,
-- 
Vinicius

Re: [RFC 1/2] Bluetooth: Create chan->ops->set_err()

[Andrei Emeltchenko]

Hi,

On Thu, May 24, 2012 at 10:18:07AM -0700, Mat Martineau wrote:
> >>On Thu, May 24, 2012 at 04:00:16AM -0300, Gustavo Padovan wrote:
> >>>-	lock_sock(sk);
> >>>-	__l2cap_state_change(chan, BT_DISCONN);
> >>>-	__l2cap_chan_set_err(chan, err);
> >>>-	release_sock(sk);
> >>>+	l2cap_state_change(chan, BT_DISCONN);
> >>>+	if(chan->ops->set_err)
> >>>+		chan->ops->set_err(chan->data, err);
> >>
> >>I do not know can it be done somehow better, currently we lock and unlock
> >>sockets for each operation.
> >
> >I'm having trouble to get your point, could you please explain?
> 
> What I see is that the lock used to be acquired once, with both
> state_change and set_err happening while locked.  With this change,
> the lock is acquired twice, once for state_change and once for
> set_err.  I'm not sure it's good to release the lock between
> state_change and set_err.

Maybe we add parameter err to l2cap_state_change and if set set error.

Then we do not lock/unlock for each operation.

Best regards 
Andrei Emeltchenko