[PATCH 0/7] LE connection refactoring and fixes

[PATCH 0/7] LE connection refactoring and fixes

[Andre Guedes]

Hi all,

This patchset contains several refactoring and fixes for LE connection code.
These patches are part of the bigger LE auto connection patchset. As discussed
in New Orleans, I'm sending these patches since they can already be applied to
bluetooth-next.

Also, I'm still implementing the suggestions collected during the Wireless
Summit and I'll send the LE auto connection patchset soon.

Regards,

Andre


Andre Guedes (7):
  Bluetooth: Initialize hci_conn fields in hci_connect_le
  Bluetooth: Use HCI request for LE connection
  Bluetooth: Remove hci_le_create_connection
  Bluetooth: Remove hci_cs_le_create_conn event handler
  Bluetooth: Refactor hci_connect_le
  Bluetooth: Refactor LE Connection Complete HCI event handler
  Bluetooth: Locking in hci_le_conn_complete_evt

 include/net/bluetooth/hci.h      |   1 +
 include/net/bluetooth/hci_core.h |   2 +
 net/bluetooth/hci_conn.c         |  70 +++++++++++++--------------
 net/bluetooth/hci_core.c         |  46 ++++++++++++++++++
 net/bluetooth/hci_event.c        | 102 +++++++++++++++++++--------------------
 5 files changed, 132 insertions(+), 89 deletions(-)

-- 
1.8.4

[PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le

[Andre Guedes]

This patch moves some hci_conn fields initialization from hci_le_
create_connection() to hci_connect_le(). It makes more sense to
initialize these fields within the function that creates the hci_
conn object.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index d2380e0..f473605 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_le_create_conn cp;
 
-	conn->state = BT_CONNECT;
-	conn->out = true;
-	conn->link_mode |= HCI_LM_MASTER;
-	conn->sec_level = BT_SECURITY_LOW;
-
 	memset(&cp, 0, sizeof(cp));
 	cp.scan_interval = __constant_cpu_to_le16(0x0060);
 	cp.scan_window = __constant_cpu_to_le16(0x0030);
@@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 			return ERR_PTR(-ENOMEM);
 
 		le->dst_type = bdaddr_to_le(dst_type);
+		le->state = BT_CONNECT;
+		le->out = true;
+		le->link_mode |= HCI_LM_MASTER;
+		le->sec_level = BT_SECURITY_LOW;
+
 		hci_le_create_connection(le);
 	}
 
-- 
1.8.4

[PATCH 2/7] Bluetooth: Use HCI request for LE connection

[Andre Guedes]

This patch adds a new helper for initiating LE conneciton which uses
the HCI request framework. This patch also changes the hci_connect_le()
so it uses the new helper instead of the old hci_le_create_connection().

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         |  7 +++++-
 net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 26cc9f7..6aa172c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1216,6 +1216,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 
 u8 bdaddr_to_le(u8 bdaddr_type);
 
+int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
+
 #define SCO_AIRMODE_MASK       0x0003
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index f473605..24d1a0a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -545,6 +545,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 				    u8 dst_type, u8 sec_level, u8 auth_type)
 {
 	struct hci_conn *le;
+	int err;
 
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
@@ -565,7 +566,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 		le->link_mode |= HCI_LM_MASTER;
 		le->sec_level = BT_SECURITY_LOW;
 
-		hci_le_create_connection(le);
+		err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
+		if (err) {
+			hci_conn_del(le);
+			return ERR_PTR(err);
+		}
 	}
 
 	le->pending_sec_level = sec_level;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4549b5c..51c1796 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3631,3 +3631,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
 		return ADDR_LE_DEV_RANDOM;
 	}
 }
+
+static void initiate_le_connection_complete(struct hci_dev *hdev, u8 status)
+{
+	struct hci_conn *conn;
+
+	if (status == 0)
+		return;
+
+	BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
+	       status);
+
+	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+	if (!conn)
+		return;
+
+	mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
+			    status);
+
+	hci_proto_connect_cfm(conn, status);
+
+	hci_dev_lock(hdev);
+	hci_conn_del(conn);
+	hci_dev_unlock(hdev);
+}
+
+int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
+{
+	struct hci_cp_le_create_conn cp;
+	struct hci_request req;
+
+	hci_req_init(&req, hdev);
+
+	memset(&cp, 0, sizeof(cp));
+	cp.scan_interval = __constant_cpu_to_le16(0x0060);
+	cp.scan_window = __constant_cpu_to_le16(0x0030);
+	bacpy(&cp.peer_addr, addr);
+	cp.peer_addr_type = type;
+	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
+	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
+	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
+	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
+	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
+	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
+
+	return hci_req_run(&req, initiate_le_connection_complete);
+}
-- 
1.8.4

[PATCH 3/7] Bluetooth: Remove hci_le_create_connection

[Andre Guedes]

Since we now use hci_initiate_le_connection() helper for creating new
LE connections, we can safely remove hci_le_create_connection().

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 24d1a0a..b89522f 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
 };
 
