linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event
@ 2020-05-06  2:53 Olivier Crête
  2020-05-06  8:47 ` Marcel Holtmann
  0 siblings, 1 reply; 3+ messages in thread
From: Olivier Crête @ 2020-05-06  2:53 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Marcel Holtmann, Johan Hedberg

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
---
 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);
+
 		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);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event
  2020-05-06  2:53 [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event Olivier Crête
@ 2020-05-06  8:47 ` Marcel Holtmann
  2020-05-25 19:37   ` Olivier Crête
  0 siblings, 1 reply; 3+ messages in thread
From: Marcel Holtmann @ 2020-05-06  8:47 UTC (permalink / raw)
  To: Olivier Crête; +Cc: linux-bluetooth, Johan Hedberg

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.

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?

> ---
> 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


^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event
  2020-05-06  8:47 ` Marcel Holtmann
@ 2020-05-25 19:37   ` Olivier Crête
  0 siblings, 0 replies; 3+ messages in thread
From: Olivier Crête @ 2020-05-25 19:37 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth, Johan Hedberg

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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-25 19:38 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06  2:53 [PATCH] Bluetooth: Avoid calling device_add() on duplicated HCI conn event Olivier Crête
2020-05-06  8:47 ` Marcel Holtmann
2020-05-25 19:37   ` Olivier Crête

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).