Turning off bluetooth consistently takes more than 5 seconds |
|||||||||||||||
Issue descriptionTurning off bluetooth should take less than 1 second. This caused crbug.com/795190 . Regression happens between platform version 10111 and 10112 (https://crosland.corp.google.com/log/10111.0.0..10112.0.0). The suspected CL is https://chromium.googlesource.com/chromiumos/third_party/kernel/+/762fa6509dd8c8b5a2e41dcfd51e413e8bd2492a. This can be consistently reproduced if instant tether is disabled. So we can reproduce it by disabling magic tether or stopping the ui altogether. Then running "btmgmt power off" should reproduce consistently. It's interesting to note that this bug is not apparent when instant tether is enabled.
,
Jan 30 2018
Hi Sonny, to turn off the bluetooth adapter requires (1) to disable advertising (2) to stop discovery (3) to abort all existing connections and (4) other state cleanup work. Hence, in kernel, it defines in kernel #define HCI_POWER_OFF_TIMEOUT msecs_to_jiffies(5000) /* 5 seconds */ to wait for 5 seconds to let all the above things done gracefully and then close the bluetooth adapter. So it does take a bit more than 5 seconds per bluez implementation.
,
Jan 30 2018
That must be the final timeout when power off is forced no? Usually all of these things happen and the adapter turns off considerably faster than 5s. We should try to figure out on those devices why specifically is it taking the full timeout.
,
Jan 31 2018
The operations look like (1) It sends a bunch of HCI commands to stop advertising, stop discovering, and terminate connections. How long those operations take depends on the parameters that we set. For example, to terminate connections may take up to SUPERVISION _TIME seconds. It is 2 seconds currently. We are considering to lengthen it even longer as apple devices and android phones use even larger values for more stable connections. If my memory serves me correctly, apple use 5 seconds and android phone uses 20+ seconds. (2) It waits for HCI_POWER_OFF_TIMEOUT (5 seconds) to let the above operations to complete gracefully. (3) Then it starts to clean up and flush everything remaining. (4) When everything is cleaned up, it sends a reset command to the controller. (5) After receiving controller's success status, it sets the power settings to off. At this time, the outer world is notified that the power is off.
,
Jan 31 2018
The operations in (1) above are asynchronous. It is also possible to set up a checklist for the operations. Whenever a command complete callback or command status callback is invoked, check if the checklist is done. It would be faster to close the adapter this way at the cost of more complicated code.
,
Jan 31 2018
Thanks Joseph for the analysis. I am wondering why turning off bluetooth was okay (under 1 second) before this CL: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/762fa6509dd8c8b5a2e41dcfd51e413e8bd2492a, and maybe part of that CL caused the regression?
,
Jan 31 2018
I understand that a lot of actions happen during adapter->off which can take a long time. This is a regression though. It didn't take that long, it does now - hence we are doing something differently. We should figure out what that is and see if we can fix it.
,
Feb 2 2018
Good catch, Sonny! The patch does lead to the regression. However, that is how conditional commands work. I will ponder over that to see whether we could mitigate the issue.
,
Feb 2 2018
Thank you Joseph for looking at this. Also it's interesting and may be useful to note that turning bluetooth off is quick if Instant Tether is enabled, although with that kernel CL. You can experiment this by enabling/disabling Instant Tether in chrome://flags.
,
Feb 2 2018
Sonny, that sounds weird to me that powering off would become faster with Instant Tethering enabled. Theoretically, powering off would be really quick only when there are no advertising, no scanning, no connections ongoing. But Instant Tethering does these activities from time to time...
,
Feb 2 2018
Sonny, I just sent a patch to fix the regression -- https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/896670 The power off would take effect immediately if there are no bluetooth activities, i.e., no advertising, no scanning, and no connections ongoing. If there is any such activity, it would still take 5+ seconds.
,
Feb 2 2018
Sorry that the patch above did not fix the issue. Will submit a new one later.
,
Feb 2 2018
Hi Sonny, I have submitted a new patch set (https://chromium-review.googlesource.com/#/c/chromiumos/third_party/kernel/+/896670/). Could you help verify if the patch works for you? Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. If both tests pass, I will submit a new patch set to fix the commit message later. (It is too late for me to fix the commit message tonight.) Thanks!
,
Feb 5 2018
A new patch set was sent which fixed the commit message. It worked for me for both tests above. Sonny, please help verify it. Thanks!
,
Feb 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/e03072ba645077f98249a2451519db61d9246706 commit e03072ba645077f98249a2451519db61d9246706 Author: Joseph Hwang <josephsih@chromium.org> Date: Wed Feb 07 08:43:57 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> [modify] https://crrev.com/e03072ba645077f98249a2451519db61d9246706/net/bluetooth/mgmt.c
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ce7f7b1fa15ab01a64d6ecd3102c3734c58a397b commit ce7f7b1fa15ab01a64d6ecd3102c3734c58a397b Author: Joseph Hwang <josephsih@chromium.org> Date: Fri Feb 09 07:42:36 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/906226 [modify] https://crrev.com/ce7f7b1fa15ab01a64d6ecd3102c3734c58a397b/net/bluetooth/mgmt.c
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/4831859246b26b9cf94c40a06cac1fcb399bc39a commit 4831859246b26b9cf94c40a06cac1fcb399bc39a Author: Joseph Hwang <josephsih@chromium.org> Date: Fri Feb 09 07:42:16 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/906225 [modify] https://crrev.com/4831859246b26b9cf94c40a06cac1fcb399bc39a/net/bluetooth/mgmt.c
,
Feb 9 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/fe13f472589a2f2667106ce4741a368e9248ecb2 commit fe13f472589a2f2667106ce4741a368e9248ecb2 Author: Joseph Hwang <josephsih@chromium.org> Date: Fri Feb 09 07:42:40 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/906227 [modify] https://crrev.com/fe13f472589a2f2667106ce4741a368e9248ecb2/net/bluetooth/mgmt.c
,
Feb 9 2018
Does this affect upstream? If so I guess sending the patch for linux-bluetooth so we can give some feedback.
,
Feb 9 2018
Hi Luiz, this does not affect upstream for now. This was a regression caused by chromium local patch which introduced "conditional commands".
With that, it would still be good if you may take a look at the commit message about why "conditional commands" were introduced. It was used to resolve possible run-time race conditions. An example is
hci_request.c:__hci_req_enable_advertising()
...
if (hci_dev_test_flag(hdev, HCI_LE_ADV))
__hci_req_disable_advertising(req);
...
This code segment determines whether advertising should be disabled at command build time for a request based on the condition at that moment. However, it is not guaranteed that the condition at command build time would stay unchanged at command execution time. This is because some other commands of other requests in the queue may change the condition as to whether it is advertising. This is possible when there are scanning, advertising, and connections activities going on simultaneously.
For example, it is possible that advertising is disabled when the above code segment is being executed; and thus __hci_req_disable_advertising() is not executed. But when the commands in the request are to be executed by the worker thread, advertising may already be enabled due to some other HCI commands of other requests in the queue. A possible result is that the HCI_OP_LE_SET_ADV_PARAM command may be executed without disabling advertising first which leads to an error.
On the contrary, it is also possible that the command to disable advertising may be sent to the controller twice.
The same concern applies to scanning as well. The idea of conditional commands is to create enabling/disabling advertising/scanning commands into the command queue at command build time. And then let the worker thread determine at run time whether the enabling/disabling commands should be executed.
Do you think such mechanism is worth upstreaming? It does fix issues for chromium os. Thanks!
,
Feb 9 2018
I would say the flags shall be evaluated at runtime not at queueing time, is that what the original patch is doing? We should always aim to have as low traffic as possible on HCI, and perhaps if we need to create NOP kind of operation to continue the execution.
,
Feb 13 2018
Should we merge this to 65? The fix for the UI part has already been merged to 65 ( bug 795190 ), but the user experience is still weird if bluetooth turn off takes 5 seconds even though the UI has been fixed to handle this gracefully.
,
Feb 13 2018
If the CL has been on ToT for a few days and no issue are observed, we should merge this. It is a really small fix.
,
Feb 13 2018
This bug requires manual review: M65 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 20 2018
,
Feb 26 2018
Thanks Bernie! Hi Sonny, this CL has been cherry-picked to 4 branches of R65. PTAL. Thanks!
,
Feb 26 2018
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/58aaa3b394e7abf2f7252b520f1020d7e5c8d82b commit 58aaa3b394e7abf2f7252b520f1020d7e5c8d82b Author: Joseph Hwang <josephsih@chromium.org> Date: Tue Feb 27 00:16:46 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/937004 Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> [modify] https://crrev.com/58aaa3b394e7abf2f7252b520f1020d7e5c8d82b/net/bluetooth/mgmt.c
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/3f26a8efd173c71f4452a11b700729d46d398c8d commit 3f26a8efd173c71f4452a11b700729d46d398c8d Author: Joseph Hwang <josephsih@chromium.org> Date: Tue Feb 27 00:21:13 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/937006 Reviewed-by: Sonny Sasaka <sonnysasaka@google.com> [modify] https://crrev.com/3f26a8efd173c71f4452a11b700729d46d398c8d/net/bluetooth/mgmt.c
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/11b313e2b636f7c570636a0eb5807b37f6684b6f commit 11b313e2b636f7c570636a0eb5807b37f6684b6f Author: Joseph Hwang <josephsih@chromium.org> Date: Tue Feb 27 00:21:17 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/936403 Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> [modify] https://crrev.com/11b313e2b636f7c570636a0eb5807b37f6684b6f/net/bluetooth/mgmt.c
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/5e9635f3a08c895afcad497970b3656d511de185 commit 5e9635f3a08c895afcad497970b3656d511de185 Author: Joseph Hwang <josephsih@chromium.org> Date: Tue Feb 27 00:21:19 2018 CHROMIUM: bluetooth: fix a regression about power off delay Conditional commands were introduced to fix run-time race conditions for advertising and scanning. To use conditional commands, all enabling and disabling commands for advertising and scanning are added to the command queue at command build time. And then the command worker thread will check if such a conditional command should be executed or skipped at command run time. This may result in a regression when powering off the bluetooth adapter. When powering off the bluetooth adapter, kernel may send a variety of commands to stop all bluetooth activities and wait for 5 seconds before forcing to power off the adapter. There are two shortcuts to avoid the 5-second long delay. The first shortcut is taken when there are no HCI commands built at all at command built time. The kernel would power off the bluetooth adapter immediately. The second shortcut is taken when the last command of the request is completed. In this case, the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter immediately. The power off regression occurs when the last command in the request is a conditional command and is skipped. In this case, no clean_up_hci_complete() callback would be invoked. The bluetooth adapter cannot be powered off until the 5-second timeout expires. This patch temporarily fixed the regression by adding a dummy command as the last command in the request. This ensures that the clean_up_hci_complete() callback is invoked to power off the bluetooth adapter. As adding a dummy command is kind of hacky, it is desirable to have a formal fix which is being tracked in chromium:808749 Bluez: clean up dummy commands used for conditional HCI command. BUG= chromium:805329 TEST=Conduct the following two tests with bluetoothctl $ bluetoothctl [bluetooth]# power off Test 1: set power off when magic tethering is not running and there are no other bluetooth activities (advertising, scanning, and connections) ongoing. Test 2: set power off when magic tethering is running. The power off should take effect immediately in both tests. Change-Id: If014efe56200c2a9a81fa8711d53ef1bf37e857c Signed-off-by: Joseph Hwang <josephsih@chromium.org> Reviewed-on: https://chromium-review.googlesource.com/896670 Commit-Ready: Shyh-In Hwang <josephsih@chromium.org> Tested-by: Sonny Sasaka <sonnysasaka@chromium.org> Tested-by: Shyh-In Hwang <josephsih@chromium.org> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> Reviewed-by: Dmitry Grinberg <dmitrygr@google.com> (cherry picked from commit e03072ba645077f98249a2451519db61d9246706) Reviewed-on: https://chromium-review.googlesource.com/937005 Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org> [modify] https://crrev.com/5e9635f3a08c895afcad497970b3656d511de185/net/bluetooth/mgmt.c
,
Feb 27 2018
The patch has been merged to R65. Marked as fixed. Thanks. |
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by sonnysasaka@chromium.org
, Jan 24 2018