-static void hci_le_create_connection(struct hci_conn *conn)
-{
-	struct hci_dev *hdev = conn->hdev;
-	struct hci_cp_le_create_conn cp;
-
-	memset(&cp, 0, sizeof(cp));
-	cp.scan_interval = __constant_cpu_to_le16(0x0060);
-	cp.scan_window = __constant_cpu_to_le16(0x0030);
-	bacpy(&cp.peer_addr, &conn->dst);
-	cp.peer_addr_type = conn->dst_type;
-	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
-	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
-	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
-	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
-	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
-
-	hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
-}
-
 static void hci_le_create_connection_cancel(struct hci_conn *conn)
 {
 	hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
-- 
1.8.4

[PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler

[Andre Guedes]

This patch removes the hci_cs_le_create_conn event handler since this
handling is now done in create_le_connection_complete() callback in
hci_conn.c.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index d171c04b..1d1ffa6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
 	hci_dev_unlock(hdev);
 }
 
-static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
-{
-	struct hci_conn *conn;
-
-	BT_DBG("%s status 0x%2.2x", hdev->name, status);
-
-	if (status) {
-		hci_dev_lock(hdev);
-
-		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-		if (!conn) {
-			hci_dev_unlock(hdev);
-			return;
-		}
-
-		BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
-
-		conn->state = BT_CLOSED;
-		mgmt_connect_failed(hdev, &conn->dst, conn->type,
-				    conn->dst_type, status);
-		hci_proto_connect_cfm(conn, status);
-		hci_conn_del(conn);
-
-		hci_dev_unlock(hdev);
-	}
-}
-
 static void hci_cs_create_phylink(struct hci_dev *hdev, u8 status)
 {
 	struct hci_cp_create_phy_link *cp;
@@ -2342,10 +2315,6 @@ static void hci_cmd_status_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_cs_disconnect(hdev, ev->status);
 		break;
 
-	case HCI_OP_LE_CREATE_CONN:
-		hci_cs_le_create_conn(hdev, ev->status);
-		break;
-
 	case HCI_OP_CREATE_PHY_LINK:
 		hci_cs_create_phylink(hdev, ev->status);
 		break;
-- 
1.8.4

[PATCH 5/7] Bluetooth: Refactor hci_connect_le

[Andre Guedes]

This patch organizes hci_connect_le() logic and adds extra comments
to improve code's readability.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_conn.c | 54 ++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b89522f..d2409dc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -531,34 +531,44 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
 
+	/* If already exists a hci_conn object for the following connection
+	 * attempt, we simply update pending_sec_level and auth_type fields
+	 * and return the object found.
+	 */
 	le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
-	if (!le) {
-		le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-		if (le)
-			return ERR_PTR(-EBUSY);
-
-		le = hci_conn_add(hdev, LE_LINK, dst);
-		if (!le)
-			return ERR_PTR(-ENOMEM);
-
-		le->dst_type = bdaddr_to_le(dst_type);
-		le->state = BT_CONNECT;
-		le->out = true;
-		le->link_mode |= HCI_LM_MASTER;
-		le->sec_level = BT_SECURITY_LOW;
-
-		err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
-		if (err) {
-			hci_conn_del(le);
-			return ERR_PTR(err);
-		}
+	if (le) {
+		le->pending_sec_level = sec_level;
+		le->auth_type = auth_type;
+		goto out;
 	}
 
-	le->pending_sec_level = sec_level;
+	/* Since the controller supports only one LE connection attempt at the
+	 * time, we return busy if there is any connection attempt running.
+	 */
+	le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+	if (le)
+		return ERR_PTR(-EBUSY);
+
+	le = hci_conn_add(hdev, LE_LINK, dst);
+	if (!le)
+		return ERR_PTR(-ENOMEM);
+
+	le->dst_type = bdaddr_to_le(dst_type);
+	le->state = BT_CONNECT;
+	le->out = true;
+	le->link_mode |= HCI_LM_MASTER;
+	le->sec_level = BT_SECURITY_LOW;
+	le->pending_sec_level = BT_SECURITY_LOW;
 	le->auth_type = auth_type;
 
-	hci_conn_hold(le);
+	err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
+	if (err) {
+		hci_conn_del(le);
+		return ERR_PTR(err);
+	}
 
+out:
+	hci_conn_hold(le);
 	return le;
 }
 
-- 
1.8.4

[PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler

[Andre Guedes]

This patch does some code refactorig in LE Connection Complete HCI
event handler. It basically adds a switch statement to separate new
master connection code from new slave connection code.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 7ede266..8c98f60 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
 
 /* Low energy meta events */
 #define LE_CONN_ROLE_MASTER	0x00
+#define LE_CONN_ROLE_SLAVE	0x01
 
 #define HCI_EV_LE_CONN_COMPLETE		0x01
 struct hci_ev_le_conn_complete {
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1d1ffa6..0e4a9f4 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	hci_dev_lock(hdev);
 
-	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-	if (!conn) {
+	if (ev->status) {
+		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+		if (!conn)
+			goto unlock;
+
+		mgmt_connect_failed(hdev, &conn->dst, conn->type,
+				    conn->dst_type, ev->status);
+		hci_proto_connect_cfm(conn, ev->status);
+		conn->state = BT_CLOSED;
+		hci_conn_del(conn);
+		goto unlock;
+	}
+
+	switch (ev->role) {
+	case LE_CONN_ROLE_MASTER:
+		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
+		/* If there is no hci_conn object with the given address, it
+		 * means this new connection was triggered through HCI socket
+		 * interface. For that case, we should create a new hci_conn
+		 * object.
+		 */
+		if (!conn) {
+			conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+			if (!conn) {
+				BT_ERR("No memory for new connection");
+				goto unlock;
+			}
+
+			conn->out = true;
+			conn->link_mode |= HCI_LM_MASTER;
+			conn->sec_level = BT_SECURITY_LOW;
+			conn->dst_type = ev->bdaddr_type;
+		}
+		break;
+
+	case LE_CONN_ROLE_SLAVE:
 		conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
 		if (!conn) {
 			BT_ERR("No memory for new connection");
@@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		}
 
 		conn->dst_type = ev->bdaddr_type;
+		conn->sec_level = BT_SECURITY_LOW;
+		break;
 
-		if (ev->role == LE_CONN_ROLE_MASTER) {
-			conn->out = true;
-			conn->link_mode |= HCI_LM_MASTER;
-		}
-	}
-
-	if (ev->status) {
-		mgmt_connect_failed(hdev, &conn->dst, conn->type,
-				    conn->dst_type, ev->status);
-		hci_proto_connect_cfm(conn, ev->status);
-		conn->state = BT_CLOSED;
-		hci_conn_del(conn);
+	default:
+		BT_ERR("Used reserved Role parameter %d", ev->role);
 		goto unlock;
 	}
 
@@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
 				      conn->dst_type, 0, NULL, 0, NULL);
 
-	conn->sec_level = BT_SECURITY_LOW;
 	conn->handle = __le16_to_cpu(ev->handle);
 	conn->state = BT_CONNECTED;
 
-- 
1.8.4

[PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt

[Andre Guedes]

This patch moves hci_dev_lock and hci_dev_unlock calls to where they
are really required, reducing the critical region in hci_le_conn_
complete_evt function. hdev->lock is required only in hci_conn_del
and hci_conn_add call to protect concurrent add and remove operations
in hci_conn_hash list.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 net/bluetooth/hci_event.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 0e4a9f4..ffd5186 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3442,19 +3442,20 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%2.2x", hdev->name, ev->status);
 
-	hci_dev_lock(hdev);
-
 	if (ev->status) {
 		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
 		if (!conn)
-			goto unlock;
+			return;
 
 		mgmt_connect_failed(hdev, &conn->dst, conn->type,
 				    conn->dst_type, ev->status);
 		hci_proto_connect_cfm(conn, ev->status);
 		conn->state = BT_CLOSED;
+
+		hci_dev_lock(hdev);
 		hci_conn_del(conn);
-		goto unlock;
+		hci_dev_unlock(hdev);
+		return;
 	}
 
 	switch (ev->role) {
@@ -3466,10 +3467,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		 * object.
 		 */
 		if (!conn) {
+			hci_dev_lock(hdev);
 			conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+			hci_dev_unlock(hdev);
+
 			if (!conn) {
 				BT_ERR("No memory for new connection");
-				goto unlock;
+				return;
 			}
 
 			conn->out = true;
@@ -3480,10 +3484,13 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		break;
 
 	case LE_CONN_ROLE_SLAVE:
+		hci_dev_lock(hdev);
 		conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
+		hci_dev_unlock(hdev);
+
 		if (!conn) {
 			BT_ERR("No memory for new connection");
-			goto unlock;
+			return;
 		}
 
 		conn->dst_type = ev->bdaddr_type;
@@ -3492,7 +3499,7 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 
 	default:
 		BT_ERR("Used reserved Role parameter %d", ev->role);
-		goto unlock;
+		return;
 	}
 
 	if (!test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
@@ -3505,9 +3512,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_conn_add_sysfs(conn);
 
 	hci_proto_connect_cfm(conn, ev->status);
-
-unlock:
-	hci_dev_unlock(hdev);
 }
 
 static void hci_le_adv_report_evt(struct hci_dev *hdev, struct sk_buff *skb)
-- 
1.8.4

Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler

[Anderson Lizardo]

Hi Guedes,

On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> This patch removes the hci_cs_le_create_conn event handler since this
> handling is now done in create_le_connection_complete() callback in
> hci_conn.c.

Did you mean "... now done in initiate_le_connection_complete()
callback in hci_core.c" ?

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 5/7] Bluetooth: Refactor hci_connect_le

[Anderson Lizardo]

Hi Guedes,

On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> +       /* If already exists a hci_conn object for the following connection
> +        * attempt, we simply update pending_sec_level and auth_type fields
> +        * and return the object found.
> +        */

Small textual improvement: "If a hci_conn object already exists [...]"

>         le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> -       if (!le) {
> -               le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> -               if (le)
> -                       return ERR_PTR(-EBUSY);
> -
> -               le = hci_conn_add(hdev, LE_LINK, dst);
> -               if (!le)
> -                       return ERR_PTR(-ENOMEM);
> -
> -               le->dst_type = bdaddr_to_le(dst_type);
> -               le->state = BT_CONNECT;
> -               le->out = true;
> -               le->link_mode |= HCI_LM_MASTER;
> -               le->sec_level = BT_SECURITY_LOW;
> -
> -               err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> -               if (err) {
> -                       hci_conn_del(le);
> -                       return ERR_PTR(err);
> -               }
> +       if (le) {
> +               le->pending_sec_level = sec_level;
> +               le->auth_type = auth_type;
> +               goto out;
>         }
>
> -       le->pending_sec_level = sec_level;
> +       /* Since the controller supports only one LE connection attempt at the
> +        * time, we return busy if there is any connection attempt running.
> +        */

s/at the time/at a time/
s/busy/EBUSY/

> +       le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> +       if (le)
> +               return ERR_PTR(-EBUSY);
> +
> +       le = hci_conn_add(hdev, LE_LINK, dst);
> +       if (!le)
> +               return ERR_PTR(-ENOMEM);
> +
> +       le->dst_type = bdaddr_to_le(dst_type);
> +       le->state = BT_CONNECT;
> +       le->out = true;
> +       le->link_mode |= HCI_LM_MASTER;
> +       le->sec_level = BT_SECURITY_LOW;
> +       le->pending_sec_level = BT_SECURITY_LOW;

I think the previous statement should be:

le->pending_sec_level = sec_level;

Otherwise, we are changing semantics.

>         le->auth_type = auth_type;
>
> -       hci_conn_hold(le);
> +       err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> +       if (err) {
> +               hci_conn_del(le);
> +               return ERR_PTR(err);
> +       }
>
> +out:
> +       hci_conn_hold(le);
>         return le;
>  }

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le

[Marcel Holtmann]

Hi Andre,

> This patch moves some hci_conn fields initialization from hci_le_
> create_connection() to hci_connect_le(). It makes more sense to
> initialize these fields within the function that creates the hci_
> conn object.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index d2380e0..f473605 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
> 	struct hci_dev *hdev = conn->hdev;
> 	struct hci_cp_le_create_conn cp;
> 
> -	conn->state = BT_CONNECT;
> -	conn->out = true;
> -	conn->link_mode |= HCI_LM_MASTER;
> -	conn->sec_level = BT_SECURITY_LOW;
> -
> 	memset(&cp, 0, sizeof(cp));
> 	cp.scan_interval = __constant_cpu_to_le16(0x0060);
> 	cp.scan_window = __constant_cpu_to_le16(0x0030);
> @@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> 			return ERR_PTR(-ENOMEM);
> 
> 		le->dst_type = bdaddr_to_le(dst_type);
> +		le->state = BT_CONNECT;
> +		le->out = true;
> +		le->link_mode |= HCI_LM_MASTER;
> +		le->sec_level = BT_SECURITY_LOW;
> +
> 		hci_le_create_connection(le);
> 	}

I do not understand on how this is the same. Maybe the confusion is the use of le-> instead of conn-> as variable for hci_conn. Seems that should be fixed first.

Regards

Marcel

Re: [PATCH 2/7] Bluetooth: Use HCI request for LE connection

[Marcel Holtmann]

Hi Andre,

> This patch adds a new helper for initiating LE conneciton which uses
> the HCI request framework. This patch also changes the hci_connect_le()
> so it uses the new helper instead of the old hci_le_create_connection().
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_conn.c         |  7 +++++-
> net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 26cc9f7..6aa172c 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1216,6 +1216,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 
> u8 bdaddr_to_le(u8 bdaddr_type);
> 
> +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
> +
> #define SCO_AIRMODE_MASK       0x0003
> #define SCO_AIRMODE_CVSD       0x0000
> #define SCO_AIRMODE_TRANSP     0x0003
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index f473605..24d1a0a 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -545,6 +545,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> 				    u8 dst_type, u8 sec_level, u8 auth_type)
> {
> 	struct hci_conn *le;
> +	int err;
> 
> 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> 		return ERR_PTR(-ENOTSUPP);
> @@ -565,7 +566,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> 		le->link_mode |= HCI_LM_MASTER;
> 		le->sec_level = BT_SECURITY_LOW;
> 
> -		hci_le_create_connection(le);
> +		err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> +		if (err) {
> +			hci_conn_del(le);
> +			return ERR_PTR(err);
> +		}
> 	}
> 
> 	le->pending_sec_level = sec_level;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 4549b5c..51c1796 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3631,3 +3631,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
> 		return ADDR_LE_DEV_RANDOM;
> 	}
> }
> +
> +static void initiate_le_connection_complete(struct hci_dev *hdev, u8 status)
> +{
> +	struct hci_conn *conn;
> +
> +	if (status == 0)
> +		return;
> +
> +	BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
> +	       status);
> +
> +	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> +	if (!conn)
> +		return;
> +
> +	mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
> +			    status);
> +
> +	hci_proto_connect_cfm(conn, status);
> +
> +	hci_dev_lock(hdev);
> +	hci_conn_del(conn);
> +	hci_dev_unlock(hdev);
> +}
> +
> +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
> +{
> +	struct hci_cp_le_create_conn cp;
> +	struct hci_request req;
> +
> +	hci_req_init(&req, hdev);
> +
> +	memset(&cp, 0, sizeof(cp));
> +	cp.scan_interval = __constant_cpu_to_le16(0x0060);
> +	cp.scan_window = __constant_cpu_to_le16(0x0030);
> +	bacpy(&cp.peer_addr, addr);
> +	cp.peer_addr_type = type;
> +	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> +	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> +	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> +	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> +	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> +	hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> +
> +	return hci_req_run(&req, initiate_le_connection_complete);
> +}

