[PATCH] Bluetooth: hci_sync: Fix not using conn_timeout

[PATCH] Bluetooth: hci_sync: Fix not using conn_timeout

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

When using hci_le_create_conn_sync it shall wait for the conn_timeout
since the connection complete may take longer than just 2 seconds.

Also fix the masking of HCI_EV_LE_ENHANCED_CONN_COMPLETE and
HCI_EV_LE_CONN_COMPLETE so they are never both set so we can predict
which one the controller will use in case of HCI_OP_LE_CREATE_CONN.

Fixes: 6cd29ec6ae5e3 ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 net/bluetooth/hci_sync.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 9dbf007e3dc7..002f9c5b5371 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3265,11 +3265,17 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
 	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT)
 		events[0] |= 0x40;	/* LE Data Length Change */
 
-	/* If the controller supports LL Privacy feature, enable
-	 * the corresponding event.
+	/* If the controller supports LL Privacy feature or LE Extended
+	 * Create Connection, enable the corresponding event.
 	 */
-	if (hdev->le_features[0] & HCI_LE_LL_PRIVACY)
+	if (ll_privacy_capable(hdev) || hdev->commands[37] & 0x80) {
 		events[1] |= 0x02;	/* LE Enhanced Connection Complete */
+	} else if (hdev->commands[26] & 0x10) {
+		/* If the controller supports the LE Create Connection
+		 * command, enable the corresponding event.
+		 */
+		events[0] |= 0x01;	/* LE Connection Complete */
+	}
 
 	/* If the controller supports Extended Scanner Filter
 	 * Policies, enable the corresponding event.
@@ -3289,12 +3295,6 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
 	if (hdev->commands[26] & 0x08)
 		events[0] |= 0x02;	/* LE Advertising Report */
 
-	/* If the controller supports the LE Create Connection
-	 * command, enable the corresponding event.
-	 */
-	if (hdev->commands[26] & 0x10)
-		events[0] |= 0x01;	/* LE Connection Complete */
-
 	/* If the controller supports the LE Connection Update
 	 * command, enable the corresponding event.
 	 */
@@ -5188,7 +5188,7 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 	return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_EXT_CREATE_CONN,
 					plen, data,
 					HCI_EV_LE_ENHANCED_CONN_COMPLETE,
-					HCI_CMD_TIMEOUT, NULL);
+					conn->conn_timeout, NULL);
 }
 
 int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
@@ -5274,8 +5274,11 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
 	cp.max_ce_len = cpu_to_le16(0x0000);
 
 	err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CREATE_CONN,
-				       sizeof(cp), &cp, HCI_EV_LE_CONN_COMPLETE,
-				       HCI_CMD_TIMEOUT, NULL);
+				       sizeof(cp), &cp,
+				       ll_privacy_capable(hdev) ?
+				       HCI_EV_LE_ENHANCED_CONN_COMPLETE :
+				       HCI_EV_LE_CONN_COMPLETE,
+				       conn->conn_timeout, NULL);
 
 done:
 	/* Re-enable advertising after the connection attempt is finished. */
-- 
2.35.1

RE: Bluetooth: hci_sync: Fix not using conn_timeout

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

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

---Test result---

Test Summary:
CheckPatch                    PASS      1.64 seconds
GitLint                       PASS      1.00 seconds
SubjectPrefix                 PASS      0.92 seconds
BuildKernel                   PASS      30.38 seconds
BuildKernel32                 PASS      27.06 seconds
Incremental Build with patchesPASS      36.25 seconds
TestRunner: Setup             PASS      468.40 seconds
TestRunner: l2cap-tester      PASS      15.64 seconds
TestRunner: bnep-tester       PASS      6.13 seconds
TestRunner: mgmt-tester       PASS      102.87 seconds
TestRunner: rfcomm-tester     PASS      7.85 seconds
TestRunner: sco-tester        PASS      7.69 seconds
TestRunner: smp-tester        PASS      7.54 seconds
TestRunner: userchan-tester   PASS      6.35 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH] Bluetooth: hci_sync: Fix not using conn_timeout

[Marcel Holtmann]

Hi Luiz,

