[PATCH 0/5] Bluetooth: Improve retrying of connection attempts

[PATCH 0/5] Bluetooth: Improve retrying of connection attempts

[Jonas Dreßler]

Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, the kernel supports
trying to connect again in case the bluetooth card is busy and fails
to connect.

The logic that should handle this became a bit spotty over time, and also
cards these days appear to fail with more errors than just "Command
Disallowed".

This series tries to improve the logic for retrying "HCI Create
Connection" and adds support for two more errors that can indicate the
hardware being busy.

Jonas Dreßler (5):
  Bluetooth: Remove superfluous call to hci_conn_check_pending()
  Bluetooth: hci_event: Use HCI error defines instead of magic values
  Bluetooth: hci_event: Remove limit of 2 reconnection attempts
  Bluetooth: hci_event: Do sanity checks before retrying to connect
  Bluetooth: hci_event: Try reconnecting on more kinds of errors

 include/net/bluetooth/hci.h |  3 ++
 net/bluetooth/hci_event.c   | 57 +++++++++++++++++++++++++++++++------
 2 files changed, 51 insertions(+), 9 deletions(-)

-- 
2.43.0

[PATCH 1/5] Bluetooth: Remove superfluous call to hci_conn_check_pending()

[Jonas Dreßler]

The "pending connections" feature was originally introduced with commit
4c67bc74f016b0d360b8573e18969c0ff7926974 and
6bd57416127e92d35e6798925502c84e14a3a966 to handle controllers supporting
only a single connection request at a time. Later things were extended to
also cancel ongoing inquiries on connect() with commit
89e65975fea5c25706e8cc3a89f9f97b20fc45ad.

With commit a9de9248064bfc8eb0a183a6a951a4e7b5ca10a4,
hci_conn_check_pending() was introduced as a helper to consolidate a few
places where we check for pending connections (indicated by the
BT_CONNECT2 flag) and then try to connect.

This refactoring commit also snuck in two more calls to
hci_conn_check_pending():

- One is in the failure callback of hci_cs_inquiry(), this one probably
makes sense: If we send an "HCI Inquiry" command and then immediately
after a "Create Connection" command, the "Create Connection" command might
fail before the "HCI Inquiry" command, and then we want to retry the
"Create Connection" on failure of the "HCI Inquiry".

- The other added call to hci_conn_check_pending() is in the event handler
for the "Remote Name" event, this seems unrelated and is possibly a
copy-paste error, so remove that one.

Fixes: a9de9248064bfc8eb0a183a6a951a4e7b5ca10a4
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_event.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 31ca320ce..13396329f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3538,8 +3538,6 @@ static void hci_remote_name_evt(struct hci_dev *hdev, void *data,
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
 
-	hci_conn_check_pending(hdev);
-
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
-- 
2.43.0

RE: Bluetooth: Improve retrying of connection attempts

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 6310 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=813906

---Test result---

Test Summary:
CheckPatch                    FAIL      2.87 seconds
GitLint                       PASS      0.99 seconds
SubjectPrefix                 PASS      0.33 seconds
BuildKernel                   PASS      28.06 seconds
CheckAllWarning               PASS      30.64 seconds
CheckSparse                   WARNING   36.43 seconds
CheckSmatch                   WARNING   99.91 seconds
BuildKernel32                 PASS      27.22 seconds
TestRunnerSetup               PASS      435.07 seconds
TestRunner_l2cap-tester       PASS      22.98 seconds
TestRunner_iso-tester         PASS      94.14 seconds
TestRunner_bnep-tester        PASS      6.96 seconds
TestRunner_mgmt-tester        PASS      161.32 seconds
TestRunner_rfcomm-tester      PASS      10.76 seconds
TestRunner_sco-tester         PASS      14.49 seconds
TestRunner_ioctl-tester       PASS      12.02 seconds
TestRunner_mesh-tester        PASS      8.73 seconds
TestRunner_smp-tester         PASS      9.75 seconds
TestRunner_userchan-tester    PASS      7.32 seconds
IncrementalBuild              PASS      83.71 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[1/5] Bluetooth: Remove superfluous call to hci_conn_check_pending()
WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: 198fc841b667 ("Merge 33ba02027f8092a3b79e82372598867aac9f54ee into ef3a59b4541dfc255a914dc47880a3a28097dc7b")'
#70: 
Fixes: a9de9248064bfc8eb0a183a6a951a4e7b5ca10a4

total: 0 errors, 1 warnings, 0 checks, 8 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13509297.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[2/5] Bluetooth: hci_event: Use HCI error defines instead of magic values
WARNING: Missing commit description - Add an appropriate one

total: 0 errors, 1 warnings, 0 checks, 43 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13509298.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[4/5] Bluetooth: hci_event: Do sanity checks before retrying to connect
WARNING: quoted string split across lines
#79: FILE: net/bluetooth/hci_event.c:2339:
+						   "\"Create Connection\" returned error "
+						   "(0x%2.2x) indicating to try again, but "

WARNING: quoted string split across lines
#80: FILE: net/bluetooth/hci_event.c:2340:
+						   "(0x%2.2x) indicating to try again, but "
+						   "there's no concurrent \"Create "

WARNING: quoted string split across lines
#81: FILE: net/bluetooth/hci_event.c:2341:
+						   "there's no concurrent \"Create "
+						   "Connection\" nor an ongoing inquiry",

total: 0 errors, 3 warnings, 0 checks, 28 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13509300.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[5/5] Bluetooth: hci_event: Try reconnecting on more kinds of errors
WARNING: quoted string split across lines
#109: FILE: net/bluetooth/hci_event.c:3265:
+					   "\"Connect Complete\" event with error "
+					   "(0x%2.2x) indicating to try again, but "

WARNING: quoted string split across lines
#110: FILE: net/bluetooth/hci_event.c:3266:
+					   "(0x%2.2x) indicating to try again, but "
+					   "there's no concurrent \"Create "

WARNING: quoted string split across lines
#111: FILE: net/bluetooth/hci_event.c:3267:
+					   "there's no concurrent \"Create "
+					   "Connection\" nor an ongoing inquiry",

WARNING: else is not generally useful after a break or return
#119: FILE: net/bluetooth/hci_event.c:3275:
+			return;
+		} else {

total: 0 errors, 4 warnings, 0 checks, 50 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13509301.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):


---
Regards,
Linux Bluetooth

Re: [PATCH 1/5] Bluetooth: Remove superfluous call to hci_conn_check_pending()

[Simon Horman]

