Bluetooth: btusb: Add debugfs to force toggling remote wakeup

Commit Message

Archie Pusaka April 22, 2024, 7:25 a.m. UTC

From: Archie Pusaka <[e-mail]>

Sometimes we want the controller to not wake the host up, e.g. to
save the battery. Add some debugfs knobs to force the wake by BT
behavior.

Signed-off-by: Archie Pusaka <[e-mail]>
Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>

---

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

Comments

[e-mail] April 22, 2024, 7:56 a.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=846522

---Test result---

Test Summary:
CheckPatch                    PASS      0.61 seconds
GitLint                       PASS      0.26 seconds
SubjectPrefix                 PASS      0.13 seconds
BuildKernel                   PASS      29.89 seconds
CheckAllWarning               PASS      32.63 seconds
CheckSparse                   PASS      37.87 seconds
CheckSmatch                   FAIL      35.74 seconds
BuildKernel32                 PASS      28.73 seconds
TestRunnerSetup               PASS      527.30 seconds
TestRunner_l2cap-tester       PASS      20.41 seconds
TestRunner_iso-tester         PASS      31.07 seconds
TestRunner_bnep-tester        PASS      4.79 seconds
TestRunner_mgmt-tester        FAIL      113.58 seconds
TestRunner_rfcomm-tester      PASS      7.51 seconds
TestRunner_sco-tester         PASS      15.17 seconds
TestRunner_ioctl-tester       PASS      7.88 seconds
TestRunner_mesh-tester        PASS      5.95 seconds
TestRunner_smp-tester         PASS      7.00 seconds
TestRunner_userchan-tester    PASS      5.07 seconds
IncrementalBuild              PASS      29.30 seconds

Details
##############################
Test: CheckSmatch - FAIL
Desc: Run smatch tool with source
Output:

Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: net/bluetooth/hci_core.o] Error 139
make[4]: *** Deleting file 'net/bluetooth/hci_core.o'
make[3]: *** [scripts/Makefile.build:485: net/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: net] Error 2
make[2]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bcm203x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bcm203x.o'
make[4]: *** Waiting for unfinished jobs....
Segmentation fault (core dumped)
make[4]: *** [scripts/Makefile.build:244: drivers/bluetooth/bpa10x.o] Error 139
make[4]: *** Deleting file 'drivers/bluetooth/bpa10x.o'
make[3]: *** [scripts/Makefile.build:485: drivers/bluetooth] Error 2
make[2]: *** [scripts/Makefile.build:485: drivers] Error 2
make[1]: *** [/github/workspace/src/src/Makefile:1919: .] Error 2
make: *** [Makefile:240: __sub-make] Error 2
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 488 (99.2%), Failed: 2, Not Run: 2

Failed Test Cases
LL Privacy - Add Device 5 (2 Devices to RL)          Failed       0.170 seconds
LL Privacy - Add Device 7 (AL is full)               Failed       0.203 seconds


---
Regards,
Linux Bluetooth

Luiz Augusto von Dentz April 24, 2024, 7:04 p.m. UTC | #2

Hi Archie,

On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
>
> From: Archie Pusaka <[e-mail]>
>
> Sometimes we want the controller to not wake the host up, e.g. to
> save the battery. Add some debugfs knobs to force the wake by BT
> behavior.
>
> Signed-off-by: Archie Pusaka <[e-mail]>
> Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
>
> ---
>
>  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 8bede0a335668..846b15fc3c04c 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -873,6 +873,9 @@ struct btusb_data {
>         unsigned cmd_timeout_cnt;
>
>         struct qca_dump_info qca_dump;
> +
> +       bool force_enable_remote_wake;
> +       bool force_disable_remote_wake;
>  };
>
>  static void btusb_reset(struct hci_dev *hdev)
> @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
>
>         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
>                             &force_poll_sync_fops);
> +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> +                           &data->force_enable_remote_wake);
> +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> +                           &data->force_disable_remote_wake);
>
>         return 0;
>
> @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
>                 }
>         }
>
> +       if (!PMSG_IS_AUTO(message)) {
> +               if (data->force_enable_remote_wake) {
> +                       data->udev->do_remote_wakeup = 1;
> +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> +                               data->udev->reset_resume = 0;
> +               } else if (data->force_disable_remote_wake) {
> +                       data->udev->do_remote_wakeup = 0;
> +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> +                               data->udev->reset_resume = 1;
> +               }
> +       }
> +
>         return 0;
>  }
>
> --
> 2.44.0.769.g3c40516874-goog

There is a D-Bus interface available to overwrite the wakeup setting:

https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite

Or do you want a master switch for it? On the other hand aren't we
getting into the rfkill area if you really want to switch off radio
activity while suspended? That seems like a better idea then just
disable remote wakeup.

Archie Pusaka April 26, 2024, 9:08 a.m. UTC | #3

Hi Luiz,

