Re: [RFCv0 10/21] Bluetooth: Add logical link confirm

Hi Andrei -

On Thu, 26 Jul 2012, Andrei Emeltchenko wrote:

Hi Mat,

On Wed, Jul 25, 2012 at 04:51:02PM -0700, Mat Martineau wrote:

quoted

Signed-off-by: Mat Martineau <redacted>
---
 include/net/bluetooth/l2cap.h |   1 +
 net/bluetooth/l2cap_core.c    | 120 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 119 insertions(+), 2 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index fcc971a..7c50606 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h

@@ -461,6 +461,7 @@ struct l2cap_chan {

 	__u8		conf_req[64];
 	__u8		conf_len;
+	__u8		conf_ident;
 	__u8		num_conf_req;
 	__u8		num_conf_rsp;

diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 8d72f2e..79f9d8e 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c

@@ -3717,6 +3717,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn, struct l2cap_cmd_hdr
 		goto unlock;
 	}

+	chan->conf_ident = cmd->ident;
 	l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP, len, rsp);
 	chan->num_conf_rsp++;

@@ -4161,11 +4162,126 @@ static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
 	l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
 }

+/* Call with chan locked */
 static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
 			      u8 status)
 {
-	/* Placeholder */
-	return;
+	BT_DBG("chan %p, hchan %p, status %d", chan, hchan, status);
+
+	if (chan->state != BT_CONNECTED && !chan->chan_id)
+		return;
+
+	if (chan && !status && chan->state != BT_CONNECTED) {

If we consider chan might be NULL we would crash in the previous reference
of chan->state.

I'll fix that.

quoted

+		struct l2cap_conf_rsp rsp;
+		u8 code;
+
+		/* Create channel complete */
+		chan->hs_hcon = hchan->conn;
+		chan->hs_hcon->l2cap_data = chan->conn;
+
+		code = l2cap_build_conf_rsp(chan, &rsp,
+					    L2CAP_CONF_SUCCESS, 0);
+		l2cap_send_cmd(chan->conn, chan->conf_ident,
+			       L2CAP_CONF_RSP, code, &rsp);
+		set_bit(CONF_OUTPUT_DONE, &chan->conf_state);
+
+		if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
+			int err = 0;
+
+			set_default_fcs(chan);
+
+			if (chan->mode == L2CAP_MODE_ERTM ||
+			    chan->mode == L2CAP_MODE_STREAMING)
+				err = l2cap_ertm_init(chan);
+
+			if (err < 0)
+				l2cap_send_disconn_req(chan->conn, chan, -err);
+			else
+				l2cap_chan_ready(chan);
+		}
+	} else if (chan && !status) {

Otherwise we always reference chan so we might check for chan in the
beginning.

quoted

+		/* Channel move */
+		chan->hs_hcon = hchan->conn;
+		chan->hs_hcon->l2cap_data = chan->conn;
+
+		BT_DBG("move_state %d", chan->move_state);

in the future patches I would prefer something similar to state_to_string.

...

Future patches that print move_state?  (This is now the only place 
that prints it)

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