Re: [PATCH] Bluetooth: clean up connection in hci_cs_disconnect

From: Marcel Holtmann
Date: Wed Mar 11 2020 - 10:31:31 EST


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@xxxxxxxxxxxx>
> Signed-off-by: Manish Mandlik <mmandlik@xxxxxxxxxx>
> ---
>
> 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