From: Janne Grunau <j@jannau.net>
To: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: linux-bluetooth@vger.kernel.org, regressions@lists.linux.dev,
asahi@lists.linux.dev
Subject: Re: [PATCH v1] Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync
Date: Thu, 9 May 2024 18:06:02 +0200 [thread overview]
Message-ID: <Zjz0atzRhFykROM9@robin> (raw)
In-Reply-To: <20240405204037.3451091-1-luiz.dentz@gmail.com>
Hej,
On Fri, Apr 05, 2024 at 04:40:33PM -0400, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> 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 <luiz.von.dentz@intel.com>
> ---
> 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
>
next parent reply other threads:[~2024-05-09 16:06 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20240405204037.3451091-1-luiz.dentz@gmail.com>
2024-05-09 16:06 ` Janne Grunau [this message]
2024-05-09 16:30 ` [PATCH v1] Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync Luiz Augusto von Dentz
2024-05-09 20:09 ` Janne Grunau
2024-05-09 22:23 ` Luiz Augusto von Dentz
2024-05-09 22:41 ` Luiz Augusto von Dentz
2024-05-10 3:13 ` Hector Martin
2024-05-10 9:54 ` Sven Peter
2024-05-10 14:33 ` Luiz Augusto von Dentz
2024-05-10 14:43 ` Sven Peter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zjz0atzRhFykROM9@robin \
--to=j@jannau.net \
--cc=asahi@lists.linux.dev \
--cc=linux-bluetooth@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=regressions@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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 read-only IMAP folder(s) and NNTP newsgroup(s).