Re: [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event

["Olivier Crête" <olivier.crete@collabora.com>]

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@collabora.com>
> > Cc: stable@vger.kernel.org
> 
> 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@collabora.com