[PATCH v2 0/4] LE connection refactoring and fixes

[PATCH v2 0/4] LE connection refactoring and fixes

[Andre Guedes]

Hi all,

This v2 takes in consideration comments from Marcel and Lizardo. This patchset
is smaller since I squashed 3 patches and removed the last two patches from
the previous patchset.

Regards,

Andre


Andre Guedes (4):
  Bluetooth: Rename hci_conn variable in hci_connect_le()
  Bluetooth: Initialize hci_conn fields in hci_connect_le
  Bluetooth: Use HCI request for LE connection
  Bluetooth: Refactor hci_connect_le

 include/net/bluetooth/hci_core.h |  2 +
 net/bluetooth/hci_conn.c         | 81 ++++++++++++++++++++--------------------
 net/bluetooth/hci_core.c         | 46 +++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 31 ---------------
 4 files changed, 89 insertions(+), 71 deletions(-)

-- 
1.8.4

[PATCH v2 1/4] Bluetooth: Rename hci_conn variable in hci_connect_le()

[Andre Guedes]

This patch simply rename the hci_conn variable "le" to "conn"
since it is a better name.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 514148b..5aba520 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -549,31 +549,31 @@ EXPORT_SYMBOL(hci_get_route);
 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;
+	struct hci_conn *conn;
 
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
 
-	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)
+	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
+	if (!conn) {
+		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+		if (conn)
 			return ERR_PTR(-EBUSY);
 
-		le = hci_conn_add(hdev, LE_LINK, dst);
-		if (!le)
+		conn = hci_conn_add(hdev, LE_LINK, dst);
+		if (!conn)
 			return ERR_PTR(-ENOMEM);
 
-		le->dst_type = bdaddr_to_le(dst_type);
-		hci_le_create_connection(le);
+		conn->dst_type = bdaddr_to_le(dst_type);
+		hci_le_create_connection(conn);
 	}
 
-	le->pending_sec_level = sec_level;
-	le->auth_type = auth_type;
+	conn->pending_sec_level = sec_level;
+	conn->auth_type = auth_type;
 
-	hci_conn_hold(le);
+	hci_conn_hold(conn);
 
-	return le;
+	return conn;
 }
 
 static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
-- 
1.8.4

Re: [PATCH v2 1/4] Bluetooth: Rename hci_conn variable in hci_connect_le()

[Marcel Holtmann]

Hi Andre,

> This patch simply rename the hci_conn variable "le" to "conn"
> since it is a better name.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v2 2/4] 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 5aba520..08e601c 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);
 
 		conn->dst_type = bdaddr_to_le(dst_type);
+		conn->state = BT_CONNECT;
+		conn->out = true;
+		conn->link_mode |= HCI_LM_MASTER;
+		conn->sec_level = BT_SECURITY_LOW;
+
 		hci_le_create_connection(conn);
 	}
 
-- 
1.8.4

Re: [PATCH v2 2/4] 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(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v2 3/4] Bluetooth: Use HCI request for LE connection

[Andre Guedes]

This patch introduces a new helper, which uses the HCI request
framework, for creating LE connectons. All the handling is now
done by this function so we can remove the hci_cs_le_create_conn()
event handler.

This patch also removes the old hci_le_create_connection() since
it is not used anymore.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_conn.c         | 26 ++++++-----------------
 net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/hci_event.c        | 31 ---------------------------
 4 files changed, 54 insertions(+), 51 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e09c305..0835cf9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1217,6 +1217,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
 
 u8 bdaddr_to_le(u8 bdaddr_type);
 
+int hci_create_le_conn(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 08e601c..cb0e5d7 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);
@@ -545,6 +526,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 *conn;
+	int err;
 
 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
 		return ERR_PTR(-ENOTSUPP);
@@ -565,7 +547,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 		conn->link_mode |= HCI_LM_MASTER;
 		conn->sec_level = BT_SECURITY_LOW;
 
