| Message ID | 20210816231150.1478727-1-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Revert "Bluetooth: Shutdown controller after workqueues are flushed or cancelled" | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 28 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hi Dmitry, > This reverts commit 0ea9fd001a14ebc294f112b0361a4e601551d508. It moved > calling shutdown callback after flushing the queues. In doing so it > disabled calling the shutdown hook completely: shutdown condition > tests for HCI_UP in hdev->flags, which gets cleared now before checking > this condition (see test_and_clear_bit(HCI_UP, ...) call). Thus shutdown > hook was never called. > > This would not be a problem itself and could fixed with just removing > the HCI_UP condition (since if we are this point, we already know that > the HCI device was up before calling hci_dev_do_close(). However the > fact that shutdown hook was not called hid the fact that it is not > proper to call shutdown hook so late in the sequence. The hook would > usually call __hci_cmd_sync()/__hci_cmd_sync_ev(), which would timeout > without running queues. > > Thus I think it is more proper at this moment to revert the commit and > look for a better solution. > > Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled") > Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > net/bluetooth/hci_core.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) I just merged this patch: commit 0ea53674d07fb6db2dd7a7ec2fdc85a12eb246c2 Author: Kai-Heng Feng <kai.heng.feng@canonical.com> Date: Tue Aug 10 12:53:15 2021 +0800 Bluetooth: Move shutdown callback before flushing tx and rx queue Commit 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled") introduced a regression that makes mtkbtsdio driver stops working: Please check if this works for you. Regards Marcel
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index e1a545c8a69f..677d880068a4 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -1721,6 +1721,14 @@ int hci_dev_do_close(struct hci_dev *hdev) BT_DBG("%s %p", hdev->name, 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); + } + cancel_delayed_work(&hdev->power_off); cancel_delayed_work(&hdev->ncmd_timer); @@ -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);
This reverts commit 0ea9fd001a14ebc294f112b0361a4e601551d508. It moved calling shutdown callback after flushing the queues. In doing so it disabled calling the shutdown hook completely: shutdown condition tests for HCI_UP in hdev->flags, which gets cleared now before checking this condition (see test_and_clear_bit(HCI_UP, ...) call). Thus shutdown hook was never called. This would not be a problem itself and could fixed with just removing the HCI_UP condition (since if we are this point, we already know that the HCI device was up before calling hci_dev_do_close(). However the fact that shutdown hook was not called hid the fact that it is not proper to call shutdown hook so late in the sequence. The hook would usually call __hci_cmd_sync()/__hci_cmd_sync_ev(), which would timeout without running queues. Thus I think it is more proper at this moment to revert the commit and look for a better solution. Fixes: 0ea9fd001a14 ("Bluetooth: Shutdown controller after workqueues are flushed or cancelled") Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- net/bluetooth/hci_core.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)