[PATCH] Bluetooth: Defer SCO setup if mode change is pending

[PATCH] Bluetooth: Defer SCO setup if mode change is pending

[Marcel Holtmann]

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_conn.c         |   32 ++++++++++++++++++++++++---
 net/bluetooth/hci_event.c        |   43 ++++++++++++++++---------------------
 3 files changed, 49 insertions(+), 28 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 350b3e6..8b28962 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -256,6 +256,7 @@ enum {
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
+	HCI_CONN_SCO_SETUP_PEND,
 };
 
 static inline void hci_conn_hash_init(struct hci_dev *hdev)
@@ -336,6 +337,7 @@ void hci_acl_connect(struct hci_conn *conn);
 void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
 void hci_add_sco(struct hci_conn *conn, __u16 handle);
 void hci_setup_sync(struct hci_conn *conn, __u16 handle);
+void hci_sco_setup(struct hci_conn *conn, __u8 status);
 
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
 int hci_conn_del(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index e9fef83..0b1e460 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -155,6 +155,27 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
 
+/* Device _must_ be locked */
+void hci_sco_setup(struct hci_conn *conn, __u8 status)
+{
+	struct hci_conn *sco = conn->link;
+
+	BT_DBG("%p", conn);
+
+	if (!sco)
+		return;
+
+	if (!status) {
+		if (lmp_esco_capable(conn->hdev))
+			hci_setup_sync(sco, conn->handle);
+		else
+			hci_add_sco(sco, conn->handle);
+	} else {
+		hci_proto_connect_cfm(sco, status);
+		hci_conn_del(sco);
+	}
+}
+
 static void hci_conn_timeout(unsigned long arg)
 {
 	struct hci_conn *conn = (void *) arg;
@@ -385,10 +406,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		acl->power_save = 1;
 		hci_conn_enter_active_mode(acl);
 
-		if (lmp_esco_capable(hdev))
-			hci_setup_sync(sco, acl->handle);
-		else
-			hci_add_sco(sco, acl->handle);
+		if (test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
+			/* defer SCO setup until mode change completed */
+			set_bit(HCI_CONN_SCO_SETUP_PEND, &acl->pend);
+			return sco;
+		}
+
+		hci_sco_setup(acl, 0x00);
 	}
 
 	return sco;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2069c3b..461413c 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (!status)
-		return;
-
 	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
 	if (!cp)
 		return;
@@ -785,8 +782,13 @@ static void hci_cs_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)
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+	if (conn) {
+		if (status)
+			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
+			hci_sco_setup(conn, status);
+	}
 
 	hci_dev_unlock(hdev);
 }
@@ -798,9 +800,6 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (!status)
-		return;
-
 	cp = hci_sent_cmd_data(hdev, HCI_OP_EXIT_SNIFF_MODE);
 	if (!cp)
 		return;
@@ -808,8 +807,13 @@ 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)
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+	if (conn) {
+		if (status)
+			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
+			hci_sco_setup(conn, status);
+	}
 
 	hci_dev_unlock(hdev);
 }
@@ -915,20 +919,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 	} else
 		conn->state = BT_CLOSED;
 
-	if (conn->type == ACL_LINK) {
-		struct hci_conn *sco = conn->link;
-		if (sco) {
-			if (!ev->status) {
-				if (lmp_esco_capable(hdev))
-					hci_setup_sync(sco, conn->handle);
-				else
-					hci_add_sco(sco, conn->handle);
-			} else {
-				hci_proto_connect_cfm(sco, ev->status);
-				hci_conn_del(sco);
-			}
-		}
-	}
+	if (conn->type == ACL_LINK)
+		hci_sco_setup(conn, ev->status);
 
 	if (ev->status) {
 		hci_proto_connect_cfm(conn, ev->status);
@@ -1481,6 +1473,9 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 			else
 				conn->power_save = 0;
 		}
+
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
+			hci_sco_setup(conn, ev->status);
 	}
 
 	hci_dev_unlock(hdev);
-- 
1.7.1.1

Re: [PATCH] Bluetooth: Defer SCO setup if mode change is pending

