| Message ID | 20210629195907.64769-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] Bluetooth: hci_h5: Disable the hci_suspend_notifier for btrtl devices | expand |
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series: PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=508781 ---Test result--- Test Summary: CheckPatch PASS 0.73 seconds GitLint PASS 0.10 seconds BuildKernel PASS 512.79 seconds TestRunner: Setup PASS 336.44 seconds TestRunner: l2cap-tester PASS 2.46 seconds TestRunner: bnep-tester PASS 1.88 seconds TestRunner: mgmt-tester FAIL 31.08 seconds TestRunner: rfcomm-tester PASS 2.05 seconds TestRunner: sco-tester PASS 2.01 seconds TestRunner: smp-tester PASS 2.07 seconds TestRunner: userchan-tester PASS 1.90 seconds Details ############################## Test: CheckPatch - PASS - 0.73 seconds Run checkpatch.pl script with rule in .checkpatch.conf ############################## Test: GitLint - PASS - 0.10 seconds Run gitlint with rule in .gitlint ############################## Test: BuildKernel - PASS - 512.79 seconds Build Kernel with minimal configuration supports Bluetooth ############################## Test: TestRunner: Setup - PASS - 336.44 seconds Setup environment for running Test Runner ############################## Test: TestRunner: l2cap-tester - PASS - 2.46 seconds Run test-runner with l2cap-tester Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: bnep-tester - PASS - 1.88 seconds Run test-runner with bnep-tester Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: mgmt-tester - FAIL - 31.08 seconds Run test-runner with mgmt-tester Total: 446, Passed: 436 (97.8%), Failed: 5, Not Run: 5 Failed Test Cases Read Ext Controller Info 1 Failed 0.014 seconds Read Ext Controller Info 2 Failed 0.020 seconds Read Ext Controller Info 3 Failed 0.019 seconds Read Ext Controller Info 4 Failed 0.016 seconds Read Ext Controller Info 5 Failed 0.023 seconds ############################## Test: TestRunner: rfcomm-tester - PASS - 2.05 seconds Run test-runner with rfcomm-tester Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: sco-tester - PASS - 2.01 seconds Run test-runner with sco-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: smp-tester - PASS - 2.07 seconds Run test-runner with smp-tester Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0 ############################## Test: TestRunner: userchan-tester - PASS - 1.90 seconds Run test-runner with userchan-tester Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0 --- Regards, Linux Bluetooth
Hi Hans, > The hci_suspend_notifier which was introduced last year, is causing > problems for uart attached btrtl devices. These devices may loose their > firmware and their baudrate setting over a suspend/resume. > > Since we don't even know the baudrate after a suspend/resume recovering > from this is tricky. The driver solves this by treating these devices > the same as USB BT HCIs which drop of the bus during suspend. > > Specifically the driver: > 1. Simply unconditionally turns the device fully off during > system-suspend to save maximum power. > 2. Calls device_reprobe() from a workqueue to fully re-init the device > from scratch on system-resume (unregistering the old HCI and > registering a new HCI). > > This means that these devices do not benefit from the suspend / resume > handling work done by the hci_suspend_notifier. At best this unnecessarily > adds some time to the suspend/resume time. > > But in practice this is actually causing problems: > > 1. These btrtl devices seem to not like the HCI_OP_WRITE_SCAN_ENABLE( > SCAN_DISABLED) request being send to them when entering the > BT_SUSPEND_CONFIGURE_WAKE state. The same request send on > BT_SUSPEND_DISCONNECT works fine, but the second one send (unnecessarily?) > from the BT_SUSPEND_CONFIGURE_WAKE transition causes the device to hang: > > [ 573.497754] PM: suspend entry (s2idle) > [ 573.554615] Filesystems sync: 0.056 seconds > [ 575.837753] Bluetooth: hci0: Timed out waiting for suspend events > [ 575.837801] Bluetooth: hci0: Suspend timeout bit: 4 > [ 575.837925] Bluetooth: hci0: Suspend notifier action (3) failed: -110 > > 2. The PM_POST_SUSPEND / BT_RUNNING transition races with the > driver-unbinding done by the device_reprobe() work. > If the hci_suspend_notifier wins the race it is talking to a dead > device leading to the following errors being logged: > > [ 598.686060] Bluetooth: hci0: Timed out waiting for suspend events > [ 598.686124] Bluetooth: hci0: Suspend timeout bit: 5 > [ 598.686237] Bluetooth: hci0: Suspend notifier action (4) failed: -110 > > In both cases things still work, but the suspend-notifier is causing > these ugly errors getting logged and ut increase both the suspend- and > the resume-time by 2 seconds. > > This commit avoids these problems by disabling the hci_suspend_notifier. > > Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> > Cc: Vasily Khoruzhick <anarsoul@gmail.com> > Cc: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v3: > - Use hu->flags instead of hu->hdev_flags to store the > HCI_UART_NO_SUSPEND_NOTIFIER flag > > Changes in v2: > - Use the new HCI_QUIRK_NO_SUSPEND_NOTIFIER quirk, instead of directly > unregistering the notifier from hci_h5.c > --- > drivers/bluetooth/hci_h5.c | 7 +++++++ > drivers/bluetooth/hci_serdev.c | 3 +++ > drivers/bluetooth/hci_uart.h | 7 ++++--- > 3 files changed, 14 insertions(+), 3 deletions(-) patch has been applied to bluetooth-next tree. Regards Marcel
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c index 27e96681d583..c283504ec3b3 100644 --- a/drivers/bluetooth/hci_h5.c +++ b/drivers/bluetooth/hci_h5.c @@ -919,6 +919,13 @@ static int h5_btrtl_setup(struct h5 *h5) static void h5_btrtl_open(struct h5 *h5) { + /* + * Since h5_btrtl_resume() does a device_reprobe() the suspend handling + * done by the hci_suspend_notifier is not necessary; it actually causes + * delays and a bunch of errors to get logged, so disable it. + */ + set_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &h5->hu->flags); + /* Devices always start with these fixed parameters */ serdev_device_set_flow_control(h5->hu->serdev, false); serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN); diff --git a/drivers/bluetooth/hci_serdev.c b/drivers/bluetooth/hci_serdev.c index 9e03402ef1b3..3b00d82d36cf 100644 --- a/drivers/bluetooth/hci_serdev.c +++ b/drivers/bluetooth/hci_serdev.c @@ -343,6 +343,9 @@ int hci_uart_register_device(struct hci_uart *hu, hdev->setup = hci_uart_setup; SET_HCIDEV_DEV(hdev, &hu->serdev->dev); + if (test_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &hu->flags)) + set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks); + if (test_bit(HCI_UART_RAW_DEVICE, &hu->hdev_flags)) set_bit(HCI_QUIRK_RAW_DEVICE, &hdev->quirks); diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index 4e039d7a16f8..fb4a2d0d8cc8 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -86,9 +86,10 @@ struct hci_uart { }; /* HCI_UART proto flag bits */ -#define HCI_UART_PROTO_SET 0 -#define HCI_UART_REGISTERED 1 -#define HCI_UART_PROTO_READY 2 +#define HCI_UART_PROTO_SET 0 +#define HCI_UART_REGISTERED 1 +#define HCI_UART_PROTO_READY 2 +#define HCI_UART_NO_SUSPEND_NOTIFIER 3 /* TX states */ #define HCI_UART_SENDING 1
The hci_suspend_notifier which was introduced last year, is causing problems for uart attached btrtl devices. These devices may loose their firmware and their baudrate setting over a suspend/resume. Since we don't even know the baudrate after a suspend/resume recovering from this is tricky. The driver solves this by treating these devices the same as USB BT HCIs which drop of the bus during suspend. Specifically the driver: 1. Simply unconditionally turns the device fully off during system-suspend to save maximum power. 2. Calls device_reprobe() from a workqueue to fully re-init the device from scratch on system-resume (unregistering the old HCI and registering a new HCI). This means that these devices do not benefit from the suspend / resume handling work done by the hci_suspend_notifier. At best this unnecessarily adds some time to the suspend/resume time. But in practice this is actually causing problems: 1. These btrtl devices seem to not like the HCI_OP_WRITE_SCAN_ENABLE( SCAN_DISABLED) request being send to them when entering the BT_SUSPEND_CONFIGURE_WAKE state. The same request send on BT_SUSPEND_DISCONNECT works fine, but the second one send (unnecessarily?) from the BT_SUSPEND_CONFIGURE_WAKE transition causes the device to hang: [ 573.497754] PM: suspend entry (s2idle) [ 573.554615] Filesystems sync: 0.056 seconds [ 575.837753] Bluetooth: hci0: Timed out waiting for suspend events [ 575.837801] Bluetooth: hci0: Suspend timeout bit: 4 [ 575.837925] Bluetooth: hci0: Suspend notifier action (3) failed: -110 2. The PM_POST_SUSPEND / BT_RUNNING transition races with the driver-unbinding done by the device_reprobe() work. If the hci_suspend_notifier wins the race it is talking to a dead device leading to the following errors being logged: [ 598.686060] Bluetooth: hci0: Timed out waiting for suspend events [ 598.686124] Bluetooth: hci0: Suspend timeout bit: 5 [ 598.686237] Bluetooth: hci0: Suspend notifier action (4) failed: -110 In both cases things still work, but the suspend-notifier is causing these ugly errors getting logged and ut increase both the suspend- and the resume-time by 2 seconds. This commit avoids these problems by disabling the hci_suspend_notifier. Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Cc: Vasily Khoruzhick <anarsoul@gmail.com> Cc: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v3: - Use hu->flags instead of hu->hdev_flags to store the HCI_UART_NO_SUSPEND_NOTIFIER flag Changes in v2: - Use the new HCI_QUIRK_NO_SUSPEND_NOTIFIER quirk, instead of directly unregistering the notifier from hci_h5.c --- drivers/bluetooth/hci_h5.c | 7 +++++++ drivers/bluetooth/hci_serdev.c | 3 +++ drivers/bluetooth/hci_uart.h | 7 ++++--- 3 files changed, 14 insertions(+), 3 deletions(-)