[PATCH 0/2] Reset realtek bluetooth devices during user suspend

[PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Abhishek Pandit-Subedi]

On a Realtek USB bluetooth device, I wanted a simple and consistent way
to put the device in reset during suspend (2 reasons: to save power and
disable BT as a wakeup source). Resetting it in the suspend callback
causes a detach and the resume callback is not called. Hence the changes
in this series to do the reset in suspend_noirq.

I looked into using PERSIST and reset on resume but those seem mainly
for misbehaving devices that reset themselves.

This patch series has been tested with Realtek BT hardware as well as
Intel BT (test procedure = disable as wake source, user suspend and
observe a detach + reattach on resume).



Abhishek Pandit-Subedi (2):
  usb: support suspend_noirq
  Bluetooth: btusb: Reset realtek devices on user suspend

 drivers/bluetooth/btusb.c | 26 ++++++++++++++++++
 drivers/usb/core/driver.c | 56 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/usb.c    |  6 +++++
 include/linux/pm.h        |  8 ++++++
 include/linux/usb.h       |  3 +++
 5 files changed, 99 insertions(+)

-- 
2.23.0.237.gc6a4ce50a0-goog

[PATCH 1/2] usb: support suspend_noirq

[Abhishek Pandit-Subedi]

If we put a usb device into reset in the suspend callback, it will
disconnect and the resume will not be called. In order to support
turning off the device on suspend and restoring it on resume, we must do
the reset action in suspend_noirq.

e.g. Undesirable behavior: bluetooth driver asserts reset in suspend, it
disconnects, the resume is never called

e.g. Desirable new behavior: bluetooth driver asserts reset in
suspend_noirq, device doesn't disconnect, resume is called where power
is restored and usb disconnects and reconnects device

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

 drivers/usb/core/driver.c | 56 +++++++++++++++++++++++++++++++++++++++
 drivers/usb/core/usb.c    |  6 +++++
 include/linux/pm.h        |  8 ++++++
 include/linux/usb.h       |  3 +++
 4 files changed, 73 insertions(+)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index ebcadaad89d1..a7657d4e3177 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -1207,6 +1207,26 @@ static int usb_suspend_interface(struct usb_device *udev,
 	return status;
 }
 
+/* Ignore errors during system sleep transitions so no return code */
+static void usb_suspend_interface_noirq(struct usb_device *udev,
+				       struct usb_interface *intf,
+				       pm_message_t msg)
+{
+	struct usb_driver	*driver;
+	int			status = 0;
+
+	if (udev->state == USB_STATE_NOTATTACHED ||
+			intf->condition == USB_INTERFACE_UNBOUND)
+		return;
+	driver = to_usb_driver(intf->dev.driver);
+
+	if (driver->suspend_noirq)
+		status = driver->suspend_noirq(intf, msg);
+
+	if (status)
+		dev_err(&intf->dev, "suspend_noirq error %d\n", status);
+}
+
 static int usb_resume_interface(struct usb_device *udev,
 		struct usb_interface *intf, pm_message_t msg, int reset_resume)
 {
@@ -1369,6 +1389,39 @@ static int usb_suspend_both(struct usb_device *udev, pm_message_t msg)
 	return status;
 }
 