On Tue, Jan 02, 2024 at 07:59:28PM +0100, Jonas Dreßler wrote:
> The "pending connections" feature was originally introduced with commit
> 4c67bc74f016b0d360b8573e18969c0ff7926974 and
> 6bd57416127e92d35e6798925502c84e14a3a966 to handle controllers supporting
> only a single connection request at a time. Later things were extended to
> also cancel ongoing inquiries on connect() with commit
> 89e65975fea5c25706e8cc3a89f9f97b20fc45ad.
> 
> With commit a9de9248064bfc8eb0a183a6a951a4e7b5ca10a4,
> hci_conn_check_pending() was introduced as a helper to consolidate a few
> places where we check for pending connections (indicated by the
> BT_CONNECT2 flag) and then try to connect.
> 
> This refactoring commit also snuck in two more calls to
> hci_conn_check_pending():
> 
> - One is in the failure callback of hci_cs_inquiry(), this one probably
> makes sense: If we send an "HCI Inquiry" command and then immediately
> after a "Create Connection" command, the "Create Connection" command might
> fail before the "HCI Inquiry" command, and then we want to retry the
> "Create Connection" on failure of the "HCI Inquiry".
> 
> - The other added call to hci_conn_check_pending() is in the event handler
> for the "Remote Name" event, this seems unrelated and is possibly a
> copy-paste error, so remove that one.
> 
> Fixes: a9de9248064bfc8eb0a183a6a951a4e7b5ca10a4
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

Nit: a correct format for the fixes tag is

Fixes: a9de9248064b ("[Bluetooth] Switch from OGF+OCF to using only opcodes")

Likewise, in the patch description, it is usual to cite patches
using a similar format.

e.g. introduced in commit
     4c67bc74f016 ("[Bluetooth] Support concurrent connect requests")

Note it is usual to use a hash of at least 12 characters
(and not much more than that).

...

[PATCH 2/5] Bluetooth: hci_event: Use HCI error defines instead of magic values

