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