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
<abhishekpandit@xxxxxxxxxxxx> wrote:
>
> On Wed, May 1, 2024 at 9:52 AM Luiz Augusto von Dentz
> <luiz.dentz@xxxxxxxxx> wrote:
> >
> > Hi Abhishek,
> >
> > On Wed, May 1, 2024 at 12:34 PM Abhishek Pandit-Subedi
> > <abhishekpandit@xxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
> > > <luiz.dentz@xxxxxxxxx> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> > > > <abhishekpandit@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
> > > > > Upstreaming <chromeos-bluetooth-upstreaming@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Luiz,
> > > > > >
> > > > > > On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> > > > > > <luiz.dentz@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Hi Archie,
> > > > > > >
> > > > > > > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > > > > > > >
> > > > > > > > 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 <apusaka@xxxxxxxxxxxx>
> > > > > > > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxx>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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 chromeos-bluetooth-upstreaming+unsubscribe@xxxxxxxxxxxx.
> > > > > > To post to this group, send email to chromeos-bluetooth-upstreaming@xxxxxxxxxxxx.
> > > > > > 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