On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Archie,
>
> On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> >
> > From: Archie Pusaka <[e-mail]>
> >
> > Sometimes we want the controller to not wake the host up, e.g. to
> > save the battery. Add some debugfs knobs to force the wake by BT
> > behavior.
> >
> > Signed-off-by: Archie Pusaka <[e-mail]>
> > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> >
> > ---
> >
> >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 8bede0a335668..846b15fc3c04c 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -873,6 +873,9 @@ struct btusb_data {
> >         unsigned cmd_timeout_cnt;
> >
> >         struct qca_dump_info qca_dump;
> > +
> > +       bool force_enable_remote_wake;
> > +       bool force_disable_remote_wake;
> >  };
> >
> >  static void btusb_reset(struct hci_dev *hdev)
> > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> >                             &force_poll_sync_fops);
> > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > +                           &data->force_enable_remote_wake);
> > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > +                           &data->force_disable_remote_wake);
> >
> >         return 0;
> >
> > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> >                 }
> >         }
> >
> > +       if (!PMSG_IS_AUTO(message)) {
> > +               if (data->force_enable_remote_wake) {
> > +                       data->udev->do_remote_wakeup = 1;
> > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > +                               data->udev->reset_resume = 0;
> > +               } else if (data->force_disable_remote_wake) {
> > +                       data->udev->do_remote_wakeup = 0;
> > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > +                               data->udev->reset_resume = 1;
> > +               }
> > +       }
> > +
> >         return 0;
> >  }
> >
> > --
> > 2.44.0.769.g3c40516874-goog
>
> There is a D-Bus interface available to overwrite the wakeup setting:
>
> https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
>
> Or do you want a master switch for it? On the other hand aren't we
> getting into the rfkill area if you really want to switch off radio
> activity while suspended? That seems like a better idea then just
> disable remote wakeup.

Yes, the initial idea was a master switch.
Thanks for your suggestions.
Let me discuss it with Abhishek.
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie

Abhishek Pandit-Subedi April 26, 2024, 5:04 p.m. UTC | #4

On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
Upstreaming <[e-mail]> wrote:
>
> Hi Luiz,
>
> On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Archie,
> >
> > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > >
> > > From: Archie Pusaka <[e-mail]>
> > >
> > > Sometimes we want the controller to not wake the host up, e.g. to
> > > save the battery. Add some debugfs knobs to force the wake by BT
> > > behavior.
> > >
> > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > >
> > > ---
> > >
> > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index 8bede0a335668..846b15fc3c04c 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -873,6 +873,9 @@ struct btusb_data {
> > >         unsigned cmd_timeout_cnt;
> > >
> > >         struct qca_dump_info qca_dump;
> > > +
> > > +       bool force_enable_remote_wake;
> > > +       bool force_disable_remote_wake;
> > >  };
> > >
> > >  static void btusb_reset(struct hci_dev *hdev)
> > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > >
> > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > >                             &force_poll_sync_fops);
> > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > +                           &data->force_enable_remote_wake);
> > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > +                           &data->force_disable_remote_wake);
> > >
> > >         return 0;
> > >
> > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > >                 }
> > >         }
> > >
> > > +       if (!PMSG_IS_AUTO(message)) {
> > > +               if (data->force_enable_remote_wake) {
> > > +                       data->udev->do_remote_wakeup = 1;
> > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > +                               data->udev->reset_resume = 0;
> > > +               } else if (data->force_disable_remote_wake) {
> > > +                       data->udev->do_remote_wakeup = 0;
> > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > +                               data->udev->reset_resume = 1;
> > > +               }
> > > +       }
> > > +
> > >         return 0;
> > >  }
> > >
> > > --
> > > 2.44.0.769.g3c40516874-goog
> >
> > There is a D-Bus interface available to overwrite the wakeup setting:
> >
> > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> >
> > Or do you want a master switch for it? On the other hand aren't we
> > getting into the rfkill area if you really want to switch off radio
> > activity while suspended? That seems like a better idea then just
> > disable remote wakeup.

This DBUS api is different from the quirk this is introducing.

The `Wake Allowed` field in D-bus controls whether we add the address
to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
not whether we allow wake at the transport level (which is why
hdev->wakeup exists).

This change specifically addresses a quirk with Realtek chipsets:
RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
USB Remote Wake bit is not set. The USB remote_wake bit is normally
set by the USB stack based on whether device_may_wakeup(udev) == true.
This means that RTL88x2 will lose power around suspend/resume if there
are no wake capable devices connected.

ChromeOS decided to use idle power and resume-time to determine
whether to allow remote wake or not for these chipsets and we want to
move this control to userspace so that we don't have to hold CHROMIUM
patches in the kernel applying this policy (we want udev rules
instead). RTL8852 gets force enabled remote wake because the
RESET_RESUME behavior of this chip would otherwise increase our resume
time >1s which breaks one of our platform requirements.

The end-goal of these changes:
* We detect RTL8822 or RTL8852 with udev and apply the right policy.
* RTL8822 gets force_disable_remote_wake (idle power consumption too
high otherwise)
* RTL8852 gets force_enable_remote_wake (resume time too long otherwise)

Hope this provides enough context for why this change is necessary.

>
> Yes, the initial idea was a master switch.
> Thanks for your suggestions.
> Let me discuss it with Abhishek.
> >
> > --
> > Luiz Augusto von Dentz
>
> Thanks,
> Archie
>
> --
> You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> To post to this group, send email to [e-mail].
> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.