-		hci_le_create_connection(conn);
+		err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
+		if (err) {
+			hci_conn_del(conn);
+			return ERR_PTR(err);
+		}
 	}
 
 	conn->pending_sec_level = sec_level;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 82dbdc6..1002510 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3662,3 +3662,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
 		return ADDR_LE_DEV_RANDOM;
 	}
 }
+
+static void create_le_conn_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_create_le_conn(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, create_le_conn_complete);
+}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 4785ab0..a6c6c17 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1470,33 +1470,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;
@@ -2347,10 +2320,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

Re: [PATCH v2 3/4] Bluetooth: Use HCI request for LE connection

[Marcel Holtmann]

Hi Andre,

> This patch introduces a new helper, which uses the HCI request
> framework, for creating LE connectons. All the handling is now
> done by this function so we can remove the hci_cs_le_create_conn()
> event handler.
> 
> This patch also removes the old hci_le_create_connection() since
> it is not used anymore.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_conn.c         | 26 ++++++-----------------
> net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/hci_event.c        | 31 ---------------------------
> 4 files changed, 54 insertions(+), 51 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index e09c305..0835cf9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1217,6 +1217,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
> 
> u8 bdaddr_to_le(u8 bdaddr_type);
> 
> +int hci_create_le_conn(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 08e601c..cb0e5d7 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);
> @@ -545,6 +526,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 *conn;
> +	int err;
> 
> 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
> 		return ERR_PTR(-ENOTSUPP);
> @@ -565,7 +547,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
> 		conn->link_mode |= HCI_LM_MASTER;
> 		conn->sec_level = BT_SECURITY_LOW;
> 
> -		hci_le_create_connection(conn);
> +		err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
> +		if (err) {
> +			hci_conn_del(conn);
> +			return ERR_PTR(err);
> +		}

it is safe to sleep here? Since that is what the new call is doing.

> 	}
> 
> 	conn->pending_sec_level = sec_level;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 82dbdc6..1002510 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3662,3 +3662,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
> 		return ADDR_LE_DEV_RANDOM;
> 	}
> }
> +
> +static void create_le_conn_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);

This should say "create" instead of "initiate".

> +
> +	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> +	if (!conn)
> +		return;
> +

We are no longer setting conn->state to BT_CLOSED on purpose here?

> +	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);
> +}

You are changing the locking behavior without any explanation. Previous code has it around the whole section. I am not a huge fan of changes that get sneaked in this way. That should be a separate patch to explain why it is correct. This patch should not change the current behavior.

> +
> +int hci_create_le_conn(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, create_le_conn_complete);
> +}

Regards

Marcel

Re: [PATCH v2 3/4] Bluetooth: Use HCI request for LE connection

[Andre Guedes]

Hi Marcel,

On 10/04/2013 03:25 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch introduces a new helper, which uses the HCI request
>> framework, for creating LE connectons. All the handling is now
>> done by this function so we can remove the hci_cs_le_create_conn()
>> event handler.
>>
>> This patch also removes the old hci_le_create_connection() since
>> it is not used anymore.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |  2 ++
>> net/bluetooth/hci_conn.c         | 26 ++++++-----------------
>> net/bluetooth/hci_core.c         | 46 ++++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/hci_event.c        | 31 ---------------------------
>> 4 files changed, 54 insertions(+), 51 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index e09c305..0835cf9 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -1217,6 +1217,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __u8 rand[8],
>>
>> u8 bdaddr_to_le(u8 bdaddr_type);
>>
>> +int hci_create_le_conn(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 08e601c..cb0e5d7 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);
>> @@ -545,6 +526,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 *conn;
>> +	int err;
>>
>> 	if (test_bit(HCI_LE_PERIPHERAL, &hdev->flags))
>> 		return ERR_PTR(-ENOTSUPP);
>> @@ -565,7 +547,11 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>> 		conn->link_mode |= HCI_LM_MASTER;
>> 		conn->sec_level = BT_SECURITY_LOW;
>>
>> -		hci_le_create_connection(conn);
>> +		err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
>> +		if (err) {
>> +			hci_conn_del(conn);
>> +			return ERR_PTR(err);
>> +		}
>
> it is safe to sleep here? Since that is what the new call is doing.

