| Message ID | 20200310113816.1.I12c0712e93f74506385b67c6df287658c8fdad04@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | Bluetooth: clean up connection in hci_cs_disconnect | expand |
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
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); }