Luiz Augusto von Dentz April 30, 2024, 4:45 p.m. UTC | #5

Hi Abhishek,

On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> Upstreaming <[e-mail]> wrote:
> >
> > Hi Luiz,
> >
> > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > > >
> > > > From: Archie Pusaka <[e-mail]>
> > > >
> > > > Sometimes we want the controller to not wake the host up, e.g. to
> > > > save the battery. Add some debugfs knobs to force the wake by BT
> > > > behavior.
> > > >
> > > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > >
> > > > ---
> > > >
> > > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > index 8bede0a335668..846b15fc3c04c 100644
> > > > --- a/drivers/bluetooth/btusb.c
> > > > +++ b/drivers/bluetooth/btusb.c
> > > > @@ -873,6 +873,9 @@ struct btusb_data {
> > > >         unsigned cmd_timeout_cnt;
> > > >
> > > >         struct qca_dump_info qca_dump;
> > > > +
> > > > +       bool force_enable_remote_wake;
> > > > +       bool force_disable_remote_wake;
> > > >  };
> > > >
> > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > > >
> > > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > > >                             &force_poll_sync_fops);
> > > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > > +                           &data->force_enable_remote_wake);
> > > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > > +                           &data->force_disable_remote_wake);
> > > >
> > > >         return 0;
> > > >
> > > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > > >                 }
> > > >         }
> > > >
> > > > +       if (!PMSG_IS_AUTO(message)) {
> > > > +               if (data->force_enable_remote_wake) {
> > > > +                       data->udev->do_remote_wakeup = 1;
> > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > +                               data->udev->reset_resume = 0;
> > > > +               } else if (data->force_disable_remote_wake) {
> > > > +                       data->udev->do_remote_wakeup = 0;
> > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > +                               data->udev->reset_resume = 1;
> > > > +               }
> > > > +       }
> > > > +
> > > >         return 0;
> > > >  }
> > > >
> > > > --
> > > > 2.44.0.769.g3c40516874-goog
> > >
> > > There is a D-Bus interface available to overwrite the wakeup setting:
> > >
> > > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> > >
> > > Or do you want a master switch for it? On the other hand aren't we
> > > getting into the rfkill area if you really want to switch off radio
> > > activity while suspended? That seems like a better idea then just
> > > disable remote wakeup.
>
> This DBUS api is different from the quirk this is introducing.
>
> The `Wake Allowed` field in D-bus controls whether we add the address
> to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
> not whether we allow wake at the transport level (which is why
> hdev->wakeup exists).
>
> This change specifically addresses a quirk with Realtek chipsets:
> RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
> USB Remote Wake bit is not set. The USB remote_wake bit is normally
> set by the USB stack based on whether device_may_wakeup(udev) == true.
> This means that RTL88x2 will lose power around suspend/resume if there
> are no wake capable devices connected.
>
> ChromeOS decided to use idle power and resume-time to determine
> whether to allow remote wake or not for these chipsets and we want to
> move this control to userspace so that we don't have to hold CHROMIUM
> patches in the kernel applying this policy (we want udev rules
> instead). RTL8852 gets force enabled remote wake because the
> RESET_RESUME behavior of this chip would otherwise increase our resume
> time >1s which breaks one of our platform requirements.
>
> The end-goal of these changes:
> * We detect RTL8822 or RTL8852 with udev and apply the right policy.
> * RTL8822 gets force_disable_remote_wake (idle power consumption too
> high otherwise)
> * RTL8852 gets force_enable_remote_wake (resume time too long otherwise)

Got it, but the suggestion was to instead of using
force_enable_remote_wake, which is sort of non-standard, why don't
Chrome OS simply use rkill interface to tell the driver to shutdown?
On resume then you can just unblock via rfkill that should have the
same result as using force_enable_remote_wake, well except if there is
a bug in the driver that is not handling rfkill as a 'global
shutdown', but then you need to fix the driver not introduce yet
another debugfs entry to bypass it.

> Hope this provides enough context for why this change is necessary.
>
> >
> > Yes, the initial idea was a master switch.
> > Thanks for your suggestions.
> > Let me discuss it with Abhishek.
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Archie
> >
> > --
> > You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> > To post to this group, send email to [e-mail].
> > To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.

Abhishek Pandit-Subedi May 1, 2024, 4:34 p.m. UTC | #6

On Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Abhishek,
>
> On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> <[e-mail]> wrote:
> >
> > On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> > Upstreaming <[e-mail]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > > > >
> > > > > From: Archie Pusaka <[e-mail]>
> > > > >
> > > > > Sometimes we want the controller to not wake the host up, e.g. to
> > > > > save the battery. Add some debugfs knobs to force the wake by BT
> > > > > behavior.
> > > > >
> > > > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > >
> > > > > ---
> > > > >
> > > > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > index 8bede0a335668..846b15fc3c04c 100644
> > > > > --- a/drivers/bluetooth/btusb.c
> > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > @@ -873,6 +873,9 @@ struct btusb_data {
> > > > >         unsigned cmd_timeout_cnt;
> > > > >
> > > > >         struct qca_dump_info qca_dump;
> > > > > +
> > > > > +       bool force_enable_remote_wake;
> > > > > +       bool force_disable_remote_wake;
> > > > >  };
> > > > >
> > > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > > > >
> > > > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > > > >                             &force_poll_sync_fops);
> > > > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > > > +                           &data->force_enable_remote_wake);
> > > > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > > > +                           &data->force_disable_remote_wake);
> > > > >
> > > > >         return 0;
> > > > >
> > > > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > > > >                 }
> > > > >         }
> > > > >
> > > > > +       if (!PMSG_IS_AUTO(message)) {
> > > > > +               if (data->force_enable_remote_wake) {
> > > > > +                       data->udev->do_remote_wakeup = 1;
> > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > +                               data->udev->reset_resume = 0;
> > > > > +               } else if (data->force_disable_remote_wake) {
> > > > > +                       data->udev->do_remote_wakeup = 0;
> > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > +                               data->udev->reset_resume = 1;
> > > > > +               }
> > > > > +       }
> > > > > +
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > --
> > > > > 2.44.0.769.g3c40516874-goog
> > > >
> > > > There is a D-Bus interface available to overwrite the wakeup setting:
> > > >
> > > > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> > > >
> > > > Or do you want a master switch for it? On the other hand aren't we
> > > > getting into the rfkill area if you really want to switch off radio
> > > > activity while suspended? That seems like a better idea then just
> > > > disable remote wakeup.
> >
> > This DBUS api is different from the quirk this is introducing.
> >
> > The `Wake Allowed` field in D-bus controls whether we add the address
> > to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
> > not whether we allow wake at the transport level (which is why
> > hdev->wakeup exists).
> >
> > This change specifically addresses a quirk with Realtek chipsets:
> > RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
> > USB Remote Wake bit is not set. The USB remote_wake bit is normally
> > set by the USB stack based on whether device_may_wakeup(udev) == true.
> > This means that RTL88x2 will lose power around suspend/resume if there
> > are no wake capable devices connected.
> >
> > ChromeOS decided to use idle power and resume-time to determine
> > whether to allow remote wake or not for these chipsets and we want to
> > move this control to userspace so that we don't have to hold CHROMIUM
> > patches in the kernel applying this policy (we want udev rules
> > instead). RTL8852 gets force enabled remote wake because the
> > RESET_RESUME behavior of this chip would otherwise increase our resume
> > time >1s which breaks one of our platform requirements.
> >
> > The end-goal of these changes:
> > * We detect RTL8822 or RTL8852 with udev and apply the right policy.
> > * RTL8822 gets force_disable_remote_wake (idle power consumption too
> > high otherwise)
> > * RTL8852 gets force_enable_remote_wake (resume time too long otherwise)
>
> Got it, but the suggestion was to instead of using
> force_enable_remote_wake, which is sort of non-standard, why don't
> Chrome OS simply use rkill interface to tell the driver to shutdown?
> On resume then you can just unblock via rfkill that should have the
> same result as using force_enable_remote_wake, well except if there is
> a bug in the driver that is not handling rfkill as a 'global
> shutdown', but then you need to fix the driver not introduce yet
> another debugfs entry to bypass it.

Did you mean `force_disable_remote_wake`? rfkill will work for that
around system suspend. We preferred not to do it because we don't use
userspace suspend signals with Bluez today (preferring the kernel
suspend notifier).

`force_enable_remote_wake` still needs debugfs as rfkill can't force
an interface to stay awake as far as I know.

>
> > Hope this provides enough context for why this change is necessary.
> >
> > >
> > > Yes, the initial idea was a master switch.
> > > Thanks for your suggestions.
> > > Let me discuss it with Abhishek.
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > > Thanks,
> > > Archie
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> > > To post to this group, send email to [e-mail].
> > > To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.
>
>
>
> --
> Luiz Augusto von Dentz

Luiz Augusto von Dentz May 1, 2024, 4:52 p.m. UTC | #7

Hi Abhishek,

