Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync

From: Luiz Augusto von Dentz <[email protected]>

The extended advertising reports do report the PHYs so this store then in hci_conn so it can be later used in hci_le_ext_create_conn_sync to narrow the PHYs to be scanned since the controller will also perform a scan having a smaller set of PHYs shall reduce the time it takes to find and connect peers.

Fixes 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")

Signed-off-by Luiz Augusto von Dentz <[email protected]>

---
 include/net/bluetooth/hci_core.h |  4 +++-
 net/bluetooth/hci_conn.c         |  6 ++++--
 net/bluetooth/hci_event.c        | 20 ++++++++++++--------
 net/bluetooth/hci_sync.c         |  9 ++++++---
 net/bluetooth/l2cap_core.c       |  2 +-
 5 files changed, 26 insertions(+), 15 deletions(-)

include/net/bluetooth/hci_core.h +3 -1

index fd6fd4029452..f0e1f1ae39c5 100644
@@ -744,6 +744,8 @@ struct hci_conn {
 	__u8		le_per_adv_data[HCI_MAX_PER_AD_TOT_LEN];
 	__u16		le_per_adv_data_len;
 	__u16		le_per_adv_data_offset;
+	__u8		le_adv_phy;
+	__u8		le_adv_sec_phy;
 	__u8		le_tx_phy;
 	__u8		le_rx_phy;
 	__s8		rssi;

@@ -1519,7 +1521,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
 				     enum conn_reasons conn_reason);
 struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 				u8 dst_type, bool dst_resolved, u8 sec_level,
-				u16 conn_timeout, u8 role);
+				u16 conn_timeout, u8 role, u8 phy, u8 sec_phy);
 void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
 struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 				 u8 sec_level, u8 auth_type,

net/bluetooth/hci_conn.c +4 -2

index ce94ffaf06d4..a3b226255eb9 100644
@@ -1266,7 +1266,7 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
 
 struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 				u8 dst_type, bool dst_resolved, u8 sec_level,
