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(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index fd6fd4029452..f0e1f1ae39c5 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -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,
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index ce94ffaf06d4..a3b226255eb9 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -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,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 868ffccff773..539bbbe26176 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -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);
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index c5d8799046cc..4c707eb64e6f 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -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);
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index cf3b8e9b7b3b..3e5d2d8a2a66 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -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
From: Luiz Augusto von Dentz <[email protected]>
syzbot reported sco_sock_setsockopt() is copying data without
checking user input length.
BUG: KASAN: slab-out-of-bounds in copy_from_sockptr_offset
include/linux/sockptr.h:49 [inline]
BUG: KASAN: slab-out-of-bounds in copy_from_sockptr
include/linux/sockptr.h:55 [inline]
BUG: KASAN: slab-out-of-bounds in sco_sock_setsockopt+0xc0b/0xf90
net/bluetooth/sco.c:893
Read of size 4 at addr ffff88805f7b15a3 by task syz-executor.5/12578
Fixes: ad10b1a48754 ("Bluetooth: Add Bluetooth socket voice option")
Fixes: b96e9c671b05 ("Bluetooth: Add BT_DEFER_SETUP option to sco socket")
Fixes: 00398e1d5183 ("Bluetooth: Add support for BT_PKT_STATUS CMSG data for SCO connections")
Fixes: f6873401a608 ("Bluetooth: Allow setting of codec for HFP offload use case")
Reported-by: syzbot <[email protected]>
Signed-off-by: Eric Dumazet <[email protected]>
Signed-off-by: Luiz Augusto von Dentz <[email protected]>
---
include/net/bluetooth/bluetooth.h | 9 +++++++++
net/bluetooth/sco.c | 23 ++++++++++-------------
2 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 1ada4f85e982..431c907155e7 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -586,6 +586,15 @@ static inline struct sk_buff *bt_skb_sendmmsg(struct sock *sk,
return skb;
}
+static inline int bt_copy_from_sockptr(void *dst, size_t dst_size,
+ sockptr_t src, size_t src_size)
+{
+ if (dst_size > src_size)
+ return -EINVAL;
+
+ return copy_from_sockptr(dst, src, dst_size);
+}
+
int bt_to_errno(u16 code);
__u8 bt_status(int err);
diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
index 99c2b713d826..cc72f05d4f0e 100644
--- a/net/bluetooth/sco.c
+++ b/net/bluetooth/sco.c
@@ -839,7 +839,7 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
sockptr_t optval, unsigned int optlen)
{
struct sock *sk = sock->sk;
- int len, err = 0;
+ int err = 0;
struct bt_voice voice;
u32 opt;
struct bt_codecs *codecs;
@@ -858,10 +858,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
break;
}
- if (copy_from_sockptr(&opt, optval, sizeof(u32))) {
- err = -EFAULT;
+ err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+ if (err)
break;
- }
if (opt)
set_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags);
@@ -878,11 +877,10 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
voice.setting = sco_pi(sk)->setting;
- len = min_t(unsigned int, sizeof(voice), optlen);
- if (copy_from_sockptr(&voice, optval, len)) {
- err = -EFAULT;
+ err = bt_copy_from_sockptr(&voice, sizeof(voice), optval,
+ optlen);
+ if (err)
break;
- }
/* Explicitly check for these values */
if (voice.setting != BT_VOICE_TRANSPARENT &&
@@ -905,10 +903,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
break;
case BT_PKT_STATUS:
- if (copy_from_sockptr(&opt, optval, sizeof(u32))) {
- err = -EFAULT;
+ err = bt_copy_from_sockptr(&opt, sizeof(opt), optval, optlen);
+ if (err)
break;
- }
if (opt)
set_bit(BT_SK_PKT_STATUS, &bt_sk(sk)->flags);
@@ -949,9 +946,9 @@ static int sco_sock_setsockopt(struct socket *sock, int level, int optname,
break;
}
- if (copy_from_sockptr(buffer, optval, optlen)) {
+ err = bt_copy_from_sockptr(buffer, optlen, optval, optlen);
+ if (err) {
hci_dev_put(hdev);
- err = -EFAULT;
break;
}
--
2.44.0
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=841940
---Test result---
Test Summary:
CheckPatch PASS 1.42 seconds
GitLint PASS 0.31 seconds
SubjectPrefix PASS 0.12 seconds
BuildKernel PASS 30.80 seconds
CheckAllWarning PASS 33.87 seconds
CheckSparse WARNING 38.40 seconds
CheckSmatch FAIL 35.62 seconds
BuildKernel32 PASS 29.76 seconds
TestRunnerSetup PASS 538.83 seconds
TestRunner_l2cap-tester PASS 20.58 seconds
TestRunner_iso-tester PASS 33.21 seconds
TestRunner_bnep-tester PASS 4.84 seconds
TestRunner_mgmt-tester FAIL 117.42 seconds
TestRunner_rfcomm-tester PASS 7.38 seconds
TestRunner_sco-tester PASS 14.98 seconds
TestRunner_ioctl-tester PASS 7.67 seconds
TestRunner_mesh-tester PASS 5.74 seconds
TestRunner_smp-tester PASS 6.85 seconds
TestRunner_userchan-tester PASS 4.95 seconds
IncrementalBuild PASS 28.67 seconds
Details
##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 488 (99.2%), Failed: 2, Not Run: 2
Failed Test Cases
LL Privacy - Add Device 5 (2 Devices to RL) Failed 0.170 seconds
LL Privacy - Remove Device 4 (Disable Adv) Timed out 1.918 seconds
---
Regards,
Linux Bluetooth
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[email protected]>:
On Fri, 5 Apr 2024 16:40:33 -0400 you wrote:
> 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.
>
> [...]
Here is the summary with links:
- [v1] Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync
https://git.kernel.org/bluetooth/bluetooth-next/c/6093f28402aa
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hej,
On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> 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
> 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(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index fd6fd4029452..f0e1f1ae39c5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -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,
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index ce94ffaf06d4..a3b226255eb9 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -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,
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 868ffccff773..539bbbe26176 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -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);
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index c5d8799046cc..4c707eb64e6f 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -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);
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index cf3b8e9b7b3b..3e5d2d8a2a66 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -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,
On Thu, May 9, 2024 at 12:06 PM Janne Grunau <[email protected]> wrote:
>
> Hej,
>
> On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> > 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.
Would be great if you provide the HCI trace to confirm the problem.
> 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
>
> > 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(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index fd6fd4029452..f0e1f1ae39c5 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -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,
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index ce94ffaf06d4..a3b226255eb9 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -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,
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 868ffccff773..539bbbe26176 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -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);
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index c5d8799046cc..4c707eb64e6f 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -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);
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index cf3b8e9b7b3b..3e5d2d8a2a66 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -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
> >
--
Luiz Augusto von Dentz
On Thu, May 09, 2024 at 12:30:21PM -0400, Luiz Augusto von Dentz wrote:
> Hi Janne,
>
> On Thu, May 9, 2024 at 12:06 PM Janne Grunau <[email protected]> wrote:
> >
> > Hej,
> >
> > On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> > > 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.
>
> 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,
On Thu, May 9, 2024 at 4:09 PM Janne Grunau <[email protected]> wrote:
>
> On Thu, May 09, 2024 at 12:30:21PM -0400, Luiz Augusto von Dentz wrote:
> > Hi Janne,
> >
> > On Thu, May 9, 2024 at 12:06 PM Janne Grunau <[email protected]> wrote:
> > >
> > > Hej,
> > >
> > > On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> > > > 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.
> >
> > 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
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.
> | 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
--
Luiz Augusto von Dentz
Hi Janne,
On Thu, May 9, 2024 at 6:23 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Janne,
>
> On Thu, May 9, 2024 at 4:09 PM Janne Grunau <[email protected]> wrote:
> >
> > On Thu, May 09, 2024 at 12:30:21PM -0400, Luiz Augusto von Dentz wrote:
> > > Hi Janne,
> > >
> > > On Thu, May 9, 2024 at 12:06 PM Janne Grunau <[email protected]> wrote:
> > > >
> > > > Hej,
> > > >
> > > > On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> > > > > 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.
> > >
> > > 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
>
> 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.
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.
> > | 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
>
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi,
On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
<snip>
> 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).
- Hector
Hi,
On Fri, May 10, 2024, at 05:13, Hector Martin wrote:
> Hi,
>
> On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
> <snip>
>
>> 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.
Sven
Hi Sven, Hector,
On Fri, May 10, 2024 at 5:55 AM Sven Peter <[email protected]> wrote:
>
> Hi,
>
>
> On Fri, May 10, 2024, at 05:13, Hector Martin wrote:
> > Hi,
> >
> > On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
> > <snip>
> >
> >> 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.
> 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.
Yeah, I think I saw something like that not long ago, we might need
yet another quirk to deal with that though.
--
Luiz Augusto von Dentz
Hi Luiz,
On Fri, May 10, 2024, at 16:33, Luiz Augusto von Dentz wrote:
> Hi Sven, Hector,
>
> On Fri, May 10, 2024 at 5:55 AM Sven Peter <[email protected]> wrote:
>>
>> Hi,
>>
>>
>> On Fri, May 10, 2024, at 05:13, Hector Martin wrote:
>> > Hi,
>> >
>> > On 2024/05/10 7:41, Luiz Augusto von Dentz wrote:
>> > <snip>
>> >
>> >> 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.
>
>> 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.
>
> Yeah, I think I saw something like that not long ago, we might need
> yet another quirk to deal with that though.
Agreed. We had a similar situation with this very same broken hardware when I
originally upstreamed the driver:
Apple and/or Broadcom decided to use the upper event type bits inside LE Extended
Advertising Reports for the channel on which the frame has been received [1].
I originally proposed a quick to drop these reserved bits but we then later
decided to just do it unconditionally [2].
[1] https://lore.kernel.org/linux-bluetooth/[email protected]/
[2] https://lore.kernel.org/linux-bluetooth/[email protected]/
Best,
Sven