On Wed, May 1, 2024 at 12:34 PM Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> On Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Abhishek,
> >
> > On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> > <[e-mail]> wrote:
> > >
> > > On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> > > Upstreaming <[e-mail]> wrote:
> > > >
> > > > Hi Luiz,
> > > >
> > > > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > > > <[e-mail]> wrote:
> > > > >
> > > > > Hi Archie,
> > > > >
> > > > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > > > > >
> > > > > > From: Archie Pusaka <[e-mail]>
> > > > > >
> > > > > > Sometimes we want the controller to not wake the host up, e.g. to
> > > > > > save the battery. Add some debugfs knobs to force the wake by BT
> > > > > > behavior.
> > > > > >
> > > > > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > index 8bede0a335668..846b15fc3c04c 100644
> > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > @@ -873,6 +873,9 @@ struct btusb_data {
> > > > > >         unsigned cmd_timeout_cnt;
> > > > > >
> > > > > >         struct qca_dump_info qca_dump;
> > > > > > +
> > > > > > +       bool force_enable_remote_wake;
> > > > > > +       bool force_disable_remote_wake;
> > > > > >  };
> > > > > >
> > > > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > >
> > > > > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > > > > >                             &force_poll_sync_fops);
> > > > > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > > > > +                           &data->force_enable_remote_wake);
> > > > > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > > > > +                           &data->force_disable_remote_wake);
> > > > > >
> > > > > >         return 0;
> > > > > >
> > > > > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > > > > >                 }
> > > > > >         }
> > > > > >
> > > > > > +       if (!PMSG_IS_AUTO(message)) {
> > > > > > +               if (data->force_enable_remote_wake) {
> > > > > > +                       data->udev->do_remote_wakeup = 1;
> > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > +                               data->udev->reset_resume = 0;
> > > > > > +               } else if (data->force_disable_remote_wake) {
> > > > > > +                       data->udev->do_remote_wakeup = 0;
> > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > +                               data->udev->reset_resume = 1;
> > > > > > +               }
> > > > > > +       }
> > > > > > +
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > --
> > > > > > 2.44.0.769.g3c40516874-goog
> > > > >
> > > > > There is a D-Bus interface available to overwrite the wakeup setting:
> > > > >
> > > > > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> > > > >
> > > > > Or do you want a master switch for it? On the other hand aren't we
> > > > > getting into the rfkill area if you really want to switch off radio
> > > > > activity while suspended? That seems like a better idea then just
> > > > > disable remote wakeup.
> > >
> > > This DBUS api is different from the quirk this is introducing.
> > >
> > > The `Wake Allowed` field in D-bus controls whether we add the address
> > > to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
> > > not whether we allow wake at the transport level (which is why
> > > hdev->wakeup exists).
> > >
> > > This change specifically addresses a quirk with Realtek chipsets:
> > > RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
> > > USB Remote Wake bit is not set. The USB remote_wake bit is normally
> > > set by the USB stack based on whether device_may_wakeup(udev) == true.
> > > This means that RTL88x2 will lose power around suspend/resume if there
> > > are no wake capable devices connected.
> > >
> > > ChromeOS decided to use idle power and resume-time to determine
> > > whether to allow remote wake or not for these chipsets and we want to
> > > move this control to userspace so that we don't have to hold CHROMIUM
> > > patches in the kernel applying this policy (we want udev rules
> > > instead). RTL8852 gets force enabled remote wake because the
> > > RESET_RESUME behavior of this chip would otherwise increase our resume
> > > time >1s which breaks one of our platform requirements.
> > >
> > > The end-goal of these changes:
> > > * We detect RTL8822 or RTL8852 with udev and apply the right policy.
> > > * RTL8822 gets force_disable_remote_wake (idle power consumption too
> > > high otherwise)
> > > * RTL8852 gets force_enable_remote_wake (resume time too long otherwise)
> >
> > Got it, but the suggestion was to instead of using
> > force_enable_remote_wake, which is sort of non-standard, why don't
> > Chrome OS simply use rkill interface to tell the driver to shutdown?
> > On resume then you can just unblock via rfkill that should have the
> > same result as using force_enable_remote_wake, well except if there is
> > a bug in the driver that is not handling rfkill as a 'global
> > shutdown', but then you need to fix the driver not introduce yet
> > another debugfs entry to bypass it.
>
> Did you mean `force_disable_remote_wake`? rfkill will work for that
> around system suspend. We preferred not to do it because we don't use
> userspace suspend signals with Bluez today (preferring the kernel
> suspend notifier).

Yeah, that said rfkill has nothing to do with suspend afaik, it is
more for achieving flight mode and as far I know it is a kernel
interface.

> `force_enable_remote_wake` still needs debugfs as rfkill can't force
> an interface to stay awake as far as I know.

You mixing up, Im not saying to use rfkill to enable/disable wake
support, the remains the same, what changes is that if you want to
overwrite that it just use rfkill to block the traffic while suspended
that way wake being enable or not doesn't matter since the controller
radio shall be off if it is blocked.

>
> >
> > > Hope this provides enough context for why this change is necessary.
> > >
> > > >
> > > > Yes, the initial idea was a master switch.
> > > > Thanks for your suggestions.
> > > > Let me discuss it with Abhishek.
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > > >
> > > > Thanks,
> > > > Archie
> > > >
> > > > --
> > > > You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> > > > To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> > > > To post to this group, send email to [e-mail].
> > > > To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

Abhishek Pandit-Subedi May 1, 2024, 4:57 p.m. UTC | #8

