Skip to contentSkip to conversation

Bluetooth: btusb: Fix suspend issue for Realtek devices

PATCH In review Alex Lu 3 participants 3 messages Aug 14, 2019 – Aug 19, 2019 Bluetooth drivers

lore
Share & tools
lore

Alex Lu

author
#0 lore raw

PATCH v3 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.

---
Changes in v3:
- Change to fit for bluetooth-next
Changes in v2:
- Change flag to be more descriptive
- Delete pointless #ifdef CONFIG_BT_HCIBTUSB_RTL and #endif

 drivers/bluetooth/btusb.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)
drivers/bluetooth/btusb.c +30 -4
index 3876fee6ad13..b606c663ba57 100644
@@ -435,6 +435,7 @@ static const struct dmi_system_id btusb_needs_reset_resume_table[] = {
 #define BTUSB_OOB_WAKE_ENABLED	11
 #define BTUSB_HW_RESET_ACTIVE	12
 #define BTUSB_TX_WAIT_VND_EVT	13
+#define BTUSB_WAKEUP_DISABLE	14
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -1175,6 +1176,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;
 
@@ -1238,6 +1246,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);
 
@@ -3769,11 +3782,11 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->setup = btrtl_setup_realtek;
 		hdev->shutdown = btrtl_shutdown_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
 
@@ -3947,6 +3960,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 15 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.

---
Changes in v3:
- Change to fit for bluetooth-next
Changes in v2:
- Change flag to be more descriptive
- Delete pointless #ifdef CONFIG_BT_HCIBTUSB_RTL and #endif

Quoted 2 lines

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

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

patch has been applied to bluetooth-next tree.

Regards

Marcel

#2 lore raw

Dear Kernel Maintainer Marcel,
Sorry for the inconvenience. For the original target, this patch is edited for low power consumption hence controller should not receive DEVICE_REMOTE_WAKE_UP that it's able to save power in suspend mode because BT wake-up function is disabled. In upstream driver, there should be higher priority for function rather than performance. In other words, this patch can meet the low power consumption in suspend mode but will lose BT wake-up function. It is not a good idea for that. Please help to revert this modification. Thank you.

BRs,
Max

-----Original Message-----
From: Marcel Holtmann <[email protected]>
Sent: Wednesday, August 14, 2019 9:54 PM
To: alex_lu <[email protected]>
Cc: Johan Hedberg <[email protected]>; [email protected]; [email protected]; Max Chou <[email protected]>
Subject: Re: [PATCH v3] Bluetooth: btusb: Fix suspend issue for Realtek devices

Hi Alex,

Quoted 15 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.

---
Changes in v3:
- Change to fit for bluetooth-next
Changes in v2:
- Change flag to be more descriptive
- Delete pointless #ifdef CONFIG_BT_HCIBTUSB_RTL and #endif

Quoted 2 lines

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

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

patch has been applied to bluetooth-next tree.

Regards

Marcel

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