> When using hci_le_create_conn_sync it shall wait for the conn_timeout
> since the connection complete may take longer than just 2 seconds.
> 
> Also fix the masking of HCI_EV_LE_ENHANCED_CONN_COMPLETE and
> HCI_EV_LE_CONN_COMPLETE so they are never both set so we can predict
> which one the controller will use in case of HCI_OP_LE_CREATE_CONN.
> 
> Fixes: 6cd29ec6ae5e3 ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> net/bluetooth/hci_sync.c | 27 +++++++++++++++------------
> 1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 9dbf007e3dc7..002f9c5b5371 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3265,11 +3265,17 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
> 	if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT)
> 		events[0] |= 0x40;	/* LE Data Length Change */
> 
> -	/* If the controller supports LL Privacy feature, enable
> -	 * the corresponding event.
> +	/* If the controller supports LL Privacy feature or LE Extended
> +	 * Create Connection, enable the corresponding event.
> 	 */
> -	if (hdev->le_features[0] & HCI_LE_LL_PRIVACY)
> +	if (ll_privacy_capable(hdev) || hdev->commands[37] & 0x80) {
> 		events[1] |= 0x02;	/* LE Enhanced Connection Complete */
> +	} else if (hdev->commands[26] & 0x10) {
> +		/* If the controller supports the LE Create Connection
> +		 * command, enable the corresponding event.
> +		 */
> +		events[0] |= 0x01;	/* LE Connection Complete */
> +	}
> 
> 	/* If the controller supports Extended Scanner Filter
> 	 * Policies, enable the corresponding event.
> @@ -3289,12 +3295,6 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
> 	if (hdev->commands[26] & 0x08)
> 		events[0] |= 0x02;	/* LE Advertising Report */
> 
> -	/* If the controller supports the LE Create Connection
> -	 * command, enable the corresponding event.
> -	 */
> -	if (hdev->commands[26] & 0x10)
> -		events[0] |= 0x01;	/* LE Connection Complete */
> -

I do not understand why you are trying to intermix this with LL Privacy. If the controller supports the LE Extended Create Connection, then we should enable that event. No matter if we have LL Privacy supported or enabled.

If we have other code that intermixes this, then it needs to be untangled.

What we should be doing is to only support LL Privacy if we also have support for LE Extended Create Connection command, but the assumption the other way around makes no sense.

> 	/* If the controller supports the LE Connection Update
> 	 * command, enable the corresponding event.
> 	 */
> @@ -5188,7 +5188,7 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
> 	return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_EXT_CREATE_CONN,
> 					plen, data,
> 					HCI_EV_LE_ENHANCED_CONN_COMPLETE,
> -					HCI_CMD_TIMEOUT, NULL);
> +					conn->conn_timeout, NULL);
> }
> 
> int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> @@ -5274,8 +5274,11 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> 	cp.max_ce_len = cpu_to_le16(0x0000);
> 
> 	err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CREATE_CONN,
> -				       sizeof(cp), &cp, HCI_EV_LE_CONN_COMPLETE,
> -				       HCI_CMD_TIMEOUT, NULL);
> +				       sizeof(cp), &cp,
> +				       ll_privacy_capable(hdev) ?
> +				       HCI_EV_LE_ENHANCED_CONN_COMPLETE :
> +				       HCI_EV_LE_CONN_COMPLETE,
> +				       conn->conn_timeout, NULL);

This is stupid. We should not be using LE Create Connection in the first place here. If the LE Extended Create Connection is available, we unmask its event and also use the command.

Regards

Marcel

Re: [PATCH] Bluetooth: hci_sync: Fix not using conn_timeout

[Luiz Augusto von Dentz]

Hi Marcel,

