| Message ID | 20220216221822.2694867-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Bluetooth: hci_sync: Fix not using conn_timeout | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/checkpatch | success | Checkpatch PASS |
| tedd_an/gitlint | success | Gitlint PASS |
| tedd_an/subjectprefix | success | PASS |
| tedd_an/buildkernel | success | Build Kernel PASS |
| tedd_an/buildkernel32 | success | Build Kernel32 PASS |
| tedd_an/incremental_build | success | Pass |
| tedd_an/testrunnersetup | success | Test Runner Setup PASS |
| tedd_an/testrunnerl2cap-tester | success | Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerbnep-tester | success | Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnermgmt-tester | success | Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnerrfcomm-tester | success | Total: 10, Passed: 10 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersco-tester | success | Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunnersmp-tester | success | Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 |
| tedd_an/testrunneruserchan-tester | success | Total: 4, Passed: 4 (100.0%), Failed: 0, Not Run: 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=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
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
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 >
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
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. */