[PATCH v2 0/5] Bluetooth: Refactoring hci_connect()

[PATCH v2 0/5] Bluetooth: Refactoring hci_connect()

[Vinicius Costa Gomes]

Hi,

Changes from last version:
 - the functions are now named hci_connect_{acl,le,sco}. I didn't use
 hci_add_* because there is already a function hci_add_sco.

Cheers,

Vinicius Costa Gomes (5):
  Bluetooth: Refactor LE connection into its own function
  Bluetooth: Refactor ACL connection into its own function
  Bluetooth: Refactor SCO connection into its own function
  Bluetooth: Simplify a the connection type handling
  Bluetooth: Add type information to the hci_connect() debug statement

 net/bluetooth/hci_conn.c |   81 ++++++++++++++++++++++++++++++----------------
 1 file changed, 53 insertions(+), 28 deletions(-)

--
1.7.10.3

[PATCH v2 1/5] Bluetooth: Refactor LE connection into its own function

[Vinicius Costa Gomes]

The code that handles LE connection is already quite separated from
the rest of the connection procedure, so we can easily put it into
its own.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_conn.c |   53 +++++++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 2fcced3..31afef7 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -469,6 +469,33 @@ struct hci_dev *hci_get_route(bdaddr_t *dst, bdaddr_t *src)
 }
 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;
+
+	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);
+		hci_le_connect(le);
+	}
+
+	le->pending_sec_level = sec_level;
+	le->auth_type = auth_type;
+
+	hci_conn_hold(le);
+
+	return le;
+}
+
 /* Create SCO, ACL or LE connection.
  * Device _must_ be locked */
 struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
@@ -476,33 +503,11 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 {
 	struct hci_conn *acl;
 	struct hci_conn *sco;
-	struct hci_conn *le;
 
 	BT_DBG("%s dst %s", hdev->name, batostr(dst));
 
-	if (type == LE_LINK) {
-		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);
-			hci_le_connect(le);
-		}
-
-		le->pending_sec_level = sec_level;
-		le->auth_type = auth_type;
-
-		hci_conn_hold(le);
-
-		return le;
-	}
+	if (type == LE_LINK)
+		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
 
 	acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
 	if (!acl) {
-- 
1.7.10.3

[PATCH v2 2/5] Bluetooth: Refactor ACL connection into its own function

[Vinicius Costa Gomes]

The hci_connect() function was starting to get too complicated to be
quickly understood. We can separated the creation of a new ACL
connection into its own function.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_conn.c |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 31afef7..79c80b2 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -496,18 +496,10 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 	return le;
 }
 
-/* Create SCO, ACL or LE connection.
- * Device _must_ be locked */
-struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
-			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
+static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
+						u8 sec_level, u8 auth_type)
 {
 	struct hci_conn *acl;
-	struct hci_conn *sco;
-
-	BT_DBG("%s dst %s", hdev->name, batostr(dst));
-
-	if (type == LE_LINK)
-		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
 
 	acl = hci_conn_hash_lookup_ba(hdev, ACL_LINK, dst);
 	if (!acl) {
@@ -525,6 +517,26 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 		hci_acl_connect(acl);
 	}
 
+	return acl;
+}
+
+/* Create SCO, ACL or LE connection.
+ * Device _must_ be locked */
+struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
+			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
+{
+	struct hci_conn *acl;
+	struct hci_conn *sco;
+
+	BT_DBG("%s dst %s", hdev->name, batostr(dst));
+
+	if (type == LE_LINK)
+		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
+
+	acl = hci_connect_acl(hdev, dst, sec_level, auth_type);
+	if (IS_ERR(acl))
+		return acl;
+
 	if (type == ACL_LINK)
 		return acl;
 
-- 
1.7.10.3

Re: [PATCH v2 2/5] Bluetooth: Refactor ACL connection into its own function

[Gustavo Padovan]

Hi Vinicius,

* Vinicius Costa Gomes <vinicius.gomes@openbossa.org> [2012-05-31 22:57:53 -0300]:

> The hci_connect() function was starting to get too complicated to be
> quickly understood. We can separated the creation of a new ACL
> connection into its own function.
> 
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
>  net/bluetooth/hci_conn.c |   32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 31afef7..79c80b2 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -496,18 +496,10 @@ static struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
>  	return le;
>  }
>  
> -/* Create SCO, ACL or LE connection.
> - * Device _must_ be locked */
> -struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> -			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
> +static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> +						u8 sec_level, u8 auth_type)
>  {

So this is confusing now, we have hci_acl_connect() and now you want to add
hci_connect_acl(). We need better names here. 
Maybe hci_acl_connect() can be turned int hci_acl_create_connetion(), so
can hco_acl_connect_cancel().

	Gustavo

[PATCH v2 3/5] Bluetooth: Refactor SCO connection into its own function

[Vinicius Costa Gomes]

We can do the same that we did for the other link types, for SCO
connections. The only thing that's worth noting is that as SCO
links need an ACL link, this functions uses the function that adds
an ACL link.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_conn.c |   33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 79c80b2..e4fc946 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -520,29 +520,19 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 	return acl;
 }
 