We are not running in an atomic section here so we are allowed to sleep.

Actually, there are other calls in this flow that may sleep. If you take 
a look at a few lines up in this function, you'll find a call to 
hci_conn_add() which allocates memory using GFP_KERNEL flag (so it may 
sleep).

Anyway, I always compile my kernels using all those kernel hacking 
options which detects errors like that and it doesn't complain about this.

>
>> 	}
>>
>> 	conn->pending_sec_level = sec_level;
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 82dbdc6..1002510 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -3662,3 +3662,49 @@ u8 bdaddr_to_le(u8 bdaddr_type)
>> 		return ADDR_LE_DEV_RANDOM;
>> 	}
>> }
>> +
>> +static void create_le_conn_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);
>
> This should say "create" instead of "initiate".

I'll fix it.

>
>> +
>> +	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>> +	if (!conn)
>> +		return;
>> +
>
> We are no longer setting conn->state to BT_CLOSED on purpose here?

No, this is an error. I'll fix it in v3.

>
>> +	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);
>> +}
>
> You are changing the locking behavior without any explanation. Previous code has it around the whole section. I am not a huge fan of changes that get sneaked in this way. That should be a separate patch to explain why it is correct. This patch should not change the current behavior.

Sure, I'll fix it.

Regards,

Andre

[PATCH v2 4/4] Bluetooth: Refactor hci_connect_le

[Andre Guedes]

This patch does some code refactoring in hci_connect_le() by moving
the exception code into if statements and letting the main flow in
first level of function scope. It also adds extra comments to improve
the code readability.

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

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index cb0e5d7..ff15841 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -531,34 +531,49 @@ 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);
 
+	/* XXX: Some devices send ATT messages as soon as the physical link
+	 * is established. To be able to handle these ATT messages, the user-
+	 * space first establishes the connection and then starts the pairing
+	 * process.
+	 *
+	 * So if a hci_conn object already exists for the following connection
+	 * attempt, we simply update pending_sec_level and auth_type fields
+	 * and return the object found.
+	 */
 	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
-	if (!conn) {
-		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
-		if (conn)
-			return ERR_PTR(-EBUSY);
-
-		conn = hci_conn_add(hdev, LE_LINK, dst);
-		if (!conn)
-			return ERR_PTR(-ENOMEM);
+	if (conn) {
+		conn->pending_sec_level = sec_level;
+		conn->auth_type = auth_type;
+		goto out;
+	}
 
-		conn->dst_type = bdaddr_to_le(dst_type);
-		conn->state = BT_CONNECT;
-		conn->out = true;
-		conn->link_mode |= HCI_LM_MASTER;
-		conn->sec_level = BT_SECURITY_LOW;
+	/* Since the controller supports only one LE connection attempt at a
+	 * time, we return -EBUSY if there is any connection attempt running.
+	 */
+	conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
+	if (conn)
+		return ERR_PTR(-EBUSY);
 
-		err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
-		if (err) {
-			hci_conn_del(conn);
-			return ERR_PTR(err);
-		}
-	}
+	conn = hci_conn_add(hdev, LE_LINK, dst);
+	if (!conn)
+		return ERR_PTR(-ENOMEM);
 
+	conn->dst_type = bdaddr_to_le(dst_type);
+	conn->state = BT_CONNECT;
+	conn->out = true;
+	conn->link_mode |= HCI_LM_MASTER;
+	conn->sec_level = BT_SECURITY_LOW;
 	conn->pending_sec_level = sec_level;
 	conn->auth_type = auth_type;
 