On Wed, May 1, 2024 at 9:52 AM Luiz Augusto von Dentz
<[e-mail]> wrote:
>
> Hi Abhishek,
>
> On Wed, May 1, 2024 at 12:34 PM Abhishek Pandit-Subedi
> <[e-mail]> wrote:
> >
> > On Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
> > <[e-mail]> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> > > <[e-mail]> wrote:
> > > >
> > > > On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> > > > Upstreaming <[e-mail]> wrote:
> > > > >
> > > > > Hi Luiz,
> > > > >
> > > > > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Archie,
> > > > > >
> > > > > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Archie Pusaka <[e-mail]>
> > > > > > >
> > > > > > > Sometimes we want the controller to not wake the host up, e.g. to
> > > > > > > save the battery. Add some debugfs knobs to force the wake by BT
> > > > > > > behavior.
> > > > > > >
> > > > > > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > > > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > > > > > >  1 file changed, 19 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > > index 8bede0a335668..846b15fc3c04c 100644
> > > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > > @@ -873,6 +873,9 @@ struct btusb_data {
> > > > > > >         unsigned cmd_timeout_cnt;
> > > > > > >
> > > > > > >         struct qca_dump_info qca_dump;
> > > > > > > +
> > > > > > > +       bool force_enable_remote_wake;
> > > > > > > +       bool force_disable_remote_wake;
> > > > > > >  };
> > > > > > >
> > > > > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > > > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > > >
> > > > > > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > > > > > >                             &force_poll_sync_fops);
> > > > > > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > > > > > +                           &data->force_enable_remote_wake);
> > > > > > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > > > > > +                           &data->force_disable_remote_wake);
> > > > > > >
> > > > > > >         return 0;
> > > > > > >
> > > > > > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > > > > > >                 }
> > > > > > >         }
> > > > > > >
> > > > > > > +       if (!PMSG_IS_AUTO(message)) {
> > > > > > > +               if (data->force_enable_remote_wake) {
> > > > > > > +                       data->udev->do_remote_wakeup = 1;
> > > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > > +                               data->udev->reset_resume = 0;
> > > > > > > +               } else if (data->force_disable_remote_wake) {
> > > > > > > +                       data->udev->do_remote_wakeup = 0;
> > > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > > +                               data->udev->reset_resume = 1;
> > > > > > > +               }
> > > > > > > +       }
> > > > > > > +
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > --
> > > > > > > 2.44.0.769.g3c40516874-goog
> > > > > >
> > > > > > There is a D-Bus interface available to overwrite the wakeup setting:
> > > > > >
> > > > > > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> > > > > >
> > > > > > Or do you want a master switch for it? On the other hand aren't we
> > > > > > getting into the rfkill area if you really want to switch off radio
> > > > > > activity while suspended? That seems like a better idea then just
> > > > > > disable remote wakeup.
> > > >
> > > > This DBUS api is different from the quirk this is introducing.
> > > >
> > > > The `Wake Allowed` field in D-bus controls whether we add the address
> > > > to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
> > > > not whether we allow wake at the transport level (which is why
> > > > hdev->wakeup exists).
> > > >
> > > > This change specifically addresses a quirk with Realtek chipsets:
> > > > RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
> > > > USB Remote Wake bit is not set. The USB remote_wake bit is normally
> > > > set by the USB stack based on whether device_may_wakeup(udev) == true.
> > > > This means that RTL88x2 will lose power around suspend/resume if there
> > > > are no wake capable devices connected.
> > > >
> > > > ChromeOS decided to use idle power and resume-time to determine
> > > > whether to allow remote wake or not for these chipsets and we want to
> > > > move this control to userspace so that we don't have to hold CHROMIUM
> > > > patches in the kernel applying this policy (we want udev rules
> > > > instead). RTL8852 gets force enabled remote wake because the
> > > > RESET_RESUME behavior of this chip would otherwise increase our resume
> > > > time >1s which breaks one of our platform requirements.
> > > >
> > > > The end-goal of these changes:
> > > > * We detect RTL8822 or RTL8852 with udev and apply the right policy.
> > > > * RTL8822 gets force_disable_remote_wake (idle power consumption too
> > > > high otherwise)
> > > > * RTL8852 gets force_enable_remote_wake (resume time too long otherwise)
> > >
> > > Got it, but the suggestion was to instead of using
> > > force_enable_remote_wake, which is sort of non-standard, why don't
> > > Chrome OS simply use rkill interface to tell the driver to shutdown?
> > > On resume then you can just unblock via rfkill that should have the
> > > same result as using force_enable_remote_wake, well except if there is
> > > a bug in the driver that is not handling rfkill as a 'global
> > > shutdown', but then you need to fix the driver not introduce yet
> > > another debugfs entry to bypass it.
> >
> > Did you mean `force_disable_remote_wake`? rfkill will work for that
> > around system suspend. We preferred not to do it because we don't use
> > userspace suspend signals with Bluez today (preferring the kernel
> > suspend notifier).
>
> Yeah, that said rfkill has nothing to do with suspend afaik, it is
> more for achieving flight mode and as far I know it is a kernel
> interface.
>
> > `force_enable_remote_wake` still needs debugfs as rfkill can't force
> > an interface to stay awake as far as I know.
>
> You mixing up, Im not saying to use rfkill to enable/disable wake
> support, the remains the same, what changes is that if you want to
> overwrite that it just use rfkill to block the traffic while suspended
> that way wake being enable or not doesn't matter since the controller
> radio shall be off if it is blocked.

Ah, the problem with resume time is not how long it takes for the
Bluetooth link to come up. The problem is that it takes >1s for all
the driver resume callbacks to run after a wake IRQ occurs and blocks
user space from running at all.