On Thu, Feb 17, 2022 at 1:50 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > When using hci_le_create_conn_sync it shall wait for the conn_timeout
> > since the connection complete may take longer than just 2 seconds.
> >
> > Also fix the masking of HCI_EV_LE_ENHANCED_CONN_COMPLETE and
> > HCI_EV_LE_CONN_COMPLETE so they are never both set so we can predict
> > which one the controller will use in case of HCI_OP_LE_CREATE_CONN.
> >
> > Fixes: 6cd29ec6ae5e3 ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > net/bluetooth/hci_sync.c | 27 +++++++++++++++------------
> > 1 file changed, 15 insertions(+), 12 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> > index 9dbf007e3dc7..002f9c5b5371 100644
> > --- a/net/bluetooth/hci_sync.c
> > +++ b/net/bluetooth/hci_sync.c
> > @@ -3265,11 +3265,17 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
> >       if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT)
> >               events[0] |= 0x40;      /* LE Data Length Change */
> >
> > -     /* If the controller supports LL Privacy feature, enable
> > -      * the corresponding event.
> > +     /* If the controller supports LL Privacy feature or LE Extended
> > +      * Create Connection, enable the corresponding event.
> >        */
> > -     if (hdev->le_features[0] & HCI_LE_LL_PRIVACY)
> > +     if (ll_privacy_capable(hdev) || hdev->commands[37] & 0x80) {
> >               events[1] |= 0x02;      /* LE Enhanced Connection Complete */
> > +     } else if (hdev->commands[26] & 0x10) {
> > +             /* If the controller supports the LE Create Connection
> > +              * command, enable the corresponding event.
> > +              */
> > +             events[0] |= 0x01;      /* LE Connection Complete */
> > +     }
> >
> >       /* If the controller supports Extended Scanner Filter
> >        * Policies, enable the corresponding event.
> > @@ -3289,12 +3295,6 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
> >       if (hdev->commands[26] & 0x08)
> >               events[0] |= 0x02;      /* LE Advertising Report */
> >
> > -     /* If the controller supports the LE Create Connection
> > -      * command, enable the corresponding event.
> > -      */
> > -     if (hdev->commands[26] & 0x10)
> > -             events[0] |= 0x01;      /* LE Connection Complete */
> > -
>
> I do not understand why you are trying to intermix this with LL Privacy. If the controller supports the LE Extended Create Connection, then we should enable that event. No matter if we have LL Privacy supported or enabled.
>
> If we have other code that intermixes this, then it needs to be untangled.
>
> What we should be doing is to only support LL Privacy if we also have support for LE Extended Create Connection command, but the assumption the other way around makes no sense.

The spec does allow the use of LE Create Connection and Enhanced
Connection Complete since it does support own_address_type to be
0x02/0x03 which means LL Privacy, I believe LE Extented Create
Connection was introduced much later than LL Privacy so we may find
controllers supporting LL Privacy with LE Create Connection but
without support for LE Extended Create Connection.

> >       /* If the controller supports the LE Connection Update
> >        * command, enable the corresponding event.
> >        */
> > @@ -5188,7 +5188,7 @@ static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
> >       return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_EXT_CREATE_CONN,
> >                                       plen, data,
> >                                       HCI_EV_LE_ENHANCED_CONN_COMPLETE,
> > -                                     HCI_CMD_TIMEOUT, NULL);
> > +                                     conn->conn_timeout, NULL);
> > }
> >
> > int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> > @@ -5274,8 +5274,11 @@ int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
> >       cp.max_ce_len = cpu_to_le16(0x0000);
> >
> >       err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CREATE_CONN,
> > -                                    sizeof(cp), &cp, HCI_EV_LE_CONN_COMPLETE,
> > -                                    HCI_CMD_TIMEOUT, NULL);
> > +                                    sizeof(cp), &cp,
> > +                                    ll_privacy_capable(hdev) ?
> > +                                    HCI_EV_LE_ENHANCED_CONN_COMPLETE :
> > +                                    HCI_EV_LE_CONN_COMPLETE,
> > +                                    conn->conn_timeout, NULL);
>
> This is stupid. We should not be using LE Create Connection in the first place here. If the LE Extended Create Connection is available, we unmask its event and also use the command.

This comes from the spec actually:

BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
page 2374
When the Controller receives the HCI_LE_Create_Connection command, the
Controller sends the HCI_Command_Status event to the Host. An HCI_LE_-
Connection_Complete or HCI_LE_Enhanced_Connection_Complete event
shall be generated...