[Ron Shaffer]

Marcel:

On 7/19/2010 1:06 PM, Marcel Holtmann wrote:
> @@ -385,10 +406,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
>  		acl->power_save = 1;
>  		hci_conn_enter_active_mode(acl);
>  
> -		if (lmp_esco_capable(hdev))
> -			hci_setup_sync(sco, acl->handle);
> -		else
> -			hci_add_sco(sco, acl->handle);
> +		if (test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
> +			/* defer SCO setup until mode change completed */
> +			set_bit(HCI_CONN_SCO_SETUP_PEND, &acl->pend);
> +			return sco;
> +		}
> +
> +		hci_sco_setup(acl, 0x00);
>  	}

Not in love with the "fake" status passed here, but the overall change
to centralize this functionality provides a nice cleanup from the
original patch and makes the code more readable and maintainable.

>  
>  	return sco;
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 2069c3b..461413c 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (!status)
> -		return;
> -
>  	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
>  	if (!cp)
>  		return;
> @@ -785,8 +782,13 @@ static void hci_cs_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)
> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +	if (conn) {
> +		if (status)
> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +
> +		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
> +			hci_sco_setup(conn, status);

Probably the subject for another patch, but has there been any reported
issue where idle_timeout is set short enough that the timer expires
after exit sniff mode has been requested but before the mode change
event has been received?

Got back from other duties late tonight, so I didn't get testing fully
finished. In particular, I need to see what happens when idle_timeout is
set very low ex < 10. I'll let you know the results first thing in the
morning. If everything is good, I'll regenerate the patch with original
hcidump log included
-- 
Ron Shaffer
Employee of the Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH] Bluetooth: Defer SCO setup if mode change is pending

[Marcel Holtmann]

Hi Ron,

> > @@ -385,10 +406,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
> >  		acl->power_save = 1;
> >  		hci_conn_enter_active_mode(acl);
> >  
> > -		if (lmp_esco_capable(hdev))
> > -			hci_setup_sync(sco, acl->handle);
> > -		else
> > -			hci_add_sco(sco, acl->handle);
> > +		if (test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
> > +			/* defer SCO setup until mode change completed */
> > +			set_bit(HCI_CONN_SCO_SETUP_PEND, &acl->pend);
> > +			return sco;
> > +		}
> > +
> > +		hci_sco_setup(acl, 0x00);
> >  	}
> 
> Not in love with the "fake" status passed here, but the overall change
> to centralize this functionality provides a nice cleanup from the
> original patch and makes the code more readable and maintainable.

yeah, the 0x00 looks funny here. We could add a comment explaining it,
but it does make sense in the end.

