* RE: [v2] Bluetooth: Move shutdown callback before flushing tx and rx queue
2021-08-10 4:53 [PATCH v2] Bluetooth: Move shutdown callback before flushing tx and rx queue Kai-Heng Feng
@ 2021-08-10 5:17 ` bluez.test.bot
2021-08-11 8:25 ` [PATCH v2] " Mattijs Korpershoek
2021-08-16 15:49 ` Marcel Holtmann
2 siblings, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2021-08-10 5:17 UTC (permalink / raw)
To: linux-bluetooth, kai.heng.feng
[-- Attachment #1: Type: text/plain, Size: 3753 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=528891
---Test result---
Test Summary:
CheckPatch FAIL 0.60 seconds
GitLint FAIL 0.13 seconds
BuildKernel PASS 624.82 seconds
TestRunner: Setup PASS 407.54 seconds
TestRunner: l2cap-tester PASS 2.85 seconds
TestRunner: bnep-tester PASS 2.09 seconds
TestRunner: mgmt-tester PASS 31.70 seconds
TestRunner: rfcomm-tester PASS 2.31 seconds
TestRunner: sco-tester PASS 2.22 seconds
TestRunner: smp-tester FAIL 2.34 seconds
TestRunner: userchan-tester PASS 2.13 seconds
Details
##############################
Test: CheckPatch - FAIL - 0.60 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
Bluetooth: Move shutdown callback before flushing tx and rx queue
WARNING: Unknown commit id '0ea9fd001a14', maybe rebased or not pulled?
#7:
Commit 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues
WARNING: Unknown commit id '0ea9fd001a14', maybe rebased or not pulled?
#32:
Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled")
total: 0 errors, 2 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.
"[PATCH] Bluetooth: Move shutdown callback before flushing tx and rx" has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: GitLint - FAIL - 0.13 seconds
Run gitlint with rule in .gitlint
Bluetooth: Move shutdown callback before flushing tx and rx queue
28: B1 Line exceeds max length (96>80): "Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled")"
##############################
Test: BuildKernel - PASS - 624.82 seconds
Build Kernel with minimal configuration supports Bluetooth
##############################
Test: TestRunner: Setup - PASS - 407.54 seconds
Setup environment for running Test Runner
##############################
Test: TestRunner: l2cap-tester - PASS - 2.85 seconds
Run test-runner with l2cap-tester
Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: bnep-tester - PASS - 2.09 seconds
Run test-runner with bnep-tester
Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: mgmt-tester - PASS - 31.70 seconds
Run test-runner with mgmt-tester
Total: 448, Passed: 445 (99.3%), Failed: 0, Not Run: 3
##############################
Test: TestRunner: rfcomm-tester - PASS - 2.31 seconds
Run test-runner with rfcomm-tester
Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: sco-tester - PASS - 2.22 seconds
Run test-runner with sco-tester
Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
##############################
Test: TestRunner: smp-tester - FAIL - 2.34 seconds
Run test-runner with smp-tester
Total: 8, Passed: 7 (87.5%), Failed: 1, Not Run: 0
Failed Test Cases
SMP Client - SC Request 2 Failed 0.032 seconds
##############################
Test: TestRunner: userchan-tester - PASS - 2.13 seconds
Run test-runner with userchan-tester
Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0
---
Regards,
Linux Bluetooth
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 44386 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3593 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 616863 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11713 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9948 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11741 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5490 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] Bluetooth: Move shutdown callback before flushing tx and rx queue
2021-08-10 4:53 [PATCH v2] Bluetooth: Move shutdown callback before flushing tx and rx queue Kai-Heng Feng
2021-08-10 5:17 ` [v2] " bluez.test.bot
@ 2021-08-11 8:25 ` Mattijs Korpershoek
2021-08-16 15:49 ` Marcel Holtmann
2 siblings, 0 replies; 4+ messages in thread
From: Mattijs Korpershoek @ 2021-08-11 8:25 UTC (permalink / raw)
To: Kai-Heng Feng, marcel, johan.hedberg, luiz.dentz
Cc: Kai-Heng Feng, Hsin-Yi Wang, Guenter Roeck, David S. Miller,
Jakub Kicinski, open list:BLUETOOTH SUBSYSTEM,
open list:NETWORKING [GENERAL], open list
Hi Kai-Heng,
Thank you for your patch.
Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
> Commit 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues
> are flushed or cancelled") introduced a regression that makes mtkbtsdio
> driver stops working:
> [ 36.593956] Bluetooth: hci0: Firmware already downloaded
> [ 46.814613] Bluetooth: hci0: Execution of wmt command timed out
> [ 46.814619] Bluetooth: hci0: Failed to send wmt func ctrl (-110)
>
> The shutdown callback depends on the result of hdev->rx_work, so we
> should call it before flushing rx_work:
> -> btmtksdio_shutdown()
> -> mtk_hci_wmt_sync()
> -> __hci_cmd_send()
> -> wait for BTMTKSDIO_TX_WAIT_VND_EVT gets cleared
>
> -> btmtksdio_recv_event()
> -> hci_recv_frame()
> -> queue_work(hdev->workqueue, &hdev->rx_work)
> -> clears BTMTKSDIO_TX_WAIT_VND_EVT
>
> So move the shutdown callback before flushing TX/RX queue to resolve the
> issue.
>
> Reported-and-tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
> Move the shutdown callback before clearing HCI_UP, otherwise 1)
> shutdown callback won't be called and 2) other routines that depend on
> HCI_UP won't work.
>
> net/bluetooth/hci_core.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index cb2e9e513907..8622da2d9395 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1727,6 +1727,14 @@ int hci_dev_do_close(struct hci_dev *hdev)
> hci_request_cancel_all(hdev);
> hci_req_sync_lock(hdev);
>
> + if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> + !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> + test_bit(HCI_UP, &hdev->flags)) {
> + /* Execute vendor specific shutdown routine */
> + if (hdev->shutdown)
> + hdev->shutdown(hdev);
> + }
> +
> if (!test_and_clear_bit(HCI_UP, &hdev->flags)) {
> cancel_delayed_work_sync(&hdev->cmd_timer);
> hci_req_sync_unlock(hdev);
> @@ -1798,14 +1806,6 @@ int hci_dev_do_close(struct hci_dev *hdev)
> clear_bit(HCI_INIT, &hdev->flags);
> }
>
> - if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> - !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> - test_bit(HCI_UP, &hdev->flags)) {
> - /* Execute vendor specific shutdown routine */
> - if (hdev->shutdown)
> - hdev->shutdown(hdev);
> - }
> -
> /* flush cmd work */
> flush_work(&hdev->cmd_work);
I confirm this works fine on mt8183-pumpkin using the btmtksdio driver.
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> --
> 2.31.1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] Bluetooth: Move shutdown callback before flushing tx and rx queue
2021-08-10 4:53 [PATCH v2] Bluetooth: Move shutdown callback before flushing tx and rx queue Kai-Heng Feng
2021-08-10 5:17 ` [v2] " bluez.test.bot
2021-08-11 8:25 ` [PATCH v2] " Mattijs Korpershoek
@ 2021-08-16 15:49 ` Marcel Holtmann
2 siblings, 0 replies; 4+ messages in thread
From: Marcel Holtmann @ 2021-08-16 15:49 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Johan Hedberg, Luiz Augusto von Dentz, Mattijs Korpershoek,
Hsin-Yi Wang, Guenter Roeck, David S. Miller, Jakub Kicinski,
open list:BLUETOOTH SUBSYSTEM, open list:NETWORKING [GENERAL],
open list
Hi Kai-Heng,
> Commit 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues
> are flushed or cancelled") introduced a regression that makes mtkbtsdio
> driver stops working:
> [ 36.593956] Bluetooth: hci0: Firmware already downloaded
> [ 46.814613] Bluetooth: hci0: Execution of wmt command timed out
> [ 46.814619] Bluetooth: hci0: Failed to send wmt func ctrl (-110)
>
> The shutdown callback depends on the result of hdev->rx_work, so we
> should call it before flushing rx_work:
> -> btmtksdio_shutdown()
> -> mtk_hci_wmt_sync()
> -> __hci_cmd_send()
> -> wait for BTMTKSDIO_TX_WAIT_VND_EVT gets cleared
>
> -> btmtksdio_recv_event()
> -> hci_recv_frame()
> -> queue_work(hdev->workqueue, &hdev->rx_work)
> -> clears BTMTKSDIO_TX_WAIT_VND_EVT
>
> So move the shutdown callback before flushing TX/RX queue to resolve the
> issue.
>
> Reported-and-tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Tested-by: Hsin-Yi Wang <hsinyi@chromium.org>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled")
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
> Move the shutdown callback before clearing HCI_UP, otherwise 1)
> shutdown callback won't be called and 2) other routines that depend on
> HCI_UP won't work.
>
> net/bluetooth/hci_core.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 4+ messages in thread