Hi Luiz,
On Tue, 23 Aug 2022 at 06:54, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Archie,
>
> On Sun, Aug 21, 2022 at 11:33 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> >
> > Hi Paul,
> >
> > On Mon, 22 Aug 2022 at 14:15, Paul Menzel <pmenzel@xxxxxxxxxxxxx> wrote:
> > >
> > > Dear Archie,
> > >
> > >
> > > Thank you for the patch.
> > >
> > >
> > > Am 22.08.22 um 06:53 schrieb Archie Pusaka:
> > > > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxx> wrote:
>
> Hi Archie,
>
> On Sun, Aug 21, 2022 at 9:54 PM Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
> >
> > From: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> >
> > ---
> >
> > 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