> >  	return sco;
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 2069c3b..461413c 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
> >  
> >  	BT_DBG("%s status 0x%x", hdev->name, status);
> >  
> > -	if (!status)
> > -		return;
> > -
> >  	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
> >  	if (!cp)
> >  		return;
> > @@ -785,8 +782,13 @@ static void hci_cs_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)
> > -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> > +	if (conn) {
> > +		if (status)
> > +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> > +
> > +		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
> > +			hci_sco_setup(conn, status);
> 
> Probably the subject for another patch, but has there been any reported
> issue where idle_timeout is set short enough that the timer expires
> after exit sniff mode has been requested but before the mode change
> event has been received?

I see that this could potentially happen. The timer is not suppose to be
running during that transaction. It should be only started when the mode
change has been completed. That needs fixing, but not in this patch.

Regards

Marcel

[PATCH v2] Bluetooth: Defer SCO setup if mode change is pending

[Ron Shaffer]

From: Marcel Holtmann <marcel@holtmann.org>

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.

< HCI Command: Exit Sniff Mode (0x02|0x0004) plen 2
    handle 12
> 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
> HCI Event: Command Status (0x0f) plen 4
    Setup Synchronous Connection (0x01|0x0028) status 0x00 ncmd 1
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 12 packets 1
> HCI Event: Mode Change (0x14) plen 6
    status 0x00 handle 12 mode 0x00 interval 0
    Mode: Active
> 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

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Ron Shaffer <rshaffer@codeaurora.org>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_conn.c         |   32 ++++++++++++++++++++++---
 net/bluetooth/hci_event.c        |   47 ++++++++++++++++++-------------------
 3 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e42f6ed..edf3dfe 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -250,6 +250,7 @@ enum {
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
+	HCI_CONN_SCO_SETUP_PEND,
 };
 
 static inline void hci_conn_hash_init(struct hci_dev *hdev)
@@ -330,6 +331,7 @@ void hci_acl_connect(struct hci_conn *conn);
 void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
 void hci_add_sco(struct hci_conn *conn, __u16 handle);
 void hci_setup_sync(struct hci_conn *conn, __u16 handle);
+void hci_sco_setup(struct hci_conn *conn, __u8 status);
 
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
 int hci_conn_del(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b10e3cd..805a22c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -155,6 +155,27 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
 
+/* Device _must_ be locked */
+void hci_sco_setup(struct hci_conn *conn, __u8 status)
+{
+	struct hci_conn *sco = conn->link;
+
+	BT_DBG("%p", conn);
+
+	if (!sco)
+		return;
+
+	if (!status) {
+		if (lmp_esco_capable(conn->hdev))
+			hci_setup_sync(sco, conn->handle);
+		else
+			hci_add_sco(sco, conn->handle);
+	} else {
+		hci_proto_connect_cfm(sco, status);
+		hci_conn_del(sco);
+	}
+}
+
 static void hci_conn_timeout(unsigned long arg)
 {
 	struct hci_conn *conn = (void *) arg;
@@ -380,10 +401,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		acl->power_save = 1;
 		hci_conn_enter_active_mode(acl);
 
-		if (lmp_esco_capable(hdev))
-			hci_setup_sync(sco, acl->handle);
-		else
-			hci_add_sco(sco, acl->handle);
+		if (test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
+			/* defer SCO setup until mode change completed */
+			set_bit(HCI_CONN_SCO_SETUP_PEND, &acl->pend);
+			return sco;
+		}
+
+		hci_sco_setup(acl, 0x00);
 	}
 
 	return sco;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6c57fc7..9aee8c5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (!status)
-		return;
-
 	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
 	if (!cp)
 		return;
@@ -785,8 +782,15 @@ static void hci_cs_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)
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+	if (conn) {
+		if (status) {
+			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+
+			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
+					&conn->pend))
+				hci_sco_setup(conn, status);
+		}
+	}
 
 	hci_dev_unlock(hdev);
 }
@@ -798,9 +802,6 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (!status)
-		return;
-
 	cp = hci_sent_cmd_data(hdev, HCI_OP_EXIT_SNIFF_MODE);
 	if (!cp)
 		return;
@@ -808,8 +809,15 @@ 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)
-		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+	if (conn) {
+		if (status) {
+			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
+
+			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
+					&conn->pend))
+				hci_sco_setup(conn, status);
+		}
+	}
 
 	hci_dev_unlock(hdev);
 }
@@ -915,20 +923,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 	} else
 		conn->state = BT_CLOSED;
 
-	if (conn->type == ACL_LINK) {
-		struct hci_conn *sco = conn->link;
-		if (sco) {
-			if (!ev->status) {
-				if (lmp_esco_capable(hdev))
-					hci_setup_sync(sco, conn->handle);
-				else
-					hci_add_sco(sco, conn->handle);
-			} else {
-				hci_proto_connect_cfm(sco, ev->status);
-				hci_conn_del(sco);
-			}
-		}
-	}
+	if (conn->type == ACL_LINK)
+		hci_sco_setup(conn, ev->status);
 
 	if (ev->status) {
 		hci_proto_connect_cfm(conn, ev->status);
@@ -1478,6 +1474,9 @@ 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 if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
+				&conn->pend)) {
+			hci_sco_setup(conn, ev->status);
 		}
 	}
 
-- 
1.7.0.2
--
Ron Shaffer
Employee of the Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v2] Bluetooth: Defer SCO setup if mode change is pending

