New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Turning off bluetooth consistently takes more than 5 seconds

Project Member Reported by sonnysasaka@chromium.org, Jan 24 2018

Issue description

Turning 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.
 
According to  crbug.com/795190#c11 , this bug is confirmed to repro in Kip, Daisy, Reks, Candy, Gnawty, Quawks, Celes, Paine, Glimmer, Blaze, Swanky, Minnie, Cyan.
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.

Comment 3 by r...@chromium.org, 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.
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.
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.
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?

Comment 7 by r...@chromium.org, 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.

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.
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.
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...
Cc: dmitrygr@google.com
Status: Started (was: Assigned)
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.
Sorry that the patch above did not fix the issue. Will submit a new one later.
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!
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!
Project Member

Comment 15 by bugdroid1@chromium.org, Feb 7 2018

Labels: merge-merged-chromeos-4.4
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

Project Member

Comment 16 by bugdroid1@chromium.org, Feb 9 2018

Labels: merge-merged-chromeos-3.14
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

Project Member

Comment 17 by bugdroid1@chromium.org, Feb 9 2018

Labels: merge-merged-chromeos-3.10
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

Project Member

Comment 18 by bugdroid1@chromium.org, Feb 9 2018

Labels: merge-merged-chromeos-3.18
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

Does this affect upstream? If so I guess sending the patch for linux-bluetooth so we can give some feedback.
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!
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.
Labels: Merge-Request-65
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.

Comment 23 by r...@chromium.org, 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.

Project Member

Comment 24 by sheriffbot@chromium.org, Feb 13 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
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
Labels: -Hotlist-Merge-Review -Merge-Review-65 Merge-Approved-65
Thanks Bernie! 

Hi Sonny, this CL has been cherry-picked to 4 branches of R65. PTAL. Thanks!
Project Member

Comment 27 by sheriffbot@chromium.org, Feb 26 2018

Cc: bhthompson@google.com
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
Project Member

Comment 28 by bugdroid1@chromium.org, Feb 27 2018

Labels: merge-merged-release-R65-10323.B-chromeos-3.10
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

Project Member

Comment 29 by bugdroid1@chromium.org, Feb 27 2018

Labels: merge-merged-release-R65-10323.B-chromeos-3.18
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

Project Member

Comment 30 by bugdroid1@chromium.org, Feb 27 2018

Labels: merge-merged-release-R65-10323.B-chromeos-4.4
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

Project Member

Comment 31 by bugdroid1@chromium.org, Feb 27 2018

Labels: merge-merged-release-R65-10323.B-chromeos-3.14
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

Labels: -Merge-Approved-65
Status: Fixed (was: Started)
The patch has been merged to R65. Marked as fixed. Thanks.

Sign in to add a comment