[PATCH 1/3] Bluetooth: Simplify remote features callback function logic

[PATCH 1/3] Bluetooth: Simplify remote features callback function logic

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

The current remote and remote extended features event callbacks logic
can be made simpler by using a label and goto statements instead of the
current multiple levels of nested if statements.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/hci_event.c |   91 +++++++++++++++++++++++----------------------
 1 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 84093b0..8430276 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1162,33 +1162,33 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		if (!ev->status)
-			memcpy(conn->features, ev->features, 8);
+	if (!conn)
+		goto unlock;
 
-		if (conn->state == BT_CONFIG) {
-			if (!ev->status && lmp_ssp_capable(hdev) &&
-						lmp_ssp_capable(conn)) {
-				struct hci_cp_read_remote_ext_features cp;
-				cp.handle = ev->handle;
-				cp.page = 0x01;
-				hci_send_cmd(hdev,
-					HCI_OP_READ_REMOTE_EXT_FEATURES,
-							sizeof(cp), &cp);
-			} else if (!ev->status && conn->out &&
-					conn->sec_level == BT_SECURITY_HIGH) {
-				struct hci_cp_auth_requested cp;
-				cp.handle = ev->handle;
-				hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
+	if (!ev->status)
+		memcpy(conn->features, ev->features, 8);
+
+	if (conn->state != BT_CONFIG)
+		goto unlock;
+
+	if (!ev->status && lmp_ssp_capable(hdev) && lmp_ssp_capable(conn)) {
+		struct hci_cp_read_remote_ext_features cp;
+		cp.handle = ev->handle;
+		cp.page = 0x01;
+		hci_send_cmd(hdev, HCI_OP_READ_REMOTE_EXT_FEATURES,
 							sizeof(cp), &cp);
-			} else {
-				conn->state = BT_CONNECTED;
-				hci_proto_connect_cfm(conn, ev->status);
-				hci_conn_put(conn);
-			}
-		}
+	} else if (!ev->status && conn->out &&
+			conn->sec_level == BT_SECURITY_HIGH) {
+		struct hci_cp_auth_requested cp;
+		cp.handle = ev->handle;
+		hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
+	} else {
+		conn->state = BT_CONNECTED;
+		hci_proto_connect_cfm(conn, ev->status);
+		hci_conn_put(conn);
 	}
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
@@ -1646,32 +1646,35 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(ev->handle));
-	if (conn) {
-		if (!ev->status && ev->page == 0x01) {
-			struct inquiry_entry *ie;
+	if (!conn)
+		goto unlock;
 
-			if ((ie = hci_inquiry_cache_lookup(hdev, &conn->dst)))
-				ie->data.ssp_mode = (ev->features[0] & 0x01);
+	if (!ev->status && ev->page == 0x01) {
+		struct inquiry_entry *ie;
 
-			conn->ssp_mode = (ev->features[0] & 0x01);
-		}
+		if ((ie = hci_inquiry_cache_lookup(hdev, &conn->dst)))
+			ie->data.ssp_mode = (ev->features[0] & 0x01);
 
-		if (conn->state == BT_CONFIG) {
-			if (!ev->status && hdev->ssp_mode > 0 &&
-					conn->ssp_mode > 0 && conn->out &&
-					conn->sec_level != BT_SECURITY_SDP) {
-				struct hci_cp_auth_requested cp;
-				cp.handle = ev->handle;
-				hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
-							sizeof(cp), &cp);
-			} else {
-				conn->state = BT_CONNECTED;
-				hci_proto_connect_cfm(conn, ev->status);
-				hci_conn_put(conn);
-			}
-		}
+		conn->ssp_mode = (ev->features[0] & 0x01);
+	}
+
+	if (conn->state != BT_CONFIG)
+		goto unlock;
+
+	if (!ev->status && hdev->ssp_mode > 0 &&
+			conn->ssp_mode > 0 && conn->out &&
+			conn->sec_level != BT_SECURITY_SDP) {
+		struct hci_cp_auth_requested cp;
+		cp.handle = ev->handle;
+		hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
+				sizeof(cp), &cp);
+	} else {
+		conn->state = BT_CONNECTED;
+		hci_proto_connect_cfm(conn, ev->status);
+		hci_conn_put(conn);
 	}
 