The reason why HCI_LE_Enhanced_Connection_Complete is required is
because own_address_type can be set to 0x02/0x03 and in that case we
need the Local_Resolvable_Private_Address used by the controller. Now
you can say that we could restrict LL Privacy support to be used only
with LE Extended Create Connection but that would be our own
restriction.

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: hci_sync: Fix not using conn_timeout

[Marcel Holtmann]

Hi Luiz,

>>> When using hci_le_create_conn_sync it shall wait for the conn_timeout
>>> since the connection complete may take longer than just 2 seconds.
>>> 
>>> Also fix the masking of HCI_EV_LE_ENHANCED_CONN_COMPLETE and
>>> HCI_EV_LE_CONN_COMPLETE so they are never both set so we can predict
>>> which one the controller will use in case of HCI_OP_LE_CREATE_CONN.
>>> 
>>> Fixes: 6cd29ec6ae5e3 ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> net/bluetooth/hci_sync.c | 27 +++++++++++++++------------
>>> 1 file changed, 15 insertions(+), 12 deletions(-)
>>> 
>>> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
>>> index 9dbf007e3dc7..002f9c5b5371 100644
>>> --- a/net/bluetooth/hci_sync.c
>>> +++ b/net/bluetooth/hci_sync.c
>>> @@ -3265,11 +3265,17 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
>>>      if (hdev->le_features[0] & HCI_LE_DATA_LEN_EXT)
>>>              events[0] |= 0x40;      /* LE Data Length Change */
>>> 
>>> -     /* If the controller supports LL Privacy feature, enable
>>> -      * the corresponding event.
>>> +     /* If the controller supports LL Privacy feature or LE Extended
>>> +      * Create Connection, enable the corresponding event.
>>>       */
>>> -     if (hdev->le_features[0] & HCI_LE_LL_PRIVACY)
>>> +     if (ll_privacy_capable(hdev) || hdev->commands[37] & 0x80) {
>>>              events[1] |= 0x02;      /* LE Enhanced Connection Complete */
>>> +     } else if (hdev->commands[26] & 0x10) {
>>> +             /* If the controller supports the LE Create Connection
>>> +              * command, enable the corresponding event.
>>> +              */
>>> +             events[0] |= 0x01;      /* LE Connection Complete */
>>> +     }
>>> 
>>>      /* If the controller supports Extended Scanner Filter
>>>       * Policies, enable the corresponding event.
>>> @@ -3289,12 +3295,6 @@ static int hci_le_set_event_mask_sync(struct hci_dev *hdev)
>>>      if (hdev->commands[26] & 0x08)
>>>              events[0] |= 0x02;      /* LE Advertising Report */
>>> 
>>> -     /* If the controller supports the LE Create Connection
>>> -      * command, enable the corresponding event.
>>> -      */
>>> -     if (hdev->commands[26] & 0x10)
>>> -             events[0] |= 0x01;      /* LE Connection Complete */
>>> -
>> 
>> I do not understand why you are trying to intermix this with LL Privacy. If the controller supports the LE Extended Create Connection, then we should enable that event. No matter if we have LL Privacy supported or enabled.
>> 
>> If we have other code that intermixes this, then it needs to be untangled.
>> 
>> What we should be doing is to only support LL Privacy if we also have support for LE Extended Create Connection command, but the assumption the other way around makes no sense.
> 
> The spec does allow the use of LE Create Connection and Enhanced
> Connection Complete since it does support own_address_type to be
> 0x02/0x03 which means LL Privacy, I believe LE Extented Create
> Connection was introduced much later than LL Privacy so we may find
> controllers supporting LL Privacy with LE Create Connection but
> without support for LE Extended Create Connection.

my memory is getting old and even while I actively worked on 4.2 and 5.x specs, I keep forgetting details.

So here is the thing from the latest specs for the LE Enhanced Connection Complete event:

C24: Mandatory if the Controller supports Connection State and either LE Feature (LL Privacy) or LE Feature (Extended Advertising) is supported, otherwise optional if the Controller supports Connection State, otherwise excluded.

That means that when either of these features are listed as supported, we unmask the event. Keep the LE Connection Complete also unmasked since the spec is clear that if both are unmasked the “enhanced” version shall be used.

Maybe introduce an use_enhanced_conn_complete() macro.

Regards

Marcel