* [PATCH v2] Bluetooth: Allow suspend even when preparation has failed
@ 2020-06-05 4:28 Abhishek Pandit-Subedi
2020-06-05 11:53 ` kernel test robot
2020-06-05 20:44 ` Jakub Kicinski
0 siblings, 2 replies; 3+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-05 4:28 UTC (permalink / raw)
To: marcel, linux-bluetooth
Cc: len.brown, chromeos-bluetooth-upstreaming, linux-pm, rafael,
todd.e.brandt, rui.zhang, Abhishek Pandit-Subedi,
David S. Miller, Johan Hedberg, netdev, linux-kernel,
Jakub Kicinski
It is preferable to allow suspend even when Bluetooth has problems
preparing for sleep. When Bluetooth fails to finish preparing for
suspend, log the error and allow the suspend notifier to continue
instead.
To also make it clearer why suspend failed, change bt_dev_dbg to
bt_dev_err when handling the suspend timeout.
Fixes: dd522a7429b07e ("Bluetooth: Handle LE devices during suspend")
Reported-by: Len Brown <len.brown@intel.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
To verify this is properly working, I added an additional change to
hci_suspend_wait_event to always return -16. This validates that suspend
continues even when an error has occurred during the suspend
preparation.
Example on Chromebook:
[ 55.834524] PM: Syncing filesystems ... done.
[ 55.841930] PM: Preparing system for sleep (s2idle)
[ 55.940492] Bluetooth: hci_core.c:hci_suspend_notifier() hci0: Suspend notifier action (3) failed: -16
[ 55.940497] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 55.941692] OOM killer disabled.
[ 55.941693] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
[ 55.942632] PM: Suspending system (s2idle)
I ran this through a suspend_stress_test in the following scenarios:
* Peer classic device connected: 50+ suspends
* No devices connected: 100 suspends
* With the above test case returning -EBUSY: 50 suspends
I also ran this through our automated testing for suspend and wake on
BT from suspend continues to work.
Changes in v2:
- Added fixes and reported-by tags
net/bluetooth/hci_core.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbe2d79f233fba..54da48441423e0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3289,10 +3289,10 @@ static int hci_suspend_wait_event(struct hci_dev *hdev)
WAKE_COND, SUSPEND_NOTIFIER_TIMEOUT);
if (ret == 0) {
- bt_dev_dbg(hdev, "Timed out waiting for suspend");
+ bt_dev_err(hdev, "Timed out waiting for suspend events");
for (i = 0; i < __SUSPEND_NUM_TASKS; ++i) {
if (test_bit(i, hdev->suspend_tasks))
- bt_dev_dbg(hdev, "Bit %d is set", i);
+ bt_dev_err(hdev, "Suspend timeout bit: %d", i);
clear_bit(i, hdev->suspend_tasks);
}
@@ -3360,12 +3360,15 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
ret = hci_change_suspend_state(hdev, BT_RUNNING);
}
- /* If suspend failed, restore it to running */
- if (ret && action == PM_SUSPEND_PREPARE)
- hci_change_suspend_state(hdev, BT_RUNNING);
-
done:
- return ret ? notifier_from_errno(-EBUSY) : NOTIFY_STOP;
+ /* We always allow suspend even if suspend preparation failed and
+ * attempt to recover in resume.
+ */
+ if (ret)
+ bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
+ action, ret);
+
+ return NOTIFY_STOP;
}
/* Alloc HCI device */
--
2.27.0.278.ge193c7cf3a9-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Bluetooth: Allow suspend even when preparation has failed
2020-06-05 4:28 [PATCH v2] Bluetooth: Allow suspend even when preparation has failed Abhishek Pandit-Subedi
@ 2020-06-05 11:53 ` kernel test robot
2020-06-05 20:44 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: kernel test robot @ 2020-06-05 11:53 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]
Hi Abhishek,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on net-next/master sparc-next/master net/master v5.7 next-20200605]
[cannot apply to bluetooth/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Abhishek-Pandit-Subedi/Bluetooth-Allow-suspend-even-when-preparation-has-failed/20200605-123125
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ac47588bc4ff5927a01ed6fcd269ce86aba52a7c)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> net/bluetooth/hci_core.c:3369:7: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
action, ret);
^~~~~~
include/net/bluetooth/bluetooth.h:182:37: note: expanded from macro 'bt_dev_err'
BT_ERR("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
include/net/bluetooth/bluetooth.h:169:45: note: expanded from macro 'BT_ERR'
#define BT_ERR(fmt, ...) bt_err(fmt "n", ##__VA_ARGS__)
~~~ ^~~~~~~~~~~
1 warning generated.
vim +3369 net/bluetooth/hci_core.c
3325
3326 static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
3327 void *data)
3328 {
3329 struct hci_dev *hdev =
3330 container_of(nb, struct hci_dev, suspend_notifier);
3331 int ret = 0;
3332
3333 /* If powering down, wait for completion. */
3334 if (mgmt_powering_down(hdev)) {
3335 set_bit(SUSPEND_POWERING_DOWN, hdev->suspend_tasks);
3336 ret = hci_suspend_wait_event(hdev);
3337 if (ret)
3338 goto done;
3339 }
3340
3341 /* Suspend notifier should only act on events when powered. */
3342 if (!hdev_is_powered(hdev))
3343 goto done;
3344
3345 if (action == PM_SUSPEND_PREPARE) {
3346 /* Suspend consists of two actions:
3347 * - First, disconnect everything and make the controller not
3348 * connectable (disabling scanning)
3349 * - Second, program event filter/whitelist and enable scan
3350 */
3351 ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
3352
3353 /* Only configure whitelist if disconnect succeeded and wake
3354 * isn't being prevented.
3355 */
3356 if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev)))
3357 ret = hci_change_suspend_state(hdev,
3358 BT_SUSPEND_CONFIGURE_WAKE);
3359 } else if (action == PM_POST_SUSPEND) {
3360 ret = hci_change_suspend_state(hdev, BT_RUNNING);
3361 }
3362
3363 done:
3364 /* We always allow suspend even if suspend preparation failed and
3365 * attempt to recover in resume.
3366 */
3367 if (ret)
3368 bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
> 3369 action, ret);
3370
3371 return NOTIFY_STOP;
3372 }
3373
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 73519 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] Bluetooth: Allow suspend even when preparation has failed
2020-06-05 4:28 [PATCH v2] Bluetooth: Allow suspend even when preparation has failed Abhishek Pandit-Subedi
2020-06-05 11:53 ` kernel test robot
@ 2020-06-05 20:44 ` Jakub Kicinski
1 sibling, 0 replies; 3+ messages in thread
From: Jakub Kicinski @ 2020-06-05 20:44 UTC (permalink / raw)
To: Abhishek Pandit-Subedi
Cc: marcel, linux-bluetooth, len.brown,
chromeos-bluetooth-upstreaming, linux-pm, rafael, todd.e.brandt,
rui.zhang, David S. Miller, Johan Hedberg, netdev, linux-kernel
On Thu, 4 Jun 2020 21:28:50 -0700 Abhishek Pandit-Subedi wrote:
> It is preferable to allow suspend even when Bluetooth has problems
> preparing for sleep. When Bluetooth fails to finish preparing for
> suspend, log the error and allow the suspend notifier to continue
> instead.
>
> To also make it clearer why suspend failed, change bt_dev_dbg to
> bt_dev_err when handling the suspend timeout.
>
> Fixes: dd522a7429b07e ("Bluetooth: Handle LE devices during suspend")
> Reported-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> To verify this is properly working, I added an additional change to
> hci_suspend_wait_event to always return -16. This validates that suspend
> continues even when an error has occurred during the suspend
> preparation.
>
> Example on Chromebook:
> [ 55.834524] PM: Syncing filesystems ... done.
> [ 55.841930] PM: Preparing system for sleep (s2idle)
> [ 55.940492] Bluetooth: hci_core.c:hci_suspend_notifier() hci0: Suspend notifier action (3) failed: -16
> [ 55.940497] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [ 55.941692] OOM killer disabled.
> [ 55.941693] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> [ 55.942632] PM: Suspending system (s2idle)
>
> I ran this through a suspend_stress_test in the following scenarios:
> * Peer classic device connected: 50+ suspends
> * No devices connected: 100 suspends
> * With the above test case returning -EBUSY: 50 suspends
>
> I also ran this through our automated testing for suspend and wake on
> BT from suspend continues to work.
>
>
> Changes in v2:
> - Added fixes and reported-by tags
Building W=1 C=1 gcc-10:
In file included from ../net/bluetooth/hci_core.c:38:
../net/bluetooth/hci_core.c: In function ‘hci_suspend_notifier’:
../include/net/bluetooth/bluetooth.h:182:9: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 3 has type ‘long unsigned int’ [-Wformat=]
182 | BT_ERR("%s: " fmt, (hdev)->name, ##__VA_ARGS__)
| ^~~~~~
../include/net/bluetooth/bluetooth.h:169:33: note: in definition of macro ‘BT_ERR’
169 | #define BT_ERR(fmt, ...) bt_err(fmt "\n", ##__VA_ARGS__)
| ^~~
../net/bluetooth/hci_core.c:3368:3: note: in expansion of macro ‘bt_dev_err’
3368 | bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
| ^~~~~~~~~~
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-05 20:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-05 4:28 [PATCH v2] Bluetooth: Allow suspend even when preparation has failed Abhishek Pandit-Subedi
2020-06-05 11:53 ` kernel test robot
2020-06-05 20:44 ` Jakub Kicinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.