-	hci_conn_hold(conn);
+	err = hci_create_le_conn(hdev, &conn->dst, conn->dst_type);
+	if (err) {
+		hci_conn_del(conn);
+		return ERR_PTR(err);
+	}
 
+out:
+	hci_conn_hold(conn);
 	return conn;
 }
 
-- 
1.8.4

Re: [PATCH v2 4/4] Bluetooth: Refactor hci_connect_le

[Marcel Holtmann]

Hi Andre,

> This patch does some code refactoring in hci_connect_le() by moving
> the exception code into if statements and letting the main flow in
> first level of function scope. It also adds extra comments to improve
> the code readability.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
> net/bluetooth/hci_conn.c | 55 ++++++++++++++++++++++++++++++------------------
> 1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index cb0e5d7..ff15841 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -531,34 +531,49 @@ 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);
> 
> +	/* XXX: Some devices send ATT messages as soon as the physical link

no idea what the XXX wants to tell me here.

> +	 * is established. To be able to handle these ATT messages, the user-
> +	 * space first establishes the connection and then starts the pairing
> +	 * process.
> +	 *
> +	 * So if a hci_conn object already exists for the following connection
> +	 * attempt, we simply update pending_sec_level and auth_type fields
> +	 * and return the object found.
> +	 */
> 	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
> -	if (!conn) {
> -		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
> -		if (conn)
> -			return ERR_PTR(-EBUSY);
> -
> -		conn = hci_conn_add(hdev, LE_LINK, dst);
> -		if (!conn)
> -			return ERR_PTR(-ENOMEM);
> +	if (conn) {
> +		conn->pending_sec_level = sec_level;
> +		conn->auth_type = auth_type;
> +		goto out;
> +	}

Call such a label "done". I never really liked the out part.

If anybody wants to fix the few users of "out" as label, that would be welcome.

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

Regards

Marcel

Re: [PATCH v2 4/4] Bluetooth: Refactor hci_connect_le

[Andre Guedes]

Hi Marcel,

On 10/04/2013 03:27 AM, Marcel Holtmann wrote:
> Hi Andre,
>
>> This patch does some code refactoring in hci_connect_le() by moving
>> the exception code into if statements and letting the main flow in
>> first level of function scope. It also adds extra comments to improve
>> the code readability.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_conn.c | 55 ++++++++++++++++++++++++++++++------------------
>> 1 file changed, 35 insertions(+), 20 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index cb0e5d7..ff15841 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -531,34 +531,49 @@ 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);
>>
>> +	/* XXX: Some devices send ATT messages as soon as the physical link
>
> no idea what the XXX wants to tell me here.

The XXX: tag is just a convention programmers use to warn other 
programmers about some problematic or special condition in the code. It 
is used widely in several projects, including the Linux kernel.

I just "grep'ed" and realized we are not used to use XXX: in Bluetooth 
subsystem so I'll remove it.

>
>> +	 * is established. To be able to handle these ATT messages, the user-
>> +	 * space first establishes the connection and then starts the pairing
>> +	 * process.
>> +	 *
>> +	 * So if a hci_conn object already exists for the following connection
>> +	 * attempt, we simply update pending_sec_level and auth_type fields
>> +	 * and return the object found.
>> +	 */
>> 	conn = hci_conn_hash_lookup_ba(hdev, LE_LINK, dst);
>> -	if (!conn) {
>> -		conn = hci_conn_hash_lookup_state(hdev, LE_LINK, BT_CONNECT);
>> -		if (conn)
>> -			return ERR_PTR(-EBUSY);
>> -
>> -		conn = hci_conn_add(hdev, LE_LINK, dst);
>> -		if (!conn)
>> -			return ERR_PTR(-ENOMEM);
>> +	if (conn) {
>> +		conn->pending_sec_level = sec_level;
>> +		conn->auth_type = auth_type;
>> +		goto out;
>> +	}
>
> Call such a label "done". I never really liked the out part.

Ok, I'll rename this label.

Regards,

Andre