Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220822125221.Bluez.1.I541cbea9d6295f531c796bf3bda96b22db76bc19@changeid>
 <CABBYNZJUgTNazWNQUEsVxufkpX5g39iom-DKZqX2eTQQA+RA=A@mail.gmail.com>
In-Reply-To: <CABBYNZJUgTNazWNQUEsVxufkpX5g39iom-DKZqX2eTQQA+RA=A@mail.gmail.com>
From:   Archie Pusaka <apusaka@google.com>
Date:   Tue, 23 Aug 2022 08:28:43 +0800
Message-ID: <CAJQfnxE4dgXZYHcS0PR6yFAHa4FZhqkTPk+jhg-Xe6Ps9NbOpA@mail.gmail.com>
Subject: Re: [Bluez PATCH] adapter: Reset pending settings when receiving MGMT error
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@chromium.org>,
        Archie Pusaka <apusaka@chromium.org>,
        Sonny Sasaka <sonnysasaka@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Luiz,

On Tue, 23 Aug 2022 at 06:54, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Aug 21, 2022 at 11:33 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > Hi Paul,
> >
> > On Mon, 22 Aug 2022 at 14:15, Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > >
> > > Dear Archie,
> > >
> > >
> > > Thank you for the patch.
> > >
> > >
> > > Am 22.08.22 um 06:53 schrieb Archie Pusaka:
> > > > From: Archie Pusaka <apusaka@chromium.org>
> > >
> > > I think the tag in the email subject needs to be [PATCH BlueZ] to be
> > > detected by the build bot.
> >
> > Is the bot the one who just commented about the test result? If so
> > probably it can detect this format as well.
> > >
> > > > We set the pending settings flag when sending MGMT_SETTING_*
> > > > commands to the MGMT layer and clear them when receiving success
> > > > reply, but we don't clear them when receiving error reply. This
> > > > might cause a setting to be stuck in pending state.
> > >
> > > Were you able to reproduce a problem on real hardware?
> >
> > I only received some reports, but unfortunately I cannot repro on real
> > hardware. The symptom is BlueZ can't be turned off, snoop logs shows
> > that MGMT_OP_SET_POWERED fails to be sent, and we are stuck with it
> > since the next commands to toggle power are ignored.
>
> Weird how can you tell MGMT_OP_SET_POWERED fails to be sent or you
> meant it was sent but the kernel returned an error? It would be great
> to include these errors.

Yeah, the kernel returned an error, NOT_AUTHORIZED If I remembered
correctly. It was caused by a timed out disconnection procedure, so
it's weird that the return value is NOT_AUTHORIZED, but I didn't check
why.
Will include this in the commit message.