+unlock:
 	hci_dev_unlock(hdev);
 }
 
-- 
1.7.2.3

[PATCH 2/3] Bluetooth: Create a unified authentication request function

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

This patch adds a single function that's responsible for requesting
authentication for outgoing connections. This is preparation for the
next patch which will add automated name requests and thereby move the
authentication requests to a different location.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/hci_event.c |   58 ++++++++++++++++++++++++++++++++++-----------
 1 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 8430276..45569f2 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -677,6 +677,33 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
 	hci_dev_unlock(hdev);
 }
 
+static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
+{
+	struct hci_cp_auth_requested cp;
+	struct hci_conn *conn;
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
+	if (!conn)
+		return 0;
+
+	if (conn->state != BT_CONFIG || !conn->out)
+		return 0;
+
+	if (conn->sec_level == BT_SECURITY_SDP)
+		return 0;
+
+	/* Only request authentication for SSP connections or non-SSP
+	 * devices with sec_level HIGH */
+	if (!(hdev->ssp_mode > 0 && conn->ssp_mode > 0) &&
+					conn->sec_level != BT_SECURITY_HIGH)
+		return 0;
+
+	cp.handle = __cpu_to_le16(conn->handle);
+	hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
+
+	return 1;
+}
+
 static void hci_cs_remote_name_req(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%x", hdev->name, status);
@@ -1156,6 +1183,7 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
 {
 	struct hci_ev_remote_features *ev = (void *) skb->data;
 	struct hci_conn *conn;
+	int auth_requested;
 
 	BT_DBG("%s status %d", hdev->name, ev->status);
 
@@ -1177,12 +1205,15 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
 		cp.page = 0x01;
 		hci_send_cmd(hdev, HCI_OP_READ_REMOTE_EXT_FEATURES,
 							sizeof(cp), &cp);
-	} else if (!ev->status && conn->out &&
-			conn->sec_level == BT_SECURITY_HIGH) {
-		struct hci_cp_auth_requested cp;
-		cp.handle = ev->handle;
-		hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
-	} else {
+		goto unlock;
+	}
+
+	if (!ev->status)
+		auth_requested = request_outgoing_auth(hdev, &conn->dst);
+	else
+		auth_requested = 0;
+
+	if (!auth_requested) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
 		hci_conn_put(conn);
@@ -1640,6 +1671,7 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
 {
 	struct hci_ev_remote_ext_features *ev = (void *) skb->data;
 	struct hci_conn *conn;
+	int auth_requested;
 
 	BT_DBG("%s", hdev->name);
 
@@ -1661,14 +1693,12 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
 	if (conn->state != BT_CONFIG)
 		goto unlock;
 
-	if (!ev->status && hdev->ssp_mode > 0 &&
-			conn->ssp_mode > 0 && conn->out &&
-			conn->sec_level != BT_SECURITY_SDP) {
-		struct hci_cp_auth_requested cp;
-		cp.handle = ev->handle;
-		hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED,
-				sizeof(cp), &cp);
-	} else {
+	if (!ev->status)
+		auth_requested = request_outgoing_auth(hdev, &conn->dst);
+	else
+		auth_requested = 0;
+
+	if (!auth_requested) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
 		hci_conn_put(conn);
-- 
1.7.2.3

[PATCH 3/3] Bluetooth: Automate remote name requests

[johan.hedberg]

From: Johan Hedberg <johan.hedberg@nokia.com>

In Bluetooth there are no automatic updates of remote device names when
they get changed on the remote side. Instead, it is a good idea to do a
manual name request when a new connection gets created (for whatever
reason) since at this point it is very cheap (no costly baseband
connection creation needed just for the sake of the name request).

So far userspace has been responsible for this extra name request but
tighter control is needed in order not to flood Bluetooth controllers
with two many commands during connection creation. It has been shown
that some controllers simply fail to function correctly if they get too
many (almost) simultaneous commands during connection creation. The
simplest way to acheive better control of these commands is to move
their sending completely to the kernel side.

This patch inserts name requests into the sequence of events that the
kernel performs during connection creation. It does this after the
remote features have been successfully requested and before any pending
authentication requests are performed. The code will work sub-optimally
with userspace versions that still do the name requesting themselves (it
shouldn't break anything though) so it is recommended to combine this
with a userspace software version that doesn't have automated name
requests.

Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
---
 net/bluetooth/hci_event.c |   74 ++++++++++++++++++++++++++++++++++----------
 1 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 45569f2..cef970f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -677,9 +677,8 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
 	hci_dev_unlock(hdev);
 }
 
-static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
+static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
-	struct hci_cp_auth_requested cp;
 	struct hci_conn *conn;
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
@@ -698,15 +697,43 @@ static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
 					conn->sec_level != BT_SECURITY_HIGH)
 		return 0;
 
-	cp.handle = __cpu_to_le16(conn->handle);
-	hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
-
 	return 1;
 }
 
