Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <5378e0c1e4b8710bb54442b8ae1972fbf17c7321.camel@collabora.com>
Subject: Re: [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI
 conn event
From:   Olivier =?ISO-8859-1?Q?Cr=EAte?= <olivier.crete@collabora.com>
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Johan Hedberg <johan.hedberg@gmail.com>
Date:   Mon, 25 May 2020 15:37:58 -0400
In-Reply-To: <0858E407-99A0-4EF7-B890-B367B206F4AD@holtmann.org>
References: <20200506025358.361519-1-olivier.crete@collabora.com>
         <0858E407-99A0-4EF7-B890-B367B206F4AD@holtmann.org>
Organization: Collabora
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.2 (3.36.2-1.fc32) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

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