Bluetooth: hci_sync: complete LE connection on any event

Commit Message

Ahmad Fatoum June 16, 2022, 9:24 a.m. UTC

Commit 6cd29ec6ae5e ("Bluetooth: hci_sync: Wait for proper events when
connecting LE") changed HCI core to wait for specific events before
posting completion for a new LE connection. This commit introduced
regressions partially fixed in commits a56a1138cbd8
("Bluetooth: hci_sync: Fix not using conn_timeout") and
and c9f73a2178c1 ("Bluetooth: hci_conn: Fix hci_connect_le_sync").

Yet, a regression still remains where devices that worked previously
now timeout[1][2]. Restore working order by reverting the commit in
question until this issue can be properly resolved.

[1]: https://lore.kernel.org/linux-bluetooth/[e-mail]/
[2]: https://github.com/bluez/bluez/issues/340

Fixes: 6cd29ec6ae5e ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
Signed-off-by: Ahmad Fatoum <[e-mail]>
---
Event callbacks like hci_le_meta_evt() use hci_skb_event(hdev->sent_cmd)
for matching. I suspect the timeout is due to intermittent frames,
e.g. because of remote unpairing, replacing the sent_cmd and thus
breaking renewed pairing as the frames couldn't be matched. This is too
complex for me to fix and we have been carrying this fix for a month now,
so I think it's best we revert this upstream for now.

#regzb Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/
Cc: [e-mail]
---
 net/bluetooth/hci_sync.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

[e-mail] June 16, 2022, 10:03 a.m. UTC | #1

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

---Test result---

Test Summary:
CheckPatch                    FAIL      1.24 seconds
GitLint                       FAIL      0.68 seconds
SubjectPrefix                 PASS      0.57 seconds
BuildKernel                   PASS      30.24 seconds
BuildKernel32                 PASS      26.88 seconds
Incremental Build with patchesPASS      37.54 seconds
TestRunner: Setup             PASS      461.76 seconds
TestRunner: l2cap-tester      PASS      12.67 seconds
TestRunner: bnep-tester       PASS      5.63 seconds
TestRunner: mgmt-tester       PASS      96.61 seconds
TestRunner: rfcomm-tester     PASS      9.16 seconds
TestRunner: sco-tester        PASS      8.80 seconds
TestRunner: smp-tester        PASS      8.85 seconds
TestRunner: userchan-tester   PASS      5.89 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.24 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: hci_sync: complete LE connection on any event\WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#77: 
[1]: https://lore.kernel.org/linux-bluetooth/[e-mail]/

total: 0 errors, 1 warnings, 0 checks, 18 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/12883653.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: GitLint - FAIL - 0.68 seconds
Run gitlint with rule in .gitlint
Bluetooth: hci_sync: complete LE connection on any event
14: B1 Line exceeds max length (97>80): "[1]: https://lore.kernel.org/linux-bluetooth/[e-mail]/"




---
Regards,
Linux Bluetooth

Thorsten Leemhuis June 19, 2022, 11:57 a.m. UTC | #2

Hi, this is your Linux kernel regression tracker.

On 16.06.22 11:24, Ahmad Fatoum wrote:
> Commit 6cd29ec6ae5e ("Bluetooth: hci_sync: Wait for proper events when
> connecting LE") changed HCI core to wait for specific events before
> posting completion for a new LE connection. This commit introduced
> regressions partially fixed in commits a56a1138cbd8
> ("Bluetooth: hci_sync: Fix not using conn_timeout") and
> and c9f73a2178c1 ("Bluetooth: hci_conn: Fix hci_connect_le_sync").
> 
> Yet, a regression still remains where devices that worked previously
> now timeout[1][2]. Restore working order by reverting the commit in
> question until this issue can be properly resolved.
> 
> [1]: https://lore.kernel.org/linux-bluetooth/[e-mail]/
> [2]: https://github.com/bluez/bluez/issues/340

That way regzbot will miss the tags and not associate the fix with the
regression. You can leave it like that and add these, as explained in
the kernel docs (see for example submitting-patches.rst):

Link:
https://lore.kernel.org/linux-bluetooth/[e-mail]/
Link: https://github.com/bluez/bluez/issues/340

Or your remove the two footnotes and do it like this:

Link:
https://lore.kernel.org/linux-bluetooth/[e-mail]/
[1]
Link: https://github.com/bluez/bluez/issues/340 [2]

> Fixes: 6cd29ec6ae5e ("Bluetooth: hci_sync: Wait for proper events when connecting LE")
> Signed-off-by: Ahmad Fatoum <[e-mail]>
> ---
> Event callbacks like hci_le_meta_evt() use hci_skb_event(hdev->sent_cmd)
> for matching. I suspect the timeout is due to intermittent frames,
> e.g. because of remote unpairing, replacing the sent_cmd and thus
> breaking renewed pairing as the frames couldn't be matched. This is too
> complex for me to fix and we have been carrying this fix for a month now,
> so I think it's best we revert this upstream for now.
> 
> #regzb Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/

Thx for trying to do the right thing, but that didn't work out, as
"Link:" doesn't need a "#regzb". Maybe regzbot should handle this, not
sure, will keep it in mind.

Anyway, let's make regzbot aware of this thread, as you intended (thx
again):

#regzbot ^backmonitor
https://lore.kernel.org/linux-bluetooth/[e-mail]/

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I deal with a lot of
reports and sometimes miss something important when writing mails like
this. If that's the case here, don't hesitate to tell me in a public
reply, it's in everyone's interest to set the public record straight.

Ahmad Fatoum June 20, 2022, 9:58 a.m. UTC | #3

Hello Thorsten,

On 19.06.22 13:57, Thorsten Leemhuis wrote:
> Link:
> https://lore.kernel.org/linux-bluetooth/[e-mail]/
> [1]
> Link: https://github.com/bluez/bluez/issues/340 [2]

That looks good. Will do it this way in the future.

>> #regzb Link: https://lore.kernel.org/linux-bluetooth/[e-mail]/
> 
> Thx for trying to do the right thing, but that didn't work out, as
> "Link:" doesn't need a "#regzb". Maybe regzbot should handle this, not
> sure, will keep it in mind.

I copied the #regzb Link: approach off the last snippet here:

https://linux-regtracking.leemhuis.info/post/regzbot-approach/#even-more-problems-in-the-details

Cheers,
Ahmad

Patch

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 286d6767f017..4cecf15922d4 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -5272,7 +5272,7 @@  static int hci_le_ext_create_conn_sync(struct hci_dev *hdev,
 
 	return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_EXT_CREATE_CONN,
 					plen, data,
-					HCI_EV_LE_ENHANCED_CONN_COMPLETE,
+					0,
 					conn->conn_timeout, NULL);
 }
 
@@ -5366,9 +5366,7 @@  int hci_le_create_conn_sync(struct hci_dev *hdev, struct hci_conn *conn)
 	 */
 	err = __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CREATE_CONN,
 				       sizeof(cp), &cp,
-				       use_enhanced_conn_complete(hdev) ?
-				       HCI_EV_LE_ENHANCED_CONN_COMPLETE :
-				       HCI_EV_LE_CONN_COMPLETE,
+				       0,
 				       conn->conn_timeout, NULL);
 
 done: