| Message ID | 20210405203602.17151-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [resend] Bluetooth: hci_h5: Disable the hci_suspend_notifier for btrtl devices | expand |
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 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 27e96681d583..d79b7bbe6d94 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->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 9e03402ef1b3..113045e98c19 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 not really happy using these values here. They are for the ioctl API. Any chance you can just use hu->flags for this? Regards Marcel
Hi Marcel, On 4/9/21 3:42 PM, Marcel Holtmann wrote: > 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 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 27e96681d583..d79b7bbe6d94 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->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 9e03402ef1b3..113045e98c19 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 > > not really happy using these values here. They are for the ioctl API. Any chance you can just use hu->flags for this? I see no reason why that would not work. I'll prepare (and test) a v2 with this change. Regards, Hans
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c index 27e96681d583..d79b7bbe6d94 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->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 9e03402ef1b3..113045e98c19 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(-)