-/* Create SCO, ACL or LE connection.
- * Device _must_ be locked */
-struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
-			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
+static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, bdaddr_t *dst,
+				     u8 sec_level, u8 auth_type)
 {
 	struct hci_conn *acl;
 	struct hci_conn *sco;
 
-	BT_DBG("%s dst %s", hdev->name, batostr(dst));
-
-	if (type == LE_LINK)
-		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
-
 	acl = hci_connect_acl(hdev, dst, sec_level, auth_type);
 	if (IS_ERR(acl))
 		return acl;
 
-	if (type == ACL_LINK)
-		return acl;
-
-	sco = hci_conn_hash_lookup_ba(hdev, type, dst);
+	sco = hci_conn_hash_lookup_ba(hdev, SCO_LINK, dst);
 	if (!sco) {
-		sco = hci_conn_add(hdev, type, dst);
+		sco = hci_conn_add(hdev, SCO_LINK, dst);
 		if (!sco) {
 			hci_conn_put(acl);
 			return ERR_PTR(-ENOMEM);
@@ -571,6 +561,21 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	return sco;
 }
 
+/* Create SCO, ACL or LE connection. */
+struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
+			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
+{
+	BT_DBG("%s dst %s", hdev->name, batostr(dst));
+
+	if (type == LE_LINK)
+		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
+
+	if (type == ACL_LINK)
+		return hci_connect_acl(hdev, dst, sec_level, auth_type);
+
+	return hci_connect_sco(hdev, dst, sec_level, auth_type);
+}
+
 /* Check link security requirement */
 int hci_conn_check_link_mode(struct hci_conn *conn)
 {
-- 
1.7.10.3

[PATCH v2 4/5] Bluetooth: Simplify a the connection type handling

[Vinicius Costa Gomes]

Now that we have separate ways of doing connections for each link type,
we can do better than an "if" statement to handle each link type.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_conn.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e4fc946..70979fc 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -567,13 +567,16 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 {
 	BT_DBG("%s dst %s", hdev->name, batostr(dst));
 
-	if (type == LE_LINK)
+	switch (type) {
+	case LE_LINK:
 		return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
-
-	if (type == ACL_LINK)
+	case ACL_LINK:
 		return hci_connect_acl(hdev, dst, sec_level, auth_type);
+	case SCO_LINK:
+		return hci_connect_sco(hdev, dst, sec_level, auth_type);
+	}
 
-	return hci_connect_sco(hdev, dst, sec_level, auth_type);
+	return ERR_PTR(-EINVAL);
 }
 
 /* Check link security requirement */
-- 
1.7.10.3

[PATCH v2 5/5] Bluetooth: Add type information to the hci_connect() debug statement

[Vinicius Costa Gomes]

Now that we have a "connect" function for each link type, we should be
able to indentify which function is going to be called.

Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
---
 net/bluetooth/hci_conn.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 70979fc..6906ba3 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -565,7 +565,7 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, bdaddr_t *dst,
 struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
 			     __u8 dst_type, __u8 sec_level, __u8 auth_type)
 {
-	BT_DBG("%s dst %s", hdev->name, batostr(dst));
+	BT_DBG("%s dst %s type 0x%x", hdev->name, batostr(dst), type);
 
 	switch (type) {
 	case LE_LINK:
-- 
1.7.10.3