Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek
@ 2019-07-26 11:52 Alex Lu
  2019-07-30  9:38 ` Marcel Holtmann
  0 siblings, 1 reply; 3+ messages in thread
From: Alex Lu @ 2019-07-26 11:52 UTC (permalink / raw)
  To: Marcel Holtmann, Johan Hedberg; +Cc: linux-bluetooth, linux-kernel, Max Chou

From: Alex Lu <alex_lu@realsil.com.cn>

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 <alex_lu@realsil.com.cn>
---
 drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 50aed5259c2b..69f6b4208901 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -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_QUIRK_SUSPEND	13
 
 struct btusb_data {
 	struct hci_dev       *hdev;
@@ -1165,6 +1166,15 @@ 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.
+	 */
+#ifdef CONFIG_BT_HCIBTUSB_RTL
+	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
+		device_wakeup_disable(&data->udev->dev);
+#endif
+
 	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
 		goto done;
 
@@ -1227,6 +1237,13 @@ static int btusb_close(struct hci_dev *hdev)
 		goto failed;
 
 	data->intf->needs_remote_wakeup = 0;
+
+	/* Enable remote wake up for auto-suspend */
+#ifdef CONFIG_BT_HCIBTUSB_RTL
+	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
+		data->intf->needs_remote_wakeup = 1;
+#endif
+
 	device_wakeup_disable(&data->udev->dev);
 	usb_autopm_put_interface(data->intf);
 
@@ -3185,11 +3202,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_QUIRK_SUSPEND, &data->flags);
 	}
 #endif
 
@@ -3363,6 +3380,21 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
 		enable_irq(data->oob_wake_irq);
 	}
 
+#ifdef CONFIG_BT_HCIBTUSB_RTL
+	/* 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_QUIRK_SUSPEND, &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;
+	}
+#endif
+
 	return 0;
 }
 
-- 
2.19.2


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek
  2019-07-26 11:52 [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek Alex Lu
@ 2019-07-30  9:38 ` Marcel Holtmann
  0 siblings, 0 replies; 3+ messages in thread
From: Marcel Holtmann @ 2019-07-30  9:38 UTC (permalink / raw)
  To: Alex Lu; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel, Max Chou

Hi Alex,

> 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 <alex_lu@realsil.com.cn>
> ---
> drivers/bluetooth/btusb.c | 40 +++++++++++++++++++++++++++++++++++----
> 1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 50aed5259c2b..69f6b4208901 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -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_QUIRK_SUSPEND	13

I think this name is not really descriptive. What about BTUSB_SUSPEND_DISABLE or BTUSB_WAKEUP_DISABLE or BTUSB_REMOTE_WAKEUP or something that is rather clear.

> 
> struct btusb_data {
> 	struct hci_dev       *hdev;
> @@ -1165,6 +1166,15 @@ 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.
> +	 */
> +#ifdef CONFIG_BT_HCIBTUSB_RTL
> +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> +		device_wakeup_disable(&data->udev->dev);
> +#endif
> +

I have no idea what the #ifdef does here. Limiting it to HCIBTUSB_RTL seems pointless.

> 	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
> 		goto done;
> 
> @@ -1227,6 +1237,13 @@ static int btusb_close(struct hci_dev *hdev)
> 		goto failed;
> 
> 	data->intf->needs_remote_wakeup = 0;
> +
> +	/* Enable remote wake up for auto-suspend */
> +#ifdef CONFIG_BT_HCIBTUSB_RTL
> +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> +		data->intf->needs_remote_wakeup = 1;
> +#endif
> +

Same comment as above. No need for the #ifdef.

> 	device_wakeup_disable(&data->udev->dev);
> 	usb_autopm_put_interface(data->intf);
> 
> @@ -3185,11 +3202,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_QUIRK_SUSPEND, &data->flags);
> 	}
> #endif
> 
> @@ -3363,6 +3380,21 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> 		enable_irq(data->oob_wake_irq);
> 	}
> 
> +#ifdef CONFIG_BT_HCIBTUSB_RTL
> +	/* 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_QUIRK_SUSPEND, &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;
> +	}
> +#endif
> +

The #ifdef is also not needed here.

Regards

Marcel


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek
@ 2019-08-02  9:24 陆朱伟
  0 siblings, 0 replies; 3+ messages in thread
From: 陆朱伟 @ 2019-08-02  9:24 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: Johan Hedberg, linux-bluetooth, linux-kernel, Max Chou

Hi Marcel,
Got it, thanks.
I will change the patch and resubmit it.

Regards

> Subject: Re: [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek
> 
> Hi Alex,
> 
> > 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 <alex_lu@realsil.com.cn>
> > ---
> > drivers/bluetooth/btusb.c | 40
> +++++++++++++++++++++++++++++++++++----
> > 1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 50aed5259c2b..69f6b4208901 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -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_QUIRK_SUSPEND	13
> 
> I think this name is not really descriptive. What about
> BTUSB_SUSPEND_DISABLE or BTUSB_WAKEUP_DISABLE or
> BTUSB_REMOTE_WAKEUP or something that is rather clear.
> 
> >
> > struct btusb_data {
> > 	struct hci_dev       *hdev;
> > @@ -1165,6 +1166,15 @@ 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.
> > +	 */
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> > +		device_wakeup_disable(&data->udev->dev);
> > +#endif
> > +
> 
> I have no idea what the #ifdef does here. Limiting it to HCIBTUSB_RTL seems
> pointless.
> 
> > 	if (test_and_set_bit(BTUSB_INTR_RUNNING, &data->flags))
> > 		goto done;
> >
> > @@ -1227,6 +1237,13 @@ static int btusb_close(struct hci_dev *hdev)
> > 		goto failed;
> >
> > 	data->intf->needs_remote_wakeup = 0;
> > +
> > +	/* Enable remote wake up for auto-suspend */
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	if (test_bit(BTUSB_QUIRK_SUSPEND, &data->flags))
> > +		data->intf->needs_remote_wakeup = 1;
> > +#endif
> > +
> 
> Same comment as above. No need for the #ifdef.
> 
> > 	device_wakeup_disable(&data->udev->dev);
> > 	usb_autopm_put_interface(data->intf);
> >
> > @@ -3185,11 +3202,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_QUIRK_SUSPEND, &data->flags);
> > 	}
> > #endif
> >
> > @@ -3363,6 +3380,21 @@ static int btusb_suspend(struct usb_interface
> *intf, pm_message_t message)
> > 		enable_irq(data->oob_wake_irq);
> > 	}
> >
> > +#ifdef CONFIG_BT_HCIBTUSB_RTL
> > +	/* 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_QUIRK_SUSPEND, &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;
> > +	}
> > +#endif
> > +
> 
> The #ifdef is also not needed here.
> 
> Regards
> 
> Marcel
> 
> 
> ------Please consider the environment before printing this e-mail.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 11:52 [PATCH] Bluetooth: btusb: Fix suspend issue for Realtek Alex Lu
2019-07-30  9:38 ` Marcel Holtmann
2019-08-02  9:24 陆朱伟

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org linux-bluetooth@archiver.kernel.org
	public-inbox-index linux-bluetooth


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/ public-inbox