* [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 related [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, other threads:[~2019-08-02 9:25 UTC | newest]
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 陆朱伟
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).