+static int request_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
+{
+	struct hci_cp_auth_requested cp;
+	struct hci_conn *conn;
+
+	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
+	if (!conn)
+		return -ENOTCONN;
+
+	cp.handle = __cpu_to_le16(conn->handle);
+	return hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
+}
+
 static void hci_cs_remote_name_req(struct hci_dev *hdev, __u8 status)
 {
+	struct hci_cp_remote_name_req *cp;
+
 	BT_DBG("%s status 0x%x", hdev->name, status);
+
+	/* If successful wait for the name req complete event before
+	 * checking for the need to do authentication */
+	if (!status)
+		return;
+
+	cp = hci_sent_cmd_data(hdev, HCI_OP_REMOTE_NAME_REQ);
+	if (!cp)
+		return;
+
+	hci_dev_lock(hdev);
+
+	if (outgoing_auth_needed(hdev, &cp->bdaddr))
+		request_auth(hdev, &cp->bdaddr);
+
+	hci_dev_unlock(hdev);
 }
 
 static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
@@ -1117,9 +1144,18 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 
 static inline void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb)
 {
+	struct hci_ev_remote_name *ev = (void *) skb->data;
+
 	BT_DBG("%s", hdev->name);
 
 	hci_conn_check_pending(hdev);
+
+	hci_dev_lock(hdev);
+
+	if (outgoing_auth_needed(hdev, &ev->bdaddr))
+		request_auth(hdev, &ev->bdaddr);
+
+	hci_dev_unlock(hdev);
 }
 
 static inline void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
@@ -1183,7 +1219,6 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
 {
 	struct hci_ev_remote_features *ev = (void *) skb->data;
 	struct hci_conn *conn;
-	int auth_requested;
 
 	BT_DBG("%s status %d", hdev->name, ev->status);
 
@@ -1208,12 +1243,15 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
 		goto unlock;
 	}
 
-	if (!ev->status)
-		auth_requested = request_outgoing_auth(hdev, &conn->dst);
-	else
-		auth_requested = 0;
+	if (!ev->status) {
+		struct hci_cp_remote_name_req cp;
+		memset(&cp, 0, sizeof(cp));
+		bacpy(&cp.bdaddr, &conn->dst);
+		cp.pscan_rep_mode = 0x02;
+		hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
+	}
 