so how does this actually work. The command status handling for errors is now run twice? Once in hci_cs_le_create_conn() and once in the complete callback.

Regards

Marcel

Re: [PATCH 3/7] Bluetooth: Remove hci_le_create_connection

[Marcel Holtmann]

Hi Andre,

> Since we now use hci_initiate_le_connection() helper for creating new
> LE connections, we can safely remove hci_le_create_connection().
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 19 -------------------
> 1 file changed, 19 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 24d1a0a..b89522f 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
> 	{ EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
> };
> 
> -static void hci_le_create_connection(struct hci_conn *conn)
> -{
> -	struct hci_dev *hdev = conn->hdev;
> -	struct hci_cp_le_create_conn cp;
> -
> -	memset(&cp, 0, sizeof(cp));
> -	cp.scan_interval = __constant_cpu_to_le16(0x0060);
> -	cp.scan_window = __constant_cpu_to_le16(0x0030);
> -	bacpy(&cp.peer_addr, &conn->dst);
> -	cp.peer_addr_type = conn->dst_type;
> -	cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> -	cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> -	cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> -	cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> -	cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> -
> -	hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> -}
> -
> static void hci_le_create_connection_cancel(struct hci_conn *conn)
> {
> 	hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);

I really start to dislike all the super long naming here. Existing one and new that you are defining.

Why not name the new handler hci_le_create_conn() instead of all this initiate_something and remove the existing function at the same time. I mean you can bisect the code, but it will throw a warning of an unused function.

Regards

Marcel

Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler

[Marcel Holtmann]

Hi Andre,

