Hi Marcel, On Wed, 2020-05-06 at 10:47 +0200, Marcel Holtmann wrote: > Hi Olivier, > > > 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=204633 > > Signed-off-by: Olivier Crête <olivier.crete@xxxxxxxxxxxxx> > > Cc: stable@xxxxxxxxxxxxxxx > > please include the btmon output showing this issue. The issue is quite intermittent, and it seems to happen more right after I update the rest of the distro (from Fedora 29 to 30.. and just recently from 31 to 32).. And I have no logical explanation. And right now, I can't reproduce it anymore. > And there is no firmware update available for the Bluetooth controller in the ThinkPad. Most Broadcom devices require an actual firmware load to fix bugs. Does your device load firmware? I have a firmware file extracted from the Windows driver, without it, the Bluetooth connection is even more unstable. It comes from https://github.com/winterheart/broadcom-bt-firmware/blob/master/brcm/BCM20702A1-0a5c-21e6.hcd Olivier > > > --- > > include/net/bluetooth/hci_core.h | 3 +++ > > net/bluetooth/hci_conn.c | 1 + > > net/bluetooth/hci_event.c | 8 ++++++-- > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > index d4e28773d378..b74669397dbb 100644 > > --- a/include/net/bluetooth/hci_core.h > > +++ b/include/net/bluetooth/hci_core.h > > @@ -500,6 +500,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; > > > > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c > > index e245bc155cc2..edf12a3f46aa 100644 > > --- a/net/bluetooth/hci_conn.c > > +++ b/net/bluetooth/hci_conn.c > > @@ -532,6 +532,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; > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > > index 0a591be8b0ae..e498f70fcda9 100644 > > --- a/net/bluetooth/hci_event.c > > +++ b/net/bluetooth/hci_event.c > > @@ -2553,6 +2553,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); > > + > > We have the type bool available for these kind of things. > > > conn->handle = __le16_to_cpu(ev->handle); > > > > if (conn->type == ACL_LINK) { > > @@ -2567,8 +2569,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); > > Regards > > Marcel > -- Olivier Crête olivier.crete@xxxxxxxxxxxxx