The root cause seems to be that reset-resume (which correctly gets set
for Realtek) will block USB port resume and, since the resume path
seems to be synchronous, it blocks other drivers from resuming too.

>
> >
> > >
> > > > Hope this provides enough context for why this change is necessary.
> > > >
> > > > >
> > > > > Yes, the initial idea was a master switch.
> > > > > Thanks for your suggestions.
> > > > > Let me discuss it with Abhishek.
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > > >
> > > > > Thanks,
> > > > > Archie
> > > > >
> > > > > --
> > > > > You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> > > > > To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> > > > > To post to this group, send email to [e-mail].
> > > > > To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

Archie Pusaka June 6, 2024, 12:21 p.m. UTC | #9

Hi Luiz,

Sorry for bringing back an old topic, but can we still ask your
opinion on this proposal once again?

Thank you,
Archie


On Thu, 2 May 2024 at 00:57, Abhishek Pandit-Subedi
<[e-mail]> wrote:
>
> On Wed, May 1, 2024 at 9:52 AM Luiz Augusto von Dentz
> <[e-mail]> wrote:
> >
> > Hi Abhishek,
> >
> > On Wed, May 1, 2024 at 12:34 PM Abhishek Pandit-Subedi
> > <[e-mail]> wrote:
> > >
> > > On Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
> > > <[e-mail]> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> > > > <[e-mail]> wrote:
> > > > >
> > > > > On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> > > > > Upstreaming <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Luiz,
> > > > > >
> > > > > > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > Hi Archie,
> > > > > > >
> > > > > > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > From: Archie Pusaka <[e-mail]>
> > > > > > > >
> > > > > > > > Sometimes we want the controller to not wake the host up, e.g. to
> > > > > > > > save the battery. Add some debugfs knobs to force the wake by BT
> > > > > > > > behavior.
> > > > > > > >
> > > > > > > > Signed-off-by: Archie Pusaka <[e-mail]>
> > > > > > > > Reviewed-by: Abhishek Pandit-Subedi <[e-mail]>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > >  drivers/bluetooth/btusb.c | 19 +++++++++++++++++++
> > > > > > > >  1 file changed, 19 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > > > index 8bede0a335668..846b15fc3c04c 100644
> > > > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > > > @@ -873,6 +873,9 @@ struct btusb_data {
> > > > > > > >         unsigned cmd_timeout_cnt;
> > > > > > > >
> > > > > > > >         struct qca_dump_info qca_dump;
> > > > > > > > +
> > > > > > > > +       bool force_enable_remote_wake;
> > > > > > > > +       bool force_disable_remote_wake;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  static void btusb_reset(struct hci_dev *hdev)
> > > > > > > > @@ -4596,6 +4599,10 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > > > >
> > > > > > > >         debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
> > > > > > > >                             &force_poll_sync_fops);
> > > > > > > > +       debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
> > > > > > > > +                           &data->force_enable_remote_wake);
> > > > > > > > +       debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
> > > > > > > > +                           &data->force_disable_remote_wake);
> > > > > > > >
> > > > > > > >         return 0;
> > > > > > > >
> > > > > > > > @@ -4702,6 +4709,18 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
> > > > > > > >                 }
> > > > > > > >         }
> > > > > > > >
> > > > > > > > +       if (!PMSG_IS_AUTO(message)) {
> > > > > > > > +               if (data->force_enable_remote_wake) {
> > > > > > > > +                       data->udev->do_remote_wakeup = 1;
> > > > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > > > +                               data->udev->reset_resume = 0;
> > > > > > > > +               } else if (data->force_disable_remote_wake) {
> > > > > > > > +                       data->udev->do_remote_wakeup = 0;
> > > > > > > > +                       if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
> > > > > > > > +                               data->udev->reset_resume = 1;
> > > > > > > > +               }
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > >         return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.44.0.769.g3c40516874-goog
> > > > > > >
> > > > > > > There is a D-Bus interface available to overwrite the wakeup setting:
> > > > > > >
> > > > > > > https://github.com/bluez/bluez/blob/master/doc/org.bluez.Device.rst#boolean-wakeallowed-readwrite
> > > > > > >
> > > > > > > Or do you want a master switch for it? On the other hand aren't we
> > > > > > > getting into the rfkill area if you really want to switch off radio
> > > > > > > activity while suspended? That seems like a better idea then just
> > > > > > > disable remote wakeup.
> > > > >
> > > > > This DBUS api is different from the quirk this is introducing.
> > > > >
> > > > > The `Wake Allowed` field in D-bus controls whether we add the address
> > > > > to the Classic Event Filter (HIDP) or LE Filter Accept List (HOGP) but
> > > > > not whether we allow wake at the transport level (which is why
> > > > > hdev->wakeup exists).
> > > > >
> > > > > This change specifically addresses a quirk with Realtek chipsets:
> > > > > RTL8822/RTL8852 will do "global shutdown" and power off Bluetooth if
> > > > > USB Remote Wake bit is not set. The USB remote_wake bit is normally
> > > > > set by the USB stack based on whether device_may_wakeup(udev) == true.
> > > > > This means that RTL88x2 will lose power around suspend/resume if there
> > > > > are no wake capable devices connected.
> > > > >
> > > > > ChromeOS decided to use idle power and resume-time to determine
> > > > > whether to allow remote wake or not for these chipsets and we want to
> > > > > move this control to userspace so that we don't have to hold CHROMIUM
> > > > > patches in the kernel applying this policy (we want udev rules
> > > > > instead). RTL8852 gets force enabled remote wake because the
> > > > > RESET_RESUME behavior of this chip would otherwise increase our resume
> > > > > time >1s which breaks one of our platform requirements.
> > > > >
> > > > > The end-goal of these changes:
> > > > > * We detect RTL8822 or RTL8852 with udev and apply the right policy.
> > > > > * RTL8822 gets force_disable_remote_wake (idle power consumption too
> > > > > high otherwise)
> > > > > * RTL8852 gets force_enable_remote_wake (resume time too long otherwise)
> > > >
> > > > Got it, but the suggestion was to instead of using
> > > > force_enable_remote_wake, which is sort of non-standard, why don't
> > > > Chrome OS simply use rkill interface to tell the driver to shutdown?
> > > > On resume then you can just unblock via rfkill that should have the
> > > > same result as using force_enable_remote_wake, well except if there is
> > > > a bug in the driver that is not handling rfkill as a 'global
> > > > shutdown', but then you need to fix the driver not introduce yet
> > > > another debugfs entry to bypass it.
> > >
> > > Did you mean `force_disable_remote_wake`? rfkill will work for that
> > > around system suspend. We preferred not to do it because we don't use
> > > userspace suspend signals with Bluez today (preferring the kernel
> > > suspend notifier).
> >
> > Yeah, that said rfkill has nothing to do with suspend afaik, it is
> > more for achieving flight mode and as far I know it is a kernel
> > interface.
> >
> > > `force_enable_remote_wake` still needs debugfs as rfkill can't force
> > > an interface to stay awake as far as I know.
> >
> > You mixing up, Im not saying to use rfkill to enable/disable wake
> > support, the remains the same, what changes is that if you want to
> > overwrite that it just use rfkill to block the traffic while suspended
> > that way wake being enable or not doesn't matter since the controller
> > radio shall be off if it is blocked.
>
> Ah, the problem with resume time is not how long it takes for the
> Bluetooth link to come up. The problem is that it takes >1s for all
> the driver resume callbacks to run after a wake IRQ occurs and blocks
> user space from running at all.
>
> The root cause seems to be that reset-resume (which correctly gets set
> for Realtek) will block USB port resume and, since the resume path
> seems to be synchronous, it blocks other drivers from resuming too.
>
> >
> > >
> > > >
> > > > > Hope this provides enough context for why this change is necessary.
> > > > >
> > > > > >
> > > > > > Yes, the initial idea was a master switch.
> > > > > > Thanks for your suggestions.
> > > > > > Let me discuss it with Abhishek.
> > > > > > >
> > > > > > > --
> > > > > > > Luiz Augusto von Dentz
> > > > > >
> > > > > > Thanks,
> > > > > > Archie
> > > > > >
> > > > > > --
> > > > > > You received this message because you are subscribed to the Google Groups "ChromeOS Bluetooth Upstreaming" group.
> > > > > > To unsubscribe from this group and stop receiving emails from it, send an email to [e-mail].
> > > > > > To post to this group, send email to [e-mail].
> > > > > > To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromeos-bluetooth-upstreaming/CAJQfnxHUW%2BMdJUp9VCrF2Nq_-JZrd7mKBR9NdDoo0SOvgH5WUQ%40mail.gmail.com.
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

Patch

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 8bede0a335668..846b15fc3c04c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -873,6 +873,9 @@  struct btusb_data {
 	unsigned cmd_timeout_cnt;
 
 	struct qca_dump_info qca_dump;
+
+	bool force_enable_remote_wake;
+	bool force_disable_remote_wake;
 };
 
 static void btusb_reset(struct hci_dev *hdev)
@@ -4596,6 +4599,10 @@  static int btusb_probe(struct usb_interface *intf,
 
 	debugfs_create_file("force_poll_sync", 0644, hdev->debugfs, data,
 			    &force_poll_sync_fops);
+	debugfs_create_bool("force_enable_remote_wake", 0644, hdev->debugfs,
+			    &data->force_enable_remote_wake);
+	debugfs_create_bool("force_disable_remote_wake", 0644, hdev->debugfs,
+			    &data->force_disable_remote_wake);
 
 	return 0;
 
@@ -4702,6 +4709,18 @@  static int btusb_suspend(struct usb_interface *intf, pm_message_t message)
 		}
 	}
 
+	if (!PMSG_IS_AUTO(message)) {
+		if (data->force_enable_remote_wake) {
+			data->udev->do_remote_wakeup = 1;
+			if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
+				data->udev->reset_resume = 0;
+		} else if (data->force_disable_remote_wake) {
+			data->udev->do_remote_wakeup = 0;
+			if (test_bit(BTUSB_WAKEUP_AUTOSUSPEND, &data->flags))
+				data->udev->reset_resume = 1;
+		}
+	}
+
 	return 0;
 }