| Message ID | 20210128163313.426404-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
Hi, On Thu, Jan 28, 2021 at 8:34 AM Hans de Goede <hdegoede@redhat.com> wrote: > > 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 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 | 13 +++++++------ > 3 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c > index 7be16a7f653b..e8f3afab6587 100644 > --- a/drivers/bluetooth/hci_h5.c > +++ b/drivers/bluetooth/hci_h5.c > @@ -914,6 +914,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->hdev_flags); I'm not sure we should enable this for all RTL devices rather than based on the specific project. RTL8822C will also be using hci_h5 but intends to support wake on bt (meaning it shouldn't be losing firmware around suspend). + Max Chou: You are proposing a change to add project id to btrtl. Should we use that instead to set this quirk for 8723 devices (and others which lose fw around suspend)? (https://patchwork.kernel.org/project/bluetooth/patch/20210127030152.3940-1-max.chou@realtek.com/) > + > /* 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 ef96ad06fa54..dbc14b8ac477 100644 > --- a/drivers/bluetooth/hci_serdev.c > +++ b/drivers/bluetooth/hci_serdev.c > @@ -349,6 +349,9 @@ int hci_uart_register_device(struct hci_uart *hu, > if (test_bit(HCI_UART_EXT_CONFIG, &hu->hdev_flags)) > set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); > > + if (test_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &hu->hdev_flags)) > + set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks); > + > if (test_bit(HCI_UART_CREATE_AMP, &hu->hdev_flags)) > hdev->dev_type = HCI_AMP; > else > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h > index 4e039d7a16f8..4df2330ac103 100644 > --- a/drivers/bluetooth/hci_uart.h > +++ b/drivers/bluetooth/hci_uart.h > @@ -35,12 +35,13 @@ > #define HCI_UART_NOKIA 10 > #define HCI_UART_MRVL 11 > > -#define HCI_UART_RAW_DEVICE 0 > -#define HCI_UART_RESET_ON_INIT 1 > -#define HCI_UART_CREATE_AMP 2 > -#define HCI_UART_INIT_PENDING 3 > -#define HCI_UART_EXT_CONFIG 4 > -#define HCI_UART_VND_DETECT 5 > +#define HCI_UART_RAW_DEVICE 0 > +#define HCI_UART_RESET_ON_INIT 1 > +#define HCI_UART_CREATE_AMP 2 > +#define HCI_UART_INIT_PENDING 3 > +#define HCI_UART_EXT_CONFIG 4 > +#define HCI_UART_VND_DETECT 5 > +#define HCI_UART_NO_SUSPEND_NOTIFIER 6 > > struct hci_uart; > struct serdev_device; > -- > 2.29.2 >
Hi, On Fri, Jan 29, 2021 at 6:28 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 1/29/21 9:25 AM, Max Chou wrote: > > Hi, > > > >> I'm not sure we should enable this for all RTL devices rather than based on the specific project. RTL8822C will also be using hci_h5 but intends to support wake on bt (meaning it shouldn't be losing firmware around suspend). > > > >> + Max Chou: You are proposing a change to add project id to btrtl. > >> Should we use that instead to set this quirk for 8723 devices (and others which lose fw around suspend)? > >> (https://patchwork.kernel.org/project/bluetooth/patch/20210127030152.3940-1-max.chou@realtek.com/) > > > > Agree. Recommend to use the same way as Abhishek mentions that limiting the quirk only for RTL8723B devices if this patch is necessary. > > Therefore, some of the projects use RTL8822C devices with BT UART interface would apply BT wakes Host. > > So far, I've not heard the issue as this topic. > > So I just checked because I was not aware that the hci_h5 code was also being used for the RTL8822C. > I mainly focus on x86/ACPI use and there is no 8822 ACPI device-id in the h5_acpi_match table, but > there is indeed a match for this in the rtl_bluetooth_of_match table. > > But ATM the code is treating the 8822 exactly the same as the 8723, including doing a device_reprobe > on resume. So to me it makes sense to set the HCI_UART_NO_SUSPEND_NOTIFIER unconditionally to, as > it is used because of the device_reprobe being done. > > Now it might be a good idea to opt out of the device_reprobe for 8822 devices, and/or maybe even for > all devicetree enumerated cases (the device being completely shutoff is an ACPI thing, with dt/of we > should have more control). > > To me it seems that since for now the device_reprobe() is unconditional that the matching setting > of the HCI_UART_NO_SUSPEND_NOTIFIER flag should be unconditional too; and then when the device_reprobe() > stuff is made unconditional, then we can make the setting of the flag unconditional using the same check. I didn't realize this was currently unconditional in code. In that case, I think it's fine for you to add the flag unconditionally in hci_h5. When we add support for wake on bt to the h5 driver, we should make this flag conditional based on whether the driver reprobes on resume. I will go back and +1 the original patch. Thanks, Abhishek > > With that said if people really want it I'm happy to respin this to only apply to the 8723 case, > but that seems weird given that the device_reprobe ATM is being done on the 8822 too. > > > +Clair: Have you met this issue during suspend/resume when BT controller is RTL8822C with BT UART interface? > > Please see the patchwork. https://patchwork.kernel.org/project/bluetooth/list/?series=423915 > > Regards, > > Hans >
Hi, On Thu, Jan 28, 2021 at 8:34 AM Hans de Goede <hdegoede@redhat.com> wrote: > > 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> > --- Per the other conversation thread, since the h5 driver unconditionally reprobes on resume, this change looks good as-is. Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c index 7be16a7f653b..e8f3afab6587 100644 --- a/drivers/bluetooth/hci_h5.c +++ b/drivers/bluetooth/hci_h5.c @@ -914,6 +914,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->hdev_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 ef96ad06fa54..dbc14b8ac477 100644 --- a/drivers/bluetooth/hci_serdev.c +++ b/drivers/bluetooth/hci_serdev.c @@ -349,6 +349,9 @@ int hci_uart_register_device(struct hci_uart *hu, if (test_bit(HCI_UART_EXT_CONFIG, &hu->hdev_flags)) set_bit(HCI_QUIRK_EXTERNAL_CONFIG, &hdev->quirks); + if (test_bit(HCI_UART_NO_SUSPEND_NOTIFIER, &hu->hdev_flags)) + set_bit(HCI_QUIRK_NO_SUSPEND_NOTIFIER, &hdev->quirks); + if (test_bit(HCI_UART_CREATE_AMP, &hu->hdev_flags)) hdev->dev_type = HCI_AMP; else diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h index 4e039d7a16f8..4df2330ac103 100644 --- a/drivers/bluetooth/hci_uart.h +++ b/drivers/bluetooth/hci_uart.h @@ -35,12 +35,13 @@ #define HCI_UART_NOKIA 10 #define HCI_UART_MRVL 11 -#define HCI_UART_RAW_DEVICE 0 -#define HCI_UART_RESET_ON_INIT 1 -#define HCI_UART_CREATE_AMP 2 -#define HCI_UART_INIT_PENDING 3 -#define HCI_UART_EXT_CONFIG 4 -#define HCI_UART_VND_DETECT 5 +#define HCI_UART_RAW_DEVICE 0 +#define HCI_UART_RESET_ON_INIT 1 +#define HCI_UART_CREATE_AMP 2 +#define HCI_UART_INIT_PENDING 3 +#define HCI_UART_EXT_CONFIG 4 +#define HCI_UART_VND_DETECT 5 +#define HCI_UART_NO_SUSPEND_NOTIFIER 6 struct hci_uart; struct serdev_device;
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 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 | 13 +++++++------ 3 files changed, 17 insertions(+), 6 deletions(-)