Skip to contentSkip to conversation

Bluetooth: btusb: Fix suspend issue for Realtek devices

PATCH Superseded Alex Lu 3 participants 5 messages Aug 2, 2019 – Aug 14, 2019 Bluetooth drivers

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Alex Lu

author
#0 lore raw

PATCH v2 1/1 Bluetooth: btusb: Fix suspend issue for Realtek devices

From: Alex Lu <[email protected]>

From the perspective of controller, global suspend means there is no SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the firmware. It would consume less power. So we should not send this kind of SET_FEATURE when host goes to suspend state.
Otherwise, when making device enter selective suspend, host should send SET_FEATURE to make sure the firmware remains.

Signed-off-by Alex Lu <[email protected]>
---
 drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)
drivers/bluetooth/btusb.c +30 -4
index 50aed5259c2b..1995e26fa4cd 100644
@@ -426,6 +426,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
 #define BTUSB_DIAG_RUNNING	10
 #define BTUSB_OOB_WAKE_ENABLED	11
 #define BTUSB_HW_RESET_ACTIVE	12
+#define BTUSB_WAKEUP_DISABLE	13
 
 struct btusb_data {
 	struct hci_dev       *hdev;

@@ -1165,6 +1166,13 @@ static int btusb_open(struct hci_dev *hdev)
 	 */
 	device_wakeup_enable(&data->udev->dev);
 
+	/* Disable device remote wakeup when host is suspended
+	 * For Realtek chips, global suspend without
+	 * SET_FEATURE (DEVICE_REMOTE_WAKEUP) can save more power in device.
+	 */
+	if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags))
+		device_wakeup_disable(&data->udev->dev);
+
 	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
 		goto done;
 

@@ -1227,6 +1235,11 @@ static int btusb_close(struct hci_dev *hdev)
 		goto failed;
 
 	data->intf->needs_remote_wakeup = 0;
+
+	/* Enable remote wake up for auto-suspend */
+	if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags))
+		data->intf->needs_remote_wakeup = 1;
+
 	device_wakeup_disable(&data->udev->dev);
 	usb_autopm_put_interface(data->intf);
 

@@ -3185,11 +3198,11 @@ static int btusb_probe(struct usb_interface *intf,
 	if (id->driver_info & BTUSB_REALTEK) {
 		hdev->setup = btrtl_setup_realtek;
 
-		/* Realtek devices lose their updated firmware over suspend,
-		 * but the USB hub doesn't notice any status change.
-		 * Explicitly request a device reset on resume.
+		/* Realtek devices lose their updated firmware over global
+		 * suspend that means host doesn't send SET_FEATURE
+		 * (DEVICE_REMOTE_WAKEUP)
 		 */
-		interface_to_usbdev(intf)->quirks |= USB_QUIRK_RESET_RESUME;
+		set_bit(BTUSB_WAKEUP_DISABLE, &data->flags);
 	}
 #endif
 

@@ -3363,6 +3376,19 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
 		enable_irq(data->oob_wake_irq);
 	}
 
+	/* For global suspend, Realtek devices lose the loaded fw
+	 * in them. But for autosuspend, firmware should remain.
+	 * Actually, it depends on whether the usb host sends
+	 * set feature (enable wakeup) or not.
+	 */
+	if (test_bit(BTUSB_WAKEUP_DISABLE, &data->flags)) {
+		if (PMSG_IS_AUTO(message) &&
+		    device_can_wakeup(&data->udev->dev))
+			data->udev->do_remote_wakeup = 1;
+		else if (!PMSG_IS_AUTO(message))
+			data->udev->reset_resume = 1;
+	}
+
 	return 0;
 }
Signature · 1 line
2.19.2
maintainer
#1 lore raw

Hi Alex,

Quoted 11 lines

From the perspective of controller, global suspend means there is no SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the firmware. It would consume less power. So we should not send this kind of SET_FEATURE when host goes to suspend state.
Otherwise, when making device enter selective suspend, host should send SET_FEATURE to make sure the firmware remains.

Signed-off-by Alex Lu <[email protected]>

---
drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++----

1 file changed, 30 insertions(+), 4 deletions(-)

this one doesn’t apply cleanly to bluetooth-next. Can you please send a version that does.

Regards

Marcel

#2 lore raw
Quoted 10 lines · Marcel Holtmann

Hi Alex,

Quoted 7 lines

From the perspective of controller, global suspend means there is no SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the firmware. It would consume less power. So we should not send this kind of SET_FEATURE when host goes to suspend state.
Otherwise, when making device enter selective suspend, host should send SET_FEATURE to make sure the firmware remains.

Quoted 6 lines
Quoted 4 lines
Signed-off-by Alex Lu <[email protected]>

---
drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++----

1 file changed, 30 insertions(+), 4 deletions(-)

this one doesn’t apply cleanly to bluetooth-next. Can you please send a version that does.

Is this a chip issue or system issue? I.e. if in some system BT controller is wired so that it loses power over system suspend, this is quite different form chip itself losing firmware in certain situations, and this smells like a system issue and thus needs to be addressed on system level.

Thanks.

Signature · 1 line
Dmitry
author
#3 lore raw

Hi Dmitry,
It's only for Realtek devices.
If Realtek device firmware receives SET_FEATURE(device remote wakeup) usb cmd during usb is suspending, it will remains in suspend state.
Otherwise, firmware will drop itself and device will consume less power. But when host resumes, it needs to reload firmware. It can be accomplished by setting usb reset resume for device.

Thanks,
BRs.

Quoted 37 lines

Subject: Re: [PATCH v2] Bluetooth: btusb: Fix suspend issue for Realtek devices

Quoted 5 lines · Marcel Holtmann

Hi Alex,

Quoted 2 lines

From the perspective of controller, global suspend means there is no SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop

the

Quoted 3 lines
Quoted 3 lines

firmware. It would consume less power. So we should not send this kind of SET_FEATURE when host goes to suspend state.
Otherwise, when making device enter selective suspend, host should

send

Quoted 5 lines
Quoted 5 lines

SET_FEATURE to make sure the firmware remains.

Signed-off-by Alex Lu <[email protected]>

---
drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++---

-

Quoted 3 lines
Quoted 1 line
1 file changed, 30 insertions(+), 4 deletions(-)

this one doesn’t apply cleanly to bluetooth-next. Can you please send a

version that does.

Is this a chip issue or system issue? I.e. if in some system BT controller is wired so that it loses power over system suspend, this is quite different form chip itself losing firmware in certain situations, and this smells like a system issue and thus needs to be addressed on system level.

Thanks.

--
Dmitry

------Please consider the environment before printing this e-mail.

author
#4 lore raw
In reply to Marcel Holtmann

Hi Marcel,
Okay, I will send a version for bluetooth-next.

Thanks,
BRs.

Quoted 25 lines

Subject: Re: [PATCH v2] Bluetooth: btusb: Fix suspend issue for Realtek devices

Hi Alex,

Quoted 11 lines

From the perspective of controller, global suspend means there is no SET_FEATURE (DEVICE_REMOTE_WAKEUP) and controller would drop the firmware. It would consume less power. So we should not send this kind of SET_FEATURE when host goes to suspend state.
Otherwise, when making device enter selective suspend, host should send SET_FEATURE to make sure the firmware remains.

Signed-off-by Alex Lu <[email protected]>

---
drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++----

1 file changed, 30 insertions(+), 4 deletions(-)

this one doesn’t apply cleanly to bluetooth-next. Can you please send a version that does.

Regards

Marcel

------Please consider the environment before printing this e-mail.