> This patch removes the hci_cs_le_create_conn event handler since this
> handling is now done in create_le_connection_complete() callback in
> hci_conn.c.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_event.c | 31 -------------------------------
> 1 file changed, 31 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d171c04b..1d1ffa6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
> 	hci_dev_unlock(hdev);
> }
> 
> -static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
> -{
> -	struct hci_conn *conn;
> -
> -	BT_DBG("%s status 0x%2.2x", hdev->name, status);
> -
> -	if (status) {
> -		hci_dev_lock(hdev);
> -
> -		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> -		if (!conn) {
> -			hci_dev_unlock(hdev);
> -			return;
> -		}
> -
> -		BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
> -
> -		conn->state = BT_CLOSED;
> -		mgmt_connect_failed(hdev, &conn->dst, conn->type,
> -				    conn->dst_type, status);
> -		hci_proto_connect_cfm(conn, status);
> -		hci_conn_del(conn);
> -
> -		hci_dev_unlock(hdev);
> -	}
> -}

this is dangerous since it actually breaks bisection. The code is never complete. So while this might turn into a larger patch, you might need to do it all 3 patches at once. With a length commit message explaining exactly what happens and why this is correct.

Regards

Marcel

Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler

[Marcel Holtmann]

Hi Andre,

> This patch does some code refactorig in LE Connection Complete HCI
> event handler. It basically adds a switch statement to separate new
> master connection code from new slave connection code.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci.h |  1 +
> net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++-------------
> 2 files changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 7ede266..8c98f60 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
> 
> /* Low energy meta events */
> #define LE_CONN_ROLE_MASTER	0x00
> +#define LE_CONN_ROLE_SLAVE	0x01
> 
> #define HCI_EV_LE_CONN_COMPLETE		0x01
> struct hci_ev_le_conn_complete {
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 1d1ffa6..0e4a9f4 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 
> 	hci_dev_lock(hdev);
> 
> -	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> -	if (!conn) {
> +	if (ev->status) {
> +		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> +		if (!conn)
> +			goto unlock;
> +
> +		mgmt_connect_failed(hdev, &conn->dst, conn->type,
> +				    conn->dst_type, ev->status);
> +		hci_proto_connect_cfm(conn, ev->status);
> +		conn->state = BT_CLOSED;
> +		hci_conn_del(conn);
> +		goto unlock;
> +	}
> +
> +	switch (ev->role) {
> +	case LE_CONN_ROLE_MASTER:
> +		conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
> +		/* If there is no hci_conn object with the given address, it
> +		 * means this new connection was triggered through HCI socket
> +		 * interface. For that case, we should create a new hci_conn
> +		 * object.
> +		 */

this comments belong one level down inside the if block. You already commenting on the negative outcome of the if check.

> +		if (!conn) {
> +			conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> +			if (!conn) {
> +				BT_ERR("No memory for new connection");
> +				goto unlock;
> +			}
> +
> +			conn->out = true;
> +			conn->link_mode |= HCI_LM_MASTER;
> +			conn->sec_level = BT_SECURITY_LOW;
> +			conn->dst_type = ev->bdaddr_type;
> +		}
> +		break;
> +
> +	case LE_CONN_ROLE_SLAVE:

And why are we not checking for an existing connection here? At least a small comment is needed to make that part clear.

> 		conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> 		if (!conn) {
> 			BT_ERR("No memory for new connection");
> @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		}
> 
> 		conn->dst_type = ev->bdaddr_type;
> +		conn->sec_level = BT_SECURITY_LOW;
> +		break;
> 
> -		if (ev->role == LE_CONN_ROLE_MASTER) {
> -			conn->out = true;
> -			conn->link_mode |= HCI_LM_MASTER;
> -		}
> -	}
> -
> -	if (ev->status) {
> -		mgmt_connect_failed(hdev, &conn->dst, conn->type,
> -				    conn->dst_type, ev->status);
> -		hci_proto_connect_cfm(conn, ev->status);
> -		conn->state = BT_CLOSED;
> -		hci_conn_del(conn);
> +	default:
> +		BT_ERR("Used reserved Role parameter %d", ev->role);
> 		goto unlock;
> 	}
> 
> @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 		mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> 				      conn->dst_type, 0, NULL, 0, NULL);
> 
> -	conn->sec_level = BT_SECURITY_LOW;
> 	conn->handle = __le16_to_cpu(ev->handle);
> 	conn->state = BT_CONNECTED;

All in all, I am not really understanding why this makes it this code simpler. I actually think it turns it into more complicated code. So please explain what we are really gaining here. I just see more hash table lookup and for hci_conn_add calls with more error checks.

Regards

Marcel

Re: [PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt

[Marcel Holtmann]

Hi Andre,

> This patch moves hci_dev_lock and hci_dev_unlock calls to where they
> are really required, reducing the critical region in hci_le_conn_
> complete_evt function. hdev->lock is required only in hci_conn_del
> and hci_conn_add call to protect concurrent add and remove operations
> in hci_conn_hash list.

is this statement actually true? Because we have done this for so many HCI event, that I highly doubt that your statement tis correct. And if it is correct, you need to fix all other users first.

Regards

Marcel

Re: [PATCH 1/7] Bluetooth: Initialize hci_conn fields in hci_connect_le

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 1:57 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andre,
>
> > This patch moves some hci_conn fields initialization from hci_le_
> > create_connection() to hci_connect_le(). It makes more sense to
> > initialize these fields within the function that creates the hci_
> > conn object.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> > ---
> > net/bluetooth/hci_conn.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index d2380e0..f473605 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -54,11 +54,6 @@ static void hci_le_create_connection(struct hci_conn *conn)
> >       struct hci_dev *hdev = conn->hdev;
> >       struct hci_cp_le_create_conn cp;
> >
> > -     conn->state = BT_CONNECT;
> > -     conn->out = true;
> > -     conn->link_mode |= HCI_LM_MASTER;
> > -     conn->sec_level = BT_SECURITY_LOW;
> > -
> >       memset(&cp, 0, sizeof(cp));
> >       cp.scan_interval = __constant_cpu_to_le16(0x0060);
> >       cp.scan_window = __constant_cpu_to_le16(0x0030);
> > @@ -565,6 +560,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> >                       return ERR_PTR(-ENOMEM);
> >
> >               le->dst_type = bdaddr_to_le(dst_type);
> > +             le->state = BT_CONNECT;
> > +             le->out = true;
> > +             le->link_mode |= HCI_LM_MASTER;
> > +             le->sec_level = BT_SECURITY_LOW;
> > +
> >               hci_le_create_connection(le);
> >       }
>
> I do not understand on how this is the same. Maybe the confusion is the use of le-> instead of conn-> as variable for hci_conn. Seems that should be fixed first.

Yes, I don't really like le-> either. I'll replace le-> by conn-> in
this function in a first patch.

Andre

Re: [PATCH 2/7] Bluetooth: Use HCI request for LE connection

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 2:04 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andre,
>
> > This patch adds a new helper for initiating LE conneciton which uses
> > the HCI request framework. This patch also changes the hci_connect_le()
> > so it uses the new helper instead of the old hci_le_create_connection().
> >
> > Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> > ---
> > include/net/bluetooth/hci_core.h |  2 ++
> > net/bluetooth/hci_conn.c         |  7 +++++-
> > net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 54 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 26cc9f7..6aa172c 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1216,6 +1216,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> >
> > u8 bdaddr_to_le(u8 bdaddr_type);
> >
> > +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type);
> > +
> > #define SCO_AIRMODE_MASK       0x0003
> > #define SCO_AIRMODE_CVSD       0x0000
> > #define SCO_AIRMODE_TRANSP     0x0003
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index f473605..24d1a0a 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -545,6 +545,7 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> >                                   u8 dst_type, u8 sec_level, u8 auth_type)
> > {
> >       struct hci_conn *le;
> > +     int err;
> >
> >       if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> >               return ERR_PTR(-ENOTSUPP);
> > @@ -565,7 +566,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> >               le->link_mode |= HCI_LM_MASTER;
> >               le->sec_level = BT_SECURITY_LOW;
> >
> > -             hci_le_create_connection(le);
> > +             err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> > +             if (err) {
> > +                     hci_conn_del(le);
> > +                     return ERR_PTR(err);
> > +             }
> >       }
> >
> >       le->pending_sec_level = sec_level;
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 4549b5c..51c1796 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3631,3 +3631,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
> >               return ADDR_LE_DEV_RANDOM;
> >       }
> > }
> > +
> > +static void initiate_le_connection_complete(struct hci_dev *hdev, u8 status)
> > +{
> > +     struct hci_conn *conn;
> > +
> > +     if (status == 0)
> > +             return;
> > +
> > +     BT_ERR("HCI request failed to initiate LE connection: status 0x%2.2x",
> > +            status);
> > +
> > +     conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > +     if (!conn)
> > +             return;
> > +
> > +     mgmt_connect_failed(hdev, &conn->dst, conn->type, conn->dst_type,
> > +                         status);
> > +
> > +     hci_proto_connect_cfm(conn, status);
> > +
> > +     hci_dev_lock(hdev);
> > +     hci_conn_del(conn);
> > +     hci_dev_unlock(hdev);
> > +}
> > +
> > +int hci_initiate_le_connection(struct hci_dev *hdev, bdaddr_t *addr, u8 type)
> > +{
> > +     struct hci_cp_le_create_conn cp;
> > +     struct hci_request req;
> > +
> > +     hci_req_init(&req, hdev);
> > +
> > +     memset(&cp, 0, sizeof(cp));
> > +     cp.scan_interval = __constant_cpu_to_le16(0x0060);
> > +     cp.scan_window = __constant_cpu_to_le16(0x0030);
> > +     bacpy(&cp.peer_addr, addr);
> > +     cp.peer_addr_type = type;
> > +     cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> > +     cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> > +     cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> > +     cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> > +     cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> > +     hci_req_add(&req, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> > +
> > +     return hci_req_run(&req, initiate_le_connection_complete);
> > +}
>
> so how does this actually work. The command status handling for errors is now run twice? Once in hci_cs_le_create_conn() and once in the complete callback.

The hci_cs_le_create_conn() is removed in patch 4/7 since the handling
is done in initiate_le_connection_complete introduced by this patch.

I thought splitting this in three short patches would be easier to
review but it seems to be more confusing :) I'll squash patches 2, 3
and 4 into a bigger patch though.

