Hi Marcel,
On Thu, Feb 17, 2022 at 1:50 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
