LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Bluetooth: clean up connection in hci_cs_disconnect
@ 2020-03-10 18:38 Manish Mandlik
  2020-03-11 14:31 ` Marcel Holtmann
  0 siblings, 1 reply; 2+ messages in thread
From: Manish Mandlik @ 2020-03-10 18:38 UTC (permalink / raw)
  To: marcel
  Cc: Alain Michaud, linux-bluetooth, Miao-chen Chou, Joseph Hwang,
	Yoni Shavit, Manish Mandlik, David S. Miller, Johan Hedberg,
	netdev, linux-kernel, Jakub Kicinski

From: Joseph Hwang <josephsih@chromium.org>

In bluetooth core specification 4.2,
Vol 2, Part E, 7.8.9 LE Set Advertise Enable Command, it says

    The Controller shall continue advertising until ...
    or until a connection is created or ...
    In these cases, advertising is then disabled.

Hence, advertising would be disabled before a connection is
established. In current kernel implementation, advertising would
be re-enabled when all connections are terminated.

The correct disconnection flow looks like

  < HCI Command: Disconnect

  > HCI Event: Command Status
      Status: Success

  > HCI Event: Disconnect Complete
      Status: Success

Specifically, the last Disconnect Complete Event would trigger a
callback function hci_event.c:hci_disconn_complete_evt() to
cleanup the connection and re-enable advertising when proper.

However, sometimes, there might occur an exception in the controller
when disconnection is being executed. The disconnection flow might
then look like

  < HCI Command: Disconnect

  > HCI Event: Command Status
      Status: Unknown Connection Identifier

  Note that "> HCI Event: Disconnect Complete" is missing when such an
exception occurs. This would result in advertising staying disabled
forever since the connection in question is not cleaned up correctly.

To fix the controller exception issue, we need to do some connection
cleanup when the disconnect command status indicates an error.

Signed-off-by: Joseph Hwang <josephsih@chromium.org>
Signed-off-by: Manish Mandlik <mmandlik@google.com>
---

 net/bluetooth/hci_event.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a40ed31f6eb8f..7f7e5ba3974a8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2191,6 +2191,7 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
 {
 	struct hci_cp_disconnect *cp;
 	struct hci_conn *conn;
+	u8 type;
 
 	if (!status)
 		return;
@@ -2202,10 +2203,21 @@ static void hci_cs_disconnect(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) {
 		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
 				       conn->dst_type, status);
 
+		/* If the disconnection failed for any reason, the upper layer
+		 * does not retry to disconnect in current implementation.
+		 * Hence, we need to do some basic cleanup here and re-enable
+		 * advertising if necessary.
+		 */
+		type = conn->type;
+		hci_conn_del(conn);
+		if (type == LE_LINK)
+			hci_req_reenable_advertising(hdev);
+	}
+
 	hci_dev_unlock(hdev);
 }
 
-- 
2.25.1.481.gfbce0eb801-goog


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] Bluetooth: clean up connection in hci_cs_disconnect
  2020-03-10 18:38 [PATCH] Bluetooth: clean up connection in hci_cs_disconnect Manish Mandlik
@ 2020-03-11 14:31 ` Marcel Holtmann
  0 siblings, 0 replies; 2+ messages in thread
From: Marcel Holtmann @ 2020-03-11 14:31 UTC (permalink / raw)
  To: Manish Mandlik
  Cc: Alain Michaud, Bluez mailing list, Miao-chen Chou, Joseph Hwang,
	Yoni Shavit, David S. Miller, Johan Hedberg, netdev, LKML,
	Jakub Kicinski

Hi Manish,

> In bluetooth core specification 4.2,
> Vol 2, Part E, 7.8.9 LE Set Advertise Enable Command, it says
> 
>    The Controller shall continue advertising until ...
>    or until a connection is created or ...
>    In these cases, advertising is then disabled.
> 
> Hence, advertising would be disabled before a connection is
> established. In current kernel implementation, advertising would
> be re-enabled when all connections are terminated.
> 
> The correct disconnection flow looks like
> 
>  < HCI Command: Disconnect
> 
>> HCI Event: Command Status
>      Status: Success
> 
>> HCI Event: Disconnect Complete
>      Status: Success
> 
> Specifically, the last Disconnect Complete Event would trigger a
> callback function hci_event.c:hci_disconn_complete_evt() to
> cleanup the connection and re-enable advertising when proper.
> 
> However, sometimes, there might occur an exception in the controller
> when disconnection is being executed. The disconnection flow might
> then look like
> 
>  < HCI Command: Disconnect
> 
>> HCI Event: Command Status
>      Status: Unknown Connection Identifier
> 
>  Note that "> HCI Event: Disconnect Complete" is missing when such an
> exception occurs. This would result in advertising staying disabled
> forever since the connection in question is not cleaned up correctly.
> 
> To fix the controller exception issue, we need to do some connection
> cleanup when the disconnect command status indicates an error.
> 
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> Signed-off-by: Manish Mandlik <mmandlik@google.com>
> ---
> 
> net/bluetooth/hci_event.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a40ed31f6eb8f..7f7e5ba3974a8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2191,6 +2191,7 @@ static void hci_cs_disconnect(struct hci_dev *hdev, u8 status)
> {
> 	struct hci_cp_disconnect *cp;
> 	struct hci_conn *conn;
> +	u8 type;

remove this please.

> 
> 	if (!status)
> 		return;
> @@ -2202,10 +2203,21 @@ static void hci_cs_disconnect(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) {

And add this.

		u8 type = conn->type;

> 		mgmt_disconnect_failed(hdev, &conn->dst, conn->type,
> 				       conn->dst_type, status);
> 
> +		/* If the disconnection failed for any reason, the upper layer
> +		 * does not retry to disconnect in current implementation.
> +		 * Hence, we need to do some basic cleanup here and re-enable
> +		 * advertising if necessary.
> +		 */
> +		type = conn->type;

And then remove this.

> +		hci_conn_del(conn);
> +		if (type == LE_LINK)
> +			hci_req_reenable_advertising(hdev);
> +	}
> +
> 	hci_dev_unlock(hdev);
> }

Otherwise this looks good.

Regards

Marcel


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-03-11 14:31 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 18:38 [PATCH] Bluetooth: clean up connection in hci_cs_disconnect Manish Mandlik
2020-03-11 14:31 ` Marcel Holtmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).