Regards,

Andre

Re: [PATCH 3/7] Bluetooth: Remove hci_le_create_connection

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 2:08 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andre,
>
> > Since we now use hci_initiate_le_connection() helper for creating new
> > LE connections, we can safely remove hci_le_create_connection().
> >
> > Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> > ---
> > net/bluetooth/hci_conn.c | 19 -------------------
> > 1 file changed, 19 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index 24d1a0a..b89522f 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -49,25 +49,6 @@ static const struct sco_param sco_param_wideband[] = {
> >       { EDR_ESCO_MASK | ESCO_EV3,   0x0008 }, /* T1 */
> > };
> >
> > -static void hci_le_create_connection(struct hci_conn *conn)
> > -{
> > -     struct hci_dev *hdev = conn->hdev;
> > -     struct hci_cp_le_create_conn cp;
> > -
> > -     memset(&cp, 0, sizeof(cp));
> > -     cp.scan_interval = __constant_cpu_to_le16(0x0060);
> > -     cp.scan_window = __constant_cpu_to_le16(0x0030);
> > -     bacpy(&cp.peer_addr, &conn->dst);
> > -     cp.peer_addr_type = conn->dst_type;
> > -     cp.conn_interval_min = __constant_cpu_to_le16(0x0028);
> > -     cp.conn_interval_max = __constant_cpu_to_le16(0x0038);
> > -     cp.supervision_timeout = __constant_cpu_to_le16(0x002a);
> > -     cp.min_ce_len = __constant_cpu_to_le16(0x0000);
> > -     cp.max_ce_len = __constant_cpu_to_le16(0x0000);
> > -
> > -     hci_send_cmd(hdev, HCI_OP_LE_CREATE_CONN, sizeof(cp), &cp);
> > -}
> > -
> > static void hci_le_create_connection_cancel(struct hci_conn *conn)
> > {
> >       hci_send_cmd(conn->hdev, HCI_OP_LE_CREATE_CONN_CANCEL, 0, NULL);
>
> I really start to dislike all the super long naming here. Existing one and new that you are defining.
>
> Why not name the new handler hci_le_create_conn() instead of all this initiate_something and remove the existing function at the same time. I mean you can bisect the code, but it will throw a warning of an unused function.

Ok, I'll rename this function.

As said in previous patch, I'll squash this patch into 2/7.

Regards,

Andre

Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 2:11 AM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> Hi Andre,
>
> > This patch removes the hci_cs_le_create_conn event handler since this
> > handling is now done in create_le_connection_complete() callback in
> > hci_conn.c.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> > ---
> > net/bluetooth/hci_event.c | 31 -------------------------------
> > 1 file changed, 31 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index d171c04b..1d1ffa6 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hd=
ev, u8 status)
> >       hci_dev_unlock(hdev);
> > }
> >
> > -static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
> > -{
> > -     struct hci_conn *conn;
> > -
> > -     BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > -
> > -     if (status) {
> > -             hci_dev_lock(hdev);
> > -
> > -             conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CON=
NECT);
> > -             if (!conn) {
> > -                     hci_dev_unlock(hdev);
> > -                     return;
> > -             }
> > -
> > -             BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, =
conn);
> > -
> > -             conn->state =3D BT_CLOSED;
> > -             mgmt_connect_failed(hdev, &conn->dst, conn->type,
> > -                                 conn->dst_type, status);
> > -             hci_proto_connect_cfm(conn, status);
> > -             hci_conn_del(conn);
> > -
> > -             hci_dev_unlock(hdev);
> > -     }
> > -}
>
> this is dangerous since it actually breaks bisection. The code is never c=
omplete. So while this might turn into a larger patch, you might need to do=
 it all 3 patches at once. With a length commit message explaining exactly =
what happens and why this is correct.

I failed to see how this breaks bisection since the handling is
already done in initiate_le_connection_complete(). However, as
commented in patch 2/7, I'll squash this into patch 2/7 as you
suggested.

Regards,

Andre

Re: [PATCH 5/7] Bluetooth: Refactor hci_connect_le

[Andre Guedes]

Hi Lizardo,

On Tue, Oct 1, 2013 at 9:05 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
>
> Hi Guedes,
>
> On Tue, Oct 1, 2013 at 7:03 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> > +       /* If already exists a hci_conn object for the following connection
> > +        * attempt, we simply update pending_sec_level and auth_type fields
> > +        * and return the object found.
> > +        */
>
> Small textual improvement: "If a hci_conn object already exists [...]"

I'll fix it.