-	if (!auth_requested) {
+	if (!outgoing_auth_needed(hdev, &conn->dst)) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
 		hci_conn_put(conn);
@@ -1671,7 +1709,6 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
 {
 	struct hci_ev_remote_ext_features *ev = (void *) skb->data;
 	struct hci_conn *conn;
-	int auth_requested;
 
 	BT_DBG("%s", hdev->name);
 
@@ -1693,12 +1730,15 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
 	if (conn->state != BT_CONFIG)
 		goto unlock;
 
-	if (!ev->status)
-		auth_requested = request_outgoing_auth(hdev, &conn->dst);
-	else
-		auth_requested = 0;
+	if (!ev->status) {
+		struct hci_cp_remote_name_req cp;
+		memset(&cp, 0, sizeof(cp));
+		bacpy(&cp.bdaddr, &conn->dst);
+		cp.pscan_rep_mode = 0x02;
+		hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
+	}
 
-	if (!auth_requested) {
+	if (!outgoing_auth_needed(hdev, &conn->dst)) {
 		conn->state = BT_CONNECTED;
 		hci_proto_connect_cfm(conn, ev->status);
 		hci_conn_put(conn);
-- 
1.7.2.3

Re: [PATCH 1/3] Bluetooth: Simplify remote features callback function logic

[Gustavo F. Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2010-11-10 17:11:51 +0200]:

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> The current remote and remote extended features event callbacks logic
> can be made simpler by using a label and goto statements instead of the
> current multiple levels of nested if statements.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  net/bluetooth/hci_event.c |   91 +++++++++++++++++++++++----------------------
>  1 files changed, 47 insertions(+), 44 deletions(-)

Applied to bluetooth-next-2.6, thanks.

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 3/3] Bluetooth: Automate remote name requests

[Luiz Augusto von Dentz]

Hi,

On Wed, Nov 10, 2010 at 5:11 PM,  <johan.hedberg@gmail.com> wrote:
> From: Johan Hedberg <johan.hedberg@nokia.com>
>
> In Bluetooth there are no automatic updates of remote device names when
> they get changed on the remote side. Instead, it is a good idea to do a
> manual name request when a new connection gets created (for whatever
> reason) since at this point it is very cheap (no costly baseband
> connection creation needed just for the sake of the name request).
>
> So far userspace has been responsible for this extra name request but
> tighter control is needed in order not to flood Bluetooth controllers
> with two many commands during connection creation. It has been shown
> that some controllers simply fail to function correctly if they get too
> many (almost) simultaneous commands during connection creation. The
> simplest way to acheive better control of these commands is to move
> their sending completely to the kernel side.
>
> This patch inserts name requests into the sequence of events that the
> kernel performs during connection creation. It does this after the
> remote features have been successfully requested and before any pending
> authentication requests are performed. The code will work sub-optimally
> with userspace versions that still do the name requesting themselves (it
> shouldn't break anything though) so it is recommended to combine this
> with a userspace software version that doesn't have automated name
> requests.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  net/bluetooth/hci_event.c |   74 ++++++++++++++++++++++++++++++++++----------
>  1 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 45569f2..cef970f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -677,9 +677,8 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
>        hci_dev_unlock(hdev);
>  }
>
> -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
>  {
> -       struct hci_cp_auth_requested cp;
>        struct hci_conn *conn;
>
>        conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> @@ -698,15 +697,43 @@ static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
>                                        conn->sec_level != BT_SECURITY_HIGH)
>                return 0;
>
> -       cp.handle = __cpu_to_le16(conn->handle);
> -       hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> -
>        return 1;
>  }
>
> +static int request_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +{
> +       struct hci_cp_auth_requested cp;
> +       struct hci_conn *conn;
> +
> +       conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> +       if (!conn)
> +               return -ENOTCONN;
> +
> +       cp.handle = __cpu_to_le16(conn->handle);
> +       return hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> +}
> +
>  static void hci_cs_remote_name_req(struct hci_dev *hdev, __u8 status)
>  {
> +       struct hci_cp_remote_name_req *cp;
> +
>        BT_DBG("%s status 0x%x", hdev->name, status);
> +
> +       /* If successful wait for the name req complete event before
> +        * checking for the need to do authentication */
> +       if (!status)
> +               return;
> +
> +       cp = hci_sent_cmd_data(hdev, HCI_OP_REMOTE_NAME_REQ);
> +       if (!cp)
> +               return;
> +
> +       hci_dev_lock(hdev);
> +
> +       if (outgoing_auth_needed(hdev, &cp->bdaddr))
> +               request_auth(hdev, &cp->bdaddr);
> +
> +       hci_dev_unlock(hdev);
>  }
>
>  static void hci_cs_read_remote_features(struct hci_dev *hdev, __u8 status)
> @@ -1117,9 +1144,18 @@ static inline void hci_auth_complete_evt(struct hci_dev *hdev, struct sk_buff *s
>
>  static inline void hci_remote_name_evt(struct hci_dev *hdev, struct sk_buff *skb)
>  {
> +       struct hci_ev_remote_name *ev = (void *) skb->data;
> +
>        BT_DBG("%s", hdev->name);
>
>        hci_conn_check_pending(hdev);
> +
> +       hci_dev_lock(hdev);
> +
> +       if (outgoing_auth_needed(hdev, &ev->bdaddr))
> +               request_auth(hdev, &ev->bdaddr);
> +
> +       hci_dev_unlock(hdev);
>  }
>
>  static inline void hci_encrypt_change_evt(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -1183,7 +1219,6 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
>  {
>        struct hci_ev_remote_features *ev = (void *) skb->data;
>        struct hci_conn *conn;
> -       int auth_requested;
>
>        BT_DBG("%s status %d", hdev->name, ev->status);
>
> @@ -1208,12 +1243,15 @@ static inline void hci_remote_features_evt(struct hci_dev *hdev, struct sk_buff
>                goto unlock;
>        }
>
> -       if (!ev->status)
> -               auth_requested = request_outgoing_auth(hdev, &conn->dst);
> -       else
> -               auth_requested = 0;
> +       if (!ev->status) {
> +               struct hci_cp_remote_name_req cp;
> +               memset(&cp, 0, sizeof(cp));
> +               bacpy(&cp.bdaddr, &conn->dst);
> +               cp.pscan_rep_mode = 0x02;
> +               hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
> +       }
>
> -       if (!auth_requested) {
> +       if (!outgoing_auth_needed(hdev, &conn->dst)) {
>                conn->state = BT_CONNECTED;
>                hci_proto_connect_cfm(conn, ev->status);
>                hci_conn_put(conn);
> @@ -1671,7 +1709,6 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
>  {
>        struct hci_ev_remote_ext_features *ev = (void *) skb->data;
>        struct hci_conn *conn;
> -       int auth_requested;
>
>        BT_DBG("%s", hdev->name);
>
> @@ -1693,12 +1730,15 @@ static inline void hci_remote_ext_features_evt(struct hci_dev *hdev, struct sk_b
>        if (conn->state != BT_CONFIG)
>                goto unlock;
>
> -       if (!ev->status)
> -               auth_requested = request_outgoing_auth(hdev, &conn->dst);
> -       else
> -               auth_requested = 0;
> +       if (!ev->status) {
> +               struct hci_cp_remote_name_req cp;
> +               memset(&cp, 0, sizeof(cp));
> +               bacpy(&cp.bdaddr, &conn->dst);
> +               cp.pscan_rep_mode = 0x02;
> +               hci_send_cmd(hdev, HCI_OP_REMOTE_NAME_REQ, sizeof(cp), &cp);
> +       }
>
> -       if (!auth_requested) {
> +       if (!outgoing_auth_needed(hdev, &conn->dst)) {
>                conn->state = BT_CONNECTED;
>                hci_proto_connect_cfm(conn, ev->status);
>                hci_conn_put(conn);
> --
> 1.7.2.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Acked-by: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>

-- 
Luiz Augusto von Dentz
Computer Engineer

Re: [PATCH 3/3] Bluetooth: Automate remote name requests

[Gustavo F. Padovan]

Hi Johan,

* johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2010-11-10 17:11:53 +0200]:

> From: Johan Hedberg <johan.hedberg@nokia.com>
> 
> In Bluetooth there are no automatic updates of remote device names when
> they get changed on the remote side. Instead, it is a good idea to do a
> manual name request when a new connection gets created (for whatever
> reason) since at this point it is very cheap (no costly baseband
> connection creation needed just for the sake of the name request).
> 
> So far userspace has been responsible for this extra name request but
> tighter control is needed in order not to flood Bluetooth controllers
> with two many commands during connection creation. It has been shown
> that some controllers simply fail to function correctly if they get too
> many (almost) simultaneous commands during connection creation. The
> simplest way to acheive better control of these commands is to move
> their sending completely to the kernel side.
> 
> This patch inserts name requests into the sequence of events that the
> kernel performs during connection creation. It does this after the
> remote features have been successfully requested and before any pending
> authentication requests are performed. The code will work sub-optimally
> with userspace versions that still do the name requesting themselves (it
> shouldn't break anything though) so it is recommended to combine this
> with a userspace software version that doesn't have automated name
> requests.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
> ---
>  net/bluetooth/hci_event.c |   74 ++++++++++++++++++++++++++++++++++----------
>  1 files changed, 57 insertions(+), 17 deletions(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 45569f2..cef970f 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -677,9 +677,8 @@ static void hci_cs_set_conn_encrypt(struct hci_dev *hdev, __u8 status)
>  	hci_dev_unlock(hdev);
>  }
>  
> -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)

Can you add a hci_ in the beginning of your functions, just to keep the
coherency with the rest of the code.

>  {
> -	struct hci_cp_auth_requested cp;
>  	struct hci_conn *conn;
>  
>  	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> @@ -698,15 +697,43 @@ static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
>  					conn->sec_level != BT_SECURITY_HIGH)
>  		return 0;
>  
> -	cp.handle = __cpu_to_le16(conn->handle);
> -	hci_send_cmd(hdev, HCI_OP_AUTH_REQUESTED, sizeof(cp), &cp);
> -
>  	return 1;
>  }
>  
> +static int request_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> +{
> +	struct hci_cp_auth_requested cp;
> +	struct hci_conn *conn;
> +
> +	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, bdaddr);
> +	if (!conn)
> +		return -ENOTCONN;

I'm not happy with have to lookup the hci_conn twice when we can do that
once here. I've noted that always outgoing_auth_needed() returns 1 you do
a request_auth, and always it returns 0 you don't, so I think we can
embed request_auth() inside outgoing_auth_needed() as it was in patch
2/3 and the give a better name to outgoing_auth_needed() which you
reflect the new behavior.

Regards,

-- 
Gustavo F. Padovan
http://profusion.mobi

Re: [PATCH 3/3] Bluetooth: Automate remote name requests

[Johan Hedberg]

Hi Gustavo,

On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> > -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
> 
> Can you add a hci_ in the beginning of your functions, just to keep the
> coherency with the rest of the code.

Sure. I guess I'm too used to the userspace convention where strict
namespacing is only used for exported (non-static) functions.

> I'm not happy with have to lookup the hci_conn twice when we can do that
> once here. I've noted that always outgoing_auth_needed() returns 1 you do
> a request_auth, and always it returns 0 you don't, so I think we can
> embed request_auth() inside outgoing_auth_needed() as it was in patch
> 2/3 and the give a better name to outgoing_auth_needed() which you
> reflect the new behavior.

I don't think the check and request should be in the same function since
there are two places that need the check but not the request. What I can
do though (or actually what I already did) is move the hci_conn lookup
outside of the functions so it's not done multiple times. Will send new
patches in a minute.

Johan

Re: [PATCH 3/3] Bluetooth: Automate remote name requests

[Gustavo F. Padovan]

* Johan Hedberg <johan.hedberg@gmail.com> [2010-11-18 00:30:57 +0200]:

> Hi Gustavo,
> 
> On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> > > -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > > +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > 
> > Can you add a hci_ in the beginning of your functions, just to keep the
> > coherency with the rest of the code.
> 
> Sure. I guess I'm too used to the userspace convention where strict
> namespacing is only used for exported (non-static) functions.
> 
> > I'm not happy with have to lookup the hci_conn twice when we can do that
> > once here. I've noted that always outgoing_auth_needed() returns 1 you do
> > a request_auth, and always it returns 0 you don't, so I think we can
> > embed request_auth() inside outgoing_auth_needed() as it was in patch
> > 2/3 and the give a better name to outgoing_auth_needed() which you
> > reflect the new behavior.
> 
> I don't think the check and request should be in the same function since
> there are two places that need the check but not the request.

In that case such function will return after the check not doing any
request.

-- 
Gustavo F. Padovan
http://profusion.mobi