-				u16 conn_timeout, u8 role)
+				u16 conn_timeout, u8 role, u8 phy, u8 sec_phy)
 {
 	struct hci_conn *conn;
 	struct smp_irk *irk;

@@ -1329,6 +1329,8 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
 	conn->dst_type = dst_type;
 	conn->sec_level = BT_SECURITY_LOW;
 	conn->conn_timeout = conn_timeout;
+	conn->le_adv_phy = phy;
+	conn->le_adv_sec_phy = sec_phy;
 
 	err = hci_connect_le_sync(hdev, conn);
 	if (err) {

@@ -2276,7 +2278,7 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
 		le = hci_connect_le(hdev, dst, dst_type, false,
 				    BT_SECURITY_LOW,
 				    HCI_LE_CONN_TIMEOUT,
-				    HCI_ROLE_SLAVE);
+				    HCI_ROLE_SLAVE, 0, 0);
 	else
 		le = hci_connect_le_scan(hdev, dst, dst_type,
 					 BT_SECURITY_LOW,

net/bluetooth/hci_event.c +12 -8

index 868ffccff773..539bbbe26176 100644
@@ -6046,7 +6046,7 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, void *data,
 static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 					      bdaddr_t *addr,
 					      u8 addr_type, bool addr_resolved,
-					      u8 adv_type)
+					      u8 adv_type, u8 phy, u8 sec_phy)
 {
 	struct hci_conn *conn;
 	struct hci_conn_params *params;

@@ -6101,7 +6101,7 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 
 	conn = hci_connect_le(hdev, addr, addr_type, addr_resolved,
 			      BT_SECURITY_LOW, hdev->def_le_autoconnect_timeout,
-			      HCI_ROLE_MASTER);
+			      HCI_ROLE_MASTER, phy, sec_phy);
 	if (!IS_ERR(conn)) {
 		/* If HCI_AUTO_CONN_EXPLICIT is set, conn is already owned
 		 * by higher layer that tried to connect, if no then

@@ -6136,8 +6136,9 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 
 static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 			       u8 bdaddr_type, bdaddr_t *direct_addr,
-			       u8 direct_addr_type, s8 rssi, u8 *data, u8 len,
-			       bool ext_adv, bool ctl_time, u64 instant)
+			       u8 direct_addr_type, u8 phy, u8 sec_phy, s8 rssi,
+			       u8 *data, u8 len, bool ext_adv, bool ctl_time,
+			       u64 instant)
 {
 	struct discovery_state *d = &hdev->discovery;
 	struct smp_irk *irk;

@@ -6225,7 +6226,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
 	 * for advertising reports) and is already verified to be RPA above.
 	 */
 	conn = check_pending_le_conn(hdev, bdaddr, bdaddr_type, bdaddr_resolved,
-				     type);
+				     type, phy, sec_phy);
 	if (!ext_adv && conn && type == LE_ADV_IND &&
 	    len <= max_adv_len(hdev)) {
 		/* Store report for later inclusion by

@@ -6371,7 +6372,8 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, void *data,
 		if (info->length <= max_adv_len(hdev)) {
 			rssi = info->data[info->length];
 			process_adv_report(hdev, info->type, &info->bdaddr,
-					   info->bdaddr_type, NULL, 0, rssi,
+					   info->bdaddr_type, NULL, 0,
+					   HCI_ADV_PHY_1M, 0, rssi,
 					   info->data, info->length, false,
 					   false, instant);
 		} else {

@@ -6456,6 +6458,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, void *data,
 		if (legacy_evt_type != LE_ADV_INVALID) {
 			process_adv_report(hdev, legacy_evt_type, &info->bdaddr,
 					   info->bdaddr_type, NULL, 0,
+					   info->primary_phy,
+					   info->secondary_phy,
 					   info->rssi, info->data, info->length,
 					   !(evt_type & LE_EXT_ADV_LEGACY_PDU),
 					   false, instant);

@@ -6761,8 +6765,8 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev, void *data,
 
 		process_adv_report(hdev, info->type, &info->bdaddr,
 				   info->bdaddr_type, &info->direct_addr,
-				   info->direct_addr_type, info->rssi, NULL, 0,
-				   false, false, instant);
+				   info->direct_addr_type, HCI_ADV_PHY_1M, 0,
+				   info->rssi, NULL, 0, false, false, instant);
 	}
 
 	hci_dev_unlock(hdev);

net/bluetooth/hci_sync.c +6 -3

index c5d8799046cc..4c707eb64e6f 100644
@@ -6346,7 +6346,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 
 	plen = sizeof(*cp);
 
-	if (scan_1m(hdev)) {
+	if (scan_1m(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_1M ||
+			      conn->le_adv_sec_phy == HCI_ADV_PHY_1M)) {
 		cp->phys |= LE_SCAN_PHY_1M;
 		set_ext_conn_params(conn, p);
 

@@ -6354,7 +6355,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 		plen += sizeof(*p);
 	}
 
-	if (scan_2m(hdev)) {
+	if (scan_2m(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_2M ||
+			      conn->le_adv_sec_phy == HCI_ADV_PHY_2M)) {
 		cp->phys |= LE_SCAN_PHY_2M;
 		set_ext_conn_params(conn, p);
 

@@ -6362,7 +6364,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 		plen += sizeof(*p);
 	}
 
-	if (scan_coded(hdev)) {
+	if (scan_coded(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_CODED ||
+				 conn->le_adv_sec_phy == HCI_ADV_PHY_CODED)) {
 		cp->phys |= LE_SCAN_PHY_CODED;
 		set_ext_conn_params(conn, p);
 

net/bluetooth/l2cap_core.c +1 -1

index cf3b8e9b7b3b..3e5d2d8a2a66 100644
@@ -7028,7 +7028,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
 		if (hci_dev_test_flag(hdev, HCI_ADVERTISING))
 			hcon = hci_connect_le(hdev, dst, dst_type, false,
 					      chan->sec_level, timeout,
-					      HCI_ROLE_SLAVE);
+					      HCI_ROLE_SLAVE, 0, 0);
 		else
 			hcon = hci_connect_le_scan(hdev, dst, dst_type,
 						   chan->sec_level, timeout,

Signature · 1 line

2.44.0

Hej,

Quoted 9 lines · Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

The extended advertising reports do report the PHYs so this store then in hci_conn so it can be later used in hci_le_ext_create_conn_sync to narrow the PHYs to be scanned since the controller will also perform a scan having a smaller set of PHYs shall reduce the time it takes to find and connect peers.

Fixes 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")

This commit in v6.8.9 apparently has regressed connecting to LE devices like Logitech mices with Apple/Broadcom BCM4388 devices. Those devices carry HCI_QUIRK_BROKEN_LE_CODED which became necessary after 288c90224eec ("Bluetooth: Enable all supported LE PHY by default").
Tested so far only by reverting aaf06285498861d6caaff5b26d30af70dd2b819f on top of v6.8.9. Looking at the change I don't see anything obvious which would explain the breakage.
I would assume v6.9-rc6 is affected as well but I haven't tested this yet.

If you have an idea what could be responsible for the regression I'll happily test patches/changes.

thanks,
Janne

#regzbot introduced: v6.8.9

Quoted 185 lines

Signed-off-by Luiz Augusto von Dentz <[email protected]>

---
 include/net/bluetooth/hci_core.h |  4 +++-
 net/bluetooth/hci_conn.c         |  6 ++++--
 net/bluetooth/hci_event.c        | 20 ++++++++++++--------
 net/bluetooth/hci_sync.c         |  9 ++++++---
 net/bluetooth/l2cap_core.c       |  2 +-
 5 files changed, 26 insertions(+), 15 deletions(-)

include/net/bluetooth/hci_core.h +3 -1

index fd6fd4029452..f0e1f1ae39c5 100644
@@ -744,6 +744,8 @@ struct hci_conn {
      __u8            le_per_adv_data[HCI_MAX_PER_AD_TOT_LEN];
      __u16           le_per_adv_data_len;
      __u16           le_per_adv_data_offset;
+     __u8            le_adv_phy;
+     __u8            le_adv_sec_phy;
      __u8            le_tx_phy;
      __u8            le_rx_phy;
      __s8            rssi;

@@ -1519,7 +1521,7 @@ struct hci_conn *hci_connect_le_scan(struct hci_dev *hdev, bdaddr_t *dst,
                                   enum conn_reasons conn_reason);
 struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
                              u8 dst_type, bool dst_resolved, u8 sec_level,
-                             u16 conn_timeout, u8 role);
+                             u16 conn_timeout, u8 role, u8 phy, u8 sec_phy);
 void hci_connect_le_scan_cleanup(struct hci_conn *conn, u8 status);
 struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
                               u8 sec_level, u8 auth_type,

net/bluetooth/hci_conn.c +4 -2

index ce94ffaf06d4..a3b226255eb9 100644
@@ -1266,7 +1266,7 @@ u8 hci_conn_set_handle(struct hci_conn *conn, u16 handle)
 
 struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
                              u8 dst_type, bool dst_resolved, u8 sec_level,
-                             u16 conn_timeout, u8 role)
+                             u16 conn_timeout, u8 role, u8 phy, u8 sec_phy)
 {
      struct hci_conn *conn;
      struct smp_irk *irk;

@@ -1329,6 +1329,8 @@ struct hci_conn *hci_connect_le(struct hci_dev *hdev, bdaddr_t *dst,
      conn->dst_type = dst_type;
      conn->sec_level = BT_SECURITY_LOW;
      conn->conn_timeout = conn_timeout;
+     conn->le_adv_phy = phy;
+     conn->le_adv_sec_phy = sec_phy;
 
      err = hci_connect_le_sync(hdev, conn);
      if (err) {

@@ -2276,7 +2278,7 @@ struct hci_conn *hci_connect_cis(struct hci_dev *hdev, bdaddr_t *dst,
              le = hci_connect_le(hdev, dst, dst_type, false,
                                  BT_SECURITY_LOW,
                                  HCI_LE_CONN_TIMEOUT,
-                                 HCI_ROLE_SLAVE);
+                                 HCI_ROLE_SLAVE, 0, 0);
      else
              le = hci_connect_le_scan(hdev, dst, dst_type,
                                       BT_SECURITY_LOW,

net/bluetooth/hci_event.c +12 -8

index 868ffccff773..539bbbe26176 100644
@@ -6046,7 +6046,7 @@ static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, void *data,
 static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
                                            bdaddr_t *addr,
                                            u8 addr_type, bool addr_resolved,
-                                           u8 adv_type)
+                                           u8 adv_type, u8 phy, u8 sec_phy)
 {
      struct hci_conn *conn;
      struct hci_conn_params *params;

@@ -6101,7 +6101,7 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 
      conn = hci_connect_le(hdev, addr, addr_type, addr_resolved,
                            BT_SECURITY_LOW, hdev->def_le_autoconnect_timeout,
-                           HCI_ROLE_MASTER);
+                           HCI_ROLE_MASTER, phy, sec_phy);
      if (!IS_ERR(conn)) {
              /* If HCI_AUTO_CONN_EXPLICIT is set, conn is already owned
               * by higher layer that tried to connect, if no then

@@ -6136,8 +6136,9 @@ static struct hci_conn *check_pending_le_conn(struct hci_dev *hdev,
 
 static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
                             u8 bdaddr_type, bdaddr_t *direct_addr,
-                            u8 direct_addr_type, s8 rssi, u8 *data, u8 len,
-                            bool ext_adv, bool ctl_time, u64 instant)
+                            u8 direct_addr_type, u8 phy, u8 sec_phy, s8 rssi,
+                            u8 *data, u8 len, bool ext_adv, bool ctl_time,
+                            u64 instant)
 {
      struct discovery_state *d = &hdev->discovery;
      struct smp_irk *irk;

@@ -6225,7 +6226,7 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
       * for advertising reports) and is already verified to be RPA above.
       */
      conn = check_pending_le_conn(hdev, bdaddr, bdaddr_type, bdaddr_resolved,
-                                  type);
+                                  type, phy, sec_phy);
      if (!ext_adv && conn && type == LE_ADV_IND &&
          len <= max_adv_len(hdev)) {
              /* Store report for later inclusion by

@@ -6371,7 +6372,8 @@ static void hci_le_adv_report_evt(struct hci_dev *hdev, void *data,
              if (info->length <= max_adv_len(hdev)) {
                      rssi = info->data[info->length];
                      process_adv_report(hdev, info->type, &info->bdaddr,
-                                        info->bdaddr_type, NULL, 0, rssi,
+                                        info->bdaddr_type, NULL, 0,
+                                        HCI_ADV_PHY_1M, 0, rssi,
                                         info->data, info->length, false,
                                         false, instant);
              } else {

@@ -6456,6 +6458,8 @@ static void hci_le_ext_adv_report_evt(struct hci_dev *hdev, void *data,
              if (legacy_evt_type != LE_ADV_INVALID) {
                      process_adv_report(hdev, legacy_evt_type, &info->bdaddr,
                                         info->bdaddr_type, NULL, 0,
+                                        info->primary_phy,
+                                        info->secondary_phy,
                                         info->rssi, info->data, info->length,
                                         !(evt_type & LE_EXT_ADV_LEGACY_PDU),
                                         false, instant);

@@ -6761,8 +6765,8 @@ static void hci_le_direct_adv_report_evt(struct hci_dev *hdev, void *data,
 
              process_adv_report(hdev, info->type, &info->bdaddr,
                                 info->bdaddr_type, &info->direct_addr,
-                                info->direct_addr_type, info->rssi, NULL, 0,
-                                false, false, instant);
+                                info->direct_addr_type, HCI_ADV_PHY_1M, 0,
+                                info->rssi, NULL, 0, false, false, instant);
      }
 
      hci_dev_unlock(hdev);

net/bluetooth/hci_sync.c +6 -3

index c5d8799046cc..4c707eb64e6f 100644
@@ -6346,7 +6346,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 
      plen = sizeof(*cp);
 
-     if (scan_1m(hdev)) {
+     if (scan_1m(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_1M ||
+                           conn->le_adv_sec_phy == HCI_ADV_PHY_1M)) {
              cp->phys |= LE_SCAN_PHY_1M;
              set_ext_conn_params(conn, p);
 

@@ -6354,7 +6355,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
              plen += sizeof(*p);
      }
 
-     if (scan_2m(hdev)) {
+     if (scan_2m(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_2M ||
+                           conn->le_adv_sec_phy == HCI_ADV_PHY_2M)) {
              cp->phys |= LE_SCAN_PHY_2M;
              set_ext_conn_params(conn, p);
 

@@ -6362,7 +6364,8 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
              plen += sizeof(*p);
      }
 
-     if (scan_coded(hdev)) {
+     if (scan_coded(hdev) && (conn->le_adv_phy == HCI_ADV_PHY_CODED ||
+                              conn->le_adv_sec_phy == HCI_ADV_PHY_CODED)) {
              cp->phys |= LE_SCAN_PHY_CODED;
              set_ext_conn_params(conn, p);
 

net/bluetooth/l2cap_core.c +1 -2

index cf3b8e9b7b3b..3e5d2d8a2a66 100644
@@ -7028,7 +7028,7 @@ int l2cap_chan_connect(struct l2cap_chan *chan, __le16 psm, u16 cid,
              if (hci_dev_test_flag(hdev, HCI_ADVERTISING))
                      hcon = hci_connect_le(hdev, dst, dst_type, false,
                                            chan->sec_level, timeout,
-                                           HCI_ROLE_SLAVE);
+                                           HCI_ROLE_SLAVE, 0, 0);
              else
                      hcon = hci_connect_le_scan(hdev, dst, dst_type,
                                                 chan->sec_level, timeout,
--

2.44.0

Hi Janne,

Quoted 14 lines · Janne Grunau

Hej,

Quoted 9 lines · Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

The extended advertising reports do report the PHYs so this store then in hci_conn so it can be later used in hci_le_ext_create_conn_sync to narrow the PHYs to be scanned since the controller will also perform a scan having a smaller set of PHYs shall reduce the time it takes to find and connect peers.

Fixes 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")

Quoted 9 lines

This commit in v6.8.9 apparently has regressed connecting to LE devices like Logitech mices with Apple/Broadcom BCM4388 devices. Those devices carry HCI_QUIRK_BROKEN_LE_CODED which became necessary after 288c90224eec ("Bluetooth: Enable all supported LE PHY by default").
Tested so far only by reverting aaf06285498861d6caaff5b26d30af70dd2b819f on top of v6.8.9. Looking at the change I don't see anything obvious which would explain the breakage.
I would assume v6.9-rc6 is affected as well but I haven't tested this yet.

Would be great if you provide the HCI trace to confirm the problem.

Quoted 27 lines · Luiz Augusto von Dentz

Hi Janne,

Quoted 14 lines · Janne Grunau

Hej,

Quoted 9 lines · Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

The extended advertising reports do report the PHYs so this store then in hci_conn so it can be later used in hci_le_ext_create_conn_sync to narrow the PHYs to be scanned since the controller will also perform a scan having a smaller set of PHYs shall reduce the time it takes to find and connect peers.

Fixes 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")

Quoted 9 lines

This commit in v6.8.9 apparently has regressed connecting to LE devices like Logitech mices with Apple/Broadcom BCM4388 devices. Those devices carry HCI_QUIRK_BROKEN_LE_CODED which became necessary after 288c90224eec ("Bluetooth: Enable all supported LE PHY by default").
Tested so far only by reverting aaf06285498861d6caaff5b26d30af70dd2b819f on top of v6.8.9. Looking at the change I don't see anything obvious which would explain the breakage.
I would assume v6.9-rc6 is affected as well but I haven't tested this yet.

Quoted 1 line

Would be great if you provide the HCI trace to confirm the problem.

looks like there is an issue with initiating "LE Extended Create Connection":

| > HCI Event: LE Meta Event (0x3e) plen 26
|       LE Extended Advertising Report (0x0d)
|         Num reports: 1
|         Entry 0
|           Event type: 0x2515
|             Props: 0x0015
|               Connectable
|               Directed
|               Use legacy advertising PDUs
|             Data status: Complete
|             Reserved (0x2500)
|          Legacy PDU Type: Reserved (0x2515)
|           Address type: Random (0x01)
|           Address: DF:F4:9E:F3:A9:72 (Static)
|           Primary PHY: Reserved

|           Secondary PHY: No packets
|           SID: no ADI field (0xff)
|           TX power: 127 dBm
|           RSSI: -60 dBm (0xc4)
|           Periodic advertising interval: 0.00 msec (0x0000)
|           Direct address type: Public (0x00)
|           Direct address: 5C:1B:F4:7F:BF:6B (Apple, Inc.)
|           Data length: 0x00
| < HCI Command: LE Set Extended Scan Enable (0x08|0x0042) plen 6
|         Extended scan: Disabled (0x00)
|         Filter duplicates: Disabled (0x00)
|         Duration: 0 msec (0x0000)
|         Period: 0.00 sec (0x0000)
| > HCI Event: Command Complete (0x0e) plen 4
|       LE Set Extended Scan Enable (0x08|0x0042) ncmd 1
|         Status: Success (0x00)
| < HCI Command: LE Extended Create Connection (0x08|0x0043) plen 10
|         Filter policy: Accept list is not used (0x00)
|         Own address type: Public (0x00)
|         Peer address type: Random (0x01)
|         Peer address: DF:F4:9E:F3:A9:72 (Static)
|         Initiating PHYs: 0x00
| > HCI Event: Command Status (0x0f) plen 4
|       LE Extended Create Connection (0x08|0x0043) ncmd 1
|        Status: Unsupported Feature or Parameter Value (0x11)

Full trace attached

Janne

Hi Janne,

Quoted 34 lines · Janne Grunau

Quoted 27 lines · Luiz Augusto von Dentz

Hi Janne,

Quoted 14 lines · Janne Grunau

Hej,

Quoted 9 lines · Luiz Augusto von Dentz

From: Luiz Augusto von Dentz <[email protected]>

The extended advertising reports do report the PHYs so this store then in hci_conn so it can be later used in hci_le_ext_create_conn_sync to narrow the PHYs to be scanned since the controller will also perform a scan having a smaller set of PHYs shall reduce the time it takes to find and connect peers.

Fixes 288c90224eec ("Bluetooth: Enable all supported LE PHY by default")

Quoted 9 lines

This commit in v6.8.9 apparently has regressed connecting to LE devices like Logitech mices with Apple/Broadcom BCM4388 devices. Those devices carry HCI_QUIRK_BROKEN_LE_CODED which became necessary after 288c90224eec ("Bluetooth: Enable all supported LE PHY by default").
Tested so far only by reverting aaf06285498861d6caaff5b26d30af70dd2b819f on top of v6.8.9. Looking at the change I don't see anything obvious which would explain the breakage.
I would assume v6.9-rc6 is affected as well but I haven't tested this yet.

Quoted 1 line

Would be great if you provide the HCI trace to confirm the problem.

looks like there is an issue with initiating "LE Extended Create Connection":

Quoted 15 lines

| > HCI Event: LE Meta Event (0x3e) plen 26
|       LE Extended Advertising Report (0x0d)
|         Num reports: 1
|         Entry 0
|           Event type: 0x2515
|             Props: 0x0015
|               Connectable
|               Directed
|               Use legacy advertising PDUs
|             Data status: Complete
|             Reserved (0x2500)
|          Legacy PDU Type: Reserved (0x2515)
|           Address type: Random (0x01)
|           Address: DF:F4:9E:F3:A9:72 (Static)
|           Primary PHY: Reserved

Looks like broadcom is using Reserved value as primary PHY, no wonder it doesn't work, real piece of art that broadcom and apple manage to produce and it is not only the PHY that has out of the spec result other fields are affected as well.

Quoted 26 lines

|           Secondary PHY: No packets
|           SID: no ADI field (0xff)
|           TX power: 127 dBm
|           RSSI: -60 dBm (0xc4)
|           Periodic advertising interval: 0.00 msec (0x0000)
|           Direct address type: Public (0x00)
|           Direct address: 5C:1B:F4:7F:BF:6B (Apple, Inc.)
|           Data length: 0x00
| < HCI Command: LE Set Extended Scan Enable (0x08|0x0042) plen 6
|         Extended scan: Disabled (0x00)
|         Filter duplicates: Disabled (0x00)
|         Duration: 0 msec (0x0000)
|         Period: 0.00 sec (0x0000)
| > HCI Event: Command Complete (0x0e) plen 4
|       LE Set Extended Scan Enable (0x08|0x0042) ncmd 1
|         Status: Success (0x00)
| < HCI Command: LE Extended Create Connection (0x08|0x0043) plen 10
|         Filter policy: Accept list is not used (0x00)
|         Own address type: Public (0x00)
|         Peer address type: Random (0x01)
|         Peer address: DF:F4:9E:F3:A9:72 (Static)
|         Initiating PHYs: 0x00
| > HCI Event: Command Status (0x0f) plen 4
|       LE Extended Create Connection (0x08|0x0043) ncmd 1
|        Status: Unsupported Feature or Parameter Value (0x11)

Quoted 3 lines

Full trace attached

Janne

--
Luiz Augusto von Dentz

If I print the actual value then we got:

Primary PHY: Reserved (0x81)

I guess one needs to disregard the reserved range, well until they are no longer reserved then it will no longer work. Perhaps we should talk to broadcom to know what is up with using reserved values and if that is an apple thing then perhaps we could ask them to provide firmware that acts according to the spec.

Hi,

On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
<snip>

Quoted 9 lines

If I print the actual value then we got:

Primary PHY: Reserved (0x81)

I guess one needs to disregard the reserved range, well until they are no longer reserved then it will no longer work. Perhaps we should talk to broadcom to know what is up with using reserved values and if that is an apple thing then perhaps we could ask them to provide firmware that acts according to the spec.

Apple and Broadcom do not support Linux on this platform. The kernel has to work with the existing firmware (which is the firmware macOS uses), we don't get to request changes. If the firmware has quirks the kernel has to deal with it, that's how it goes. It would be great if we had vendor support, but that is not something we can control. This is common (Linux is full of quirks to support noncompliant hardware/firmware).

Hi,

Quoted 21 lines · Hector Martin

Hi,

On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
<snip>

Quoted 9 lines

If I print the actual value then we got:

Primary PHY: Reserved (0x81)

I guess one needs to disregard the reserved range, well until they are no longer reserved then it will no longer work. Perhaps we should talk to broadcom to know what is up with using reserved values and if that is an apple thing then perhaps we could ask them to provide firmware that acts according to the spec.

Apple and Broadcom do not support Linux on this platform. The kernel has to work with the existing firmware (which is the firmware macOS uses), we don't get to request changes. If the firmware has quirks the kernel has to deal with it, that's how it goes. It would be great if we had vendor support, but that is not something we can control. This is common (Linux is full of quirks to support noncompliant hardware/firmware).

While I agree with Hector here that they won't even talk to us it's sometimes possible to figure out what exactly they were thinking with their reserved values. Apple provides "Additional Tools for XCode" which include their "PacketLogger" which contains most of their vendor specific hacks with a human readable explanation. I wasn't able to generate this specific event with my hardware here but I was able to inject a custom event into their trace format and then load it and see how it's decoded.

From a very brief look it appears that they AND Primary_PHY/Secondary_PHY with 0x1f to decode it and then support all values defined in the bluetooth spec, i.e. "no packets", "LE 1M", "LE 2M" and "LE coded". No idea what the higher bits mean though since they are ignored and don't seem to be decoded.

Hi Sven, Hector,

Quoted 26 lines · Sven Peter

Hi,

Quoted 21 lines · Hector Martin

Hi,

On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
<snip>

Quoted 9 lines

If I print the actual value then we got:

Primary PHY: Reserved (0x81)

I guess one needs to disregard the reserved range, well until they are no longer reserved then it will no longer work. Perhaps we should talk to broadcom to know what is up with using reserved values and if that is an apple thing then perhaps we could ask them to provide firmware that acts according to the spec.

Apple and Broadcom do not support Linux on this platform. The kernel has to work with the existing firmware (which is the firmware macOS uses), we don't get to request changes. If the firmware has quirks the kernel has to deal with it, that's how it goes. It would be great if we had vendor support, but that is not something we can control. This is common (Linux is full of quirks to support noncompliant hardware/firmware).

While this may work for a dedicated driver I don't think it is that easy when we are talking about a standard interface, except if you are going to maintain a separate HCI layer at the driver to intercept the traffic this will cause a lot of regression to users unaware to the fact, and it is not like you can't get rid of it, just plug an external Bluetooth dongle that behaves properly.

Quoted 8 lines

While I agree with Hector here that they won't even talk to us it's sometimes possible to figure out what exactly they were thinking with their reserved values. Apple provides "Additional Tools for XCode" which include their "PacketLogger" which contains most of their vendor specific hacks with a human readable explanation. I wasn't able to generate this specific event with my hardware here but I was able to inject a custom event into their trace format and then load it and see how it's decoded.

Quoted 4 lines

From a very brief look it appears that they AND Primary_PHY/Secondary_PHY with 0x1f to decode it and then support all values defined in the bluetooth spec, i.e. "no packets", "LE 1M", "LE 2M" and "LE coded". No idea what the higher bits mean though since they are ignored and don't seem to be decoded.

Yeah, I think I saw something like that not long ago, we might need yet another quirk to deal with that though.