>
>
>
> >         le = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> > -       if (!le) {
> > -               le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > -               if (le)
> > -                       return ERR_PTR(-EBUSY);
> > -
> > -               le = hci_conn_add(hdev, LE_LINK, dst);
> > -               if (!le)
> > -                       return ERR_PTR(-ENOMEM);
> > -
> > -               le->dst_type = bdaddr_to_le(dst_type);
> > -               le->state = BT_CONNECT;
> > -               le->out = true;
> > -               le->link_mode |= HCI_LM_MASTER;
> > -               le->sec_level = BT_SECURITY_LOW;
> > -
> > -               err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> > -               if (err) {
> > -                       hci_conn_del(le);
> > -                       return ERR_PTR(err);
> > -               }
> > +       if (le) {
> > +               le->pending_sec_level = sec_level;
> > +               le->auth_type = auth_type;
> > +               goto out;
> >         }
> >
> > -       le->pending_sec_level = sec_level;
> > +       /* Since the controller supports only one LE connection attempt at the
> > +        * time, we return busy if there is any connection attempt running.
> > +        */
>
> s/at the time/at a time/
> s/busy/EBUSY/

I'll fix it.

>
>
>
> > +       le = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > +       if (le)
> > +               return ERR_PTR(-EBUSY);
> > +
> > +       le = hci_conn_add(hdev, LE_LINK, dst);
> > +       if (!le)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       le->dst_type = bdaddr_to_le(dst_type);
> > +       le->state = BT_CONNECT;
> > +       le->out = true;
> > +       le->link_mode |= HCI_LM_MASTER;
> > +       le->sec_level = BT_SECURITY_LOW;
> > +       le->pending_sec_level = BT_SECURITY_LOW;
>
> I think the previous statement should be:
>
> le->pending_sec_level = sec_level;
>
> Otherwise, we are changing semantics.

Yes, you're right. I'll fix this.

>
>
> >         le->auth_type = auth_type;
> >
> > -       hci_conn_hold(le);
> > +       err = hci_initiate_le_connection(hdev, &le->dst, le->dst_type);
> > +       if (err) {
> > +               hci_conn_del(le);
> > +               return ERR_PTR(err);
> > +       }
> >
> > +out:
> > +       hci_conn_hold(le);
> >         return le;
> >  }

Thanks for reviewing,

Andre

Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 2:21 AM, Marcel Holtmann <marcel@holtmann.org> wrote=
:
>
> Hi Andre,
>
> > This patch does some code refactorig in LE Connection Complete HCI
> > event handler. It basically adds a switch statement to separate new
> > master connection code from new slave connection code.
> >
> > Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> > ---
> > include/net/bluetooth/hci.h |  1 +
> > net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++------=
-------
> > 2 files changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 7ede266..8c98f60 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
> >
> > /* Low energy meta events */
> > #define LE_CONN_ROLE_MASTER   0x00
> > +#define LE_CONN_ROLE_SLAVE   0x01
> >
> > #define HCI_EV_LE_CONN_COMPLETE               0x01
> > struct hci_ev_le_conn_complete {
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 1d1ffa6..0e4a9f4 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_=
dev *hdev, struct sk_buff *skb)
> >
> >       hci_dev_lock(hdev);
> >
> > -     conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> > -     if (!conn) {
> > +     if (ev->status) {
> > +             conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CON=
NECT);
> > +             if (!conn)
> > +                     goto unlock;
> > +
> > +             mgmt_connect_failed(hdev, &conn->dst, conn->type,
> > +                                 conn->dst_type, ev->status);
> > +             hci_proto_connect_cfm(conn, ev->status);
> > +             conn->state =3D BT_CLOSED;
> > +             hci_conn_del(conn);
> > +             goto unlock;
> > +     }
> > +
> > +     switch (ev->role) {
> > +     case LE_CONN_ROLE_MASTER:
> > +             conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdad=
dr);
> > +             /* If there is no hci_conn object with the given address,=
 it
> > +              * means this new connection was triggered through HCI so=
cket
> > +              * interface. For that case, we should create a new hci_c=
onn
> > +              * object.
> > +              */
>
> this comments belong one level down inside the if block. You already comm=
enting on the negative outcome of the if check.

Ok, I'll fix it.

>
>
> > +             if (!conn) {
> > +                     conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr)=
;
> > +                     if (!conn) {
> > +                             BT_ERR("No memory for new connection");
> > +                             goto unlock;
> > +                     }
> > +
> > +                     conn->out =3D true;
> > +                     conn->link_mode |=3D HCI_LM_MASTER;
> > +                     conn->sec_level =3D BT_SECURITY_LOW;
> > +                     conn->dst_type =3D ev->bdaddr_type;
> > +             }
> > +             break;
> > +
> > +     case LE_CONN_ROLE_SLAVE:
>
> And why are we not checking for an existing connection here? At least a s=
mall comment is needed to make that part clear.

Differently from master connection, there is no existing hci_conn for
slave connections. For that reason we don't check for an existing
connection here. I'll add a comment.

>
>
> >               conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
> >               if (!conn) {
> >                       BT_ERR("No memory for new connection");
> > @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci=
_dev *hdev, struct sk_buff *skb)
> >               }
> >
> >               conn->dst_type =3D ev->bdaddr_type;
> > +             conn->sec_level =3D BT_SECURITY_LOW;
> > +             break;
> >
> > -             if (ev->role =3D=3D LE_CONN_ROLE_MASTER) {
> > -                     conn->out =3D true;
> > -                     conn->link_mode |=3D HCI_LM_MASTER;
> > -             }
> > -     }
> > -
> > -     if (ev->status) {
> > -             mgmt_connect_failed(hdev, &conn->dst, conn->type,
> > -                                 conn->dst_type, ev->status);
> > -             hci_proto_connect_cfm(conn, ev->status);
> > -             conn->state =3D BT_CLOSED;
> > -             hci_conn_del(conn);
> > +     default:
> > +             BT_ERR("Used reserved Role parameter %d", ev->role);
> >               goto unlock;
> >       }
> >
> > @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_d=
ev *hdev, struct sk_buff *skb)
> >               mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
> >                                     conn->dst_type, 0, NULL, 0, NULL);
> >
> > -     conn->sec_level =3D BT_SECURITY_LOW;
> >       conn->handle =3D __le16_to_cpu(ev->handle);
> >       conn->state =3D BT_CONNECTED;
>
> All in all, I am not really understanding why this makes it this code sim=
pler. I actually think it turns it into more complicated code. So please ex=
plain what we are really gaining here. I just see more hash table lookup an=
d for hci_conn_add calls with more error checks.

When controller sends a LE Connection Complete event to host, we have
three different handling: failure, new master connection and new slave
connection. Additionally, new master connection has a special handling
since the connection could be triggered by HCI socket interface.
Before, all these logic were mixed up, making hard to add specific
code for new master connection for instance.

So this patch explicitly separates the three different handling and
adds extra comments aiming to improve hci_le_conn_complete_evt()
readability. Besides that, keeping these different handling separated,
it'll be easier to add the auto connection hooks.

Regards,

Andre

Re: [PATCH 7/7] Bluetooth: Locking in hci_le_conn_complete_evt

[Andre Guedes]

Hi Marcel,

On Wed, Oct 2, 2013 at 2:23 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Andre,
>
> > This patch moves hci_dev_lock and hci_dev_unlock calls to where they
> > are really required, reducing the critical region in hci_le_conn_
> > complete_evt function. hdev->lock is required only in hci_conn_del
> > and hci_conn_add call to protect concurrent add and remove operations
> > in hci_conn_hash list.
>
> is this statement actually true? Because we have done this for so many HCI event, that I highly doubt that your statement tis correct. And if it is correct, you need to fix all other users first.