[Marcel Holtmann]

Hi Ron,

> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 6c57fc7..9aee8c5 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (!status)
> -		return;
> -
>  	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
>  	if (!cp)
>  		return;
> @@ -785,8 +782,15 @@ static void hci_cs_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)
> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +	if (conn) {
> +		if (status) {
> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +
> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> +					&conn->pend))
> +				hci_sco_setup(conn, status);
> +		}
> +	}
>  
>  	hci_dev_unlock(hdev);
>  }

this change now makes it worse, please revert to initial

	if (!status)
		return;

check at the top and the just do

	if (conn) {
		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);

		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
			hci_sco_setup(conn, status);
	}

The only reason why I moved the status check down, because I was not
thinking straight and overlooked that in case of success we don't wanna
do a single thing here.

> @@ -798,9 +802,6 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (!status)
> -		return;
> -
>  	cp = hci_sent_cmd_data(hdev, HCI_OP_EXIT_SNIFF_MODE);
>  	if (!cp)
>  		return;
> @@ -808,8 +809,15 @@ 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)
> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +	if (conn) {
> +		if (status) {
> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> +
> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> +					&conn->pend))
> +				hci_sco_setup(conn, status);
> +		}
> +	}
>  
>  	hci_dev_unlock(hdev);
>  }

Same applies here btw.

> @@ -915,20 +923,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
>  	} else
>  		conn->state = BT_CLOSED;
>  
> -	if (conn->type == ACL_LINK) {
> -		struct hci_conn *sco = conn->link;
> -		if (sco) {
> -			if (!ev->status) {
> -				if (lmp_esco_capable(hdev))
> -					hci_setup_sync(sco, conn->handle);
> -				else
> -					hci_add_sco(sco, conn->handle);
> -			} else {
> -				hci_proto_connect_cfm(sco, ev->status);
> -				hci_conn_del(sco);
> -			}
> -		}
> -	}
> +	if (conn->type == ACL_LINK)
> +		hci_sco_setup(conn, ev->status);
>  
>  	if (ev->status) {
>  		hci_proto_connect_cfm(conn, ev->status);
> @@ -1478,6 +1474,9 @@ 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 if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> +				&conn->pend)) {
> +			hci_sco_setup(conn, ev->status);
>  		}
>  	}

Please don't do else if here. Just add a separate check like I had in my
initial patch. We don't wanna depend on two flags pending flags set
here. If the SCO setup is pending, then we execute it. No matter how we
reached that point. It is important that even if we receive a mode
change without us triggering it, that we clear any potential SCO pending
flag.

Regards

Marcel

Re: [PATCH v2] Bluetooth: Defer SCO setup if mode change is pending

[Ron Shaffer]

Marcel:

> Hi Ron,
> 
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index 6c57fc7..9aee8c5 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
>>  
>>  	BT_DBG("%s status 0x%x", hdev->name, status);
>>  
>> -	if (!status)
>> -		return;
>> -
>>  	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
>>  	if (!cp)
>>  		return;
>> @@ -785,8 +782,15 @@ static void hci_cs_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)
>> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
>> +	if (conn) {
>> +		if (status) {
>> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
>> +
>> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
>> +					&conn->pend))
>> +				hci_sco_setup(conn, status);
>> +		}
>> +	}
>>  
>>  	hci_dev_unlock(hdev);
>>  }
> 
> this change now makes it worse, please revert to initial
> 
> 	if (!status)
> 		return;
> 
> check at the top and the just do
> 
> 	if (conn) {
> 		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> 
> 		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
> 			hci_sco_setup(conn, status);
> 	}
> 
> The only reason why I moved the status check down, because I was not
> thinking straight and overlooked that in case of success we don't wanna
> do a single thing here.

Makes sense. Done.

