Bluetooth: Avoid calling device_add() on duplicated HCI conn event

The BCM20702A1 device in the ThinkPad x230 seems to send the HCI
Connection Complete event twice for the same connection, for which the
stack seems to recover, except for the core device_add() function
which is not meant to be called twice for the same device. So let's
just avoid calling it in that case.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=204633Signed-off-by: Olivier Crête <olivier.crete@collabora.com>

include/net/bluetooth/hci_core.h

@@ -449,6 +449,9 @@ struct hci_dev {
 
 #define HCI_PHY_HANDLE(handle)	(handle & 0xff)
 
+/* Valid HCI handles are in the 0x0000-0x0EFF range per spec */
+#define HCI_INVALID_HANDLE 0xFFFF
+
 struct hci_conn {
 	struct list_head list;
 

net/bluetooth/hci_conn.c

@@ -516,6 +516,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
 	conn->rssi = HCI_RSSI_INVALID;
 	conn->tx_power = HCI_TX_POWER_INVALID;
 	conn->max_tx_power = HCI_TX_POWER_INVALID;
+	conn->handle = HCI_INVALID_HANDLE;
 
 	set_bit(HCI_CONN_POWER_SAVE, &conn->flags);
 	conn->disc_timeout = HCI_DISCONN_TIMEOUT;

net/bluetooth/hci_event.c

@@ -2493,6 +2493,8 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	}
 
 	if (!ev->status) {
+		int first_connection = (conn->handle == HCI_INVALID_HANDLE);
+
 		conn->handle = __le16_to_cpu(ev->handle);
 
 		if (conn->type == ACL_LINK) {
@@ -2507,8 +2509,10 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, struct sk_buff *skb)
 		} else
 			conn->state = BT_CONNECTED;
 
-		hci_debugfs_create_conn(conn);
-		hci_conn_add_sysfs(conn);
+		if (first_connection) {
+			hci_debugfs_create_conn(conn);
+			hci_conn_add_sysfs(conn);
+		}
 
 		if (test_bit(HCI_AUTH, &hdev->flags))
 			set_bit(HCI_CONN_AUTH, &conn->flags);

[Gabriel Krisman Bertazi]

Can't you just use conn->state being already BT_CONNECTED|BT_CONFIG, instead of poisoning the handler?

old_state = conn->state; ... if (old_state != BT_CONNECTED && old_state != BT_CONFIG) { hci_conn_add_sysfs() }

seems simpler.

[Gabriel Krisman Bertazi]

The fix make sense and looks good. Not sure if gitlab is playing stupid, but the signoff line is mangled with the bugzilla line.

Remember to cc kernel@collabora.com since you are signing-off with your work email.

[Olivier Crête]

@krisman Using the state, but it gets reset by another event, so it's useless.

[Gustavo Padovan]

The fixes makes sense to me, I just wonder if there is a better way to check that. @ocrete when you say the state gets reset is that because of the duplicated event? Is it possible to check it before the reset?

A trace with btmon would help understand what is happening too.

[Olivier Crête]

From what I can see from the attached log from the kernel, it seems that the controller decides to send a connection request event again with the same connection handle (which resets the ->state member BT_CONNECT), then it sends a new connection complete (which is where this code is called). Obviously, now that I try to make a log with btmon with more info, I can't reproduce the problem (with either kernel).