Message-ID: <CANFp7mU2Chj_cZ_26kfM8TE1OToZzUeFKz61D7j-0ykMBQeG4A@mail.gmail.com>
Date: Fri, 26 Apr 2024 10:04:03 -0700
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Archie Pusaka <apusaka@...gle.com>
Cc: Luiz Augusto von Dentz <luiz.dentz@...il.com>, linux-bluetooth <linux-bluetooth@...r.kernel.org>, 
	Johan Hedberg <johan.hedberg@...il.com>, Marcel Holtmann <marcel@...tmann.org>, 
	CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>, 
	Archie Pusaka <apusaka@...omium.org>, Abhishek Pandit-Subedi <abhishekpandit@...gle.com>, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bluetooth: btusb: Add debugfs to force toggling remote wakeup

On Fri, Apr 26, 2024 at 2:08 AM 'Archie Pusaka' via ChromeOS Bluetooth
Upstreaming <chromeos-bluetooth-upstreaming@...omium.org> wrote:
>
> Hi Luiz,
>
> On Thu, 25 Apr 2024 at 03:05, Luiz Augusto von Dentz
> <luiz.dentz@...il.com> wrote:
> >
> > Hi Archie,
> >
> > On Mon, Apr 22, 2024 at 3:25 AM Archie Pusaka <apusaka@...gle.com> wrote:
> > >
> > > From: Archie Pusaka <apusaka@...omium.org>
> > >
> > > 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@...omium.org>
> > > Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@...gle.com>
> > >
> > > ---
> > >
> > >  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 chromeos-bluetooth-upstreaming+unsubscribe@...omium.org.
> To post to this group, send email to chromeos-bluetooth-upstreaming@...omium.org.
> 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.

Powered by blists - more mailing lists