> 
>> @@ -798,9 +802,6 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
>>  
>>  	BT_DBG("%s status 0x%x", hdev->name, status);
>>  
>> -	if (!status)
>> -		return;
>> -
>>  	cp = hci_sent_cmd_data(hdev, HCI_OP_EXIT_SNIFF_MODE);
>>  	if (!cp)
>>  		return;
>> @@ -808,8 +809,15 @@ 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)
>> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
>> +	if (conn) {
>> +		if (status) {
>> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
>> +
>> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
>> +					&conn->pend))
>> +				hci_sco_setup(conn, status);
>> +		}
>> +	}
>>  
>>  	hci_dev_unlock(hdev);
>>  }
> 
> Same applies here btw.

Same. Done.

> 
>> @@ -915,20 +923,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
>>  	} else
>>  		conn->state = BT_CLOSED;
>>  
>> -	if (conn->type == ACL_LINK) {
>> -		struct hci_conn *sco = conn->link;
>> -		if (sco) {
>> -			if (!ev->status) {
>> -				if (lmp_esco_capable(hdev))
>> -					hci_setup_sync(sco, conn->handle);
>> -				else
>> -					hci_add_sco(sco, conn->handle);
>> -			} else {
>> -				hci_proto_connect_cfm(sco, ev->status);
>> -				hci_conn_del(sco);
>> -			}
>> -		}
>> -	}
>> +	if (conn->type == ACL_LINK)
>> +		hci_sco_setup(conn, ev->status);
>>  
>>  	if (ev->status) {
>>  		hci_proto_connect_cfm(conn, ev->status);
>> @@ -1478,6 +1474,9 @@ 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 if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
>> +				&conn->pend)) {
>> +			hci_sco_setup(conn, ev->status);
>>  		}
>>  	}
> 
> Please don't do else if here. Just add a separate check like I had in my
> initial patch. We don't wanna depend on two flags pending flags set
> here. If the SCO setup is pending, then we execute it. No matter how we
> reached that point. It is important that even if we receive a mode
> change without us triggering it, that we clear any potential SCO pending
> flag.

Hmmm. I know I found a bug here, but I can't remember how it manifested
itself. Today, I can't reproduce it, so either I was seeing things or
I'm losing it.

I'll revert it back, and if I run across the issue again, we can fix it
later.

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

Re: [PATCH v2] Bluetooth: Defer SCO setup if mode change is pending

[Marcel Holtmann]

Hi Ron,

> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index 6c57fc7..9aee8c5 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -775,9 +775,6 @@ static void hci_cs_sniff_mode(struct hci_dev *hdev, __u8 status)
> >>  
> >>  	BT_DBG("%s status 0x%x", hdev->name, status);
> >>  
> >> -	if (!status)
> >> -		return;
> >> -
> >>  	cp = hci_sent_cmd_data(hdev, HCI_OP_SNIFF_MODE);
> >>  	if (!cp)
> >>  		return;
> >> @@ -785,8 +782,15 @@ static void hci_cs_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)
> >> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> >> +	if (conn) {
> >> +		if (status) {
> >> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> >> +
> >> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> >> +					&conn->pend))
> >> +				hci_sco_setup(conn, status);
> >> +		}
> >> +	}
> >>  
> >>  	hci_dev_unlock(hdev);
> >>  }
> > 
> > this change now makes it worse, please revert to initial
> > 
> > 	if (!status)
> > 		return;
> > 
> > check at the top and the just do
> > 
> > 	if (conn) {
> > 		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> > 
> > 		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
> > 			hci_sco_setup(conn, status);
> > 	}
> > 
> > The only reason why I moved the status check down, because I was not
> > thinking straight and overlooked that in case of success we don't wanna
> > do a single thing here.
> 
> Makes sense. Done.
> 
> > 
> >> @@ -798,9 +802,6 @@ static void hci_cs_exit_sniff_mode(struct hci_dev *hdev, __u8 status)
> >>  
> >>  	BT_DBG("%s status 0x%x", hdev->name, status);
> >>  
> >> -	if (!status)
> >> -		return;
> >> -
> >>  	cp = hci_sent_cmd_data(hdev, HCI_OP_EXIT_SNIFF_MODE);
> >>  	if (!cp)
> >>  		return;
> >> @@ -808,8 +809,15 @@ 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)
> >> -		clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> >> +	if (conn) {
> >> +		if (status) {
> >> +			clear_bit(HCI_CONN_MODE_CHANGE_PEND, &conn->pend);
> >> +
> >> +			if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> >> +					&conn->pend))
> >> +				hci_sco_setup(conn, status);
> >> +		}
> >> +	}
> >>  
> >>  	hci_dev_unlock(hdev);
> >>  }
> > 
> > Same applies here btw.
> 
> Same. Done.
> 
> > 
> >> @@ -915,20 +923,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
> >>  	} else
> >>  		conn->state = BT_CLOSED;
> >>  
> >> -	if (conn->type == ACL_LINK) {
> >> -		struct hci_conn *sco = conn->link;
> >> -		if (sco) {
> >> -			if (!ev->status) {
> >> -				if (lmp_esco_capable(hdev))
> >> -					hci_setup_sync(sco, conn->handle);
> >> -				else
> >> -					hci_add_sco(sco, conn->handle);
> >> -			} else {
> >> -				hci_proto_connect_cfm(sco, ev->status);
> >> -				hci_conn_del(sco);
> >> -			}
> >> -		}
> >> -	}
> >> +	if (conn->type == ACL_LINK)
> >> +		hci_sco_setup(conn, ev->status);
> >>  
> >>  	if (ev->status) {
> >>  		hci_proto_connect_cfm(conn, ev->status);
> >> @@ -1478,6 +1474,9 @@ 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 if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
> >> +				&conn->pend)) {
> >> +			hci_sco_setup(conn, ev->status);
> >>  		}
> >>  	}
> > 
> > Please don't do else if here. Just add a separate check like I had in my
> > initial patch. We don't wanna depend on two flags pending flags set
> > here. If the SCO setup is pending, then we execute it. No matter how we
> > reached that point. It is important that even if we receive a mode
> > change without us triggering it, that we clear any potential SCO pending
> > flag.
> 
> Hmmm. I know I found a bug here, but I can't remember how it manifested
> itself. Today, I can't reproduce it, so either I was seeing things or
> I'm losing it.
> 
> I'll revert it back, and if I run across the issue again, we can fix it
> later.

please send v3 of this patch then.

Regards

Marcel

[PATCH v3] Bluetooth: Defer SCO setup if mode change is pending

[Ron Shaffer]

From: Marcel Holtmann <marcel@holtmann.org>

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.

< HCI Command: Exit Sniff Mode (0x02|0x0004) plen 2
    handle 12
> 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
> HCI Event: Command Status (0x0f) plen 4
    Setup Synchronous Connection (0x01|0x0028) status 0x00 ncmd 1
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 12 packets 1
> HCI Event: Mode Change (0x14) plen 6
    status 0x00 handle 12 mode 0x00 interval 0
    Mode: Active
> 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

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: Ron Shaffer <rshaffer@codeaurora.org>
---
 include/net/bluetooth/hci_core.h |    2 ++
 net/bluetooth/hci_conn.c         |   32 ++++++++++++++++++++++++++++----
 net/bluetooth/hci_event.c        |   33 +++++++++++++++++----------------
 3 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e42f6ed..edf3dfe 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -250,6 +250,7 @@ enum {
 	HCI_CONN_ENCRYPT_PEND,
 	HCI_CONN_RSWITCH_PEND,
 	HCI_CONN_MODE_CHANGE_PEND,
+	HCI_CONN_SCO_SETUP_PEND,
 };
 
 static inline void hci_conn_hash_init(struct hci_dev *hdev)
@@ -330,6 +331,7 @@ void hci_acl_connect(struct hci_conn *conn);
 void hci_acl_disconn(struct hci_conn *conn, __u8 reason);
 void hci_add_sco(struct hci_conn *conn, __u16 handle);
 void hci_setup_sync(struct hci_conn *conn, __u16 handle);
+void hci_sco_setup(struct hci_conn *conn, __u8 status);
 
 struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
 int hci_conn_del(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b10e3cd..805a22c 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -155,6 +155,27 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
 	hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
 }
 
