[PATCH] Revert "Bluetooth: Shutdown controller after workqueues are flushed or cancelled"

[PATCH] Revert "Bluetooth: Shutdown controller after workqueues are flushed or cancelled"

[Dmitry Baryshkov]

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(-)

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);
 
-- 
2.30.2

Re: [PATCH] Revert "Bluetooth: Shutdown controller after workqueues are flushed or cancelled"

[Marcel Holtmann]

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