Well, if we take a look at each function called inside
le_conn_complete_evt(), we'll find:
* hci_conn_hash_lookup_state which traverses hdev->conn_hash
(protected by RCU). So it doesn't require hdev->lock.
* mgmt_connect_failed which accesses hdev->id. hdev->id is written
only at hci_register_dev(). So it doesn't require hdev->lock.
* hci_proto_connect_cfm: it handles connection layer stuff, So it
doesn't require hdev->lock.
* mgmt_device_connected which access hdev->name. hdev->name is written
only at hci_register_dev(). So it doesn't require hdev->lock.
* hci_conn_add_sysfs which access only hdev->name. So it doesn't
require hdev->lock.
* hci_conn_del which removes a element from hdev->conn_hash. Since
hdev->conn_hash is protected by RCU, we have to guarantee updaters
mutual exclusion. So it requires hdev->lock.
* hci_conn_add which adds a new element to hdev->conn_hash. Since we
have to guarantee updaters mutual exclusion, it requires hdev->lock.

That being said, I'm not sure the same applies for all others HCI
handlers. We have to analyze each handler to make sure we can safely
move the locking.

Regards,

Andre

Re: [PATCH 4/7] Bluetooth: Remove hci_cs_le_create_conn event handler

[Marcel Holtmann]

Hi Andre,

>>> This patch removes the hci_cs_le_create_conn event handler since this
>>> handling is now done in create_le_connection_complete() callback in
>>> hci_conn.c.
>>> 
>>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>>> ---
>>> net/bluetooth/hci_event.c | 31 -------------------------------
>>> 1 file changed, 31 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index d171c04b..1d1ffa6 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -1465,33 +1465,6 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
>>>      hci_dev_unlock(hdev);
>>> }
>>> 
>>> -static void hci_cs_le_create_conn(struct hci_dev *hdev, __u8 status)
>>> -{
>>> -     struct hci_conn *conn;
>>> -
>>> -     BT_DBG("%s status 0x%2.2x", hdev->name, status);
>>> -
>>> -     if (status) {
>>> -             hci_dev_lock(hdev);
>>> -
>>> -             conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>>> -             if (!conn) {
>>> -                     hci_dev_unlock(hdev);
>>> -                     return;
>>> -             }
>>> -
>>> -             BT_DBG("%s bdaddr %pMR conn %p", hdev->name, &conn->dst, conn);
>>> -
>>> -             conn->state = BT_CLOSED;
>>> -             mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>> -                                 conn->dst_type, status);
>>> -             hci_proto_connect_cfm(conn, status);
>>> -             hci_conn_del(conn);
>>> -
>>> -             hci_dev_unlock(hdev);
>>> -     }
>>> -}
>> 
>> this is dangerous since it actually breaks bisection. The code is never complete. So while this might turn into a larger patch, you might need to do it all 3 patches at once. With a length commit message explaining exactly what happens and why this is correct.
> 
> I failed to see how this breaks bisection since the handling is
> already done in initiate_le_connection_complete(). However, as
> commented in patch 2/7, I'll squash this into patch 2/7 as you
> suggested.

once you actually run the code it will break. Which means that bisecting is not possible anymore since now you are chasing the bug of duplicated connection handling.

Regards

Marcel

Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler

[Marcel Holtmann]

Hi Andre,

>>> This patch does some code refactorig in LE Connection Complete HCI
>>> event handler. It basically adds a switch statement to separate new
>>> master connection code from new slave connection code.
>>> 
>>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>>> ---
>>> include/net/bluetooth/hci.h |  1 +
>>> net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++-------------
>>> 2 files changed, 41 insertions(+), 15 deletions(-)
>>> 
>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>> index 7ede266..8c98f60 100644
>>> --- a/include/net/bluetooth/hci.h
>>> +++ b/include/net/bluetooth/hci.h
>>> @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
>>> 
>>> /* Low energy meta events */
>>> #define LE_CONN_ROLE_MASTER   0x00
>>> +#define LE_CONN_ROLE_SLAVE   0x01
>>> 
>>> #define HCI_EV_LE_CONN_COMPLETE               0x01
>>> struct hci_ev_le_conn_complete {
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index 1d1ffa6..0e4a9f4 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> 
>>>      hci_dev_lock(hdev);
>>> 
>>> -     conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>>> -     if (!conn) {
>>> +     if (ev->status) {
>>> +             conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>>> +             if (!conn)
>>> +                     goto unlock;
>>> +
>>> +             mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>> +                                 conn->dst_type, ev->status);
>>> +             hci_proto_connect_cfm(conn, ev->status);
>>> +             conn->state = BT_CLOSED;
>>> +             hci_conn_del(conn);
>>> +             goto unlock;
>>> +     }
>>> +
>>> +     switch (ev->role) {
>>> +     case LE_CONN_ROLE_MASTER:
>>> +             conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bdaddr);
>>> +             /* If there is no hci_conn object with the given address, it
>>> +              * means this new connection was triggered through HCI socket
>>> +              * interface. For that case, we should create a new hci_conn
>>> +              * object.
>>> +              */
>> 
>> this comments belong one level down inside the if block. You already commenting on the negative outcome of the if check.
> 
> Ok, I'll fix it.
> 
>> 
>> 
>>> +             if (!conn) {
>>> +                     conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
>>> +                     if (!conn) {
>>> +                             BT_ERR("No memory for new connection");
>>> +                             goto unlock;
>>> +                     }
>>> +
>>> +                     conn->out = true;
>>> +                     conn->link_mode |= HCI_LM_MASTER;
>>> +                     conn->sec_level = BT_SECURITY_LOW;
>>> +                     conn->dst_type = ev->bdaddr_type;
>>> +             }
>>> +             break;
>>> +
>>> +     case LE_CONN_ROLE_SLAVE:
>> 
>> And why are we not checking for an existing connection here? At least a small comment is needed to make that part clear.
> 
> Differently from master connection, there is no existing hci_conn for
> slave connections. For that reason we don't check for an existing
> connection here. I'll add a comment.
> 
>> 
>> 
>>>              conn = hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
>>>              if (!conn) {
>>>                      BT_ERR("No memory for new connection");
>>> @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>>              }
>>> 
>>>              conn->dst_type = ev->bdaddr_type;
>>> +             conn->sec_level = BT_SECURITY_LOW;
>>> +             break;
>>> 
>>> -             if (ev->role == LE_CONN_ROLE_MASTER) {
>>> -                     conn->out = true;
>>> -                     conn->link_mode |= HCI_LM_MASTER;
>>> -             }
>>> -     }
>>> -
>>> -     if (ev->status) {
>>> -             mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>> -                                 conn->dst_type, ev->status);
>>> -             hci_proto_connect_cfm(conn, ev->status);
>>> -             conn->state = BT_CLOSED;
>>> -             hci_conn_del(conn);
>>> +     default:
>>> +             BT_ERR("Used reserved Role parameter %d", ev->role);
>>>              goto unlock;
>>>      }
>>> 
>>> @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>>              mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
>>>                                    conn->dst_type, 0, NULL, 0, NULL);
>>> 
>>> -     conn->sec_level = BT_SECURITY_LOW;
>>>      conn->handle = __le16_to_cpu(ev->handle);
>>>      conn->state = BT_CONNECTED;
>> 
>> All in all, I am not really understanding why this makes it this code simpler. I actually think it turns it into more complicated code. So please explain what we are really gaining here. I just see more hash table lookup and for hci_conn_add calls with more error checks.
> 
> When controller sends a LE Connection Complete event to host, we have
> three different handling: failure, new master connection and new slave
> connection. Additionally, new master connection has a special handling
> since the connection could be triggered by HCI socket interface.
> Before, all these logic were mixed up, making hard to add specific
> code for new master connection for instance.