+/**
+ * usb_suspend_noirq - additional suspend activity without interrupt handlers
+ * @udev: the usb_device to suspend
+ * @msg: Power Management message describing this state transition
+ *
+ * As a final step in suspend, we allow interfaces to take action without the
+ * interrupt handler enabled. For example, a driver may want to power down
+ * a device during suspend (to save power) and re-enable it on resume. Doing it
+ * in the noirq callback will make sure the resume runs (device doesn't get
+ * removed on suspend path) and the device can complete a reset.
+ *
+ * This routine can run only in process context.
+ *
+ * Return: 0 once complete.
+ */
+
+static int usb_suspend_noirq(struct usb_device *udev, pm_message_t msg)
+{
+	int			i = 0, n = 0;
+	struct usb_interface	*intf;
+
+	/* Run noirq callbacks on all interfaces */
+	if (udev->state != USB_STATE_NOTATTACHED && udev->actconfig) {
+		n = udev->actconfig->desc.bNumInterfaces;
+		for (i = n - 1; i >= 0; --i) {
+			intf = udev->actconfig->interface[i];
+			usb_suspend_interface_noirq(udev, intf, msg);
+		}
+	}
+
+	return 0;
+}
+
 /**
  * usb_resume_both - resume a USB device and its interfaces
  * @udev: the usb_device to resume
@@ -1455,6 +1508,9 @@ int usb_suspend(struct device *dev, pm_message_t msg)
 	struct usb_device	*udev = to_usb_device(dev);
 	int r;
 
+	if (PMSG_IS_NOIRQ(msg))
+		return usb_suspend_noirq(udev, msg);
+
 	unbind_no_pm_drivers_interfaces(udev);
 
 	/* From now on we are sure all drivers support suspend/resume
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 0ab8738047da..a236477de50a 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -465,6 +465,11 @@ static int usb_dev_suspend(struct device *dev)
 	return usb_suspend(dev, PMSG_SUSPEND);
 }
 
+static int usb_dev_suspend_noirq(struct device *dev)
+{
+	return usb_suspend(dev, PMSG_SUSPEND_NOIRQ);
+}
+
 static int usb_dev_resume(struct device *dev)
 {
 	return usb_resume(dev, PMSG_RESUME);
@@ -494,6 +499,7 @@ static const struct dev_pm_ops usb_device_pm_ops = {
 	.prepare =	usb_dev_prepare,
 	.complete =	usb_dev_complete,
 	.suspend =	usb_dev_suspend,
+	.suspend_noirq =	usb_dev_suspend_noirq,
 	.resume =	usb_dev_resume,
 	.freeze =	usb_dev_freeze,
 	.thaw =		usb_dev_thaw,
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 3619a870eaa4..9f5e7899f076 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -410,6 +410,9 @@ const struct dev_pm_ops name = { \
  *		memory contents from a hibernation image has failed, call
  *		->thaw() and ->complete() for all devices.
  *
+ * SUSPEND_NOIRQ	System is going to suspend without interrupt handlers
+ *			enabled, call ->suspend_noirq() for all devices.
+ *
  * The following PM_EVENT_ messages are defined for internal use by
  * kernel subsystems.  They are never issued by the PM core.
  *
@@ -439,6 +442,7 @@ const struct dev_pm_ops name = { \
 #define PM_EVENT_USER		0x0100
 #define PM_EVENT_REMOTE		0x0200
 #define PM_EVENT_AUTO		0x0400
+#define PM_EVENT_NOIRQ		0x0800
 
 #define PM_EVENT_SLEEP		(PM_EVENT_SUSPEND | PM_EVENT_HIBERNATE)
 #define PM_EVENT_USER_SUSPEND	(PM_EVENT_USER | PM_EVENT_SUSPEND)
@@ -446,6 +450,7 @@ const struct dev_pm_ops name = { \
 #define PM_EVENT_REMOTE_RESUME	(PM_EVENT_REMOTE | PM_EVENT_RESUME)
 #define PM_EVENT_AUTO_SUSPEND	(PM_EVENT_AUTO | PM_EVENT_SUSPEND)
 #define PM_EVENT_AUTO_RESUME	(PM_EVENT_AUTO | PM_EVENT_RESUME)
+#define PM_EVENT_SUSPEND_NOIRQ	(PM_EVENT_NOIRQ | PM_EVENT_SUSPEND)
 
 #define PMSG_INVALID	((struct pm_message){ .event = PM_EVENT_INVALID, })
 #define PMSG_ON		((struct pm_message){ .event = PM_EVENT_ON, })
@@ -457,6 +462,8 @@ const struct dev_pm_ops name = { \
 #define PMSG_THAW	((struct pm_message){ .event = PM_EVENT_THAW, })
 #define PMSG_RESTORE	((struct pm_message){ .event = PM_EVENT_RESTORE, })
 #define PMSG_RECOVER	((struct pm_message){ .event = PM_EVENT_RECOVER, })
+#define PMSG_SUSPEND_NOIRQ	((struct pm_message){ \
+					.event = PM_EVENT_SUSPEND_NOIRQ, })
 #define PMSG_USER_SUSPEND	((struct pm_message) \
 					{ .event = PM_EVENT_USER_SUSPEND, })
 #define PMSG_USER_RESUME	((struct pm_message) \
@@ -469,6 +476,7 @@ const struct dev_pm_ops name = { \
 					{ .event = PM_EVENT_AUTO_RESUME, })
 
 #define PMSG_IS_AUTO(msg)	(((msg).event & PM_EVENT_AUTO) != 0)
+#define PMSG_IS_NOIRQ(msg)	(((msg).event & PM_EVENT_NOIRQ) != 0)
 
 /*
  * Device run-time power management status.
diff --git a/include/linux/usb.h b/include/linux/usb.h
index e87826e23d59..0e2079661ae6 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1139,6 +1139,8 @@ struct usbdrv_wrap {
  *	try to continue using the device if suspend fails in this case.
  *	Instead, let the resume or reset-resume routine recover from
  *	the failure.
+ * @suspend_noirq: Similar to suspend() but called with the usb interrupt
+ *	handler disabled.
  * @resume: Called when the device is being resumed by the system.
  * @reset_resume: Called when the suspended device has been reset instead
  *	of being resumed.
@@ -1191,6 +1193,7 @@ struct usb_driver {
 			void *buf);
 
 	int (*suspend) (struct usb_interface *intf, pm_message_t message);
+	int (*suspend_noirq)(struct usb_interface *intf, pm_message_t message);
 	int (*resume) (struct usb_interface *intf);
 	int (*reset_resume)(struct usb_interface *intf);
 
-- 
2.23.0.237.gc6a4ce50a0-goog

Typo error in bluez mamagement API

[Jean-Pierre Garcia]

Found typo error in 
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/mgmt-api.txt,In 
Set Appearance Command, command code is 0x0042, should be 0x0043.

-

Re: [PATCH 1/2] usb: support suspend_noirq

[Greg Kroah-Hartman]

On Tue, Sep 17, 2019 at 02:27:01PM -0700, Abhishek Pandit-Subedi wrote:
> If we put a usb device into reset in the suspend callback, it will
> disconnect and the resume will not be called. In order to support
> turning off the device on suspend and restoring it on resume, we must do
> the reset action in suspend_noirq.

That's a bit odd given that USB drivers/devices _require_ irqs to be
enabled in order to talk to them and work properly.

So I don't see how this works at all, nor how any USB driver could even
use this.

greg k-h

[PATCH 2/2] Bluetooth: btusb: Reset realtek devices on user suspend

[Abhishek Pandit-Subedi]

Reset realtek devices on user suspend if not configured as a wakeup source.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---

 drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index a9c35ebb30f8..ce3a10642f4e 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -493,6 +493,8 @@ struct btusb_data {
 
 	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
 	unsigned cmd_timeout_cnt;
+
+	bool reset_on_suspend; /* reset on suspend if not a wakeup source */
 };
 
 
