Bluetooth: Synchronize SCO/eSCO connection requests to ACL state

PATCH In review Ron Shaffer 4 participants 10 messages May 24, 2010 – May 27, 2010

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Ron Shaffer

author

2010-05-24 21:11 UTC #0 lore raw

PATCH 1/1 Bluetooth: Synchronize SCO/eSCO connection requests to ACL state

Certain headsets such as the Motorola H350 will reject SCO and eSCO connection requests while the ACL is transitioning from sniff mode to active mode. Add synchronization so that SCO and eSCO connection requests will wait until the ACL has fully transitioned to active mode.

Signed-off-by Ron Shaffer <[email protected]>

---
 include/net/bluetooth/hci_core.h |   17 +++++++++--------
 net/bluetooth/hci_conn.c         |   20 +++++++++++++++++++-
 net/bluetooth/hci_event.c        |   26 ++++++++++++++++++++++++--
 3 files changed, 52 insertions(+), 11 deletions(-)

include/net/bluetooth/hci_core.h +9 -8

index e42f6ed..c4a37fc 100644
@@ -1,6 +1,6 @@
-/* 
+/*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

@@ -12,13 +12,13 @@
    OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
    IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
-   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES 
-   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN 
-   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF 
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
-   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS, 
-   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS 
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
    SOFTWARE IS DISCLAIMED.
 */

@@ -250,6 +250,7 @@ enum {
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
+	HCI_CONN_SCO_PEND,
 };
 
 static inline void hci_conn_hash_init(struct hci_dev *hdev)

@@ -380,7 +381,7 @@ static inline void __hci_dev_put(struct hci_dev *d)
 }
 
 static inline void hci_dev_put(struct hci_dev *d)
-{ 
+{
 	__hci_dev_put(d);
 	module_put(d->owner);
 }

net/bluetooth/hci_conn.c +19 -1

index b10e3cd..5c4e3c1 100644
@@ -1,6 +1,6 @@
 /*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

@@ -117,9 +117,18 @@ void hci_add_sco(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_add_sco cp;
+	struct hci_conn *acl = conn->link;
 
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);
+
 	conn->state = BT_CONNECT;
 	conn->out = 1;

@@ -135,9 +144,18 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_setup_sync_conn cp;
+	struct hci_conn *acl = conn->link;
 
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);
+
 	conn->state = BT_CONNECT;
 	conn->out = 1;

net/bluetooth/hci_event.c +24 -2

index 6c57fc7..22fb095 100644
@@ -1,6 +1,6 @@
 /*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

@@ -615,6 +615,7 @@ static void hci_cs_add_sco(struct hci_dev *hdev, __u8 status)
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
 	if (acl && (sco = acl->link)) {
 		sco->state = BT_CLOSED;
+		clear_bit(HCI_CONN_SCO_PEND, &sco->pend);
 
 		hci_proto_connect_cfm(sco, status);
 		hci_conn_del(sco);

@@ -760,6 +761,7 @@ static void hci_cs_setup_sync_conn(struct hci_dev *hdev, __u8 status)
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
 	if (acl && (sco = acl->link)) {
 		sco->state = BT_CLOSED;
+		clear_bit(HCI_CONN_SCO_PEND, &sco->pend);
 
 		hci_proto_connect_cfm(sco, status);
 		hci_conn_del(sco);

@@ -795,6 +797,7 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 {
 	struct hci_cp_exit_sniff_mode *cp;
 	struct hci_conn *conn;
+	struct hci_conn *sco;
 
 	BT_DBG("%s status 0x%x", hdev->name, status);

@@ -808,9 +811,18 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn)
+	if (conn) {
 		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
 
+		sco = conn->link;
+		if (sco) {
+			if (test_and_clear_bit(HCI_CONN_SCO_PEND, &sco->pend)) {
+				hci_proto_connect_cfm(sco, status);
+				hci_conn_del(sco);
+			}
+		}
+	}
+
 	hci_dev_unlock(hdev);
 }

@@ -1463,6 +1475,7 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 {
 	struct hci_ev_mode_change *ev = (void *) skb->data;
 	struct hci_conn *conn;
+	struct hci_conn *sco;
 
 	BT_DBG("%s status %d", hdev->name, ev->status);

@@ -1478,6 +1491,15 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 				conn->power_save = 1;
 			else
 				conn->power_save = 0;
+		} else {
+			sco = conn->link;
+			if (sco && test_and_clear_bit(HCI_CONN_SCO_PEND,
+					&sco->pend)) {
+				if (lmp_esco_capable(hdev))
+					hci_setup_sync(sco, conn->handle);
+				else
+					hci_add_sco(sco, conn->handle);
+			}
 		}
 	}

Signature · 6 lines

1.7.0.2

-- 
Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-05-24 21:52 UTC #1 lore raw

Hi Ron,

Quoted 75 lines · Ron Shaffer

Signed-off-by Ron Shaffer <[email protected]>

---
 include/net/bluetooth/hci_core.h |   17 +++++++++--------
 net/bluetooth/hci_conn.c         |   20 +++++++++++++++++++-
 net/bluetooth/hci_event.c        |   26 ++++++++++++++++++++++++--
 3 files changed, 52 insertions(+), 11 deletions(-)

include/net/bluetooth/hci_core.h +9 -8

index e42f6ed..c4a37fc 100644
@@ -1,6 +1,6 @@
-/* 
+/*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

@@ -12,13 +12,13 @@
    OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
    FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS.
    IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY
-   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES 
-   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN 
-   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF 
+   CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES
+   WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+   ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
    OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
-   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS, 
-   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS 
+   ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS,
+   COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
    SOFTWARE IS DISCLAIMED.
 */

@@ -250,6 +250,7 @@ enum {
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
+	HCI_CONN_SCO_PEND,
 };
 
 static inline void hci_conn_hash_init(struct hci_dev *hdev)

@@ -380,7 +381,7 @@ static inline void __hci_dev_put(struct hci_dev *d)
 }
 
 static inline void hci_dev_put(struct hci_dev *d)