[Jonas Dreßler]

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 include/net/bluetooth/hci.h | 2 ++
 net/bluetooth/hci_event.c   | 8 ++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 111e8f8e5..fef723afd 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -641,6 +641,7 @@ enum {
 #define HCI_ERROR_PIN_OR_KEY_MISSING	0x06
 #define HCI_ERROR_MEMORY_EXCEEDED	0x07
 #define HCI_ERROR_CONNECTION_TIMEOUT	0x08
+#define HCI_ERROR_COMMAND_DISALLOWED	0x0c
 #define HCI_ERROR_REJ_LIMITED_RESOURCES	0x0d
 #define HCI_ERROR_REJ_BAD_ADDR		0x0f
 #define HCI_ERROR_INVALID_PARAMETERS	0x12
@@ -649,6 +650,7 @@ enum {
 #define HCI_ERROR_REMOTE_POWER_OFF	0x15
 #define HCI_ERROR_LOCAL_HOST_TERM	0x16
 #define HCI_ERROR_PAIRING_NOT_ALLOWED	0x18
+#define HCI_ERROR_UNSUPPORTED_REMOTE_FEATURE	0x1e
 #define HCI_ERROR_INVALID_LL_PARAMS	0x1e
 #define HCI_ERROR_UNSPECIFIED		0x1f
 #define HCI_ERROR_ADVERTISING_TIMEOUT	0x3c
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 13396329f..e8b4a0126 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -92,11 +92,11 @@ static u8 hci_cc_inquiry_cancel(struct hci_dev *hdev, void *data,
 	/* It is possible that we receive Inquiry Complete event right
 	 * before we receive Inquiry Cancel Command Complete event, in
 	 * which case the latter event should have status of Command
-	 * Disallowed (0x0c). This should not be treated as error, since
+	 * Disallowed. This should not be treated as error, since
 	 * we actually achieve what Inquiry Cancel wants to achieve,
 	 * which is to end the last Inquiry session.
 	 */
-	if (rp->status == 0x0c && !test_bit(HCI_INQUIRY, &hdev->flags)) {
+	if (rp->status == HCI_ERROR_COMMAND_DISALLOWED && !test_bit(HCI_INQUIRY, &hdev->flags)) {
 		bt_dev_warn(hdev, "Ignoring error of Inquiry Cancel command");
 		rp->status = 0x00;
 	}
@@ -2323,7 +2323,7 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
 
 	if (status) {
 		if (conn && conn->state == BT_CONNECT) {
-			if (status != 0x0c || conn->attempt > 2) {
+			if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
 				conn->state = BT_CLOSED;
 				hci_connect_cfm(conn, status);
 				hci_conn_del(conn);
@@ -6578,7 +6578,7 @@ static void hci_le_remote_feat_complete_evt(struct hci_dev *hdev, void *data,
 			 * transition into connected state and mark it as
 			 * successful.
 			 */
-			if (!conn->out && ev->status == 0x1a &&
+			if (!conn->out && ev->status == HCI_ERROR_UNSUPPORTED_REMOTE_FEATURE &&
 			    (hdev->le_features[0] & HCI_LE_PERIPHERAL_FEATURES))
 				status = 0x00;
 			else
-- 
2.43.0

[PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Jonas Dreßler]

Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
later when we get a "Command Disallowed" error returned by "Create
Connection".

In this commit the intention was to retry only once, and give up if we see
"Command Disallowed" again on the second try.

This made sense back then when the retry was initiated *only* from the
"Connect Complete" event. If we received that event, we knew that now the
card now must have a "free slot" for a new connection request again. These
days we call hci_conn_check_pending() from a few more places though, and
in these places we can't really be sure that there's a "free slot" on the
card, so the second try to "Create Connection" might fail again.

Deal with this by being less strict about these retries and try again
every time we get "Command Disallowed" errors, removing the limitation to
only two attempts.

Since this can potentially cause us to enter an endless cycle of
reconnection attempts, we'll add some guarding against that with the next
commit.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_event.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e8b4a0126..e1f5b6f90 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
 
 	if (status) {
 		if (conn && conn->state == BT_CONNECT) {
-			if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
+			if (status == HCI_ERROR_COMMAND_DISALLOWED) {
+				conn->state = BT_CONNECT2;
+			} else {
 				conn->state = BT_CLOSED;
 				hci_connect_cfm(conn, status);
 				hci_conn_del(conn);
-			} else
-				conn->state = BT_CONNECT2;
+			}
 		}
 	} else {
 		if (!conn) {
-- 
2.43.0

Re: [PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Luiz Augusto von Dentz]

Hi Jonas,

On Tue, Jan 2, 2024 at 1:59 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
> later when we get a "Command Disallowed" error returned by "Create
> Connection".
>
> In this commit the intention was to retry only once, and give up if we see
> "Command Disallowed" again on the second try.
>
> This made sense back then when the retry was initiated *only* from the
> "Connect Complete" event. If we received that event, we knew that now the
> card now must have a "free slot" for a new connection request again. These
> days we call hci_conn_check_pending() from a few more places though, and
> in these places we can't really be sure that there's a "free slot" on the
> card, so the second try to "Create Connection" might fail again.
>
> Deal with this by being less strict about these retries and try again
> every time we get "Command Disallowed" errors, removing the limitation to
> only two attempts.
>
> Since this can potentially cause us to enter an endless cycle of
> reconnection attempts, we'll add some guarding against that with the next
> commit.

Don't see where you are doing such guarding, besides you seem to
assume HCI_ERROR_COMMAND_DISALLOWED would always means the controller
is busy, or something like that, but it could perform the connection
later, but that may not always be the case, thus why I think
reconnecting just a few number of times is better, if you really need
to keep retrying then this needs to be controlled by a policy in
userspace not hardcoded in the kernel, well I can even argument that
perhaps the initial number of reconnection shall be configurable so
one don't have to recompile the kernel if that needs changing.

> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  net/bluetooth/hci_event.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index e8b4a0126..e1f5b6f90 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
>
>         if (status) {
>                 if (conn && conn->state == BT_CONNECT) {
> -                       if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
> +                       if (status == HCI_ERROR_COMMAND_DISALLOWED) {
> +                               conn->state = BT_CONNECT2;
> +                       } else {
>                                 conn->state = BT_CLOSED;
>                                 hci_connect_cfm(conn, status);
>                                 hci_conn_del(conn);
> -                       } else
> -                               conn->state = BT_CONNECT2;
> +                       }
>                 }
>         } else {
>                 if (!conn) {
> --
> 2.43.0
>


-- 
Luiz Augusto von Dentz

Re: [PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Jonas Dreßler]

Hi Luiz,

On 1/3/24 17:05, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Tue, Jan 2, 2024 at 1:59 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
>> later when we get a "Command Disallowed" error returned by "Create
>> Connection".
>>
>> In this commit the intention was to retry only once, and give up if we see
>> "Command Disallowed" again on the second try.
>>
>> This made sense back then when the retry was initiated *only* from the
>> "Connect Complete" event. If we received that event, we knew that now the
>> card now must have a "free slot" for a new connection request again. These
>> days we call hci_conn_check_pending() from a few more places though, and
>> in these places we can't really be sure that there's a "free slot" on the
>> card, so the second try to "Create Connection" might fail again.
>>
>> Deal with this by being less strict about these retries and try again
>> every time we get "Command Disallowed" errors, removing the limitation to
>> only two attempts.
>>
>> Since this can potentially cause us to enter an endless cycle of
>> reconnection attempts, we'll add some guarding against that with the next
>> commit.
> 
> Don't see where you are doing such guarding, besides you seem to
> assume HCI_ERROR_COMMAND_DISALLOWED would always means the controller
> is busy, or something like that, but it could perform the connection
> later, but that may not always be the case, thus why I think
> reconnecting just a few number of times is better, if you really need
> to keep retrying then this needs to be controlled by a policy in
> userspace not hardcoded in the kernel, well I can even argument that
> perhaps the initial number of reconnection shall be configurable so
> one don't have to recompile the kernel if that needs changing.

Yes, fair enough, the next commit assumes that COMMAND_DISALLOWED always 
means busy. The guarding is that we stop retrying as soon as there's no 
(competing) ongoing connection attempt nor an active inquiry, which 
should eventually be the case no matter what, no?

I agree it's probably still better to not rely on this fairly complex 
sanity check and keep the checking of attempts nonetheless.

I think we could keep doing that if we check for 
!hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) && 
!test_bit(HCI_INQUIRY, &hdev->flags) in hci_conn_check_pending() before 
we actually retry, to make sure the retry counter doesn't get 
incremented wrongly. I'll give that a try.

> 
>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
>> ---
>>   net/bluetooth/hci_event.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index e8b4a0126..e1f5b6f90 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
>>
>>          if (status) {
>>                  if (conn && conn->state == BT_CONNECT) {
>> -                       if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
>> +                       if (status == HCI_ERROR_COMMAND_DISALLOWED) {
>> +                               conn->state = BT_CONNECT2;
>> +                       } else {
>>                                  conn->state = BT_CLOSED;
>>                                  hci_connect_cfm(conn, status);
>>                                  hci_conn_del(conn);
>> -                       } else
>> -                               conn->state = BT_CONNECT2;
>> +                       }
>>                  }
>>          } else {
>>                  if (!conn) {
>> --
>> 2.43.0
>>
> 
> 

Cheers,
Jonas

Re: [PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Luiz Augusto von Dentz]

Hi Jonas,

On Fri, Jan 5, 2024 at 10:54 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Hi Luiz,
>
> On 1/3/24 17:05, Luiz Augusto von Dentz wrote:
> > Hi Jonas,
> >
> > On Tue, Jan 2, 2024 at 1:59 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
> >>
> >> Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
> >> later when we get a "Command Disallowed" error returned by "Create
> >> Connection".
> >>
> >> In this commit the intention was to retry only once, and give up if we see
> >> "Command Disallowed" again on the second try.
> >>
> >> This made sense back then when the retry was initiated *only* from the
> >> "Connect Complete" event. If we received that event, we knew that now the
> >> card now must have a "free slot" for a new connection request again. These
> >> days we call hci_conn_check_pending() from a few more places though, and
> >> in these places we can't really be sure that there's a "free slot" on the
> >> card, so the second try to "Create Connection" might fail again.
> >>
> >> Deal with this by being less strict about these retries and try again
> >> every time we get "Command Disallowed" errors, removing the limitation to
> >> only two attempts.
> >>
> >> Since this can potentially cause us to enter an endless cycle of
> >> reconnection attempts, we'll add some guarding against that with the next
> >> commit.
> >
> > Don't see where you are doing such guarding, besides you seem to
> > assume HCI_ERROR_COMMAND_DISALLOWED would always means the controller
> > is busy, or something like that, but it could perform the connection
> > later, but that may not always be the case, thus why I think
> > reconnecting just a few number of times is better, if you really need
> > to keep retrying then this needs to be controlled by a policy in
> > userspace not hardcoded in the kernel, well I can even argument that
> > perhaps the initial number of reconnection shall be configurable so
> > one don't have to recompile the kernel if that needs changing.
>
> Yes, fair enough, the next commit assumes that COMMAND_DISALLOWED always
> means busy. The guarding is that we stop retrying as soon as there's no
> (competing) ongoing connection attempt nor an active inquiry, which
> should eventually be the case no matter what, no?
>
> I agree it's probably still better to not rely on this fairly complex
> sanity check and keep the checking of attempts nonetheless.
>
> I think we could keep doing that if we check for
> !hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
> !test_bit(HCI_INQUIRY, &hdev->flags) in hci_conn_check_pending() before
> we actually retry, to make sure the retry counter doesn't get
> incremented wrongly. I'll give that a try.

Perhaps I'm missing something, but it should be possible to block
concurrent access to HCI while a command is pending with use of
hci_cmd_sync, at least on LE we do that by waiting the connection
complete event so connection attempts are serialized but we don't seem
to be doing the same for BR/EDR.


> >
> >> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> >> ---
> >>   net/bluetooth/hci_event.c | 7 ++++---
> >>   1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index e8b4a0126..e1f5b6f90 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
> >>
> >>          if (status) {
> >>                  if (conn && conn->state == BT_CONNECT) {
> >> -                       if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
> >> +                       if (status == HCI_ERROR_COMMAND_DISALLOWED) {
> >> +                               conn->state = BT_CONNECT2;
> >> +                       } else {
> >>                                  conn->state = BT_CLOSED;
> >>                                  hci_connect_cfm(conn, status);
> >>                                  hci_conn_del(conn);
> >> -                       } else
> >> -                               conn->state = BT_CONNECT2;
> >> +                       }
> >>                  }
> >>          } else {
> >>                  if (!conn) {
> >> --
> >> 2.43.0
> >>
> >
> >
>
> Cheers,
> Jonas



--
Luiz Augusto von Dentz

Re: [PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Jonas Dreßler]

Hi Luiz,

On 1/5/24 17:05, Luiz Augusto von Dentz wrote:
> Hi Jonas,
> 
> On Fri, Jan 5, 2024 at 10:54 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>
>> Hi Luiz,
>>
>> On 1/3/24 17:05, Luiz Augusto von Dentz wrote:
>>> Hi Jonas,
>>>
>>> On Tue, Jan 2, 2024 at 1:59 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>>>>
>>>> Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
>>>> later when we get a "Command Disallowed" error returned by "Create
>>>> Connection".
>>>>
>>>> In this commit the intention was to retry only once, and give up if we see
>>>> "Command Disallowed" again on the second try.
>>>>
>>>> This made sense back then when the retry was initiated *only* from the
>>>> "Connect Complete" event. If we received that event, we knew that now the
>>>> card now must have a "free slot" for a new connection request again. These
>>>> days we call hci_conn_check_pending() from a few more places though, and
>>>> in these places we can't really be sure that there's a "free slot" on the
>>>> card, so the second try to "Create Connection" might fail again.
>>>>
>>>> Deal with this by being less strict about these retries and try again
>>>> every time we get "Command Disallowed" errors, removing the limitation to
>>>> only two attempts.
>>>>
>>>> Since this can potentially cause us to enter an endless cycle of
>>>> reconnection attempts, we'll add some guarding against that with the next
>>>> commit.
>>>
>>> Don't see where you are doing such guarding, besides you seem to
>>> assume HCI_ERROR_COMMAND_DISALLOWED would always means the controller
>>> is busy, or something like that, but it could perform the connection
>>> later, but that may not always be the case, thus why I think
>>> reconnecting just a few number of times is better, if you really need
>>> to keep retrying then this needs to be controlled by a policy in
>>> userspace not hardcoded in the kernel, well I can even argument that
>>> perhaps the initial number of reconnection shall be configurable so
>>> one don't have to recompile the kernel if that needs changing.
>>
>> Yes, fair enough, the next commit assumes that COMMAND_DISALLOWED always
>> means busy. The guarding is that we stop retrying as soon as there's no
>> (competing) ongoing connection attempt nor an active inquiry, which
>> should eventually be the case no matter what, no?
>>
>> I agree it's probably still better to not rely on this fairly complex
>> sanity check and keep the checking of attempts nonetheless.
>>
>> I think we could keep doing that if we check for
>> !hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
>> !test_bit(HCI_INQUIRY, &hdev->flags) in hci_conn_check_pending() before
>> we actually retry, to make sure the retry counter doesn't get
>> incremented wrongly. I'll give that a try.
> 
> Perhaps I'm missing something, but it should be possible to block
> concurrent access to HCI while a command is pending with use of
> hci_cmd_sync, at least on LE we do that by waiting the connection
> complete event so connection attempts are serialized but we don't seem
> to be doing the same for BR/EDR.
> 

That's a good point, it might even allow for a nice little cleanup 
because we can then cancel inquiries in hci_acl_create_connection() 
synchronously, too.

Question is just whether there's any devices out there that actually do 
support paging with more than a single device at a time and if we want 
to support that?

> 
>>>
>>>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
>>>> ---
>>>>    net/bluetooth/hci_event.c | 7 ++++---
>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index e8b4a0126..e1f5b6f90 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
>>>>
>>>>           if (status) {
>>>>                   if (conn && conn->state == BT_CONNECT) {
>>>> -                       if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
>>>> +                       if (status == HCI_ERROR_COMMAND_DISALLOWED) {
>>>> +                               conn->state = BT_CONNECT2;
>>>> +                       } else {
>>>>                                   conn->state = BT_CLOSED;
>>>>                                   hci_connect_cfm(conn, status);
>>>>                                   hci_conn_del(conn);
>>>> -                       } else
>>>> -                               conn->state = BT_CONNECT2;
>>>> +                       }
>>>>                   }
>>>>           } else {
>>>>                   if (!conn) {
>>>> --
>>>> 2.43.0
>>>>
>>>
>>>
>>
>> Cheers,
>> Jonas
> 
> 
> 
> --
> Luiz Augusto von Dentz

Re: [PATCH 3/5] Bluetooth: hci_event: Remove limit of 2 reconnection attempts

[Luiz Augusto von Dentz]

Hi Jonas,

On Sun, Jan 7, 2024 at 5:20 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> Hi Luiz,
>
> On 1/5/24 17:05, Luiz Augusto von Dentz wrote:
> > Hi Jonas,
> >
> > On Fri, Jan 5, 2024 at 10:54 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
> >>
> >> Hi Luiz,
> >>
> >> On 1/3/24 17:05, Luiz Augusto von Dentz wrote:
> >>> Hi Jonas,
> >>>
> >>> On Tue, Jan 2, 2024 at 1:59 PM Jonas Dreßler <verdre@v0yd.nl> wrote:
> >>>>
> >>>> Since commit 4c67bc74f016b0d360b8573e18969c0ff7926974, we retry connecting
> >>>> later when we get a "Command Disallowed" error returned by "Create
> >>>> Connection".
> >>>>
> >>>> In this commit the intention was to retry only once, and give up if we see
> >>>> "Command Disallowed" again on the second try.
> >>>>
> >>>> This made sense back then when the retry was initiated *only* from the
> >>>> "Connect Complete" event. If we received that event, we knew that now the
> >>>> card now must have a "free slot" for a new connection request again. These
> >>>> days we call hci_conn_check_pending() from a few more places though, and
> >>>> in these places we can't really be sure that there's a "free slot" on the
> >>>> card, so the second try to "Create Connection" might fail again.
> >>>>
> >>>> Deal with this by being less strict about these retries and try again
> >>>> every time we get "Command Disallowed" errors, removing the limitation to
> >>>> only two attempts.
> >>>>
> >>>> Since this can potentially cause us to enter an endless cycle of
> >>>> reconnection attempts, we'll add some guarding against that with the next
> >>>> commit.
> >>>
> >>> Don't see where you are doing such guarding, besides you seem to
> >>> assume HCI_ERROR_COMMAND_DISALLOWED would always means the controller
> >>> is busy, or something like that, but it could perform the connection
> >>> later, but that may not always be the case, thus why I think
> >>> reconnecting just a few number of times is better, if you really need
> >>> to keep retrying then this needs to be controlled by a policy in
> >>> userspace not hardcoded in the kernel, well I can even argument that
> >>> perhaps the initial number of reconnection shall be configurable so
> >>> one don't have to recompile the kernel if that needs changing.
> >>
> >> Yes, fair enough, the next commit assumes that COMMAND_DISALLOWED always
> >> means busy. The guarding is that we stop retrying as soon as there's no
> >> (competing) ongoing connection attempt nor an active inquiry, which
> >> should eventually be the case no matter what, no?
> >>
> >> I agree it's probably still better to not rely on this fairly complex
> >> sanity check and keep the checking of attempts nonetheless.
> >>
> >> I think we could keep doing that if we check for
> >> !hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
> >> !test_bit(HCI_INQUIRY, &hdev->flags) in hci_conn_check_pending() before
> >> we actually retry, to make sure the retry counter doesn't get
> >> incremented wrongly. I'll give that a try.
> >
> > Perhaps I'm missing something, but it should be possible to block
> > concurrent access to HCI while a command is pending with use of
> > hci_cmd_sync, at least on LE we do that by waiting the connection
> > complete event so connection attempts are serialized but we don't seem
> > to be doing the same for BR/EDR.
> >
>
> That's a good point, it might even allow for a nice little cleanup
> because we can then cancel inquiries in hci_acl_create_connection()
> synchronously, too.
>
> Question is just whether there's any devices out there that actually do
> support paging with more than a single device at a time and if we want
> to support that?

I think the controller would serialize the paging anyway, so the fact
that we would serialize it on the host side might actually means that
we have a common behavior across controllers rather than having to
handle errors when the controller is not capable of serializing
connections by themselves.

> >
> >>>
> >>>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> >>>> ---
> >>>>    net/bluetooth/hci_event.c | 7 ++++---
> >>>>    1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>>> index e8b4a0126..e1f5b6f90 100644
> >>>> --- a/net/bluetooth/hci_event.c
> >>>> +++ b/net/bluetooth/hci_event.c
> >>>> @@ -2323,12 +2323,13 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
> >>>>
> >>>>           if (status) {
> >>>>                   if (conn && conn->state == BT_CONNECT) {
> >>>> -                       if (status != HCI_ERROR_COMMAND_DISALLOWED || conn->attempt > 2) {
> >>>> +                       if (status == HCI_ERROR_COMMAND_DISALLOWED) {
> >>>> +                               conn->state = BT_CONNECT2;
> >>>> +                       } else {
> >>>>                                   conn->state = BT_CLOSED;
> >>>>                                   hci_connect_cfm(conn, status);
> >>>>                                   hci_conn_del(conn);
> >>>> -                       } else
> >>>> -                               conn->state = BT_CONNECT2;
> >>>> +                       }
> >>>>                   }
> >>>>           } else {
> >>>>                   if (!conn) {
> >>>> --
> >>>> 2.43.0
> >>>>
> >>>
> >>>
> >>
> >> Cheers,
> >> Jonas
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

[PATCH 4/5] Bluetooth: hci_event: Do sanity checks before retrying to connect

[Jonas Dreßler]

When we receive "Command Disallowed" response to HCI_CREATE_CONNECTION,
we'll try to connect again later, assuming that the command failed either
because there's already concurrent "Create Connection" requests on the
card and all "slots" for new connections are exhausted, or the card is
in the middle of doing an HCI Inquiry.

Both of those conditions we should know about, so do some sanity checking
to ensure one of them actually applies. If they don't, log an error and
delete the connection.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_event.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e1f5b6f90..1376092c5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2323,8 +2323,28 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
 
 	if (status) {
 		if (conn && conn->state == BT_CONNECT) {
+			/* If the request failed with "Command Disallowed", the
+			 * card is either using all its available "slots" for
+			 * attempting new connections, or it's currently
+			 * doing an HCI Inquiry. In these cases we'll try to
+			 * do the "Create Connection" request again later.
+			 */
 			if (status == HCI_ERROR_COMMAND_DISALLOWED) {
 				conn->state = BT_CONNECT2;
+
+				if (!hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
+				    !test_bit(HCI_INQUIRY, &hdev->flags)) {
+					bt_dev_err(hdev,
+						   "\"Create Connection\" returned error "
+						   "(0x%2.2x) indicating to try again, but "
+						   "there's no concurrent \"Create "
+						   "Connection\" nor an ongoing inquiry",
+						   status);
+
+					conn->state = BT_CLOSED;
+					hci_connect_cfm(conn, status);
+					hci_conn_del(conn);
+				}
 			} else {
 				conn->state = BT_CLOSED;
 				hci_connect_cfm(conn, status);
-- 
2.43.0

[PATCH 5/5] Bluetooth: hci_event: Try reconnecting on more kinds of errors

[Jonas Dreßler]

While some hardware seems to return "HCI Command Disallowed" errors when
trying to connect to too many devices at once, other hardware (eg. the
BCM4378 found in M1 macbooks) returns "HCI Hardware Failure" in this case.

And the Marvell 88W8897 in various Microsoft Surface devices behaves
different again: Here the "HCI Create Connection" succeeds, but later
a "HCI Connection Complete" event with status "Rejected Limited Resources"
comes in.

Handle all these cases as expected by userspace and reuse the existing
BT_CONNECT2 logic to try "HCI Create Connection" again after the ongoing
connection attempts have been completed.

Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_event.c   | 26 +++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index fef723afd..23890f53e 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -637,6 +637,7 @@ enum {
 
 /* ---- HCI Error Codes ---- */
 #define HCI_ERROR_UNKNOWN_CONN_ID	0x02
+#define HCI_ERROR_HARDWARE_FAILURE	0x03
 #define HCI_ERROR_AUTH_FAILURE		0x05
 #define HCI_ERROR_PIN_OR_KEY_MISSING	0x06
 #define HCI_ERROR_MEMORY_EXCEEDED	0x07
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1376092c5..46b6d7e27 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2323,13 +2323,14 @@ static void hci_cs_create_conn(struct hci_dev *hdev, __u8 status)
 
 	if (status) {
 		if (conn && conn->state == BT_CONNECT) {
-			/* If the request failed with "Command Disallowed", the
+			/* If the request failed with a certain status, the
 			 * card is either using all its available "slots" for
 			 * attempting new connections, or it's currently
 			 * doing an HCI Inquiry. In these cases we'll try to
 			 * do the "Create Connection" request again later.
 			 */
-			if (status == HCI_ERROR_COMMAND_DISALLOWED) {
+			if (status == HCI_ERROR_COMMAND_DISALLOWED ||
+			    status == HCI_ERROR_HARDWARE_FAILURE) {
 				conn->state = BT_CONNECT2;
 
 				if (!hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
@@ -3254,7 +3255,26 @@ static void hci_conn_complete_evt(struct hci_dev *hdev, void *data,
 
 done:
 	if (status) {
-		hci_conn_failed(conn, status);
+		if (status == HCI_ERROR_REJ_LIMITED_RESOURCES) {
+			conn->state = BT_CONNECT2;
+
+			if (!hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT) &&
+			    !test_bit(HCI_INQUIRY, &hdev->flags)) {
+				bt_dev_err(hdev,
+					   "\"Connect Complete\" event with error "
+					   "(0x%2.2x) indicating to try again, but "
+					   "there's no concurrent \"Create "
+					   "Connection\" nor an ongoing inquiry",
+					   status);
+
+				hci_conn_failed(conn, status);
+			}
+
+			hci_dev_unlock(hdev);
+			return;
+		} else {
+			hci_conn_failed(conn, status);
+		}
 	} else if (ev->link_type == SCO_LINK) {
 		switch (conn->setting & SCO_AIRMODE_MASK) {
 		case SCO_AIRMODE_CVSD:
-- 
2.43.0

[PATCH v2 1/4] Bluetooth: Remove superfluous call to hci_conn_check_pending()

[Jonas Dreßler]

The "pending connections" feature was originally introduced with commit
4c67bc74f016 ("[Bluetooth] Support concurrent connect requests") and
6bd57416127e ("[Bluetooth] Handling pending connect attempts after
inquiry") to handle controllers supporting only a single connection request
at a time. Later things were extended to also cancel ongoing inquiries on
connect() with commit 89e65975fea5 ("Bluetooth: Cancel Inquiry before
Create Connection").

With commit a9de9248064b ("[Bluetooth] Switch from OGF+OCF to using only
opcodes"), hci_conn_check_pending() was introduced as a helper to
consolidate a few places where we check for pending connections (indicated
by the BT_CONNECT2 flag) and then try to connect.

This refactoring commit also snuck in two more calls to
hci_conn_check_pending():

- One is in the failure callback of hci_cs_inquiry(), this one probably
makes sense: If we send an "HCI Inquiry" command and then immediately
after a "Create Connection" command, the "Create Connection" command might
fail before the "HCI Inquiry" command, and then we want to retry the
"Create Connection" on failure of the "HCI Inquiry".

- The other added call to hci_conn_check_pending() is in the event handler
for the "Remote Name" event, this seems unrelated and is possibly a
copy-paste error, so remove that one.

Fixes: a9de9248064b ("[Bluetooth] Switch from OGF+OCF to using only opcodes")
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_event.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 31ca320ce..13396329f 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3538,8 +3538,6 @@ static void hci_remote_name_evt(struct hci_dev *hdev, void *data,
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
 
-	hci_conn_check_pending(hdev);
-
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
-- 
2.43.0

RE: Bluetooth: Improve retrying of connection attempts

[bluez.test.bot]

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

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: net/bluetooth/hci_conn.c:229
error: net/bluetooth/hci_conn.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[PATCH v3 1/4] Bluetooth: Remove superfluous call to hci_conn_check_pending()

[Jonas Dreßler]

The "pending connections" feature was originally introduced with commit
4c67bc74f016 ("[Bluetooth] Support concurrent connect requests") and
6bd57416127e ("[Bluetooth] Handling pending connect attempts after
inquiry") to handle controllers supporting only a single connection request
at a time. Later things were extended to also cancel ongoing inquiries on
connect() with commit 89e65975fea5 ("Bluetooth: Cancel Inquiry before
Create Connection").

With commit a9de9248064b ("[Bluetooth] Switch from OGF+OCF to using only
opcodes"), hci_conn_check_pending() was introduced as a helper to
consolidate a few places where we check for pending connections (indicated
by the BT_CONNECT2 flag) and then try to connect.

This refactoring commit also snuck in two more calls to
hci_conn_check_pending():

- One is in the failure callback of hci_cs_inquiry(), this one probably
makes sense: If we send an "HCI Inquiry" command and then immediately
after a "Create Connection" command, the "Create Connection" command might
fail before the "HCI Inquiry" command, and then we want to retry the
"Create Connection" on failure of the "HCI Inquiry".

- The other added call to hci_conn_check_pending() is in the event handler
for the "Remote Name" event, this seems unrelated and is possibly a
copy-paste error, so remove that one.

Fixes: a9de9248064b ("[Bluetooth] Switch from OGF+OCF to using only opcodes")
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 net/bluetooth/hci_event.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 1e1c91473..9423394f6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3547,8 +3547,6 @@ static void hci_remote_name_evt(struct hci_dev *hdev, void *data,
 
 	bt_dev_dbg(hdev, "status 0x%2.2x", ev->status);
 
-	hci_conn_check_pending(hdev);
-
 	hci_dev_lock(hdev);
 
 	conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
-- 
2.43.0

RE: Bluetooth: Improve retrying of connection attempts

[bluez.test.bot]

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

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: include/net/bluetooth/hci.h:437
error: include/net/bluetooth/hci.h: patch does not apply
error: patch failed: net/bluetooth/hci_conn.c:178
error: net/bluetooth/hci_conn.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

[PATCH v4 1/2] Bluetooth: hci_conn: Only do ACL connections sequentially

[Jonas Dreßler]

Pretty much all bluetooth chipsets only support paging a single device at
a time, and if they don't reject a secondary "Create Connection" request
while another is still ongoing, they'll most likely serialize those
requests in the firware.

With commit 4c67bc74f016 ("[Bluetooth] Support concurrent connect
requests") we started adding some serialization of our own in case the
adapter returns "Command Disallowed" HCI error.

This commit was using the BT_CONNECT2 state for the serialization, this
state is also used for a few more things (most notably to indicate we're
waiting for an inquiry to cancel) and therefore a bit unreliable. Also
not all BT firwares would respond with "Command Disallowed" on too many
connection requests, some will also respond with "Hardware Failure"
(BCM4378), and others will error out later and send a "Connect Complete"
event with error "Rejected Limited Resources" (Marvell 88W8897).

We can clean things up a bit and also make the serialization more reliable
by using our hci_sync machinery to always do "Create Connection" requests
in a sequential manner.

This is very similar to what we're already doing for establishing LE
connections, and it works well there.

Note that this causes a test failure in mgmt-tester (test "Pair Device
- Power off 1") because the hci_abort_conn_sync() changes the error we
return on timeout of the "Create Connection". We'll fix this on the
mgmt-tester side by adjusting the expected error for the test.
---
 include/net/bluetooth/hci.h      |  1 +
 include/net/bluetooth/hci_sync.h |  3 ++
 net/bluetooth/hci_conn.c         | 69 ++++---------------------------
 net/bluetooth/hci_sync.c         | 70 ++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 60 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index aa6c69053d..08cb5cb249 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -437,6 +437,7 @@ enum {
 #define HCI_NCMD_TIMEOUT	msecs_to_jiffies(4000)	/* 4 seconds */
 #define HCI_ACL_TX_TIMEOUT	msecs_to_jiffies(45000)	/* 45 seconds */
 #define HCI_AUTO_OFF_TIMEOUT	msecs_to_jiffies(2000)	/* 2 seconds */
+#define HCI_ACL_CONN_TIMEOUT	msecs_to_jiffies(20000)	/* 20 seconds */
 #define HCI_LE_CONN_TIMEOUT	msecs_to_jiffies(20000)	/* 20 seconds */
 #define HCI_LE_AUTOCONN_TIMEOUT	msecs_to_jiffies(4000)	/* 4 seconds */
 
diff --git a/include/net/bluetooth/hci_sync.h b/include/net/bluetooth/hci_sync.h
index e2582c2425..824660f8f3 100644
--- a/include/net/bluetooth/hci_sync.h
+++ b/include/net/bluetooth/hci_sync.h
@@ -138,3 +138,6 @@ int hci_le_terminate_big_sync(struct hci_dev *hdev, u8 handle, u8 reason);
 int hci_le_big_terminate_sync(struct hci_dev *hdev, u8 handle);
 
 int hci_le_pa_terminate_sync(struct hci_dev *hdev, u16 handle);
+
+int hci_acl_create_connection_sync(struct hci_dev *hdev,
+				   struct hci_conn *conn);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index fc4d72f83a..816be7667a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -178,64 +178,6 @@ static void hci_conn_cleanup(struct hci_conn *conn)
 	hci_dev_put(hdev);
 }
 
-static void hci_acl_create_connection(struct hci_conn *conn)
-{
-	struct hci_dev *hdev = conn->hdev;
-	struct inquiry_entry *ie;
-	struct hci_cp_create_conn cp;
-
-	BT_DBG("hcon %p", conn);
-
-	/* Many controllers disallow HCI Create Connection while it is doing
-	 * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
-	 * Connection. This may cause the MGMT discovering state to become false
-	 * without user space's request but it is okay since the MGMT Discovery
-	 * APIs do not promise that discovery should be done forever. Instead,
-	 * the user space monitors the status of MGMT discovering and it may
-	 * request for discovery again when this flag becomes false.
-	 */
-	if (test_bit(HCI_INQUIRY, &hdev->flags)) {
-		/* Put this connection to "pending" state so that it will be
-		 * executed after the inquiry cancel command complete event.
-		 */
-		conn->state = BT_CONNECT2;
-		hci_send_cmd(hdev, HCI_OP_INQUIRY_CANCEL, 0, NULL);
-		return;
-	}
-
-	conn->state = BT_CONNECT;
-	conn->out = true;
-	conn->role = HCI_ROLE_MASTER;
-
-	conn->attempt++;
-
-	conn->link_policy = hdev->link_policy;
-
-	memset(&cp, 0, sizeof(cp));
-	bacpy(&cp.bdaddr, &conn->dst);
-	cp.pscan_rep_mode = 0x02;
-
-	ie = hci_inquiry_cache_lookup(hdev, &conn->dst);
-	if (ie) {
-		if (inquiry_entry_age(ie) <= INQUIRY_ENTRY_AGE_MAX) {
-			cp.pscan_rep_mode = ie->data.pscan_rep_mode;
-			cp.pscan_mode     = ie->data.pscan_mode;
-			cp.clock_offset   = ie->data.clock_offset |
-					    cpu_to_le16(0x8000);
-		}
-
-		memcpy(conn->dev_class, ie->data.dev_class, 3);
-	}
-
-	cp.pkt_type = cpu_to_le16(conn->pkt_type);
-	if (lmp_rswitch_capable(hdev) && !(hdev->link_mode & HCI_LM_MASTER))
-		cp.role_switch = 0x01;
-	else
-		cp.role_switch = 0x00;
-
-	hci_send_cmd(hdev, HCI_OP_CREATE_CONN, sizeof(cp), &cp);
-}
-
 int hci_disconnect(struct hci_conn *conn, __u8 reason)
 {
 	BT_DBG("hcon %p", conn);
@@ -1696,10 +1638,17 @@ struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
 
 	acl->conn_reason = conn_reason;
 	if (acl->state == BT_OPEN || acl->state == BT_CLOSED) {
+		int err;
+
 		acl->sec_level = BT_SECURITY_LOW;
 		acl->pending_sec_level = sec_level;
 		acl->auth_type = auth_type;
-		hci_acl_create_connection(acl);
+
+		err = hci_acl_create_connection_sync(hdev, acl);
+		if (err) {
+			hci_conn_del(acl);
+			return ERR_PTR(err);
+		}
 	}
 
 	return acl;
@@ -2654,7 +2603,7 @@ void hci_conn_check_pending(struct hci_dev *hdev)
 
 	conn = hci_conn_hash_lookup_state(hdev, ACL_LINK, BT_CONNECT2);
 	if (conn)
-		hci_acl_create_connection(conn);
+		hci_acl_create_connection_sync(hdev, conn);
 
 	hci_dev_unlock(hdev);
 }
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 1122296ce3..617407b81f 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -6492,3 +6492,73 @@ int hci_update_adv_data(struct hci_dev *hdev, u8 instance)
 	return hci_cmd_sync_queue(hdev, _update_adv_data_sync,
 				  UINT_PTR(instance), NULL);
 }
+
+static int __hci_acl_create_connection_sync(struct hci_dev *hdev, void *data)
+{
+	struct hci_conn *conn = data;
+	struct inquiry_entry *ie;
+	struct hci_cp_create_conn cp;
+	int err;
+
+	/* Many controllers disallow HCI Create Connection while it is doing
+	 * HCI Inquiry. So we cancel the Inquiry first before issuing HCI Create
+	 * Connection. This may cause the MGMT discovering state to become false
+	 * without user space's request but it is okay since the MGMT Discovery
+	 * APIs do not promise that discovery should be done forever. Instead,
+	 * the user space monitors the status of MGMT discovering and it may
+	 * request for discovery again when this flag becomes false.
+	 */
+	if (test_bit(HCI_INQUIRY, &hdev->flags)) {
+		err = __hci_cmd_sync_status(hdev, HCI_OP_INQUIRY_CANCEL, 0,
+					    NULL, HCI_CMD_TIMEOUT);
+		if (err)
+			bt_dev_warn(hdev, "Failed to cancel inquiry %d", err);
+	}
+
+	conn->state = BT_CONNECT;
+	conn->out = true;
+	conn->role = HCI_ROLE_MASTER;
+
+	conn->attempt++;
+
+	conn->link_policy = hdev->link_policy;
+
+	memset(&cp, 0, sizeof(cp));
+	bacpy(&cp.bdaddr, &conn->dst);
+	cp.pscan_rep_mode = 0x02;
+
+	ie = hci_inquiry_cache_lookup(hdev, &conn->dst);
+	if (ie) {
+		if (inquiry_entry_age(ie) <= INQUIRY_ENTRY_AGE_MAX) {
+			cp.pscan_rep_mode = ie->data.pscan_rep_mode;
+			cp.pscan_mode     = ie->data.pscan_mode;
+			cp.clock_offset   = ie->data.clock_offset |
+					    cpu_to_le16(0x8000);
+		}
+
+		memcpy(conn->dev_class, ie->data.dev_class, 3);
+	}
+
+	cp.pkt_type = cpu_to_le16(conn->pkt_type);
+	if (lmp_rswitch_capable(hdev) && !(hdev->link_mode & HCI_LM_MASTER))
+		cp.role_switch = 0x01;
+	else
+		cp.role_switch = 0x00;
+
+	err = __hci_cmd_sync_status_sk(hdev, HCI_OP_CREATE_CONN,
+				       sizeof(cp), &cp,
+				       HCI_EV_CONN_COMPLETE,
+				       HCI_ACL_CONN_TIMEOUT, NULL);
+
+	if (err == -ETIMEDOUT)
+		hci_abort_conn_sync(hdev, conn, HCI_ERROR_LOCAL_HOST_TERM);
+
+	return err;
+}
+
+int hci_acl_create_connection_sync(struct hci_dev *hdev,
+				   struct hci_conn *conn)
+{
+	return hci_cmd_sync_queue(hdev, __hci_acl_create_connection_sync,
+				  conn, NULL);
+}
-- 
2.43.0

RE: Bluetooth: Improve retrying of connection attempts

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 4211 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=823530

---Test result---

Test Summary:
CheckPatch                    FAIL      2.36 seconds
GitLint                       PASS      0.45 seconds
SubjectPrefix                 PASS      0.15 seconds
BuildKernel                   PASS      27.68 seconds
CheckAllWarning               PASS      30.95 seconds
CheckSparse                   WARNING   36.26 seconds
CheckSmatch                   WARNING   98.20 seconds
BuildKernel32                 PASS      26.92 seconds
TestRunnerSetup               PASS      501.62 seconds
TestRunner_l2cap-tester       PASS      19.21 seconds
TestRunner_iso-tester         PASS      30.81 seconds
TestRunner_bnep-tester        PASS      4.82 seconds
TestRunner_mgmt-tester        FAIL      170.51 seconds
TestRunner_rfcomm-tester      PASS      7.42 seconds
TestRunner_sco-tester         PASS      17.06 seconds
TestRunner_ioctl-tester       PASS      7.70 seconds
TestRunner_mesh-tester        PASS      5.88 seconds
TestRunner_smp-tester         PASS      6.88 seconds
TestRunner_userchan-tester    PASS      5.01 seconds
IncrementalBuild              PASS      48.73 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[v4,1/2] Bluetooth: hci_conn: Only do ACL connections sequentially
WARNING: 'firware' may be misspelled - perhaps 'firmware'?
#62: 
requests in the firware.
                ^^^^^^^

ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 1 warnings, 0 checks, 176 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13547031.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[v4,2/2] Bluetooth: Remove pending ACL connection attempts
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 0 checks, 87 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/src/13547032.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: CheckSparse - WARNING
Desc: Run sparse tool with linux kernel
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: CheckSmatch - WARNING
Desc: Run smatch tool with source
Output:
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 497, Passed: 479 (96.4%), Failed: 11, Not Run: 7

Failed Test Cases
Read Ext Controller Info 1                           Failed       0.052 seconds
Read Ext Controller Info 2                           Failed       0.060 seconds
Read Ext Controller Info 3                           Failed       0.054 seconds
Read Ext Controller Info 4                           Failed       0.057 seconds
Read Ext Controller Info 5                           Failed       0.064 seconds
LL Privacy - Add Device 4 (2 Devices to AL)          Failed       0.111 seconds
LL Privacy - Add Device 5 (2 Devices to RL)          Failed       0.114 seconds
LL Privacy - Add Device 6 (RL is full)               Failed       0.143 seconds
LL Privacy - Add Device 7 (AL is full)               Failed       0.151 seconds
LL Privacy - Remove Device 1 (Remove from AL)        Timed out    2.023 seconds
LL Privacy - Remove Device 4 (Disable Adv)           Timed out    2.687 seconds


---
Regards,
Linux Bluetooth