Message-ID: <CANFp7mWWYmm9gPrsDBho3sDGu5q_fQztR+LJJUWQ_Faw0QRXFQ@mail.gmail.com>
Date: Wed, 1 May 2024 09:34:41 -0700
From: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
To: Luiz Augusto von Dentz <luiz.dentz@...il.com>
Cc: Archie Pusaka <apusaka@...gle.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 Tue, Apr 30, 2024 at 9:46 AM Luiz Augusto von Dentz
<luiz.dentz@...il.com> wrote:
>
> Hi Abhishek,
>
> On Fri, Apr 26, 2024 at 1:04 PM Abhishek Pandit-Subedi
> <abhishekpandit@...omium.org> wrote:
> >
> > 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)
>
> 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 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.
>
>
>
> --
> Luiz Augusto von Dentz

Powered by blists - more mailing lists