Re: [Bluez PATCH v1] adapter: Don't remove device if adapter is powered off

[Archie Pusaka <apusaka@google.com>]

Hi Luiz,

Thanks for the suggestion. I had sent another patch (titled "adapter:
Remove temporary devices before power off") which implements your
idea.
That should make this patch obsolete.

Thanks,
Archie

On Wed, 6 Jan 2021 at 02:33, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Mon, Jan 4, 2021 at 7:17 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Luiz,
> >
> > Sorry for being unclear.
> >
> >
> > On Tue, 5 Jan 2021 at 03:17, Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Archie,
> > >
> > > On Mon, Jan 4, 2021 at 11:09 AM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Archie,
> > > >
> > > > On Mon, Dec 28, 2020 at 10:34 PM Archie Pusaka <apusaka@google.com> wrote:
> > > > >
> > > > > From: Archie Pusaka <apusaka@chromium.org>
> > > > >
> > > > > If adapter is powered off when a device is being removed, there is a
> > > > > possibility that the kernel couldn't clean the device's information,
> > > > > for example the pairing information. This causes the kernel to
> > > > > disagree with the user space about whether the device is paired.
> > > > >
> > > > > Therefore, to avoid discrepancy we must not proceed to remove the
> > > > > device within the user space as well.
> > > >
> > > > This sounds like we have a bug in the kernel, aren't we calling
> > > > btd_adapter_remove_bonding or is that failing if the adapter is not
> > > > powered? Hmm it does like it:
> > > >
> > > > This command can only be used when the controller is powered.
> > > >
> > > > > Reviewed-by: Daniel Winkler <danielwinkler@google.com>
> > > > > ---
> > > > >
> > > > >  src/adapter.c | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/src/adapter.c b/src/adapter.c
> > > > > index ec6a6a64c5..a2abc46706 100644
> > > > > --- a/src/adapter.c
> > > > > +++ b/src/adapter.c
> > > > > @@ -1238,6 +1238,14 @@ void btd_adapter_remove_device(struct btd_adapter *adapter,
> > > > >  {
> > > > >         GList *l;
> > > > >
> > > > > +       /* Test if adapter is or will be powered off.
> > > > > +        * This is to prevent removing the device information only on user
> > > > > +        * space, but failing to do so on the kernel.
> > > > > +        */
> > > > > +       if (!(adapter->current_settings & MGMT_SETTING_POWERED) ||
> > > > > +                       (adapter->pending_settings & MGMT_SETTING_POWERED))
> > > > > +               return;
> > > >
> > > > We might need to return an error here so we can reply with an error on
> > > > Adapter.RemoveDevice.
> > >
> > Should be unnecessary due to the check you mentioned below.
> >
> > > After some investigation it looks like there is already a similar check:
> > >
> > > https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/src/adapter.c#n3238
> > >
> > > That perhaps needs to updated or perhaps this is the result of the
> > > device being set to temporary which sets a timer to remove the device
> > > and then in the meantime the adapter is powered off? In that case
> > > perhaps we should clean up the devices set as temporary.
> > >
> > The problem occurs when the device is paired and is currently
> > connected, then Adapter.RemoveDevice is called. This would make us
> > disconnect the device first before actually removing the device.
> > During this disconnection phase, there is a chance the adapter is
> > powered off.
> >
> > If this happens, we would still attempt to remove the device anyway.
> > No problem on the user space, but it will fail on the kernel side (as
> > per the API, it requires adapter to be on). The check you mentioned is
> > unfortunately not executed during this phase.
> >
> > About the timer, I didn't have the exact issue you described, but this
> > version of patch might have other problems with it, because the timer
> > would still be running when we do the early return from
> > btd_adapter_remove_device. Although nothing will happen if the timer
> > runs out when the power is still off (we would just do the early
> > return again), but it might be unpleasant if the adapter is re-powered
> > and the timer kicks off to remove the device.
>
> Right, Id guess if RemoveDevice is called and either way we end up
> powering off the adapter I suppose we want the device removed either
> way thus why I suggested cleaning up the temporary devices before
> powering down.
>
> > > > >         adapter->connect_list = g_slist_remove(adapter->connect_list, dev);
> > > > >
> > > > >         adapter->devices = g_slist_remove(adapter->devices, dev);
> > > > > --
> > > > > 2.29.2.729.g45daf8777d-goog
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > Thanks,
> > Archie
>
>
>
> --
> Luiz Augusto von Dentz