I do not follow the difference between master and slave roles. Why do we have a difference here in the first place.

> So this patch explicitly separates the three different handling and
> adds extra comments aiming to improve hci_le_conn_complete_evt()
> readability. Besides that, keeping these different handling separated,
> it'll be easier to add the auto connection hooks.

I am getting the feeling the overall code gets more complicated than simpler. The goal is to make the connection handling simpler. And right now, I do not see this.

Regards

Marcel

Re: [PATCH 6/7] Bluetooth: Refactor LE Connection Complete HCI event handler

[Andre Guedes]

Hi Marcel,

On Thu, Oct 3, 2013 at 11:15 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andre,
>
>>>> This patch does some code refactorig in LE Connection Complete HCI
>>>> event handler. It basically adds a switch statement to separate new
>>>> master connection code from new slave connection code.
>>>>
>>>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>>>> ---
>>>> include/net/bluetooth/hci.h |  1 +
>>>> net/bluetooth/hci_event.c   | 55 ++++++++++++++++++++++++++++++++-----=
--------
>>>> 2 files changed, 41 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>> index 7ede266..8c98f60 100644
>>>> --- a/include/net/bluetooth/hci.h
>>>> +++ b/include/net/bluetooth/hci.h
>>>> @@ -1442,6 +1442,7 @@ struct hci_ev_num_comp_blocks {
>>>>
>>>> /* Low energy meta events */
>>>> #define LE_CONN_ROLE_MASTER   0x00
>>>> +#define LE_CONN_ROLE_SLAVE   0x01
>>>>
>>>> #define HCI_EV_LE_CONN_COMPLETE               0x01
>>>> struct hci_ev_le_conn_complete {
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index 1d1ffa6..0e4a9f4 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -3444,8 +3444,42 @@ static void hci_le_conn_complete_evt(struct hci=
_dev *hdev, struct sk_buff *skb)
>>>>
>>>>      hci_dev_lock(hdev);
>>>>
>>>> -     conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>>>> -     if (!conn) {
>>>> +     if (ev->status) {
>>>> +             conn =3D hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CO=
NNECT);
>>>> +             if (!conn)
>>>> +                     goto unlock;
>>>> +
>>>> +             mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>>> +                                 conn->dst_type, ev->status);
>>>> +             hci_proto_connect_cfm(conn, ev->status);
>>>> +             conn->state =3D BT_CLOSED;
>>>> +             hci_conn_del(conn);
>>>> +             goto unlock;
>>>> +     }
>>>> +
>>>> +     switch (ev->role) {
>>>> +     case LE_CONN_ROLE_MASTER:
>>>> +             conn =3D hci_conn_hash_lookup_ba(hdev, LE_LINK, &ev->bda=
ddr);
>>>> +             /* If there is no hci_conn object with the given address=
, it
>>>> +              * means this new connection was triggered through HCI s=
ocket
>>>> +              * interface. For that case, we should create a new hci_=
conn
>>>> +              * object.
>>>> +              */
>>>
>>> this comments belong one level down inside the if block. You already co=
mmenting on the negative outcome of the if check.
>>
>> Ok, I'll fix it.
>>
>>>
>>>
>>>> +             if (!conn) {
>>>> +                     conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr=
);
>>>> +                     if (!conn) {
>>>> +                             BT_ERR("No memory for new connection");
>>>> +                             goto unlock;
>>>> +                     }
>>>> +
>>>> +                     conn->out =3D true;
>>>> +                     conn->link_mode |=3D HCI_LM_MASTER;
>>>> +                     conn->sec_level =3D BT_SECURITY_LOW;
>>>> +                     conn->dst_type =3D ev->bdaddr_type;
>>>> +             }
>>>> +             break;
>>>> +
>>>> +     case LE_CONN_ROLE_SLAVE:
>>>
>>> And why are we not checking for an existing connection here? At least a=
 small comment is needed to make that part clear.
>>
>> Differently from master connection, there is no existing hci_conn for
>> slave connections. For that reason we don't check for an existing
>> connection here. I'll add a comment.
>>
>>>
>>>
>>>>              conn =3D hci_conn_add(hdev, LE_LINK, &ev->bdaddr);
>>>>              if (!conn) {
>>>>                      BT_ERR("No memory for new connection");
>>>> @@ -3453,19 +3487,11 @@ static void hci_le_conn_complete_evt(struct hc=
i_dev *hdev, struct sk_buff *skb)
>>>>              }
>>>>
>>>>              conn->dst_type =3D ev->bdaddr_type;
>>>> +             conn->sec_level =3D BT_SECURITY_LOW;
>>>> +             break;
>>>>
>>>> -             if (ev->role =3D=3D LE_CONN_ROLE_MASTER) {
>>>> -                     conn->out =3D true;
>>>> -                     conn->link_mode |=3D HCI_LM_MASTER;
>>>> -             }
>>>> -     }
>>>> -
>>>> -     if (ev->status) {
>>>> -             mgmt_connect_failed(hdev, &conn->dst, conn->type,
>>>> -                                 conn->dst_type, ev->status);
>>>> -             hci_proto_connect_cfm(conn, ev->status);
>>>> -             conn->state =3D BT_CLOSED;
>>>> -             hci_conn_del(conn);
>>>> +     default:
>>>> +             BT_ERR("Used reserved Role parameter %d", ev->role);
>>>>              goto unlock;
>>>>      }
>>>>
>>>> @@ -3473,7 +3499,6 @@ static void hci_le_conn_complete_evt(struct hci_=
dev *hdev, struct sk_buff *skb)
>>>>              mgmt_device_connected(hdev, &ev->bdaddr, conn->type,
>>>>                                    conn->dst_type, 0, NULL, 0, NULL);
>>>>
>>>> -     conn->sec_level =3D BT_SECURITY_LOW;
>>>>      conn->handle =3D __le16_to_cpu(ev->handle);
>>>>      conn->state =3D BT_CONNECTED;
>>>
>>> All in all, I am not really understanding why this makes it this code s=
impler. I actually think it turns it into more complicated code. So please =
explain what we are really gaining here. I just see more hash table lookup =
and for hci_conn_add calls with more error checks.
>>
>> When controller sends a LE Connection Complete event to host, we have
>> three different handling: failure, new master connection and new slave
>> connection. Additionally, new master connection has a special handling
>> since the connection could be triggered by HCI socket interface.
>> Before, all these logic were mixed up, making hard to add specific
>> code for new master connection for instance.
>
> I do not follow the difference between master and slave roles. Why do we =
have a difference here in the first place.
>
>> So this patch explicitly separates the three different handling and
>> adds extra comments aiming to improve hci_le_conn_complete_evt()
>> readability. Besides that, keeping these different handling separated,
>> it'll be easier to add the auto connection hooks.
>
> I am getting the feeling the overall code gets more complicated than simp=
ler. The goal is to make the connection handling simpler. And right now, I =
do not see this.

Ok, I'll drop this patch in v2.

I'll recheck if we really need to differentiate between master and
slave new connection for auto connection. If positive, I can add this
patch to auto connection patchset.

Thanks,

Andre