-{ 
+{
 	__hci_dev_put(d);
 	module_put(d->owner);
 }

net/bluetooth/hci_conn.c +1 -1

index b10e3cd..5c4e3c1 100644
@@ -1,6 +1,6 @@
 /*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 125

Quoted 9 lines

net/bluetooth/hci_conn.c +3 -0

@@ -117,9 +117,18 @@ void hci_add_sco(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_add_sco cp;
+	struct hci_conn *acl = conn->link;
 
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {

Wrong indentation here. Add a extra tab for test_bit.

net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 130

Quoted 5 lines

net/bluetooth/hci_conn.c +5 -0

+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);

You already clear the bit on hci_mode_change_evt with test_and_clear_bit. No need to clear it again here. The same happens for hci_setup_sync().

Quoted 5 lines

net/bluetooth/hci_conn.c +1 -0

+
 	conn->state = BT_CONNECT;
 	conn->out = 1;

net/bluetooth/hci_conn.c void hci_setup_sync(struct hci_conn *conn, __u16 handle) · line 152

Quoted 9 lines

net/bluetooth/hci_conn.c +3 -0

@@ -135,9 +144,18 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 {
 	struct hci_dev *hdev = conn->hdev;
 	struct hci_cp_setup_sync_conn cp;
+	struct hci_conn *acl = conn->link;
 
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {

Wrong indentation here too.

Quoted 46 lines

net/bluetooth/hci_conn.c +6 -0

+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);
+
 	conn->state = BT_CONNECT;
 	conn->out = 1;

net/bluetooth/hci_event.c +4 -1

index 6c57fc7..22fb095 100644
@@ -1,6 +1,6 @@
 /*
    BlueZ - Bluetooth protocol stack for Linux
-   Copyright (C) 2000-2001 Qualcomm Incorporated
+   Copyright (c) 2000-2001, 2010, Code Aurora Forum. All rights reserved.
 
    Written 2000,2001 by Maxim Krasnyansky <[email protected]>

@@ -615,6 +615,7 @@ static void hci_cs_add_sco(struct hci_dev *hdev, __u8 status)
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
 	if (acl && (sco = acl->link)) {
 		sco->state = BT_CLOSED;
+		clear_bit(HCI_CONN_SCO_PEND, &sco->pend);
 
 		hci_proto_connect_cfm(sco, status);
 		hci_conn_del(sco);

@@ -760,6 +761,7 @@ static void hci_cs_setup_sync_conn(struct hci_dev *hdev, __u8 status)
 	acl = hci_conn_hash_lookup_handle(hdev, handle);
 	if (acl && (sco = acl->link)) {
 		sco->state = BT_CLOSED;
+		clear_bit(HCI_CONN_SCO_PEND, &sco->pend);
 
 		hci_proto_connect_cfm(sco, status);
 		hci_conn_del(sco);

@@ -795,6 +797,7 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 {
 	struct hci_cp_exit_sniff_mode *cp;
 	struct hci_conn *conn;
+	struct hci_conn *sco;
 
 	BT_DBG("%s status 0x%x", hdev->name, status);

net/bluetooth/hci_event.c static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status) · line 819

Quoted 10 lines

net/bluetooth/hci_event.c +4 -1

@@ -808,9 +811,18 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_handle(hdev, __le16_to_cpu(cp->handle));
-	if (conn)
+	if (conn) {
 		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
 
+		sco = conn->link;
+		if (sco) {
+			if (test_and_clear_bit(HCI_CONN_SCO_PEND, &sco->pend)) {

Wouldn't be better 'if (sco && test_and_clear_bit(...' ?

Quoted 44 lines

net/bluetooth/hci_event.c +16 -0

+				hci_proto_connect_cfm(sco, status);
+				hci_conn_del(sco);
+			}
+		}
+	}
+
 	hci_dev_unlock(hdev);
 }

@@ -1463,6 +1475,7 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 {
 	struct hci_ev_mode_change *ev = (void *) skb->data;
 	struct hci_conn *conn;
+	struct hci_conn *sco;
 
 	BT_DBG("%s status %d", hdev->name, ev->status);

@@ -1478,6 +1491,15 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 				conn->power_save = 1;
 			else
 				conn->power_save = 0;
+		} else {
+			sco = conn->link;
+			if (sco && test_and_clear_bit(HCI_CONN_SCO_PEND,
+					&sco->pend)) {
+				if (lmp_esco_capable(hdev))
+					hci_setup_sync(sco, conn->handle);
+				else
+					hci_add_sco(sco, conn->handle);
+			}
 		}
 	}

Signature · 10 lines

1.7.0.2

-- 
Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Ron Shaffer

author

2010-05-25 17:00 UTC #2 lore raw

trimmed.

Quoted 24 lines

Hi Ron,
Quoted 7 lines · Ron Shaffer
Certain headsets such as the Motorola H350 will reject SCO and eSCO connection requests while the ACL is transitioning from sniff mode to active mode. Add synchronization so that SCO and eSCO connection requests will wait until the ACL has fully transitioned to active mode.
Signed-off-by Ron Shaffer <[email protected]>
(quoted diff) +0 -0
@@ -117,9 +117,18 @@ void hci_add_sco(struct hci_conn *conn, __u16 handle)
 >  {
 >  	struct hci_dev *hdev = conn->hdev;
 >  	struct hci_cp_add_sco cp;
 > +	struct hci_conn *acl = conn->link;
net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 125
Quoted 5 lines
net/bluetooth/hci_conn.c +2 -0
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
Wrong indentation here. Add a extra tab for test_bit.

Quoted 12 lines

net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 130
Quoted 5 lines
net/bluetooth/hci_conn.c +5 -0
+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);
You already clear the bit on hci_mode_change_evt with test_and_clear_bit. No need to clear it again here. The same happens for hci_setup_sync().

Clearing the bit and clearing it in hci_change_mode_evt are independent actions for two different scenarios.

Scenario 1:
A SCO/eSCO connection is requested and the ACL is in active mode.

Scenario 2:
A SCO/eSCO connection is requested and the ACL is transitioning modes.

For scenario 1, I cleared the bit here because I did not want to assume the bit had been initialized. If it's set, then any future changes to the ACL mode (sniff, active) requested by the local side would result in a request for a SCO or eSCO connection when one already exists.

For scenario 2, the first conditional is TRUE, and the bit gets set. Later when the mode change event is received, the bit is cleared and the SCO/eSCO connection is requested.

Signature · 3 lines

Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Gustavo F. Padovan

2010-05-25 20:09 UTC #3 lore raw

Hi Ron,

Quoted 48 lines · Ron Shaffer

trimmed.
Quoted 24 lines
Hi Ron,
Quoted 7 lines · Ron Shaffer
Certain headsets such as the Motorola H350 will reject SCO and eSCO connection requests while the ACL is transitioning from sniff mode to active mode. Add synchronization so that SCO and eSCO connection requests will wait until the ACL has fully transitioned to active mode.
Signed-off-by Ron Shaffer <[email protected]>
(quoted diff) +0 -0
@@ -117,9 +117,18 @@ void hci_add_sco(struct hci_conn *conn, __u16 handle)
 >  {
 >  	struct hci_dev *hdev = conn->hdev;
 >  	struct hci_cp_add_sco cp;
 > +	struct hci_conn *acl = conn->link;
net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 125
Quoted 5 lines
net/bluetooth/hci_conn.c +2 -0
 	BT_DBG("%p", conn);
 
+	if (acl->mode == HCI_CM_SNIFF &&
+		test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
Wrong indentation here. Add a extra tab for test_bit.
Quoted 12 lines
net/bluetooth/hci_conn.c void hci_add_sco(struct hci_conn *conn, __u16 handle) · line 130
Quoted 5 lines
net/bluetooth/hci_conn.c +5 -0
+		set_bit(HCI_CONN_SCO_PEND, &conn->pend);
+		return;
+	}
+
+	clear_bit(HCI_CONN_SCO_PEND, &conn->pend);
You already clear the bit on hci_mode_change_evt with test_and_clear_bit. No need to clear it again here. The same happens for hci_setup_sync().
Clearing the bit and clearing it in hci_change_mode_evt are independent actions for two different scenarios.
Scenario 1:
A SCO/eSCO connection is requested and the ACL is in active mode.
Scenario 2:
A SCO/eSCO connection is requested and the ACL is transitioning modes.

Quoted 8 lines

For scenario 1, I cleared the bit here because I did not want to assume the bit had been initialized. If it's set, then any future changes to the ACL mode (sniff, active) requested by the local side would result in a request for a SCO or eSCO connection when one already exists.
For scenario 2, the first conditional is TRUE, and the bit gets set. Later when the mode change event is received, the bit is cleared and the SCO/eSCO connection is requested.

Acked.

Quoted 5 lines

Signature · 4 lines

-- 
Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Signature · 2 lines

Gustavo F. Padovan
http://padovan.org

Marcel Holtmann

maintainer

2010-05-26 08:59 UTC #4 lore raw

In reply to Ron Shaffer

Hi Ron,

Quoted 4 lines

Certain headsets such as the Motorola H350 will reject SCO and eSCO connection requests while the ACL is transitioning from sniff mode to active mode. Add synchronization so that SCO and eSCO connection requests will wait until the ACL has fully transitioned to active mode.

I am not completely happy with this patch. First please split it like Gustavo asked since otherwise it becomes pretty hard to review. I don't really bother to look into its technical nature before the split and the coding style is in place.

Second, please don't make a special case for sniff mode. If we do this then we also do it for park state and hold mode.

And third, I think this needs to be done a little bit more in the way we handle attempts for connecting ACL links when the baseband is busy. So can you send hcidump -X -V traces for the failure of SCO/eSCO setup when sniff is active. I like to see which error codes are returned. Hacking this directly into sniff exit handling makes the code pretty hard to understand in the future.

Regards

Marcel

Ron Shaffer

author

2010-05-26 14:30 UTC #5 lore raw

Marcel:

Quoted 22 lines

Hi Ron,
Quoted 4 lines
Certain headsets such as the Motorola H350 will reject SCO and eSCO connection requests while the ACL is transitioning from sniff mode to active mode. Add synchronization so that SCO and eSCO connection requests will wait until the ACL has fully transitioned to active mode.
I am not completely happy with this patch. First please split it like Gustavo asked since otherwise it becomes pretty hard to review. I don't really bother to look into its technical nature before the split and the coding style is in place.
Second, please don't make a special case for sniff mode. If we do this then we also do it for park state and hold mode.
And third, I think this needs to be done a little bit more in the way we handle attempts for connecting ACL links when the baseband is busy. So can you send hcidump -X -V traces for the failure of SCO/eSCO setup when sniff is active. I like to see which error codes are returned. Hacking this directly into sniff exit handling makes the code pretty hard to understand in the future.

Quoted 5 lines

Regards
Marcel

Thanks for the review and comments.

I'll split the patch into three. No problem. When I resubmit, I'll push the hcidump log out as well. Because the failure occurs during eSCO negotiation and SCO request with the remote, most of the interesting stuff is in LMP, so I'll include the FTS sniffer trace as well.

Signature · 3 lines

Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Marcel Holtmann

maintainer

2010-05-26 15:31 UTC #6 lore raw

Hi Ron,

Quoted 4 lines

I'll split the patch into three. No problem. When I resubmit, I'll push the hcidump log out as well. Because the failure occurs during eSCO negotiation and SCO request with the remote, most of the interesting stuff is in LMP, so I'll include the FTS sniffer trace as well.

don't bother with FTS traces. I don't care. I just need to see the output of hcidump -X -V so that I can figure out what the problem is here and what are possible solutions. Send the hcidump now before redoing the patches.

Regards

Marcel

Ron Shaffer

author

2010-05-26 21:56 UTC #7 lore raw

Marcel,

Quoted 7 lines

Hi Ron,
Quoted 4 lines
I'll split the patch into three. No problem. When I resubmit, I'll push the hcidump log out as well. Because the failure occurs during eSCO negotiation and SCO request with the remote, most of the interesting stuff is in LMP, so I'll include the FTS sniffer trace as well.

Quoted 10 lines

don't bother with FTS traces. I don't care. I just need to see the output of hcidump -X -V so that I can figure out what the problem is here and what are possible solutions. Send the hcidump now before redoing the patches.
Regards
Marcel

Here's the hcidump for this scenario. I've also taken a look at the ACL retry mechanism used in the case where the baseband is too busy and the connection attempt fails. The same mechanism is already in place for SCO and eSCO. The difference with this particular scenario is this:

1. The failure is caused be an issue on the remote
2. Once the failure has occurred it is generally unrecoverable i.e. the remote side is hosed and needs to be reset, so repeated attempts continue to fail.
3. The only way to make a successful connection and keep from hosing the remote is to prevent the scenario from occurring by not requesting eSCO or SCO during the transition period.

As for hold and park, we can generalize it by making eSCO or SCO pend until we also exit from hold. As for park, it's unclear as I didn't really see any support for park. It doesn't look like we prevent commands or data from being sent to a remote that is parked.

< HCI Command: Exit Sniff Mode (0x02|0x0004) plen 2
handle 12

Quoted 1 line

HCI Event: Command Status (0x0f) plen 4

    Exit Sniff Mode (0x02|0x0004) status 0x00 ncmd 1
< HCI Command: Setup Synchronous Connection (0x01|0x0028) plen 17
    handle 12 voice setting 0x0040

Quoted 1 line

HCI Event: Command Status (0x0f) plen 4

    Setup Synchronous Connection (0x01|0x0028) status 0x00 ncmd 1

Quoted 1 line

HCI Event: Number of Completed Packets (0x13) plen 5

handle 12 packets 1

Quoted 1 line

HCI Event: Mode Change (0x14) plen 6

    status 0x00 handle 12 mode 0x00 interval 0
    Mode: Active

Quoted 1 line

HCI Event: Synchronous Connect Complete (0x2c) plen 17

status 0x10 handle 14 bdaddr 00:1A:0E:50:28:A4 type SCO
Error: Connection Accept Timeout Exceeded
< HCI Command: Setup Synchronous Connection (0x01|0x0028) plen 17

    handle 12 voice setting 0x0040

Quoted 1 line

HCI Event: Command Status (0x0f) plen 4

    Setup Synchronous Connection (0x01|0x0028) status 0x00 ncmd 1
< ACL data: handle 12 flags 0x00 dlen 22
    0000: 12 00 41 00 0b ef 1d 0d  0a 2b 43 49 45 56 3a 20  ..A..ï...+CIEV: 
    0010: 35 2c 34 0d 0a 9a                                 5,4...

Quoted 1 line

HCI Event: Number of Completed Packets (0x13) plen 5

handle 12 packets 1
< ACL data: handle 12 flags 0x00 dlen 22

    0000: 12 00 41 00 0b ef 1d 0d  0a 2b 43 49 45 56 3a 20  ..A..ï...+CIEV: 
    0010: 35 2c 35 0d 0a 9a                                 5,5...

Quoted 1 line

HCI Event: Number of Completed Packets (0x13) plen 5

handle 12 packets 1

Quoted 1 line

HCI Event: Synchronous Connect Complete (0x2c) plen 17

status 0x10 handle 14 bdaddr 00:1A:0E:50:28:A4 type SCO
Error: Connection Accept Timeout Exceeded

Signature · 3 lines

Ron Shaffer
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Perelet, Oleg

2010-05-27 03:38 UTC #8 lore raw

Marcel, Here's some prehistory of this patch. 1st Nick did:

http://android.git.kernel.org/?p=kernel/common.git;a=commitdiff;h=201ac2f225a31dffcb05f1db4d609c467c9c694c

which unsniffs connection before doing SCO, otherwise establishing SCO took few seconds on some headsets. Then this patch broke whole bunch of headsets that expected SCO only after unsiff is Acked - that what Ron did.

I had email conversation with Nick about that. IMHO this logic belongs to user land, but because of GPL/Apache conflict libbluetooth can not be lifted in to Android Java/JNI - that's why initial change went to kernel.

If you guys can have some clever way of doing it in bluetoothd - nice. Otherwise this gargantuan kernel patch does fix the problem

Oleg.

PS. Sorry if this email conflicts with top posting rules, that's my email client of this moment. I'll buy you sushi:)

Marcel Holtmann

maintainer

2010-05-27 10:34 UTC #9 lore raw

Hi Oleg,

Quoted 9 lines

Marcel, Here's some prehistory of this patch. 1st Nick did:
http://android.git.kernel.org/?p=kernel/common.git;a=commitdiff;h=201ac2f225a31dffcb05f1db4d609c467c9c694c
which unsniffs connection before doing SCO, otherwise establishing SCO took few seconds on some headsets. Then this patch broke whole bunch of headsets that expected SCO only after unsiff is Acked - that what Ron did.
I had email conversation with Nick about that. IMHO this logic belongs to user land, but because of GPL/Apache conflict libbluetooth can not be lifted in to Android Java/JNI - that's why initial change went to kernel.
If you guys can have some clever way of doing it in bluetoothd - nice. Otherwise this gargantuan kernel patch does fix the problem

I am fine with doing this inside the kernel. It makes perfect sense, but it needs to be aligned a little bit with the overall way on how we handle connection setup.

My main question was if it a retry because of failure or if we preemptively wait until we are in active mode. And that got answered since the headsets will lock up if we even try.

Regards

Marcel