On Tue, 23 Aug 2022 at 06:51, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Archie,
>
> On Sun, Aug 21, 2022 at 9:54 PM Archie Pusaka <apusaka@google.com> wrote:
> >
> > From: Archie Pusaka <apusaka@chromium.org>
> >
> > We set the pending settings flag when sending MGMT_SETTING_*
> > commands to the MGMT layer and clear them when receiving success
> > reply, but we don't clear them when receiving error reply. This
> > might cause a setting to be stuck in pending state.
> >
> > Therefore, also clear the pending flag when receiving error.
> > Furthermore, this patch also postpone setting the pending flag
> > until we queue the MGMT command in order to avoid setting it too
> > soon but we return early.
> >
> > Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
> >
> > ---
> >
> >  src/adapter.c | 45 +++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 37 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/adapter.c b/src/adapter.c
> > index ec26aab1a7..4da1fcc3e5 100644
> > --- a/src/adapter.c
> > +++ b/src/adapter.c
> > @@ -640,14 +640,21 @@ static void new_settings_callback(uint16_t index, uint16_t length,
> >         settings_changed(adapter, settings);
> >  }
> >
> > +struct set_mode_data {
> > +       struct btd_adapter *adapter;
> > +       uint32_t setting;
> > +};
> > +
> >  static void set_mode_complete(uint8_t status, uint16_t length,
> >                                         const void *param, void *user_data)
> >  {
> > -       struct btd_adapter *adapter = user_data;
> > +       struct set_mode_data *data = user_data;
> > +       struct btd_adapter *adapter = data->adapter;
> >
> >         if (status != MGMT_STATUS_SUCCESS) {
> >                 btd_error(adapter->dev_id, "Failed to set mode: %s (0x%02x)",
> >                                                 mgmt_errstr(status), status);
> > +               adapter->pending_settings &= ~data->setting;
> >                 return;
> >         }
> >
> > @@ -677,6 +684,7 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> >  {
> >         struct mgmt_mode cp;
> >         uint32_t setting = 0;
> > +       struct set_mode_data *data;
> >
> >         memset(&cp, 0, sizeof(cp));
> >         cp.val = mode;
> > @@ -699,15 +707,23 @@ static bool set_mode(struct btd_adapter *adapter, uint16_t opcode,
> >                 break;
> >         }
> >
> > -       adapter->pending_settings |= setting;
> > -
> >         DBG("sending set mode command for index %u", adapter->dev_id);
> >
> > +       data = g_try_new0(struct set_mode_data, 1);
>
> Use new0 instead of g_try_new0.

Hmm, we use g_try_new0 in property_set_mode() though, so I thought I
should follow.
But anyway, will change to g_new0.
>
> > +       if (!data)
> > +               goto failed;
> > +
> > +       data->adapter = adapter;
> > +       data->setting = setting;
> > +
> >         if (mgmt_send(adapter->mgmt, opcode,
> >                                 adapter->dev_id, sizeof(cp), &cp,
> > -                               set_mode_complete, adapter, NULL) > 0)
> > +                               set_mode_complete, data, g_free) > 0) {
> > +               adapter->pending_settings |= setting;
> >                 return true;
> > +       }
> >
> > +failed:
> >         btd_error(adapter->dev_id, "Failed to set mode for index %u",
> >                                                         adapter->dev_id);
> >
> > @@ -718,6 +734,7 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
> >                                                         uint16_t timeout)
> >  {
> >         struct mgmt_cp_set_discoverable cp;
> > +       struct set_mode_data *data;
> >
> >         memset(&cp, 0, sizeof(cp));
> >         cp.val = mode;
> > @@ -734,11 +751,19 @@ static bool set_discoverable(struct btd_adapter *adapter, uint8_t mode,
> >                                                                         mode);
> >         }
> >
> > +       data = g_try_new0(struct set_mode_data, 1);
> > +       if (!data)
> > +               goto failed;
> > +
> > +       data->adapter = adapter;
> > +       data->setting = 0;
> > +
> >         if (mgmt_send(adapter->mgmt, MGMT_OP_SET_DISCOVERABLE,
> >                                 adapter->dev_id, sizeof(cp), &cp,
> > -                               set_mode_complete, adapter, NULL) > 0)
> > +                               set_mode_complete, data, g_free) > 0)
> >                 return true;
> >
> > +failed:
> >         btd_error(adapter->dev_id, "Failed to set mode for index %u",
> >                                                         adapter->dev_id);
>
> Looks like the data pointer is leaked in case it fails to be sent/queued.

Sorry, my bad. Will fix this.
>
> > @@ -2877,6 +2902,7 @@ static gboolean property_get_mode(struct btd_adapter *adapter,
> >
> >  struct property_set_data {
> >         struct btd_adapter *adapter;
> > +       uint32_t setting;
> >         GDBusPendingPropertySet id;
> >  };
> >
> > @@ -2901,6 +2927,8 @@ static void property_set_mode_complete(uint8_t status, uint16_t length,
> >
> >                 g_dbus_pending_property_error(data->id, dbus_err,
> >                                                         mgmt_errstr(status));
> > +
> > +               adapter->pending_settings &= ~data->setting;
> >                 return;
> >         }
> >
> > @@ -2969,8 +2997,6 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
> >
> >         mode = (enable == TRUE) ? 0x01 : 0x00;
> >
> > -       adapter->pending_settings |= setting;
> > -
> >         switch (setting) {
> >         case MGMT_SETTING_POWERED:
> >                 opcode = MGMT_OP_SET_POWERED;
> > @@ -3024,11 +3050,14 @@ static void property_set_mode(struct btd_adapter *adapter, uint32_t setting,
> >                 goto failed;
> >
> >         data->adapter = adapter;
> > +       data->setting = setting;
> >         data->id = id;
> >
> >         if (mgmt_send(adapter->mgmt, opcode, adapter->dev_id, len, param,
> > -                       property_set_mode_complete, data, g_free) > 0)
> > +                       property_set_mode_complete, data, g_free) > 0) {
> > +               adapter->pending_settings |= setting;
> >                 return;
> > +       }
> >
> >         g_free(data);
> >
> > --
> > 2.37.1.595.g718a3a8f04-goog
>
> --
> Luiz Augusto von Dentz

Thanks,
Archie