+/* Device _must_ be locked */
+void hci_sco_setup(struct hci_conn *conn, __u8 status)
+{
+	struct hci_conn *sco = conn->link;
+
+	BT_DBG("%p", conn);
+
+	if (!sco)
+		return;
+
+	if (!status) {
+		if (lmp_esco_capable(conn->hdev))
+			hci_setup_sync(sco, conn->handle);
+		else
+			hci_add_sco(sco, conn->handle);
+	} else {
+		hci_proto_connect_cfm(sco, status);
+		hci_conn_del(sco);
+	}
+}
+
 static void hci_conn_timeout(unsigned long arg)
 {
 	struct hci_conn *conn = (void *) arg;
@@ -380,10 +401,13 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		acl->power_save = 1;
 		hci_conn_enter_active_mode(acl);
 
-		if (lmp_esco_capable(hdev))
-			hci_setup_sync(sco, acl->handle);
-		else
-			hci_add_sco(sco, acl->handle);
+		if (test_bit(HCI_CONN_MODE_CHANGE_PEND, &acl->pend)) {
+			/* defer SCO setup until mode change completed */
+			set_bit(HCI_CONN_SCO_SETUP_PEND, &acl->pend);
+			return sco;
+		}
+
+		hci_sco_setup(acl, 0x00);
 	}
 
 	return sco;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 6c57fc7..4fda049 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -785,9 +785,14 @@ static void hci_cs_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);
 
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
+				&conn->pend))
+			hci_sco_setup(conn, status);
+	}
+
 	hci_dev_unlock(hdev);
 }
 
@@ -808,9 +813,14 @@ 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);
 
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND,
+				&conn->pend))
+			hci_sco_setup(conn, status);
+	}
+
 	hci_dev_unlock(hdev);
 }
 
@@ -915,20 +925,8 @@ static inline void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *s
 	} else
 		conn->state = BT_CLOSED;
 
-	if (conn->type == ACL_LINK) {
-		struct hci_conn *sco = conn->link;
-		if (sco) {
-			if (!ev->status) {
-				if (lmp_esco_capable(hdev))
-					hci_setup_sync(sco, conn->handle);
-				else
-					hci_add_sco(sco, conn->handle);
-			} else {
-				hci_proto_connect_cfm(sco, ev->status);
-				hci_conn_del(sco);
-			}
-		}
-	}
+	if (conn->type == ACL_LINK)
+		hci_sco_setup(conn, ev->status);
 
 	if (ev->status) {
 		hci_proto_connect_cfm(conn, ev->status);
@@ -1479,6 +1477,9 @@ static inline void hci_mode_change_evt(struct hci_dev *hdev, struct sk_buff *skb
 			else
 				conn->power_save = 0;
 		}
+
+		if (test_and_clear_bit(HCI_CONN_SCO_SETUP_PEND, &conn->pend))
+			hci_sco_setup(conn, ev->status);
 	}
 
 	hci_dev_unlock(hdev);
-- 
1.7.0.2
--
Ron Shaffer
Employee of the Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCH v3] Bluetooth: Defer SCO setup if mode change is pending

[Marcel Holtmann]

Hi Ron,

> 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.
> 
> < HCI Command: Exit Sniff Mode (0x02|0x0004) plen 2
>     handle 12
> > 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
> > HCI Event: Command Status (0x0f) plen 4
>     Setup Synchronous Connection (0x01|0x0028) status 0x00 ncmd 1
> > HCI Event: Number of Completed Packets (0x13) plen 5
>     handle 12 packets 1
> > HCI Event: Mode Change (0x14) plen 6
>     status 0x00 handle 12 mode 0x00 interval 0
>     Mode: Active
> > 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
> 
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> Signed-off-by: Ron Shaffer <rshaffer@codeaurora.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 ++
>  net/bluetooth/hci_conn.c         |   32 ++++++++++++++++++++++++++++----
>  net/bluetooth/hci_event.c        |   33 +++++++++++++++++----------------
>  3 files changed, 47 insertions(+), 20 deletions(-)

I did a small cleanup, but patch has been applied now.

Regards

Marcel