[v1] Bluetooth: hci_sync: Use advertised PHYs on hci_le_ext_create_conn_sync

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")
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(-)

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri,  5 Apr 2024 16:40:33 -0400 you 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.
> 
> [...]

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!

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
>

Hi Janne,

On Thu, May 9, 2024 at 12:06 PM Janne Grunau <j@jannau.net> wrote:
>
> 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.

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

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 <j@jannau.net> wrote:
> >
> > 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.
> 
> 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 <j@jannau.net> 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 <j@jannau.net> wrote:
> > >
> > > 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.
> >
> > 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

Hi Janne,

On Thu, May 9, 2024 at 6:23 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Janne,
>
> On Thu, May 9, 2024 at 4:09 PM Janne Grunau <j@jannau.net> 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 <j@jannau.net> wrote:
> > > >
> > > > 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.
> > >
> > > 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

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.

Hi Sven, Hector,

On Fri, May 10, 2024 at 5:55 AM Sven Peter <sven@svenpeter.dev> 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.

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 <sven@svenpeter.dev> 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/20220801103633.27772-4-sven@svenpeter.dev/
[2] https://lore.kernel.org/linux-bluetooth/220ab728-ed5b-415d-ab15-47a7153e8e5c@www.fastmail.com/


Best,


Sven