Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CANFp7mVOh5KPYPPnbGO_YnV-mBGTV3gYgWcPNSMq+=aBzBM00Q@mail.gmail.com>
 <12B499CA-FAC4-459D-B75C-1CEE801406BA@holtmann.org>
In-Reply-To: <12B499CA-FAC4-459D-B75C-1CEE801406BA@holtmann.org>
From:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date:   Mon, 11 Nov 2019 17:16:37 -0800
Message-ID: <CANFp7mXFp-5PooVVLwbBqFSzMXomf4y+5jgwPUW9b+Br1jyDmg@mail.gmail.com>
Subject: Re: RFC: Managing devices around system suspend in bluez
To:     Marcel Holtmann <marcel@holtmann.org>
Cc:     Johan Hedberg <johan.hedberg@gmail.com>,
        Yoni Shavit <yshavit@chromium.org>, alainmichaud@google.com,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 8, 2019 at 6:24 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Abhishek,
>
> > On ChromeOS, we are currently trying to design how bluez should behave
> > during system suspend. This is motivated by the fact that bluetooth
> > can be a noisy source of wakeups on a system and historically has been
> > noisy on ChromeOS.
> >
> > Here are some problems we've seen:
> > - If the system suspends while discovery is active, advertisements
> > will continue arriving on the host and will wake the system
>
> it is indeed a bad idea to run an active scan without whitelist when we a=
re going to suspend.
>
> > - Pairing a LE keyboard/mouse and disconnecting it (via link loss)
> > results in a passive scan of all advertisements (and these will wake
> > the host)
>
> This one I do not get, the passive scan should always be using the whitel=
ist.

This one is related to IRK resolution. In the existing kernel code
when configuring the le passive scan, if there are any addresses in
the whitelist which also have an IRK stored, the whitelist filter
policy is 0.

>
> > To resolve this, we propose adding a SuspendImminent and SuspendDone
> > dbus api to inform bluez that suspend is about to occur and the system
> > has resumed.
> > (These names are based off the ChromeOS Power Manager's existing
> > design: https://chromium.googlesource.com/chromiumos/platform2/+/master=
/power_manager/docs/suspend_resume.md)
>
> An interesting question is really if we need this around the corner path =
or if it would be done better directly from the kernel to bluetoothd via mg=
mt socket. So lets explore if we can make this work in a totally generic wa=
y that benefits all Linux distros.

Yes, we could let the kernel inform bluetooth that suspend is about to
occur (maybe via PM_SUSPEND_PREPARE) and wait for bluez to finish the
suspend tasks before continuing. We could probably do this via event
pairs of SUSPEND_PREPARE/SUSPEND_PREPARE_DONE and
RESUME_PREPARE/RESUME_PREPARE_DONE.

>
> > In the suspend imminent handler, we would do the following in order:
> > * Pause any discovery
> > * Set an event filter for all paired devices capable of waking the
> > system (anything that creates uhid or uinput virtual devices)
> > * Disconnect all connected devices (with a soft disconnect)
> > * Enable background scan with whitelist of devices that should be able
> > to wake the system (** see below for comments about IRK resolution)
>
> I agree that when a system is going to suspend, we should disable any act=
ive scanning without whitelist. If we want to differentiate between device =
allowed to wake us up and device that don=E2=80=99t, then we need to create=
 that list first. Right now we don=E2=80=99t really do that. And I think we=
 don=E2=80=99t have an easy way to tell the kernel via mgmt what this list =
will be.

We will want to introduce a new mgmt op or update an existing op to
set a wake capable bit for the device. MGMT_OP_SET_CONN_PARAMS might
be a good candidate to update with this info.
As for how bluez holds this information, I think we mark all devices
that create uhid or uinput as wake capable and then disable them if we
don't want them to wake us up. Maybe a "WakeSource" property in the
device properties?

>
> > In the suspend done handler, we would do the following in order:
> > * Clear the event filter
> > * Enable background scan with non-suspend logic (** see below for
> > comments about IRK resolution)
> > * Unpause discovery (if it was running before suspend)
>
> We need to define what discovery is. My understand is that discovery is a=
lways user triggered way of actively finding a specific device around you. =
In most cases because you want to pair with it. The way how mgmt works is t=
hat we discovery only for a period of time. And bluetoothd turns it into a =
continuous discovery as long as a specific UI portion is open and requestin=
g it. Unless discovery has a different meaning, then a simple restart is en=
ough.

The use-case I'm thinking of is the user has the scanning window open
and then closes the lid of their laptop. Right before suspend, they
were Discovering but in order to suspend, we need to stop. I think
maybe pause and unpause are ambiguous; it is actually just stop
discovery and start discovery.

>
> > We expect this will result in the following:
> > * Classic: A paired device can wake the host if it's in the event filte=
r
> > * LE: A paired device can wake the host if it's in the whitelist and
> > it sends an advertisement (undirected if in the whitelist, directed if
> > targeting our host; i.e. filter_policy =3D 0x1 or 0x3)
>
> Do we need this for classic? I agree that we should abort the inquiry pro=
cedure if one is active, but do we really want to deal with the event filte=
r? It is one of these old Bluetooth 1.0b concepts in HCI that are not as we=
ll defined as others since you have no idea what the controllers supports a=
nd how many devices they support.

Yes, I think we do. While we could initially leave this out, if we
wanted policy based wakeup control, we would need this.

>
> > Do you think the actions taken in the suspend handlers are sufficient?
> > Any concerns or things to look out for?
>
> I would try to handle this inside the kernel and only add some extra noti=
fications to mgmt so that bluetoothd can be told that we are suspended righ=
t now.

I think when we upstream this, we would want to do it this way
(especially because I don't think there's a userspace power manager
that's common across distros). I will look into using the PM notifiers
further.

>
> > Thanks
> > Abhishek
> >
> > IRK Resolution:
> > With this design, we have some problems with IRK resolution on BT
> > version < 4.2. Devices supporting BT Privacy 1.2 may start using
> > resolvable private addresses for both initiator and destination.
> > Without address resolution in the controller, we have to set the
> > filter policy to 0 (allow all) so that we can do address resolution on
> > the host.
> > Implementing these privacy features are outside the scope of this RFC
> > so we will disallow wake up from suspend for these devices (set filter
> > policy to accept only whitelist and directed). Once bluez supports
> > Privacy 1.2, wakeup from these devices will work on controllers with
> > BT version >=3D 4.2.
>
> It is either support for controller side IRK resolution or forwarding of =
unresolved directed advertising with RPA. The latter will cause wakeups, bu=
t it is limited to directed advertising and will be fine.
>
> Regards
>
> Marcel
>