@@ -3818,6 +3820,8 @@ static int btusb_probe(struct usb_interface *intf,
 		 * (DEVICE_REMOTE_WAKEUP)
 		 */
 		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
+		/* We'll explicitly reset the device around user suspend too. */
+		data->reset_on_suspend = true;
 	}
 #endif
 
@@ -4007,6 +4011,22 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
 	return 0;
 }
 
+static int btusb_suspend_noirq(struct usb_interface *intf, pm_message_t message)
+{
+	struct btusb_data *data = usb_get_intfdata(intf);
+
+	BT_DBG("suspend_noirq %p", intf);
+
+	/* Only reset if not configured for wakeup */
+	if (!device_may_wakeup(&data->udev->dev) &&
+	    data->reset_on_suspend && data->reset_gpio) {
+		BT_DBG("resetting in suspend_noirq\n");
+		gpiod_set_value_cansleep(data->reset_gpio, 1);
+	}
+
+	return 0;
+}
+
 static void play_deferred(struct btusb_data *data)
 {
 	struct urb *urb;
@@ -4048,6 +4068,11 @@ static int btusb_resume(struct usb_interface *intf)
 	if (--data->suspend_count)
 		return 0;
 
+	if (data->reset_on_suspend && data->reset_gpio) {
+		BT_DBG("restoring reset in resume\n");
+		gpiod_set_value_cansleep(data->reset_gpio, 0);
+	}
+
 	/* Disable only if not already disabled (keep it balanced) */
 	if (test_and_clear_bit(BTUSB_OOB_WAKE_ENABLED, &data->flags)) {
 		disable_irq(data->oob_wake_irq);
@@ -4107,6 +4132,7 @@ static struct usb_driver btusb_driver = {
 	.disconnect	= btusb_disconnect,
 #ifdef CONFIG_PM
 	.suspend	= btusb_suspend,
+	.suspend_noirq	= btusb_suspend_noirq,
 	.resume		= btusb_resume,
 #endif
 	.id_table	= btusb_table,
-- 
2.23.0.237.gc6a4ce50a0-goog

Re: [PATCH 2/2] Bluetooth: btusb: Reset realtek devices on user suspend

[Greg KH]

On Tue, Sep 17, 2019 at 02:27:02PM -0700, Abhishek Pandit-Subedi wrote:
> Reset realtek devices on user suspend if not configured as a wakeup source.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
> 
>  drivers/bluetooth/btusb.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index a9c35ebb30f8..ce3a10642f4e 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -493,6 +493,8 @@ struct btusb_data {
>  
>  	int oob_wake_irq;   /* irq for out-of-band wake-on-bt */
>  	unsigned cmd_timeout_cnt;
> +
> +	bool reset_on_suspend; /* reset on suspend if not a wakeup source */
>  };
>  
>  
> @@ -3818,6 +3820,8 @@ static int btusb_probe(struct usb_interface *intf,
>  		 * (DEVICE_REMOTE_WAKEUP)
>  		 */
>  		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
> +		/* We'll explicitly reset the device around user suspend too. */
> +		data->reset_on_suspend = true;
>  	}
>  #endif
>  
> @@ -4007,6 +4011,22 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>  	return 0;
>  }
>  
> +static int btusb_suspend_noirq(struct usb_interface *intf, pm_message_t message)
> +{
> +	struct btusb_data *data = usb_get_intfdata(intf);
> +
> +	BT_DBG("suspend_noirq %p", intf);
> +
> +	/* Only reset if not configured for wakeup */
> +	if (!device_may_wakeup(&data->udev->dev) &&
> +	    data->reset_on_suspend && data->reset_gpio) {
> +		BT_DBG("resetting in suspend_noirq\n");
> +		gpiod_set_value_cansleep(data->reset_gpio, 1);

That's a totally out-of-band message for a USB driver to do.  Why can it
suddenly set a GPIO pin and not talk to the USB device here?

If this is a GPIO driver, then why not get the suspend message from that
subsystem?


greg k-h

Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Alan Stern]

On Tue, 17 Sep 2019, Abhishek Pandit-Subedi wrote:

> On a Realtek USB bluetooth device, I wanted a simple and consistent way
> to put the device in reset during suspend (2 reasons: to save power and
> disable BT as a wakeup source). Resetting it in the suspend callback
> causes a detach and the resume callback is not called. Hence the changes
> in this series to do the reset in suspend_noirq.

What about people who _want_ BT to be a wakeup source?

Why does putting the device in reset save power?  That is, a suspended
device is very strictly limited in the amount of current it's allowed
to draw from the USB bus; why should it draw significantly less when it
is reset?

> I looked into using PERSIST and reset on resume but those seem mainly
> for misbehaving devices that reset themselves.

They are, but that doesn't mean you can't use them for other things 
too.

> This patch series has been tested with Realtek BT hardware as well as
> Intel BT (test procedure = disable as wake source, user suspend and
> observe a detach + reattach on resume).

This series really seems like overkill for a single kind of device.

Is there any way to turn off the device's BT radio during suspend (if
wakeup is disabled) and then turn it back on during resume?  Wouldn't 
that accomplish what you want just as well?

Alan Stern

Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Abhishek Pandit-Subedi]

Sorry, last reply went out with HTML. Re-sending in plain text.

On Wed, Sep 18, 2019 at 7:23 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, 17 Sep 2019, Abhishek Pandit-Subedi wrote:
>
> > On a Realtek USB bluetooth device, I wanted a simple and consistent way
> > to put the device in reset during suspend (2 reasons: to save power and
> > disable BT as a wakeup source). Resetting it in the suspend callback
> > causes a detach and the resume callback is not called. Hence the changes
> > in this series to do the reset in suspend_noirq.
>
> What about people who _want_ BT to be a wakeup source?

When BT is enabled as a wakeup source, there is no reset.

> Why does putting the device in reset save power?  That is, a suspended
> device is very strictly limited in the amount of current it's allowed
> to draw from the USB bus; why should it draw significantly less when it
> is reset?

I don't know that it's significantly less (only that it's OFF). My
greater motivation is to make sure the bluetooth chip isn't
accumulating events while the host is turned off. Sorry, I should have
made that more clear in the cover letter.

When the host is off, it continues to accumulate events for the host
to process (packets from connected devices, LE advertisements, etc).
At some point, the firmware buffers fill up and no more events can be
stored. When the host is resumed later on, the firmware is in a bad
state and doesn't respond. I had originally just reset in ->resume but
then connected wireless devices wouldn't disconnect from the BT either
and I had trouble getting them to reconnect.

>
> > I looked into using PERSIST and reset on resume but those seem mainly
> > for misbehaving devices that reset themselves.
>
> They are, but that doesn't mean you can't use them for other things
> too.
>
> > This patch series has been tested with Realtek BT hardware as well as
> > Intel BT (test procedure = disable as wake source, user suspend and
> > observe a detach + reattach on resume).
>
> This series really seems like overkill for a single kind of device.
>
> Is there any way to turn off the device's BT radio during suspend (if
> wakeup is disabled) and then turn it back on during resume?  Wouldn't
> that accomplish what you want just as well?

Probably (but I couldn't find a way to do that). I want to prevent
bluetooth from waking up the host and to reliably be in a good state
when the host resumes. The reset logic I implemented causes the hci
device to disappear and reappear, which userspace seems to handle
gracefully.

> Alan Stern
>

Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Alan Stern]

On Wed, 18 Sep 2019, Abhishek Pandit-Subedi wrote:

> Sorry, last reply went out with HTML. Re-sending in plain text.
> 
> On Wed, Sep 18, 2019 at 7:23 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Tue, 17 Sep 2019, Abhishek Pandit-Subedi wrote:
> >
> > > On a Realtek USB bluetooth device, I wanted a simple and consistent way
> > > to put the device in reset during suspend (2 reasons: to save power and
> > > disable BT as a wakeup source). Resetting it in the suspend callback
> > > causes a detach and the resume callback is not called. Hence the changes
> > > in this series to do the reset in suspend_noirq.
> >
> > What about people who _want_ BT to be a wakeup source?
> 
> When BT is enabled as a wakeup source, there is no reset.
> 
> > Why does putting the device in reset save power?  That is, a suspended
> > device is very strictly limited in the amount of current it's allowed
> > to draw from the USB bus; why should it draw significantly less when it
> > is reset?
> 
> I don't know that it's significantly less (only that it's OFF). My
> greater motivation is to make sure the bluetooth chip isn't
> accumulating events while the host is turned off. Sorry, I should have
> made that more clear in the cover letter.
> 
> When the host is off, it continues to accumulate events for the host
> to process (packets from connected devices, LE advertisements, etc).
> At some point, the firmware buffers fill up and no more events can be
> stored. When the host is resumed later on, the firmware is in a bad
> state and doesn't respond. I had originally just reset in ->resume but
> then connected wireless devices wouldn't disconnect from the BT either
> and I had trouble getting them to reconnect.
> 
> >
> > > I looked into using PERSIST and reset on resume but those seem mainly
> > > for misbehaving devices that reset themselves.
> >
> > They are, but that doesn't mean you can't use them for other things
> > too.
> >
> > > This patch series has been tested with Realtek BT hardware as well as
> > > Intel BT (test procedure = disable as wake source, user suspend and
> > > observe a detach + reattach on resume).
> >
> > This series really seems like overkill for a single kind of device.
> >
> > Is there any way to turn off the device's BT radio during suspend (if
> > wakeup is disabled) and then turn it back on during resume?  Wouldn't
> > that accomplish what you want just as well?
> 
> Probably (but I couldn't find a way to do that).

There's no way to turn off the device's BT radio?  Then what happens 
when the user turns off Bluetooth from the wireless control panel?

>  I want to prevent
> bluetooth from waking up the host and to reliably be in a good state
> when the host resumes. The reset logic I implemented causes the hci
> device to disappear and reappear, which userspace seems to handle
> gracefully.

Have you tried out the persist/reset-on-resume approach?

Alan Stern

Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Abhishek Pandit-Subedi]

On Wed, Sep 18, 2019 at 11:51 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 18 Sep 2019, Abhishek Pandit-Subedi wrote:
>
> > Sorry, last reply went out with HTML. Re-sending in plain text.
> >
> > On Wed, Sep 18, 2019 at 7:23 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Tue, 17 Sep 2019, Abhishek Pandit-Subedi wrote:
> > >
> > > > On a Realtek USB bluetooth device, I wanted a simple and consistent way
> > > > to put the device in reset during suspend (2 reasons: to save power and
> > > > disable BT as a wakeup source). Resetting it in the suspend callback
> > > > causes a detach and the resume callback is not called. Hence the changes
> > > > in this series to do the reset in suspend_noirq.
> > >
> > > What about people who _want_ BT to be a wakeup source?
> >
> > When BT is enabled as a wakeup source, there is no reset.
> >
> > > Why does putting the device in reset save power?  That is, a suspended
> > > device is very strictly limited in the amount of current it's allowed
> > > to draw from the USB bus; why should it draw significantly less when it
> > > is reset?
> >
> > I don't know that it's significantly less (only that it's OFF). My
> > greater motivation is to make sure the bluetooth chip isn't
> > accumulating events while the host is turned off. Sorry, I should have
> > made that more clear in the cover letter.
> >
> > When the host is off, it continues to accumulate events for the host
> > to process (packets from connected devices, LE advertisements, etc).
> > At some point, the firmware buffers fill up and no more events can be
> > stored. When the host is resumed later on, the firmware is in a bad
> > state and doesn't respond. I had originally just reset in ->resume but
> > then connected wireless devices wouldn't disconnect from the BT either
> > and I had trouble getting them to reconnect.
> >
> > >
> > > > I looked into using PERSIST and reset on resume but those seem mainly
> > > > for misbehaving devices that reset themselves.
> > >
> > > They are, but that doesn't mean you can't use them for other things
> > > too.
> > >
> > > > This patch series has been tested with Realtek BT hardware as well as
> > > > Intel BT (test procedure = disable as wake source, user suspend and
> > > > observe a detach + reattach on resume).
> > >
> > > This series really seems like overkill for a single kind of device.
> > >
> > > Is there any way to turn off the device's BT radio during suspend (if
> > > wakeup is disabled) and then turn it back on during resume?  Wouldn't
> > > that accomplish what you want just as well?
> >
> > Probably (but I couldn't find a way to do that).
>
> There's no way to turn off the device's BT radio?  Then what happens
> when the user turns off Bluetooth from the wireless control panel?

It looks like bluez invokes hci_dev_do_close. This gracefully clears
any packets in flight, clears any pending actions and disables the
device as a wakeup source (which for Realtek allows it to enter global
suspend). This is approximately what I was trying to achieve.

> >  I want to prevent
> > bluetooth from waking up the host and to reliably be in a good state
> > when the host resumes. The reset logic I implemented causes the hci
> > device to disappear and reappear, which userspace seems to handle
> > gracefully.
>
> Have you tried out the persist/reset-on-resume approach?
>
> Alan Stern
>

I think I'll abandon this patch series. The general sentiment seems to
be "don't do this" and it looks like closing the hci device is better
in my case.

Thanks for the feedback and pointing me this way.

Abhishek

Re: [PATCH 0/2] Reset realtek bluetooth devices during user suspend

[Oliver Neukum]

Am Dienstag, den 17.09.2019, 14:27 -0700 schrieb Abhishek Pandit-
Subedi:
> On a Realtek USB bluetooth device, I wanted a simple and consistent way
> to put the device in reset during suspend (2 reasons: to save power and

The device really uses less power if you reset it before suspendening
it?

> disable BT as a wakeup source). Resetting it in the suspend callback

Then do not enable it. Something is strange.

> causes a detach and the resume callback is not called. Hence the changes
> in this series to do the reset in suspend_noirq.
> 
> I looked into using PERSIST and reset on resume but those seem mainly
> for misbehaving devices that reset themselves.

No, not really. It is also for device that need to be reset if you use
the RESET_RESUME quirk. But that is on resume().
You could introduce a new flag. But the _noirq method is kind of
a hack, as USB really cannot operate without interrupts.

	Regards
		Oliver